| Message ID | 20241110083017.367565-5-ilias.apalodimas@linaro.org |
|---|---|
| State | Accepted |
| Commit | 5907c81647055a03580dae850f82d85f7d810f7e |
| Headers | show |
| Series | [v3,1/6] mbedtls: Enable TLS 1.2 support | expand |
On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > With the recent changes of lwip & mbedTLS we can now download from > https:// urls instead of just http://. > Adjust our wget lwip version parsing to support both URLs. > While at it adjust the default TCP window for QEMU since https seems to > require at least 16384 > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/Kconfig | 19 +++++++++++ > net/lwip/Kconfig | 2 +- > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > 3 files changed, 97 insertions(+), 10 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org> Some nits / questions I am not sure what mbedtls_hardware_poll() is, but if @len is too short, would it be acceptable to return an error? How many bytes is it requesting in the https case? uclass_first_device() if you want the first device, not uclass_get_device(...0) Regards, Simon
Thanks Simon, On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > With the recent changes of lwip & mbedTLS we can now download from > > https:// urls instead of just http://. > > Adjust our wget lwip version parsing to support both URLs. > > While at it adjust the default TCP window for QEMU since https seems to > > require at least 16384 > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > cmd/Kconfig | 19 +++++++++++ > > net/lwip/Kconfig | 2 +- > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > 3 files changed, 97 insertions(+), 10 deletions(-) > > Reviewed-by: Simon Glass <sjg@chromium.org> > > Some nits / questions > > I am not sure what mbedtls_hardware_poll() is, It's an entropy collector used by mbedTLS to ensure the platform has enough entropy. It's required if your platform doesn't support standards like the /dev/urandom or Windows CryptoAPI. > but if @len is too > short, would it be acceptable to return an error? How many bytes is it > requesting in the https case? If you don't return enough entropy https:// will fail and mbedTLS & lwIP will print an error. I think we currently use 128 and the default for mbedTLS is 32. > > uclass_first_device() if you want the first device, not uclass_get_device(...0) Sure Thanks /Ilias > > Regards, > Simon
On 11/10/24 08:28, Ilias Apalodimas wrote: > With the recent changes of lwip & mbedTLS we can now download from > https:// urls instead of just http://. > Adjust our wget lwip version parsing to support both URLs. > While at it adjust the default TCP window for QEMU since https seems to > require at least 16384 > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > cmd/Kconfig | 19 +++++++++++ > net/lwip/Kconfig | 2 +- > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > 3 files changed, 97 insertions(+), 10 deletions(-) > Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Regards,
HI Ilias, On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Thanks Simon, > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > https:// urls instead of just http://. > > > Adjust our wget lwip version parsing to support both URLs. > > > While at it adjust the default TCP window for QEMU since https seems to > > > require at least 16384 > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > --- > > > cmd/Kconfig | 19 +++++++++++ > > > net/lwip/Kconfig | 2 +- > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > Some nits / questions > > > > I am not sure what mbedtls_hardware_poll() is, > > It's an entropy collector used by mbedTLS to ensure the platform has > enough entropy. > It's required if your platform doesn't support standards like the > /dev/urandom or Windows CryptoAPI. > > > but if @len is too > > short, would it be acceptable to return an error? How many bytes is it > > requesting in the https case? > > If you don't return enough entropy https:// will fail and mbedTLS & > lwIP will print an error. I think we currently use 128 and the default > for mbedTLS is 32. OK, then the code is quite strange to me. It seems like it should check that 'len' is large enough. But what does 'len' actually mean? Its arguments are not described in mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? Why call it with only 8 bytes? It might be more bytes than is requested (and larger than the buffer), or fewer. It seems that your function is written with knowledge of the internals of mbedtls. BTW mbedtls_hardware_poll() is a strange name as it actually reads random data, so I think it should be renamed. > > > > > uclass_first_device() if you want the first device, not uclass_get_device(...0) > > Sure > Regards, SImon
On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > HI Ilias, > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Thanks Simon, > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > https:// urls instead of just http://. > > > > Adjust our wget lwip version parsing to support both URLs. > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > require at least 16384 > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > --- > > > > cmd/Kconfig | 19 +++++++++++ > > > > net/lwip/Kconfig | 2 +- > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > Some nits / questions > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > enough entropy. > > It's required if your platform doesn't support standards like the > > /dev/urandom or Windows CryptoAPI. > > > > > but if @len is too > > > short, would it be acceptable to return an error? How many bytes is it > > > requesting in the https case? > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > lwIP will print an error. I think we currently use 128 and the default > > for mbedTLS is 32. > > OK, then the code is quite strange to me. It seems like it should > check that 'len' is large enough. > > But what does 'len' actually mean? Its arguments are not described in > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? The entry point is mbedtls_entropy_func(). mbedtls then calls entropy_gather_internal() which asks for some bytes of entropy and is controlled by a config option (and defaults to 128 for the config we use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually MBEDTLS_ENTROPY_MAX_GATHER. > Why call it with only 8 bytes? It might be more bytes than is > requested (and larger than the buffer), or fewer. It doesn't matter because we copy back the correct amount of bytes(what the caller requested). If it's less mbedTLS calls that function until it gathers all requested entropy. > It seems that your > function is written with knowledge of the internals of mbedtls. Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it requires internal knowledge of it (and is probably part of the ABI, but I'll have to check that). What happens in the TLS case is that 64b are required. We either make 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. I could rewrite it as uclass_first_device(UCLASS_RNG, &dev); if (!dev) { log_err("Failed to get an rng device\n"); return ret; } rng = malloc(len); if (!rng) return -ENOMEM; ret = dm_rng_read(dev, rng, len); if (ret) { free(rng); return ret; } memcpy(output, rng, len); *olen = len; free(rng); It does the same thing but gets 128b in one go. I don't have a strong opinion on that. Let me know what you prefer The tradeoff seems pretty straightforward. you either gather potentially more entropy than required and call that function once, or you gather exactly what's required on 8b steps -- but call that function multiple times. > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > random data, so I think it should be renamed. I don't think that can't change as you would break all projects using it, but you can try sending a patch. Thanks /Ilias > > > > > > > > > uclass_first_device() if you want the first device, not uclass_get_device(...0) > > > > Sure > > > > Regards, > SImon
Hi Ilias, On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > > > HI Ilias, > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Thanks Simon, > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > > https:// urls instead of just http://. > > > > > Adjust our wget lwip version parsing to support both URLs. > > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > > require at least 16384 > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > --- > > > > > cmd/Kconfig | 19 +++++++++++ > > > > > net/lwip/Kconfig | 2 +- > > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > Some nits / questions > > > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > > enough entropy. > > > It's required if your platform doesn't support standards like the > > > /dev/urandom or Windows CryptoAPI. > > > > > > > but if @len is too > > > > short, would it be acceptable to return an error? How many bytes is it > > > > requesting in the https case? > > > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > > lwIP will print an error. I think we currently use 128 and the default > > > for mbedTLS is 32. > > > > OK, then the code is quite strange to me. It seems like it should > > check that 'len' is large enough. > > > > But what does 'len' actually mean? Its arguments are not described in > > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? > > The entry point is mbedtls_entropy_func(). mbedtls then calls > entropy_gather_internal() which asks for some bytes of entropy and is > controlled by a config option (and defaults to 128 for the config we > use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually > MBEDTLS_ENTROPY_MAX_GATHER. > > > Why call it with only 8 bytes? It might be more bytes than is > > requested (and larger than the buffer), or fewer. > > It doesn't matter because we copy back the correct amount of > bytes(what the caller requested). If it's less mbedTLS calls that > function until it gathers all requested entropy. > > > It seems that your > > function is written with knowledge of the internals of mbedtls. > > Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it > requires internal knowledge of it (and is probably part of the ABI, > but I'll have to check that). > > What happens in the TLS case is that 64b are required. We either make > 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. > > I could rewrite it as > uclass_first_device(UCLASS_RNG, &dev); > if (!dev) { > log_err("Failed to get an rng device\n"); > return ret; > } > > rng = malloc(len); > if (!rng) > return -ENOMEM; > > ret = dm_rng_read(dev, rng, len); > if (ret) { > free(rng); > return ret; > } > > memcpy(output, rng, len); > *olen = len; > free(rng); > > It does the same thing but gets 128b in one go. I don't have a strong > opinion on that. Let me know what you prefer > The tradeoff seems pretty straightforward. you either gather > potentially more entropy than required and call that function once, or > you gather exactly what's required on 8b steps -- but call that > function multiple times. > > > > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > > random data, so I think it should be renamed. > > I don't think that can't change as you would break all projects using > it, but you can try sending a patch. I am wondering why we can't just do: ret = dm_rng_read(dev, rng, len); if (ret) return ret; *olen = len; Regards, Simon
On Wed, 13 Nov 2024 at 17:09, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > > > > > HI Ilias, > > > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Thanks Simon, > > > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > > > https:// urls instead of just http://. > > > > > > Adjust our wget lwip version parsing to support both URLs. > > > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > > > require at least 16384 > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > --- > > > > > > cmd/Kconfig | 19 +++++++++++ > > > > > > net/lwip/Kconfig | 2 +- > > > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > Some nits / questions > > > > > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > > > enough entropy. > > > > It's required if your platform doesn't support standards like the > > > > /dev/urandom or Windows CryptoAPI. > > > > > > > > > but if @len is too > > > > > short, would it be acceptable to return an error? How many bytes is it > > > > > requesting in the https case? > > > > > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > > > lwIP will print an error. I think we currently use 128 and the default > > > > for mbedTLS is 32. > > > > > > OK, then the code is quite strange to me. It seems like it should > > > check that 'len' is large enough. > > > > > > But what does 'len' actually mean? Its arguments are not described in > > > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? > > > > The entry point is mbedtls_entropy_func(). mbedtls then calls > > entropy_gather_internal() which asks for some bytes of entropy and is > > controlled by a config option (and defaults to 128 for the config we > > use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually > > MBEDTLS_ENTROPY_MAX_GATHER. > > > > > Why call it with only 8 bytes? It might be more bytes than is > > > requested (and larger than the buffer), or fewer. > > > > It doesn't matter because we copy back the correct amount of > > bytes(what the caller requested). If it's less mbedTLS calls that > > function until it gathers all requested entropy. > > > > > It seems that your > > > function is written with knowledge of the internals of mbedtls. > > > > Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it > > requires internal knowledge of it (and is probably part of the ABI, > > but I'll have to check that). > > > > What happens in the TLS case is that 64b are required. We either make > > 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. > > > > I could rewrite it as > > uclass_first_device(UCLASS_RNG, &dev); > > if (!dev) { > > log_err("Failed to get an rng device\n"); > > return ret; > > } > > > > rng = malloc(len); > > if (!rng) > > return -ENOMEM; > > > > ret = dm_rng_read(dev, rng, len); > > if (ret) { > > free(rng); > > return ret; > > } > > > > memcpy(output, rng, len); > > *olen = len; > > free(rng); > > > > It does the same thing but gets 128b in one go. I don't have a strong > > opinion on that. Let me know what you prefer > > The tradeoff seems pretty straightforward. you either gather > > potentially more entropy than required and call that function once, or > > you gather exactly what's required on 8b steps -- but call that > > function multiple times. > > > > > > > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > > > random data, so I think it should be renamed. > > > > I don't think that can't change as you would break all projects using > > it, but you can try sending a patch. > > I am wondering why we can't just do: > > ret = dm_rng_read(dev, rng, len); > if (ret) > return ret; > > *olen = len; I am not sure I am following. Do that were? Thanks /Ilias > > Regards, > Simon
Hi Ilias, On Wed, 13 Nov 2024 at 08:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 13 Nov 2024 at 17:09, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > HI Ilias, > > > > > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Thanks Simon, > > > > > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > > > > https:// urls instead of just http://. > > > > > > > Adjust our wget lwip version parsing to support both URLs. > > > > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > > > > require at least 16384 > > > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > --- > > > > > > > cmd/Kconfig | 19 +++++++++++ > > > > > > > net/lwip/Kconfig | 2 +- > > > > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > Some nits / questions > > > > > > > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > > > > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > > > > enough entropy. > > > > > It's required if your platform doesn't support standards like the > > > > > /dev/urandom or Windows CryptoAPI. > > > > > > > > > > > but if @len is too > > > > > > short, would it be acceptable to return an error? How many bytes is it > > > > > > requesting in the https case? > > > > > > > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > > > > lwIP will print an error. I think we currently use 128 and the default > > > > > for mbedTLS is 32. > > > > > > > > OK, then the code is quite strange to me. It seems like it should > > > > check that 'len' is large enough. > > > > > > > > But what does 'len' actually mean? Its arguments are not described in > > > > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? > > > > > > The entry point is mbedtls_entropy_func(). mbedtls then calls > > > entropy_gather_internal() which asks for some bytes of entropy and is > > > controlled by a config option (and defaults to 128 for the config we > > > use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually > > > MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > > Why call it with only 8 bytes? It might be more bytes than is > > > > requested (and larger than the buffer), or fewer. > > > > > > It doesn't matter because we copy back the correct amount of > > > bytes(what the caller requested). If it's less mbedTLS calls that > > > function until it gathers all requested entropy. > > > > > > > It seems that your > > > > function is written with knowledge of the internals of mbedtls. > > > > > > Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it > > > requires internal knowledge of it (and is probably part of the ABI, > > > but I'll have to check that). > > > > > > What happens in the TLS case is that 64b are required. We either make > > > 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > I could rewrite it as > > > uclass_first_device(UCLASS_RNG, &dev); > > > if (!dev) { > > > log_err("Failed to get an rng device\n"); > > > return ret; > > > } > > > > > > rng = malloc(len); > > > if (!rng) > > > return -ENOMEM; > > > > > > ret = dm_rng_read(dev, rng, len); > > > if (ret) { > > > free(rng); > > > return ret; > > > } > > > > > > memcpy(output, rng, len); > > > *olen = len; > > > free(rng); > > > > > > It does the same thing but gets 128b in one go. I don't have a strong > > > opinion on that. Let me know what you prefer > > > The tradeoff seems pretty straightforward. you either gather > > > potentially more entropy than required and call that function once, or > > > you gather exactly what's required on 8b steps -- but call that > > > function multiple times. > > > > > > > > > > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > > > > random data, so I think it should be renamed. > > > > > > I don't think that can't change as you would break all projects using > > > it, but you can try sending a patch. > > > > I am wondering why we can't just do: > > > > ret = dm_rng_read(dev, rng, len); > > if (ret) > > return ret; > > > > *olen = len; > > I am not sure I am following. Do that were? Oh, I had that wrong, so very confusing, sorry. Something like this: +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len, + size_t *olen) +{ + struct udevice *dev; + int ret; + + ret = uclass_get_device(UCLASS_RNG, 0, &dev); + if (ret) { + log_err("Failed to get an rng: %d\n", ret); + return ret; + } + ret = dm_rng_read(dev, output, len); + if (ret) + return ret; + + *olen = len; + + return 0; +} Regards, Simon
On Wed, 13 Nov 2024 at 18:04, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Wed, 13 Nov 2024 at 08:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > > On Wed, 13 Nov 2024 at 17:09, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Ilias, > > > > > > On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > HI Ilias, > > > > > > > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > Thanks Simon, > > > > > > > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > > > > > https:// urls instead of just http://. > > > > > > > > Adjust our wget lwip version parsing to support both URLs. > > > > > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > > > > > require at least 16384 > > > > > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > > --- > > > > > > > > cmd/Kconfig | 19 +++++++++++ > > > > > > > > net/lwip/Kconfig | 2 +- > > > > > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > > > Some nits / questions > > > > > > > > > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > > > > > > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > > > > > enough entropy. > > > > > > It's required if your platform doesn't support standards like the > > > > > > /dev/urandom or Windows CryptoAPI. > > > > > > > > > > > > > but if @len is too > > > > > > > short, would it be acceptable to return an error? How many bytes is it > > > > > > > requesting in the https case? > > > > > > > > > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > > > > > lwIP will print an error. I think we currently use 128 and the default > > > > > > for mbedTLS is 32. > > > > > > > > > > OK, then the code is quite strange to me. It seems like it should > > > > > check that 'len' is large enough. > > > > > > > > > > But what does 'len' actually mean? Its arguments are not described in > > > > > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? > > > > > > > > The entry point is mbedtls_entropy_func(). mbedtls then calls > > > > entropy_gather_internal() which asks for some bytes of entropy and is > > > > controlled by a config option (and defaults to 128 for the config we > > > > use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually > > > > MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > > > > Why call it with only 8 bytes? It might be more bytes than is > > > > > requested (and larger than the buffer), or fewer. > > > > > > > > It doesn't matter because we copy back the correct amount of > > > > bytes(what the caller requested). If it's less mbedTLS calls that > > > > function until it gathers all requested entropy. > > > > > > > > > It seems that your > > > > > function is written with knowledge of the internals of mbedtls. > > > > > > > > Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it > > > > requires internal knowledge of it (and is probably part of the ABI, > > > > but I'll have to check that). > > > > > > > > What happens in the TLS case is that 64b are required. We either make > > > > 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > > > I could rewrite it as > > > > uclass_first_device(UCLASS_RNG, &dev); > > > > if (!dev) { > > > > log_err("Failed to get an rng device\n"); > > > > return ret; > > > > } > > > > > > > > rng = malloc(len); > > > > if (!rng) > > > > return -ENOMEM; > > > > > > > > ret = dm_rng_read(dev, rng, len); > > > > if (ret) { > > > > free(rng); > > > > return ret; > > > > } > > > > > > > > memcpy(output, rng, len); > > > > *olen = len; > > > > free(rng); > > > > > > > > It does the same thing but gets 128b in one go. I don't have a strong > > > > opinion on that. Let me know what you prefer > > > > The tradeoff seems pretty straightforward. you either gather > > > > potentially more entropy than required and call that function once, or > > > > you gather exactly what's required on 8b steps -- but call that > > > > function multiple times. > > > > > > > > > > > > > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > > > > > random data, so I think it should be renamed. > > > > > > > > I don't think that can't change as you would break all projects using > > > > it, but you can try sending a patch. > > > > > > I am wondering why we can't just do: > > > > > > ret = dm_rng_read(dev, rng, len); > > > if (ret) > > > return ret; > > > > > > *olen = len; > > > > I am not sure I am following. Do that were? > > Oh, I had that wrong, so very confusing, sorry. Something like this: > > +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len, > + size_t *olen) > +{ > + struct udevice *dev; > + int ret; > + > + ret = uclass_get_device(UCLASS_RNG, 0, &dev); > + if (ret) { > + log_err("Failed to get an rng: %d\n", ret); > + return ret; > + } > + ret = dm_rng_read(dev, output, len); > + if (ret) > + return ret; > + > + *olen = len; > + > + return 0; Yep, that's identical to what I had above without the allocation, which indeed isn't needed. Both of the versions are correct and I ask internally mbedTLS devs if they have a preference. In any case feel free to send this, since Tom picked up the patches already Thanks /Ilias > +} > > Regards, > Simon
Hi Ilias, On Wed, 13 Nov 2024 at 09:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Wed, 13 Nov 2024 at 18:04, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Wed, 13 Nov 2024 at 08:11, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > > > > On Wed, 13 Nov 2024 at 17:09, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > Hi Ilias, > > > > > > > > On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > HI Ilias, > > > > > > > > > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > Thanks Simon, > > > > > > > > > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <sjg@chromium.org> wrote: > > > > > > > > > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas > > > > > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > > > > > > > > > With the recent changes of lwip & mbedTLS we can now download from > > > > > > > > > https:// urls instead of just http://. > > > > > > > > > Adjust our wget lwip version parsing to support both URLs. > > > > > > > > > While at it adjust the default TCP window for QEMU since https seems to > > > > > > > > > require at least 16384 > > > > > > > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > > > > --- > > > > > > > > > cmd/Kconfig | 19 +++++++++++ > > > > > > > > > net/lwip/Kconfig | 2 +- > > > > > > > > > net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- > > > > > > > > > 3 files changed, 97 insertions(+), 10 deletions(-) > > > > > > > > > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org> > > > > > > > > > > > > > > > > Some nits / questions > > > > > > > > > > > > > > > > I am not sure what mbedtls_hardware_poll() is, > > > > > > > > > > > > > > It's an entropy collector used by mbedTLS to ensure the platform has > > > > > > > enough entropy. > > > > > > > It's required if your platform doesn't support standards like the > > > > > > > /dev/urandom or Windows CryptoAPI. > > > > > > > > > > > > > > > but if @len is too > > > > > > > > short, would it be acceptable to return an error? How many bytes is it > > > > > > > > requesting in the https case? > > > > > > > > > > > > > > If you don't return enough entropy https:// will fail and mbedTLS & > > > > > > > lwIP will print an error. I think we currently use 128 and the default > > > > > > > for mbedTLS is 32. > > > > > > > > > > > > OK, then the code is quite strange to me. It seems like it should > > > > > > check that 'len' is large enough. > > > > > > > > > > > > But what does 'len' actually mean? Its arguments are not described in > > > > > > mbedtls. Shouldn't you just pass it on to the dm_rng_read() function? > > > > > > > > > > The entry point is mbedtls_entropy_func(). mbedtls then calls > > > > > entropy_gather_internal() which asks for some bytes of entropy and is > > > > > controlled by a config option (and defaults to 128 for the config we > > > > > use in U-Boot -- MBEDTLS_ENTROPY_MAX_GATHER). len is actually > > > > > MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > > > > > > Why call it with only 8 bytes? It might be more bytes than is > > > > > > requested (and larger than the buffer), or fewer. > > > > > > > > > > It doesn't matter because we copy back the correct amount of > > > > > bytes(what the caller requested). If it's less mbedTLS calls that > > > > > function until it gathers all requested entropy. > > > > > > > > > > > It seems that your > > > > > > function is written with knowledge of the internals of mbedtls. > > > > > > > > > > Ofc it is. It's a function needed by mbedTLS -- not U-Boot, it > > > > > requires internal knowledge of it (and is probably part of the ABI, > > > > > but I'll have to check that). > > > > > > > > > > What happens in the TLS case is that 64b are required. We either make > > > > > 8 calls of 8b or make a single call with MBEDTLS_ENTROPY_MAX_GATHER. > > > > > > > > > > I could rewrite it as > > > > > uclass_first_device(UCLASS_RNG, &dev); > > > > > if (!dev) { > > > > > log_err("Failed to get an rng device\n"); > > > > > return ret; > > > > > } > > > > > > > > > > rng = malloc(len); > > > > > if (!rng) > > > > > return -ENOMEM; > > > > > > > > > > ret = dm_rng_read(dev, rng, len); > > > > > if (ret) { > > > > > free(rng); > > > > > return ret; > > > > > } > > > > > > > > > > memcpy(output, rng, len); > > > > > *olen = len; > > > > > free(rng); > > > > > > > > > > It does the same thing but gets 128b in one go. I don't have a strong > > > > > opinion on that. Let me know what you prefer > > > > > The tradeoff seems pretty straightforward. you either gather > > > > > potentially more entropy than required and call that function once, or > > > > > you gather exactly what's required on 8b steps -- but call that > > > > > function multiple times. > > > > > > > > > > > > > > > > > BTW mbedtls_hardware_poll() is a strange name as it actually reads > > > > > > random data, so I think it should be renamed. > > > > > > > > > > I don't think that can't change as you would break all projects using > > > > > it, but you can try sending a patch. > > > > > > > > I am wondering why we can't just do: > > > > > > > > ret = dm_rng_read(dev, rng, len); > > > > if (ret) > > > > return ret; > > > > > > > > *olen = len; > > > > > > I am not sure I am following. Do that were? > > > > Oh, I had that wrong, so very confusing, sorry. Something like this: > > > > +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len, > > + size_t *olen) > > +{ > > + struct udevice *dev; > > + int ret; > > + > > + ret = uclass_get_device(UCLASS_RNG, 0, &dev); > > + if (ret) { > > + log_err("Failed to get an rng: %d\n", ret); > > + return ret; > > + } > > + ret = dm_rng_read(dev, output, len); > > + if (ret) > > + return ret; > > + > > + *olen = len; > > + > > + return 0; > > Yep, that's identical to what I had above without the allocation, > which indeed isn't needed. > Both of the versions are correct and I ask internally mbedTLS devs if > they have a preference. > > In any case feel free to send this, since Tom picked up the patches already OK. It will be interesting to see if coverity picks this up. Regards, Simon
diff --git a/cmd/Kconfig b/cmd/Kconfig index 636833646f6e..b2d0348fe309 100644 --- a/cmd/Kconfig +++ b/cmd/Kconfig @@ -2124,6 +2124,25 @@ config CMD_WGET wget is a simple command to download kernel, or other files, from a http server over TCP. +config WGET_HTTPS + bool "wget https" + depends on CMD_WGET + depends on PROT_TCP_LWIP + depends on MBEDTLS_LIB + select SHA256 + select RSA + select ASYMMETRIC_KEY_TYPE + select ASYMMETRIC_PUBLIC_KEY_SUBTYPE + select X509_CERTIFICATE_PARSER + select PKCS7_MESSAGE_PARSER + select MBEDTLS_LIB_CRYPTO + select MBEDTLS_LIB_TLS + select RSA_VERIFY_WITH_PKEY + select X509_CERTIFICATE_PARSER + select PKCS7_MESSAGE_PARSER + help + Enable TLS over http for wget. + endif # if CMD_NET config CMD_PXE diff --git a/net/lwip/Kconfig b/net/lwip/Kconfig index 8a67de4cf335..a9ae9bf7fa2a 100644 --- a/net/lwip/Kconfig +++ b/net/lwip/Kconfig @@ -37,7 +37,7 @@ config PROT_UDP_LWIP config LWIP_TCP_WND int "Value of TCP_WND" - default 8000 if ARCH_QEMU + default 32768 if ARCH_QEMU default 3000000 help Default value for TCP_WND in the lwIP configuration diff --git a/net/lwip/wget.c b/net/lwip/wget.c index b495ebd1aa96..ba8579899002 100644 --- a/net/lwip/wget.c +++ b/net/lwip/wget.c @@ -7,13 +7,17 @@ #include <efi_loader.h> #include <image.h> #include <lwip/apps/http_client.h> +#include "lwip/altcp_tls.h" #include <lwip/timeouts.h> +#include <rng.h> #include <mapmem.h> #include <net.h> #include <time.h> +#include <dm/uclass.h> #define SERVER_NAME_SIZE 200 #define HTTP_PORT_DEFAULT 80 +#define HTTPS_PORT_DEFAULT 443 #define PROGRESS_PRINT_STEP_BYTES (100 * 1024) enum done_state { @@ -32,18 +36,56 @@ struct wget_ctx { enum done_state done; }; -static int parse_url(char *url, char *host, u16 *port, char **path) +bool wget_validate_uri(char *uri); + +int mbedtls_hardware_poll(void *data, unsigned char *output, size_t len, + size_t *olen) +{ + struct udevice *dev; + u64 rng = 0; + int ret; + + *olen = 0; + + ret = uclass_get_device(UCLASS_RNG, 0, &dev); + if (ret) { + log_err("Failed to get an rng: %d\n", ret); + return ret; + } + ret = dm_rng_read(dev, &rng, sizeof(rng)); + if (ret) + return ret; + + memcpy(output, &rng, len); + *olen = sizeof(rng); + + return 0; +} + +static int parse_url(char *url, char *host, u16 *port, char **path, + bool *is_https) { char *p, *pp; long lport; + size_t prefix_len = 0; + + if (!wget_validate_uri(url)) { + log_err("Invalid URL. Use http(s)://\n"); + return -EINVAL; + } + *is_https = false; + *port = HTTP_PORT_DEFAULT; + prefix_len = strlen("http://"); p = strstr(url, "http://"); if (!p) { - log_err("only http:// is supported\n"); - return -EINVAL; + p = strstr(url, "https://"); + prefix_len = strlen("https://"); + *port = HTTPS_PORT_DEFAULT; + *is_https = true; } - p += strlen("http://"); + p += prefix_len; /* Parse hostname */ pp = strchr(p, ':'); @@ -67,9 +109,8 @@ static int parse_url(char *url, char *host, u16 *port, char **path) if (lport > 65535) return -EINVAL; *port = (u16)lport; - } else { - *port = HTTP_PORT_DEFAULT; } + if (*pp != '/') return -EINVAL; *path = pp; @@ -210,12 +251,16 @@ static void httpc_result_cb(void *arg, httpc_result_t httpc_result, static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri) { char server_name[SERVER_NAME_SIZE]; +#if defined CONFIG_WGET_HTTPS + altcp_allocator_t tls_allocator; +#endif httpc_connection_t conn; httpc_state_t *state; struct netif *netif; struct wget_ctx ctx; char *path; u16 port; + bool is_https; ctx.daddr = dst_addr; ctx.saved_daddr = dst_addr; @@ -224,7 +269,7 @@ static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri) ctx.prevsize = 0; ctx.start_time = 0; - if (parse_url(uri, server_name, &port, &path)) + if (parse_url(uri, server_name, &port, &path, &is_https)) return CMD_RET_USAGE; netif = net_lwip_new_netif(udev); @@ -232,6 +277,22 @@ static int wget_loop(struct udevice *udev, ulong dst_addr, char *uri) return -1; memset(&conn, 0, sizeof(conn)); +#if defined CONFIG_WGET_HTTPS + if (is_https) { + tls_allocator.alloc = &altcp_tls_alloc; + tls_allocator.arg = + altcp_tls_create_config_client(NULL, 0, server_name); + + if (!tls_allocator.arg) { + log_err("error: Cannot create a TLS connection\n"); + net_lwip_remove_netif(netif); + return -1; + } + + conn.altcp_allocator = &tls_allocator; + } +#endif + conn.result_fn = httpc_result_cb; ctx.path = path; if (httpc_get_file_dns(server_name, port, path, &conn, httpc_recv_cb, @@ -316,6 +377,7 @@ bool wget_validate_uri(char *uri) char c; bool ret = true; char *str_copy, *s, *authority; + size_t prefix_len = 0; for (c = 0x1; c < 0x21; c++) { if (strchr(uri, c)) { @@ -323,15 +385,21 @@ bool wget_validate_uri(char *uri) return false; } } + if (strchr(uri, 0x7f)) { log_err("invalid character is used\n"); return false; } - if (strncmp(uri, "http://", 7)) { - log_err("only http:// is supported\n"); + if (!strncmp(uri, "http://", strlen("http://"))) { + prefix_len = strlen("http://"); + } else if (!strncmp(uri, "https://", strlen("https://"))) { + prefix_len = strlen("https://"); + } else { + log_err("only http(s):// is supported\n"); return false; } + str_copy = strdup(uri); if (!str_copy) return false;
With the recent changes of lwip & mbedTLS we can now download from https:// urls instead of just http://. Adjust our wget lwip version parsing to support both URLs. While at it adjust the default TCP window for QEMU since https seems to require at least 16384 Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- cmd/Kconfig | 19 +++++++++++ net/lwip/Kconfig | 2 +- net/lwip/wget.c | 86 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 97 insertions(+), 10 deletions(-)