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 = §ions[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
