[PATCH v2] efi_loader: Fix memory corruption on 32bit systems

[Simon Glass]

Hi,

On Thu, 27 Jul 2023 at 08:36, Dan Carpenter <dan.carpenter at linaro.org> wrote:
>
> On Thu, Jul 27, 2023 at 11:22:15AM +0300, Ilias Apalodimas wrote:
> > Hi Dan,
> >
> > [...]
> >
> > > @@ -313,7 +313,7 @@ static int cmp_pe_section(const void *arg1, const void *arg2)
> > >   *
> > >   * Return: valid pointer to a image, return NULL if allocation fails.
> > >   */
> > > -void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
> > > +void *efi_prepare_aligned_image(void *efi, size_t *efi_size)
> > >  {
> > >     size_t new_efi_size;
> > >     void *new_efi;
> > > @@ -600,7 +600,7 @@ static bool efi_image_authenticate(void *efi, size_t efi_size)
> > >     if (!efi_secure_boot_enabled())
> > >             return true;
> > >
> > > -   new_efi = efi_prepare_aligned_image(efi, (u64 *)&efi_size);
> > > +   new_efi = efi_prepare_aligned_image(efi, &efi_size);
> > >     if (!new_efi)
> > >             return false;
> > >
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index 49f8a5e77cbf..d57afd0c498b 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -882,7 +882,7 @@ out:
> > >   *
> > >   * Return: status code
> > >   */
> > > -static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
> > > +static efi_status_t tcg2_hash_pe_image(void *efi, size_t efi_size,
> > >                                    struct tpml_digest_values *digest_list)
> >
> > Unfortunately the rabbit hole is a bit deeper with this one.
> > tcg2_hash_pe_image() is called in
> > - tcg2_measure_pe_image(). This one is called in efi_load_pe() and the type
> >   is indeed a size_t there, so that's fine
> > - efi_tcg2_hash_log_extend_event(), this one is different...
> > The function is described by the EFI spec [0] which mandates a u64... I
> > think that was the reason efi_prepare_aligned_image() is using a u64 to
> > begin with.  This one uses the size only though not the pointer, but in a
> > 32bit platform it would truncate s size > UINT_MAX.
> >
> > [0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
>
> I have maybe misread something...  I don't think this is a real issue.
> 32bit systems aren't going to be able to allocate that much memory
> anyway.  Also there are a lot of size_t parameters already so it's not
> a new issue.

We should really use ulong for addresses and malloc() sizes.

Regards,
Simon