On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny <o...@linux.microsoft.com> wrote: > > Hi Ard, > > On 3/1/2024 3:58 AM, Ard Biesheuvel wrote: > > Hi Oliver, > > > > On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny > > <o...@linux.microsoft.com> wrote: > >> > >> When an ImageRecord is stored by ImagePropertiesRecordLib, it reports the > >> CodeSegmentSize as the SizeOfRawData from the image. However, the image > >> as loaded into memory is aligned to the SectionAlignment, so > >> SizeOfRawData is under the actual size in memory. This is important, > >> because the memory attributes table uses these image records to create > >> its entries and it will report that the alignment of an image is > >> incorrect, even though the actual image is correct. > >> > >> This was discovered on ARM64, which has a 64k runtime page granularity > >> alignment, which is backed by a 64k section alignment for > >> DXE_RUNTIME_DRIVERs. The runtime code and data was correctly being > >> loaded into memory, however the memory attribute table was incorrectly > >> reporting misaligned ranges to the OS, causing attributes to be > >> ignored for these sections for OSes using greater than 4k pages. > >> > >> This patch correctly aligns the CodeSegmentSize to the SectionAlignment > >> and the corresponding memory attribute table entries are now correctly > >> aligned and pointing to the right places in memory. > >> > > > > Can you explain how these can differ in the first place? Our flaky > > ELF-to-PE/COFF converter should never generate such images to begin > > with (which is probably how we ended up with this problem in the first > > place), so I suppose this is native PE/COFF tooling emitting sections > > either using a non-1:1 file:memory mapping, or with unallocated holes > > in the file representation? > > > > This is an artifact of PE/COFF itself and is useful for having smaller > FW images. In PE/COFF we have the section alignment (which is how we get > loaded into memory) and the file alignment (how the actual file is > aligned). It is valid for these two to be different. For example, these > runtime DXE drivers, which are not XIP (which the case we do need the > section and file alignment to be the same, as we are executing from > the file image) can be a smaller size in the file, but when loaded into > memory we will relocate them and do the proper rebasing to set these on > 64k boundaries. Different toolchains have different ways of specifying > the two things, on gcc I have seen common-page-size affect the section > alignment and max-page-size affect both section and file alignment. For > msvc there are /ALIGN and /FILEALIGN commands which directly manipulate > these values. > > The issue here was not that we have different section and file > alignment, in fact, the issue still exists if section alignement == > file alignment. This is because SizeOfRawData in the PE/COFF header is > the raw size of bytes used, not even page aligned. So no matter what, > the image records we were creating here had bad sizes being set which > do not match what the image loader was actually doing. >
IIUC the SizeOfRawData is the file view not the memory view, and must always be aligned to the FileAlignment. > I do think there is a fair argument that would say the image loader > should create the image records when it loads images, since in the > end we want the record to match exactly what the image in memory is, > creating after the fact is a poor pattern. > Yeah, no disagreement there. > >> Cc: Liming Gao <gaolim...@byosoft.com.cn> > >> Cc: Leif Lindholm <quic_llind...@quicinc.com> > >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> > >> Cc: Sami Mujawar <sami.muja...@arm.com> > >> Cc: Taylor Beebe <taylor.d.be...@gmail.com> > >> > >> Signed-off-by: Oliver Smith-Denny <o...@linux.microsoft.com> > >> --- > >> MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git > >> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> index e53ce086c54c..07ced0e54e38 100644 > >> --- > >> a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> +++ > >> b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c > >> @@ -1090,7 +1090,7 @@ CreateImagePropertiesRecord ( > >> ImageRecordCodeSection->Signature = > >> IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; > >> > >> ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageBase + > >> Section[Index].VirtualAddress; > >> - ImageRecordCodeSection->CodeSegmentSize = > >> Section[Index].SizeOfRawData; > >> + ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE > >> (Section[Index].SizeOfRawData, SectionAlignment); > >> > > > > This should be the virtual size, not the file size, right? > > Correct, SectionAlignment is the alignment of the image as loaded in > memory, so in the case of a DXE runtime driver on ARM64, it will be > 64k. > No, I mean we should not be using SizeOfRawData here but VirtualSize. I understand this is unlikely to make a difference in practice, but VirtualSize may exceed SizeOfRawData by a wide margin, and we need the whole region to be covered. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116338): https://edk2.groups.io/g/devel/message/116338 Mute This Topic: https://groups.io/mt/104610770/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
