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...

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