Re: [PATCH] efi_loader: Allow overlapped extra data for PE hashing

[Su, Bao Cheng]

On Mon, 2022-06-27 at 16:32 +0200, Heinrich Schuchardt wrote:
> On 6/24/22 07:32, Su, Bao Cheng wrote:
> > During PE hashing, when holes exists between sections, the extra
> > data
> > calculated could be a dupulicated region of the last section.
> > 
> > Such PE image with holes existing between sections may contain the
> > symbol table for the kernel, for example.
> > 
> > The Authenticode_PE spec does not rule how to deal with such
> > scenario,
> > however, other tools such as pesign and sbsign both have the
> > overlapped
> > regions hashed. And EDK2 hash the overlapped area as well.
> > 
> > Signed-off-by: Baocheng Su <baocheng...@siemens.com>
> > ---
> >   lib/efi_loader/efi_image_loader.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_image_loader.c
> > b/lib/efi_loader/efi_image_loader.c
> > index 9611398885..d85fb6ba08 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -481,7 +481,7 @@ bool efi_image_parse(void *efi, size_t len,
> > struct
> > efi_image_regions **regp,
> >                 EFI_PRINT("extra data for hash: %zu\n",
> >                           len - (bytes_hashed + authsz));
> >                 efi_image_region_add(regs, efi + bytes_hashed,
> > -                                    efi + len - authsz, 0);
> > +                                    efi + len - authsz, 1);
> >         }
> > 
> >         /* Return Certificates Table */
> 
> Let us consider the case that the sum of gaps between sections is
> greater than the size of the last section N.
> 
> start[N] > efi + bytes_hashed
> end[N] < efi + len - authsz
> 
> Sbsigntool and EDK II sort regions by start address before adding the
> extra data region and will accept this situation.
> 
> U-Boot's efi_image_region_add(nocheck = 1) will throw an error "%s:
> new
> region already part of another\n".
> 

This is the original code of efi_image_region_add:

```
        for (i = 0; i < regs->num; i++) {
                reg = &regs->reg[i];
                if (nocheck)
                        continue;

                /* new data after registered region */
                if (start >= reg->data + reg->size)
                        continue;

                /* new data preceding registered region */
                if (end <= reg->data) {
                        for (j = regs->num - 1; j >= i; j--)
                                memcpy(&regs->reg[j + 1], &regs-
>reg[j],
                                       sizeof(*reg));
                        break;
                }

                /* new data overlapping registered region */
                EFI_PRINT("%s: new region already part of another\n",
__func__);
                return EFI_INVALID_PARAMETER;
        }
```

Notice the `if (nocheck) continue;`, I would not say the `new region
already part of another` be executed.

- Baocheng

> It seems that this patch is not a complete solution.
> 
> Best regards
> 
> Heinrich