* [edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes @ 2024-02-27 20:27 Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Oliver Smith-Denny ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-02-27 20:27 UTC (permalink / raw) To: devel; +Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe ImagePropertiesRecordLib is currently creating Image Records that are not accurate. It is setting the CodeSegmentSize to be the size of the raw data in the image file, however, when the image is loaded into memory, the raw data size is aligned to the section alignment. This caused the memory attributes table to have incorrect entries for systems, like ARM64, where the section alignment is not 4k for all modules. In fixing this, I noticed that MemoryProtection.c is using its own version of image record creation where this logic was actually correct. ImagePropertiesRecordLib was created to consolidate the logic around creating and managing image records, so this patchset also updates MemoryProtection.c to use ImagePropertiesRecordsLib after making a few small adjustments to ensure the same functionality is present. This patchset was tested on ArmVirtQemu to ensure that all image records were the same before and after this, other than fixing the CodeSegmentSize. Github PR: https://github.com/tianocore/edk2/pull/5402 Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> Cc: Sami Mujawar <sami.mujawar@arm.com> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> Oliver Smith-Denny (3): MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++----------------- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 84 +++++-- 2 files changed, 92 insertions(+), 233 deletions(-) -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116059): https://edk2.groups.io/g/devel/message/116059 Mute This Topic: https://groups.io/mt/104610769/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-02-27 20:27 [edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes Oliver Smith-Denny @ 2024-02-27 20:27 ` Oliver Smith-Denny 2024-03-01 11:58 ` Ard Biesheuvel 2024-02-27 20:27 ` [edk2-devel][PATCH v1 2/3] MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Oliver Smith-Denny 2 siblings, 1 reply; 12+ messages in thread From: Oliver Smith-Denny @ 2024-02-27 20:27 UTC (permalink / raw) To: devel; +Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe 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. Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Leif Lindholm <quic_llindhol@quicinc.com> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> Cc: Sami Mujawar <sami.mujawar@arm.com> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> Signed-off-by: Oliver Smith-Denny <osde@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); InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link); ImageRecord->CodeSegmentCount++; -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116060): https://edk2.groups.io/g/devel/message/116060 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-02-27 20:27 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Oliver Smith-Denny @ 2024-03-01 11:58 ` Ard Biesheuvel 2024-03-04 17:49 ` Oliver Smith-Denny 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2024-03-01 11:58 UTC (permalink / raw) To: Oliver Smith-Denny Cc: devel, Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe Hi Oliver, On Tue, 27 Feb 2024 at 21:27, Oliver Smith-Denny <osde@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? > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Leif Lindholm <quic_llindhol@quicinc.com> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> > Cc: Sami Mujawar <sami.mujawar@arm.com> > Cc: Taylor Beebe <taylor.d.beebe@gmail.com> > > Signed-off-by: Oliver Smith-Denny <osde@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? > InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link); > ImageRecord->CodeSegmentCount++; > -- > 2.40.1 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116237): https://edk2.groups.io/g/devel/message/116237 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-01 11:58 ` Ard Biesheuvel @ 2024-03-04 17:49 ` Oliver Smith-Denny 2024-03-04 18:54 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Oliver Smith-Denny @ 2024-03-04 17:49 UTC (permalink / raw) To: Ard Biesheuvel Cc: devel, Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe 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 > <osde@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. 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. >> Cc: Liming Gao <gaoliming@byosoft.com.cn> >> Cc: Leif Lindholm <quic_llindhol@quicinc.com> >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> >> Cc: Sami Mujawar <sami.mujawar@arm.com> >> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> >> >> Signed-off-by: Oliver Smith-Denny <osde@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. > >> InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link); >> ImageRecord->CodeSegmentCount++; >> -- >> 2.40.1 >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116334): https://edk2.groups.io/g/devel/message/116334 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-04 17:49 ` Oliver Smith-Denny @ 2024-03-04 18:54 ` Ard Biesheuvel 2024-03-04 19:24 ` Oliver Smith-Denny [not found] ` <17B9A63355E03AE5.30946@groups.io> 0 siblings, 2 replies; 12+ messages in thread From: Ard Biesheuvel @ 2024-03-04 18:54 UTC (permalink / raw) To: Oliver Smith-Denny Cc: devel, Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny <osde@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 > > <osde@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 <gaoliming@byosoft.com.cn> > >> Cc: Leif Lindholm <quic_llindhol@quicinc.com> > >> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> > >> Cc: Sami Mujawar <sami.mujawar@arm.com> > >> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> > >> > >> Signed-off-by: Oliver Smith-Denny <osde@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/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-04 18:54 ` Ard Biesheuvel @ 2024-03-04 19:24 ` Oliver Smith-Denny [not found] ` <17B9A63355E03AE5.30946@groups.io> 1 sibling, 0 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-03-04 19:24 UTC (permalink / raw) To: devel, ardb Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: > On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny > <osde@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 >>> <osde@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 relooked at the spec, you are correct, SizeOfRawData is aligned to the FileAlignment. So this is only a bug for a file with SectionAlignment != 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 <gaoliming@byosoft.com.cn> >>>> Cc: Leif Lindholm <quic_llindhol@quicinc.com> >>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org> >>>> Cc: Sami Mujawar <sami.mujawar@arm.com> >>>> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> >>>> >>>> Signed-off-by: Oliver Smith-Denny <osde@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. > I see, yes I do believe VirtualSize could be used here instead. Two things give me pause. One is that the PE spec states that SizeOfRawData is rounded and VirtualSize is not, so that SizeOfRawData may be greater than the VirtualSize in some cases (which seems incorrect). The other is that the image loader partially uses VirtualSize when loading: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 However, when determining the size of a loaded image (and therefore the number of pages to allocate) it will allocate an extra page: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 as ImageSize here is: https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 Which according to the spec, SizeOfImage is the size of the image as loaded into memory and must be a multiple of section alignment. So, reflecting on this, let me test with VirtualSize here, I think that is the right value to use, the only time we would have a SizeOfRawData that is greater than the VirtualSize would be if our FileAlignment is greater than our SectionAlignment, which would be a misconfiguration. I do think the ImageLoader should also be fixed to only allocate ImageSize number of pages (which should be the sum of the section VirtualSizes + any headers that aren't included). Might as well not waste an extra page for each image and then our image record code is simpler as well (ensuring we are protecting the right pages). I think this patch series should go in, as it is fixing an active bug, but I will also take a look at the image loader creating the image records and having a protocol it produces to retrieve the list, to attempt to avoid issues like this in the future. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116340): https://edk2.groups.io/g/devel/message/116340 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <17B9A63355E03AE5.30946@groups.io>]
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize [not found] ` <17B9A63355E03AE5.30946@groups.io> @ 2024-03-04 22:38 ` Oliver Smith-Denny [not found] ` <17B9B0CE1BDDC06B.30946@groups.io> 1 sibling, 0 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-03-04 22:38 UTC (permalink / raw) To: devel, ardb Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: > On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: >> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny >> <osde@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 >>>> <osde@linux.microsoft.com> wrote: >>>>> >>>>> - 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. >> > > I see, yes I do believe VirtualSize could be used here instead. Two > things give me pause. One is that the PE spec states that SizeOfRawData > is rounded and VirtualSize is not, so that SizeOfRawData may be greater > than the VirtualSize in some cases (which seems incorrect). > > The other is that the image loader partially uses VirtualSize when > loading: > > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 > > However, when determining the size of a loaded image (and therefore the > number of pages to allocate) it will allocate an extra page: > > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 > > as ImageSize here is: > > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 > > Which according to the spec, SizeOfImage is the size of the image as > loaded into memory and must be a multiple of section alignment. > > So, reflecting on this, let me test with VirtualSize here, I think > that is the right value to use, the only time we would have a > SizeOfRawData that is greater than the VirtualSize would be if our > FileAlignment is greater than our SectionAlignment, which would be > a misconfiguration. > > I do think the ImageLoader should also be fixed to only allocate > ImageSize number of pages (which should be the sum of the section > VirtualSizes + any headers that aren't included). Might as well not > waste an extra page for each image and then our image record code is > simpler as well (ensuring we are protecting the right pages). > > I think this patch series should go in, as it is fixing an active bug, > but I will also take a look at the image loader creating the image > records and having a protocol it produces to retrieve the list, to > attempt to avoid issues like this in the future. > Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. I am looking deeper into GenFw to make sure it is correctly getting set to align to SectionAlignment in the section headers. When I use dumpbin to dump the headers, it shows each section having VirtualSize as 64k aligned for a runtime image, but the same image doesn't show that in FW. I'll do some digging here. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116343): https://edk2.groups.io/g/devel/message/116343 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <17B9B0CE1BDDC06B.30946@groups.io>]
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize [not found] ` <17B9B0CE1BDDC06B.30946@groups.io> @ 2024-03-11 19:34 ` Oliver Smith-Denny 2024-03-12 8:32 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Oliver Smith-Denny @ 2024-03-11 19:34 UTC (permalink / raw) To: devel, ardb Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: > On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: >> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: >>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny >>> <osde@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 >>>>> <osde@linux.microsoft.com> wrote: >>>>>> >>>>>> - 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. >>> >> >> I see, yes I do believe VirtualSize could be used here instead. Two >> things give me pause. One is that the PE spec states that SizeOfRawData >> is rounded and VirtualSize is not, so that SizeOfRawData may be greater >> than the VirtualSize in some cases (which seems incorrect). >> >> The other is that the image loader partially uses VirtualSize when >> loading: >> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 >> >> However, when determining the size of a loaded image (and therefore the >> number of pages to allocate) it will allocate an extra page: >> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 >> >> as ImageSize here is: >> >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 >> >> Which according to the spec, SizeOfImage is the size of the image as >> loaded into memory and must be a multiple of section alignment. >> >> So, reflecting on this, let me test with VirtualSize here, I think >> that is the right value to use, the only time we would have a >> SizeOfRawData that is greater than the VirtualSize would be if our >> FileAlignment is greater than our SectionAlignment, which would be >> a misconfiguration. >> >> I do think the ImageLoader should also be fixed to only allocate >> ImageSize number of pages (which should be the sum of the section >> VirtualSizes + any headers that aren't included). Might as well not >> waste an extra page for each image and then our image record code is >> simpler as well (ensuring we are protecting the right pages). >> >> I think this patch series should go in, as it is fixing an active bug, >> but I will also take a look at the image loader creating the image >> records and having a protocol it produces to retrieve the list, to >> attempt to avoid issues like this in the future. >> > > Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. > I am looking deeper into GenFw to make sure it is correctly getting set > to align to SectionAlignment in the section headers. When I use dumpbin > to dump the headers, it shows each section having VirtualSize as 64k > aligned for a runtime image, but the same image doesn't show that in FW. > > I'll do some digging here. > Following up on this: Not surprisingly, different toolchains do different things here. gcc obviously creates ELF files and ElfConvert*.c converts these to PE images. However, when it does this, it always sets the section and file alignment to the same value (I'm not as familiar with ELF images, I'm not sure if there is the same concept as file vs section alignment). GenFw could probably update this information to shrink the file alignment, but that's a space optimization for gcc built binaries. In ElfConvert.c, the VirtualSize does get set to the section aligned value, which is what I would expect from the spec. In MSVC PE images, VirtualSize is not section aligned and the expectation appears to be that loaders will align the VirtualSize to the section alignment. I am planning on reaching out to the MSVC folks to learn why this is the case, is it intended, etc., as my understanding is that the VirtualSize in the section headers should be section aligned. We are stuck with this for existing MSVC toolchains, however, so we will need to align either the VirtualSize or SizeOfRawData to the section alignment when we create the image records. I don't have a preference between the two, they end up being the same when we align them, so I can send a v2 with aligning the VirtualSize. This will be a no-op on gcc built binaries and will set it to the correct value for MSVC. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116653): https://edk2.groups.io/g/devel/message/116653 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-11 19:34 ` Oliver Smith-Denny @ 2024-03-12 8:32 ` Ard Biesheuvel 2024-03-12 14:38 ` Oliver Smith-Denny 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2024-03-12 8:32 UTC (permalink / raw) To: devel, osde Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny <osde@linux.microsoft.com> wrote: > > On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: > > On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: > >> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: > >>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny > >>> <osde@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 > >>>>> <osde@linux.microsoft.com> wrote: > >>>>>> > >>>>>> - 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. > >>> > >> > >> I see, yes I do believe VirtualSize could be used here instead. Two > >> things give me pause. One is that the PE spec states that SizeOfRawData > >> is rounded and VirtualSize is not, so that SizeOfRawData may be greater > >> than the VirtualSize in some cases (which seems incorrect). > >> > >> The other is that the image loader partially uses VirtualSize when > >> loading: > >> > >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 > >> > >> However, when determining the size of a loaded image (and therefore the > >> number of pages to allocate) it will allocate an extra page: > >> > >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 > >> > >> as ImageSize here is: > >> > >> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 > >> > >> Which according to the spec, SizeOfImage is the size of the image as > >> loaded into memory and must be a multiple of section alignment. > >> > >> So, reflecting on this, let me test with VirtualSize here, I think > >> that is the right value to use, the only time we would have a > >> SizeOfRawData that is greater than the VirtualSize would be if our > >> FileAlignment is greater than our SectionAlignment, which would be > >> a misconfiguration. > >> > >> I do think the ImageLoader should also be fixed to only allocate > >> ImageSize number of pages (which should be the sum of the section > >> VirtualSizes + any headers that aren't included). Might as well not > >> waste an extra page for each image and then our image record code is > >> simpler as well (ensuring we are protecting the right pages). > >> > >> I think this patch series should go in, as it is fixing an active bug, > >> but I will also take a look at the image loader creating the image > >> records and having a protocol it produces to retrieve the list, to > >> attempt to avoid issues like this in the future. > >> > > > > Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. > > I am looking deeper into GenFw to make sure it is correctly getting set > > to align to SectionAlignment in the section headers. When I use dumpbin > > to dump the headers, it shows each section having VirtualSize as 64k > > aligned for a runtime image, but the same image doesn't show that in FW. > > > > I'll do some digging here. > > > > Following up on this: > > Not surprisingly, different toolchains do different things here. > > gcc obviously creates ELF files and ElfConvert*.c converts these to PE > images. However, when it does this, it always sets the section and file > alignment to the same value (I'm not as familiar with ELF images, I'm > not sure if there is the same concept as file vs section alignment). > GenFw could probably update this information to shrink the file > alignment, but that's a space optimization for gcc built binaries. > > In ElfConvert.c, the VirtualSize does get set to the section aligned > value, which is what I would expect from the spec. > > In MSVC PE images, VirtualSize is not section aligned and the > expectation appears to be that loaders will align the VirtualSize > to the section alignment. I am planning on reaching out to the MSVC > folks to learn why this is the case, is it intended, etc., as my > understanding is that the VirtualSize in the section headers should > be section aligned. > > We are stuck with this for existing MSVC toolchains, however, so we > will need to align either the VirtualSize or SizeOfRawData to the > section alignment when we create the image records. I don't have a > preference between the two, they end up being the same when we align > them, so I can send a v2 with aligning the VirtualSize. This will be > a no-op on gcc built binaries and will set it to the correct value for > MSVC. > I don't think this is correct. The VirtualSize could be much larger than the SizeOfRawData (to the extent where the delta exceeds the alignment padding), in cases where some of the memory is data-initialized and some of it is zero-initialized. We certainly make use of this in Linux when constructing the PE/COFF view of the kernel image. So for the memory view of the image, only VirtualSize is appropriate - SizeOfRawData just gives you the size of the data to copy to the start of this section. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116671): https://edk2.groups.io/g/devel/message/116671 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-12 8:32 ` Ard Biesheuvel @ 2024-03-12 14:38 ` Oliver Smith-Denny 0 siblings, 0 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-03-12 14:38 UTC (permalink / raw) To: devel, ardb Cc: Liming Gao, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/12/2024 1:32 AM, Ard Biesheuvel wrote: > On Mon, 11 Mar 2024 at 20:34, Oliver Smith-Denny > <osde@linux.microsoft.com> wrote: >> >> On 3/4/2024 2:38 PM, Oliver Smith-Denny wrote: >>> On 3/4/2024 11:24 AM, Oliver Smith-Denny wrote: >>>> On 3/4/2024 10:54 AM, Ard Biesheuvel wrote: >>>>> On Mon, 4 Mar 2024 at 18:49, Oliver Smith-Denny >>>>> <osde@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 >>>>>>> <osde@linux.microsoft.com> wrote: >>>>>>>> >>>>>>>> - 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. >>>>> >>>> >>>> I see, yes I do believe VirtualSize could be used here instead. Two >>>> things give me pause. One is that the PE spec states that SizeOfRawData >>>> is rounded and VirtualSize is not, so that SizeOfRawData may be greater >>>> than the VirtualSize in some cases (which seems incorrect). >>>> >>>> The other is that the image loader partially uses VirtualSize when >>>> loading: >>>> >>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 >>>> >>>> However, when determining the size of a loaded image (and therefore the >>>> number of pages to allocate) it will allocate an extra page: >>>> >>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 >>>> >>>> as ImageSize here is: >>>> >>>> https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 >>>> >>>> Which according to the spec, SizeOfImage is the size of the image as >>>> loaded into memory and must be a multiple of section alignment. >>>> >>>> So, reflecting on this, let me test with VirtualSize here, I think >>>> that is the right value to use, the only time we would have a >>>> SizeOfRawData that is greater than the VirtualSize would be if our >>>> FileAlignment is greater than our SectionAlignment, which would be >>>> a misconfiguration. >>>> >>>> I do think the ImageLoader should also be fixed to only allocate >>>> ImageSize number of pages (which should be the sum of the section >>>> VirtualSizes + any headers that aren't included). Might as well not >>>> waste an extra page for each image and then our image record code is >>>> simpler as well (ensuring we are protecting the right pages). >>>> >>>> I think this patch series should go in, as it is fixing an active bug, >>>> but I will also take a look at the image loader creating the image >>>> records and having a protocol it produces to retrieve the list, to >>>> attempt to avoid issues like this in the future. >>>> >>> >>> Surprisingly, I am seeing that the VirtualSize is not 64k aligned there. >>> I am looking deeper into GenFw to make sure it is correctly getting set >>> to align to SectionAlignment in the section headers. When I use dumpbin >>> to dump the headers, it shows each section having VirtualSize as 64k >>> aligned for a runtime image, but the same image doesn't show that in FW. >>> >>> I'll do some digging here. >>> >> >> Following up on this: >> >> Not surprisingly, different toolchains do different things here. >> >> gcc obviously creates ELF files and ElfConvert*.c converts these to PE >> images. However, when it does this, it always sets the section and file >> alignment to the same value (I'm not as familiar with ELF images, I'm >> not sure if there is the same concept as file vs section alignment). >> GenFw could probably update this information to shrink the file >> alignment, but that's a space optimization for gcc built binaries. >> >> In ElfConvert.c, the VirtualSize does get set to the section aligned >> value, which is what I would expect from the spec. >> >> In MSVC PE images, VirtualSize is not section aligned and the >> expectation appears to be that loaders will align the VirtualSize >> to the section alignment. I am planning on reaching out to the MSVC >> folks to learn why this is the case, is it intended, etc., as my >> understanding is that the VirtualSize in the section headers should >> be section aligned. >> >> We are stuck with this for existing MSVC toolchains, however, so we >> will need to align either the VirtualSize or SizeOfRawData to the >> section alignment when we create the image records. I don't have a >> preference between the two, they end up being the same when we align >> them, so I can send a v2 with aligning the VirtualSize. This will be >> a no-op on gcc built binaries and will set it to the correct value for >> MSVC. >> > > I don't think this is correct. The VirtualSize could be much larger > than the SizeOfRawData (to the extent where the delta exceeds the > alignment padding), in cases where some of the memory is > data-initialized and some of it is zero-initialized. We certainly make > use of this in Linux when constructing the PE/COFF view of the kernel > image. > > So for the memory view of the image, only VirtualSize is appropriate - > SizeOfRawData just gives you the size of the data to copy to the start > of this section. > Sure, I agree that per spec, the VirtualSize should be the correct value to use. For MSVC built binaries, I have only seen VirtualSize < SizeOfRawData, but this is just anecdotal. I will follow up with the MSVC folks and give more information on why MSVC built binaries do not have the VirtualSize aligned with the SectionAlignment, which would be my expectation, as VirtualSize should be the size of the image as loaded into memory. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116678): https://edk2.groups.io/g/devel/message/116678 Mute This Topic: https://groups.io/mt/104610770/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 12+ messages in thread
* [edk2-devel][PATCH v1 2/3] MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage 2024-02-27 20:27 [edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Oliver Smith-Denny @ 2024-02-27 20:27 ` Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Oliver Smith-Denny 2 siblings, 0 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-02-27 20:27 UTC (permalink / raw) To: devel; +Cc: Liming Gao, Taylor Beebe Currently, there are multiple instances of code create image records. ImagePropertiesRecordLib was created to only have this code in one place. Update the lib to use additional logic from the copy in MemoryProtection.c before converging that code to use the lib. Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com> --- MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c | 82 +++++++++++++++----- 1 file changed, 63 insertions(+), 19 deletions(-) diff --git a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c index 07ced0e54e38..9b99cb0f77d2 100644 --- a/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c +++ b/MdeModulePkg/Library/ImagePropertiesRecordLib/ImagePropertiesRecordLib.c @@ -60,6 +60,39 @@ EfiSizeToPages ( return RShiftU64 (Size, EFI_PAGE_SHIFT) + ((((UINTN)Size) & EFI_PAGE_MASK) ? 1 : 0); } +/** + Frees the memory for each ImageRecordCodeSection within an ImageRecord + and removes the entries from the list. It does not free the ImageRecord + itself. + + @param[in] ImageRecord The ImageRecord in which to free code sections +**/ +STATIC +VOID +FreeImageRecordCodeSections ( + IMAGE_PROPERTIES_RECORD *ImageRecord + ) +{ + LIST_ENTRY *CodeSegmentListHead; + IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection; + + if (ImageRecord == NULL) { + return; + } + + CodeSegmentListHead = &ImageRecord->CodeSegmentList; + while (!IsListEmpty (CodeSegmentListHead)) { + ImageRecordCodeSection = CR ( + CodeSegmentListHead->ForwardLink, + IMAGE_PROPERTIES_RECORD_CODE_SECTION, + Link, + IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE + ); + RemoveEntryList (&ImageRecordCodeSection->Link); + FreePool (ImageRecordCodeSection); + } +} + /** Sort memory map entries based upon PhysicalStart from low to high. @@ -993,6 +1026,7 @@ CreateImagePropertiesRecord ( UINT8 *Name; UINT32 SectionAlignment; UINT32 PeCoffHeaderOffset; + CHAR8 *PdbPointer; if ((ImageRecord == NULL) || (ImageBase == NULL)) { return EFI_INVALID_PARAMETER; @@ -1016,6 +1050,11 @@ CreateImagePropertiesRecord ( InitializeListHead (&ImageRecord->Link); InitializeListHead (&ImageRecord->CodeSegmentList); + PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageBase); + if (PdbPointer != NULL) { + DEBUG ((DEBUG_ERROR, " Image - %a\n", PdbPointer)); + } + // Check PE/COFF image DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageBase; PeCoffHeaderOffset = 0; @@ -1084,7 +1123,8 @@ CreateImagePropertiesRecord ( // Record code section(s) ImageRecordCodeSection = AllocatePool (sizeof (*ImageRecordCodeSection)); if (ImageRecordCodeSection == NULL) { - return EFI_OUT_OF_RESOURCES; + Status = EFI_OUT_OF_RESOURCES; + goto CreateImagePropertiesRecordEnd; } ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; @@ -1101,6 +1141,27 @@ CreateImagePropertiesRecord ( SortImageRecordCodeSection (ImageRecord); } + // + // Check overlap all section in ImageBase/Size + // + if (!IsImageRecordCodeSectionValid (ImageRecord)) { + DEBUG ((DEBUG_ERROR, "IsImageRecordCodeSectionValid - FAIL\n")); + Status = EFI_INVALID_PARAMETER; + goto CreateImagePropertiesRecordEnd; + } + + // + // Round up the ImageSize, some CPU arch may return EFI_UNSUPPORTED if ImageSize is not aligned. + // Given that the loader always allocates full pages, we know the space after the image is not used. + // + ImageRecord->ImageSize = ALIGN_VALUE (ImageRecord->ImageSize, EFI_PAGE_SIZE); + +CreateImagePropertiesRecordEnd: + if (EFI_ERROR (Status)) { + // we failed to create a valid record, free the section memory that was allocated + FreeImageRecordCodeSections (ImageRecord); + } + return Status; } @@ -1117,24 +1178,7 @@ DeleteImagePropertiesRecord ( IN IMAGE_PROPERTIES_RECORD *ImageRecord ) { - LIST_ENTRY *CodeSegmentListHead; - IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection; - - if (ImageRecord == NULL) { - return; - } - - CodeSegmentListHead = &ImageRecord->CodeSegmentList; - while (!IsListEmpty (CodeSegmentListHead)) { - ImageRecordCodeSection = CR ( - CodeSegmentListHead->ForwardLink, - IMAGE_PROPERTIES_RECORD_CODE_SECTION, - Link, - IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE - ); - RemoveEntryList (&ImageRecordCodeSection->Link); - FreePool (ImageRecordCodeSection); - } + FreeImageRecordCodeSections (ImageRecord); if (!IsListEmpty (&ImageRecord->Link)) { RemoveEntryList (&ImageRecord->Link); -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116061): https://edk2.groups.io/g/devel/message/116061 Mute This Topic: https://groups.io/mt/104610771/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [edk2-devel][PATCH v1 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib 2024-02-27 20:27 [edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 2/3] MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage Oliver Smith-Denny @ 2024-02-27 20:27 ` Oliver Smith-Denny 2 siblings, 0 replies; 12+ messages in thread From: Oliver Smith-Denny @ 2024-02-27 20:27 UTC (permalink / raw) To: devel; +Cc: Liming Gao, Taylor Beebe The functionality to create and delete Image Records has been consolidated in a library and ensured that MemoryProtection.c's usage is encapsulated there. This patch moves MemoryProtection.c to reuse the code in the lib and to prevent issues in the future where code is updated in one place but not the other. Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Taylor Beebe <taylor.d.beebe@gmail.com> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com> --- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 241 +++----------------- 1 file changed, 28 insertions(+), 213 deletions(-) diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c index 977239d08afc..ae9f8ae60bc7 100644 --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c @@ -281,80 +281,43 @@ SetUefiImageProtectionAttributes ( } /** - Return if the PE image section is aligned. + Return the section alignment requirement for the PE image section type. - @param[in] SectionAlignment PE/COFF section alignment - @param[in] MemoryType PE/COFF image memory type + @param[in] MemoryType PE/COFF image memory type + + @retval The required section alignment for this memory type - @retval TRUE The PE image section is aligned. - @retval FALSE The PE image section is not aligned. **/ -BOOLEAN -IsMemoryProtectionSectionAligned ( - IN UINT32 SectionAlignment, +STATIC +UINT32 +GetMemoryProtectionSectionAlignment ( IN EFI_MEMORY_TYPE MemoryType ) { - UINT32 PageAlignment; + UINT32 SectionAlignment; switch (MemoryType) { case EfiRuntimeServicesCode: case EfiACPIMemoryNVS: - PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; + SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; break; case EfiRuntimeServicesData: case EfiACPIReclaimMemory: ASSERT (FALSE); - PageAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; + SectionAlignment = RUNTIME_PAGE_ALLOCATION_GRANULARITY; break; case EfiBootServicesCode: case EfiLoaderCode: case EfiReservedMemoryType: - PageAlignment = EFI_PAGE_SIZE; + SectionAlignment = EFI_PAGE_SIZE; break; default: ASSERT (FALSE); - PageAlignment = EFI_PAGE_SIZE; + SectionAlignment = EFI_PAGE_SIZE; break; } - if ((SectionAlignment & (PageAlignment - 1)) != 0) { - return FALSE; - } else { - return TRUE; - } -} - -/** - Free Image record. - - @param[in] ImageRecord A UEFI image record -**/ -VOID -FreeImageRecord ( - IN IMAGE_PROPERTIES_RECORD *ImageRecord - ) -{ - LIST_ENTRY *CodeSegmentListHead; - IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection; - - CodeSegmentListHead = &ImageRecord->CodeSegmentList; - while (!IsListEmpty (CodeSegmentListHead)) { - ImageRecordCodeSection = CR ( - CodeSegmentListHead->ForwardLink, - IMAGE_PROPERTIES_RECORD_CODE_SECTION, - Link, - IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE - ); - RemoveEntryList (&ImageRecordCodeSection->Link); - FreePool (ImageRecordCodeSection); - } - - if (ImageRecord->Link.ForwardLink != NULL) { - RemoveEntryList (&ImageRecord->Link); - } - - FreePool (ImageRecord); + return SectionAlignment; } /** @@ -369,19 +332,10 @@ ProtectUefiImage ( IN EFI_DEVICE_PATH_PROTOCOL *LoadedImageDevicePath ) { - VOID *ImageAddress; - EFI_IMAGE_DOS_HEADER *DosHdr; - UINT32 PeCoffHeaderOffset; - UINT32 SectionAlignment; - EFI_IMAGE_SECTION_HEADER *Section; - EFI_IMAGE_OPTIONAL_HEADER_PTR_UNION Hdr; - UINT8 *Name; - UINTN Index; - IMAGE_PROPERTIES_RECORD *ImageRecord; - CHAR8 *PdbPointer; - IMAGE_PROPERTIES_RECORD_CODE_SECTION *ImageRecordCodeSection; - BOOLEAN IsAligned; - UINT32 ProtectionPolicy; + IMAGE_PROPERTIES_RECORD *ImageRecord; + UINT32 ProtectionPolicy; + EFI_STATUS Status; + UINT32 RequiredAlignment; DEBUG ((DEBUG_INFO, "ProtectUefiImageCommon - 0x%x\n", LoadedImage)); DEBUG ((DEBUG_INFO, " - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase, LoadedImage->ImageSize)); @@ -406,160 +360,21 @@ ProtectUefiImage ( return; } - ImageRecord->Signature = IMAGE_PROPERTIES_RECORD_SIGNATURE; + RequiredAlignment = GetMemoryProtectionSectionAlignment (LoadedImage->ImageCodeType); - // - // Step 1: record whole region - // - ImageRecord->ImageBase = (EFI_PHYSICAL_ADDRESS)(UINTN)LoadedImage->ImageBase; - ImageRecord->ImageSize = LoadedImage->ImageSize; - - ImageAddress = LoadedImage->ImageBase; - - PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress); - if (PdbPointer != NULL) { - DEBUG ((DEBUG_VERBOSE, " Image - %a\n", PdbPointer)); - } - - // - // Check PE/COFF image - // - DosHdr = (EFI_IMAGE_DOS_HEADER *)(UINTN)ImageAddress; - PeCoffHeaderOffset = 0; - if (DosHdr->e_magic == EFI_IMAGE_DOS_SIGNATURE) { - PeCoffHeaderOffset = DosHdr->e_lfanew; - } - - Hdr.Pe32 = (EFI_IMAGE_NT_HEADERS32 *)((UINT8 *)(UINTN)ImageAddress + PeCoffHeaderOffset); - if (Hdr.Pe32->Signature != EFI_IMAGE_NT_SIGNATURE) { - DEBUG ((DEBUG_VERBOSE, "Hdr.Pe32->Signature invalid - 0x%x\n", Hdr.Pe32->Signature)); - // It might be image in SMM. - goto Finish; - } - - // - // Get SectionAlignment - // - if (Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) { - SectionAlignment = Hdr.Pe32->OptionalHeader.SectionAlignment; - } else { - SectionAlignment = Hdr.Pe32Plus->OptionalHeader.SectionAlignment; - } - - IsAligned = IsMemoryProtectionSectionAligned (SectionAlignment, LoadedImage->ImageCodeType); - if (!IsAligned) { - DEBUG (( - DEBUG_VERBOSE, - "!!!!!!!! ProtectUefiImageCommon - Section Alignment(0x%x) is incorrect !!!!!!!!\n", - SectionAlignment - )); - PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress); - if (PdbPointer != NULL) { - DEBUG ((DEBUG_VERBOSE, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer)); - } + Status = CreateImagePropertiesRecord ( + LoadedImage->ImageBase, + LoadedImage->ImageSize, + &RequiredAlignment, + ImageRecord + ); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_ERROR, "%a failed to create image properties record\n", __func__)); + FreePool (ImageRecord); goto Finish; } - Section = (EFI_IMAGE_SECTION_HEADER *)( - (UINT8 *)(UINTN)ImageAddress + - PeCoffHeaderOffset + - sizeof (UINT32) + - sizeof (EFI_IMAGE_FILE_HEADER) + - Hdr.Pe32->FileHeader.SizeOfOptionalHeader - ); - ImageRecord->CodeSegmentCount = 0; - InitializeListHead (&ImageRecord->CodeSegmentList); - for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; Index++) { - Name = Section[Index].Name; - DEBUG (( - DEBUG_VERBOSE, - " Section - '%c%c%c%c%c%c%c%c'\n", - Name[0], - Name[1], - Name[2], - Name[3], - Name[4], - Name[5], - Name[6], - Name[7] - )); - - // - // Instead of assuming that a PE/COFF section of type EFI_IMAGE_SCN_CNT_CODE - // can always be mapped read-only, classify a section as a code section only - // if it has the executable attribute set and the writable attribute cleared. - // - // This adheres more closely to the PE/COFF spec, and avoids issues with - // Linux OS loaders that may consist of a single read/write/execute section. - // - if ((Section[Index].Characteristics & (EFI_IMAGE_SCN_MEM_WRITE | EFI_IMAGE_SCN_MEM_EXECUTE)) == EFI_IMAGE_SCN_MEM_EXECUTE) { - DEBUG ((DEBUG_VERBOSE, " VirtualSize - 0x%08x\n", Section[Index].Misc.VirtualSize)); - DEBUG ((DEBUG_VERBOSE, " VirtualAddress - 0x%08x\n", Section[Index].VirtualAddress)); - DEBUG ((DEBUG_VERBOSE, " SizeOfRawData - 0x%08x\n", Section[Index].SizeOfRawData)); - DEBUG ((DEBUG_VERBOSE, " PointerToRawData - 0x%08x\n", Section[Index].PointerToRawData)); - DEBUG ((DEBUG_VERBOSE, " PointerToRelocations - 0x%08x\n", Section[Index].PointerToRelocations)); - DEBUG ((DEBUG_VERBOSE, " PointerToLinenumbers - 0x%08x\n", Section[Index].PointerToLinenumbers)); - DEBUG ((DEBUG_VERBOSE, " NumberOfRelocations - 0x%08x\n", Section[Index].NumberOfRelocations)); - DEBUG ((DEBUG_VERBOSE, " NumberOfLinenumbers - 0x%08x\n", Section[Index].NumberOfLinenumbers)); - DEBUG ((DEBUG_VERBOSE, " Characteristics - 0x%08x\n", Section[Index].Characteristics)); - - // - // Step 2: record code section - // - ImageRecordCodeSection = AllocatePool (sizeof (*ImageRecordCodeSection)); - if (ImageRecordCodeSection == NULL) { - return; - } - - ImageRecordCodeSection->Signature = IMAGE_PROPERTIES_RECORD_CODE_SECTION_SIGNATURE; - - ImageRecordCodeSection->CodeSegmentBase = (UINTN)ImageAddress + Section[Index].VirtualAddress; - ImageRecordCodeSection->CodeSegmentSize = ALIGN_VALUE (Section[Index].SizeOfRawData, SectionAlignment); - - DEBUG ((DEBUG_VERBOSE, "ImageCode: 0x%016lx - 0x%016lx\n", ImageRecordCodeSection->CodeSegmentBase, ImageRecordCodeSection->CodeSegmentSize)); - - InsertTailList (&ImageRecord->CodeSegmentList, &ImageRecordCodeSection->Link); - ImageRecord->CodeSegmentCount++; - } - } - - if (ImageRecord->CodeSegmentCount == 0) { - // - // If a UEFI executable consists of a single read+write+exec PE/COFF - // section, that isn't actually an error. The image can be launched - // alright, only image protection cannot be applied to it fully. - // - // One example that elicits this is (some) Linux kernels (with the EFI stub - // of course). - // - DEBUG ((DEBUG_WARN, "!!!!!!!! ProtectUefiImageCommon - CodeSegmentCount is 0 !!!!!!!!\n")); - PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)(UINTN)ImageAddress); - if (PdbPointer != NULL) { - DEBUG ((DEBUG_WARN, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer)); - } - - goto Finish; - } - - // - // Final - // - SortImageRecordCodeSection (ImageRecord); - // - // Check overlap all section in ImageBase/Size - // - if (!IsImageRecordCodeSectionValid (ImageRecord)) { - DEBUG ((DEBUG_ERROR, "IsImageRecordCodeSectionValid - FAIL\n")); - goto Finish; - } - - // - // Round up the ImageSize, some CPU arch may return EFI_UNSUPPORTED if ImageSize is not aligned. - // Given that the loader always allocates full pages, we know the space after the image is not used. - // - ImageRecord->ImageSize = ALIGN_VALUE (LoadedImage->ImageSize, EFI_PAGE_SIZE); - // // CPU ARCH present. Update memory attribute directly. // @@ -607,7 +422,7 @@ UnprotectUefiImage ( ImageRecord->ImageSize, 0 ); - FreeImageRecord (ImageRecord); + DeleteImagePropertiesRecord (ImageRecord); return; } } -- 2.40.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116062): https://edk2.groups.io/g/devel/message/116062 Mute This Topic: https://groups.io/mt/104610772/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-03-12 14:38 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-27 20:27 [edk2-devel][PATCH v1 0/3] MdeModulePkg: ImagePropertiesRecordLib Fixes Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Oliver Smith-Denny 2024-03-01 11:58 ` Ard Biesheuvel 2024-03-04 17:49 ` Oliver Smith-Denny 2024-03-04 18:54 ` Ard Biesheuvel 2024-03-04 19:24 ` Oliver Smith-Denny [not found] ` <17B9A63355E03AE5.30946@groups.io> 2024-03-04 22:38 ` Oliver Smith-Denny [not found] ` <17B9B0CE1BDDC06B.30946@groups.io> 2024-03-11 19:34 ` Oliver Smith-Denny 2024-03-12 8:32 ` Ard Biesheuvel 2024-03-12 14:38 ` Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 2/3] MdeModulePkg: ImagePropertiesRecordLib: Consolidate Usage Oliver Smith-Denny 2024-02-27 20:27 ` [edk2-devel][PATCH v1 3/3] MdeModulePkg: MemoryProtection: Use ImageRecordPropertiesLib Oliver Smith-Denny
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox