Re: [PATCH] efi_loader: don't load beyond VirtualSize

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] efi_loader: don't load beyond VirtualSize

Heinrich Schuchardt
Am 9. Februar 2021 07:19:48 MEZ schrieb Asherah Connor <[hidden email]>:
>PE section table entries' SizeOfRawData must be a multiple of
>FileAlignment, and thus may be rounded up and larger than their
>VirtualSize.
>
>We should not load beyond the VirtualSize, which is "the total size of
>the section when loaded into memory" -- we may clobber real data at the
>target in some other section, since we load sections in reverse order
>and sections are usually laid out sequentially.

Thank you for reporting and addressing the issue.

Is this patch related to an observed problem or is it resulting from code review?

Should we load in forward order?

>
>Signed-off-by: Asherah Connor <[hidden email]>
>CC: Heinrich Schuchardt <[hidden email]>
>---
> 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 d4dd9e9433..f53ef367ec 100644
>--- a/lib/efi_loader/efi_image_loader.c
>+++ b/lib/efi_loader/efi_image_loader.c
>@@ -843,7 +843,7 @@ efi_status_t efi_load_pe(struct
>efi_loaded_image_obj *handle,
>       sec->Misc.VirtualSize);
> memcpy(efi_reloc + sec->VirtualAddress,
>       efi + sec->PointerToRawData,
>-       sec->SizeOfRawData);
>+       min(sec->Misc.VirtualSize, sec->SizeOfRawData));
> }

If SizeOfRawData must be >= VirtualSize, why do we have to consider both fields?

Best regards

Heinrich


>
> /* Run through relocations */

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] efi_loader: don't load beyond VirtualSize

Asherah Connor
Hi Heinrich.  Thanks for the review!

On 21/02/09 07:02:p, Heinrich Schuchardt wrote:
> Thank you for reporting and addressing the issue.
>
> Is this patch related to an observed problem or is it resulting from
> code review?

Yes, this was seen in action (and took quite a bit of logging and
debugging to understand).  In my case, we have an EFI application with
the following sections:

.text: virt 0x00000200 len 0000364c -- phys 0x00000200 len 00003800
.data: virt 0x00003860 len 00001a75 -- phys 0x00003a00 len 00001c00
.reloc: virt 0x000052e0 len 00000068 -- phys 0x00005600 len 00000200

Note the physical lengths (SizeOfRawData) are all rounded up to the
nearest 0x200 (512 bytes), the usual FileAlignment.  But the virtual
lengths are all smaller.  When U-Boot loaded these sections, it loaded
.reloc first, then .data, then .text.  During loading of .data, it
copied 1c00 bytes even though there are only actually 1a75 bytes of real
data to copy. The rest is just padding in the file.

Those extra bytes overlap with the start of .reloc, and the result was
U-Boot simply performing no relocations at all (since it zeroed out the
start of the section).  I observed QEMU (with EDK2) correctly accessing
relocated data and a Rockpro64 (on U-Boot) trying to access
non-relocated addresses and hitting synchronous aborts instead.  This
patch fixes the relocating issue by ensuring .reloc isn't clobbered.

> Should we load in forward order?

We can, and it might be enough.  There's a chance someone someday will
generate a PE32+ with sections in non-sequential order anyway.

This would be non-standard, so it's a small chance, but I feel the
better solution is to only load bytes that fit within the size of
VirtualSize, because those are the only bytes that are meant to be
loaded.

> > memcpy(efi_reloc + sec->VirtualAddress,
> >       efi + sec->PointerToRawData,
> >-       sec->SizeOfRawData);
> >+       min(sec->Misc.VirtualSize, sec->SizeOfRawData));
> > }
>
> If SizeOfRawData must be >= VirtualSize, why do we have to consider
> both fields?

It can be smaller -- the patch didn't show the full context including
the memset before:

        for (i = num_sections - 1; i >= 0; i--) {
                IMAGE_SECTION_HEADER *sec = &sections[i];
                memset(efi_reloc + sec->VirtualAddress, 0,
                       sec->Misc.VirtualSize);
                memcpy(efi_reloc + sec->VirtualAddress,
                       efi + sec->PointerToRawData,
-       sec->SizeOfRawData);
+       min(sec->Misc.VirtualSize, sec->SizeOfRawData));
        }

In other words:

* if VirtualSize = SizeOfRawData, copy exactly that many bytes.
* if VirtualSize > SizeOfRawData, copy SizeOfRawData bytes, and pad the
  remaining space (from SizeOfRawData up until VirtualSize) with zeroes.
  This is used for bss-style zero-initialised data.

And new in this patch:

* if VirtualSize < SizeOfRawData, we should only copy VirtualSize bytes,
  and no more.

This appears to be the authority on the definitions of these fields:
https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#section-table-section-headers

VirtualSize is documented as follows:

> The total size of the section when loaded into memory. If this value
> is greater than SizeOfRawData, the section is zero-padded.

Below, SizeOfRawData has this comment:

> Because the SizeOfRawData field is rounded but the VirtualSize field
> is not, it is possible for SizeOfRawData to be greater than
> VirtualSize as well.

This alerts us to the fact that we shouldn't copy SizeOfRawData bytes
without considering VirtualSize first -- if the total size of the
section in memory is smaller than this (because it was rounded up to the
nearest 512 bytes), we shouldn't copy beyond it.  Nothing beyond
VirtualSize is meant to be exposed, and it affects our other sections
here.

We could paper over this by loading the sections in forward-order, but
we'd still be writing bytes past the end of sections and possibly
causing issues for ourselves later, so I think this patch represents the
most accurate solution.

All the best,

Asherah
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] efi_loader: don't load beyond VirtualSize

Heinrich Schuchardt
In reply to this post by Heinrich Schuchardt
On 2/9/21 7:48 AM, Heinrich Schuchardt wrote:

> Am 9. Februar 2021 07:19:48 MEZ schrieb Asherah Connor <[hidden email]>:
>> PE section table entries' SizeOfRawData must be a multiple of
>> FileAlignment, and thus may be rounded up and larger than their
>> VirtualSize.
>>
>> We should not load beyond the VirtualSize, which is "the total size of
>> the section when loaded into memory" -- we may clobber real data at the
>> target in some other section, since we load sections in reverse order
>> and sections are usually laid out sequentially.
>
> Thank you for reporting and addressing the issue.
>
> Is this patch related to an observed problem or is it resulting from code review?
>
> Should we load in forward order?
>
>>
>> Signed-off-by: Asherah Connor <[hidden email]>
>> CC: Heinrich Schuchardt <[hidden email]>
>> ---
>> 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 d4dd9e9433..f53ef367ec 100644
>> --- a/lib/efi_loader/efi_image_loader.c
>> +++ b/lib/efi_loader/efi_image_loader.c
>> @@ -843,7 +843,7 @@ efi_status_t efi_load_pe(struct
>> efi_loaded_image_obj *handle,
>>       sec->Misc.VirtualSize);
>> memcpy(efi_reloc + sec->VirtualAddress,
>>       efi + sec->PointerToRawData,
>> -       sec->SizeOfRawData);
>> +       min(sec->Misc.VirtualSize, sec->SizeOfRawData));
>> }
>
> If SizeOfRawData must be >= VirtualSize, why do we have to consider both fields?

The PE-COFF spec [1] says:

SizeOfRawData

The size of the section (for object files) or the size of the
initialized data on disk (for image files). For executable images, this
must be a multiple of FileAlignment from the optional header. If this is
less than VirtualSize, the remainder of the section is zero-filled.
Because the SizeOfRawData field is rounded but the VirtualSize field is
not, it is possible for SizeOfRawData to be greater than VirtualSize as
well. When a section contains only uninitialized data, this field should
be zero.

So SizeOfRawData can be both smaller and greater then VirtualSize.

We zero the memory before copying the data. This covers the case
SizeOfRawData < VirtualSize.

Reviewed-by: Heinrich Schuchardt <[hidden email]>

[1] https://docs.microsoft.com/en-us/windows/win32/debug/pe-format

>
> Best regards
>
> Heinrich
>
>
>>
>> /* Run through relocations */
>