On Thu, 14 Nov 2024 at 05:53, Simon Glass <s...@chromium.org> wrote:
>
> Hi Ilias,
>
> On Wed, 13 Nov 2024 at 09:11, Ilias Apalodimas
> <ilias.apalodi...@linaro.org> wrote:
> >
> > On Wed, 13 Nov 2024 at 18:04, Simon Glass <s...@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Wed, 13 Nov 2024 at 08:11, Ilias Apalodimas
> > > <ilias.apalodi...@linaro.org> wrote:
> > > >
> > > > On Wed, 13 Nov 2024 at 17:09, Simon Glass <s...@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Wed, 13 Nov 2024 at 07:46, Ilias Apalodimas
> > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 13 Nov 2024 at 15:39, Simon Glass <s...@chromium.org> wrote:
> > > > > > >
> > > > > > > HI Ilias,
> > > > > > >
> > > > > > > On Mon, 11 Nov 2024 at 07:20, Ilias Apalodimas
> > > > > > > <ilias.apalodi...@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Thanks Simon,
> > > > > > > >
> > > > > > > > On Mon, 11 Nov 2024 at 15:03, Simon Glass <s...@chromium.org>
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Sun, 10 Nov 2024 at 01:32, Ilias Apalodimas
> > > > > > > > > <ilias.apalodi...@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.apalodi...@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 <s...@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.
Pick up what ? There's nothing wrong with the merged code. It does
gathers entropy in rounds of 8b
Thanks
/Ilias
>
> Regards,
> Simon
