[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.su at 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