* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize [not found] ` <531b3cfe-b57c-4b5a-97fd-45e428f97853@...> @ 2024-03-14 9:57 ` Marvin Häuser 2024-03-14 14:45 ` Oliver Smith-Denny 0 siblings, 1 reply; 15+ messages in thread From: Marvin Häuser @ 2024-03-14 9:57 UTC (permalink / raw) To: Oliver Smith-Denny Cc: edk2-devel-groups-io, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe [-- Attachment #1: Type: text/plain, Size: 6092 bytes --] Good day everyone, Sorry for interjecting, but I’d like to avoid premature changes to PE code that would only make the current mess even worse. > On 4. Mar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote: > > 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. Still no, for some reasons you already mentioned, but also reasons you didn’t: - VirtualSize often is not aligned, so they can still trivially mismatch. - VirtualSize includes zero-initialized data not part of the file. - In the old days, VirtualSize = 0 was used to disable loading of debug data, but SizeOfRawData remained intact, so it could be loaded on demand. - SizeOfRawData may be unaligned due to bugs. In fact, there is no reason to trust FileAlignment, it has no real meaning in an UEFI context (I am not even sure it does on Windows). > I see, yes I do believe VirtualSize could be used here instead. *Must*. As Ard said, SizeOfRawData is only interesting to know how many bytes to copy from the file and for *nothing* else. > 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 > Sorry, I don’t understand the “however”, this is sound. > 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. Side note, SizeOfImage may be borked, not only due to bugs, but also due to malice. There is literally no reason whatsoever to use it - to validate it, you need to calculate a sane value, and once you have that, you obviously do not need SizeOfImage anymore. You could reject images with malformed SizeOfImage, but there likely are various legitimate-bugged ones out there. This is just an ancient artifact from before memory safety was a thing. :) > 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. No, it can simply be that FileAlignment = SectionAlignment and SizeOfRawData is aligned and VirtualSize is not. Even when both are aligned, the former may carry extra optional data (like previously mentioned debug data), or it might be some weird I/O performance related padding or something. > 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). NO! :) There are *many* issues surrounding this: - Sections may overlap (observed with early macOS EFI images, could be ignored, but needs to be validated across the landscape). - Sections may have gaps (observed with “old" Linux EFI Shims iirc, there’ll be plenty of broken images in the wild yet). - The extra page is not pointless - as you can see, it is allocated when exceeding EFI page size, which is the only granularity you can page-allocate at. So, when you require > 4K alignment, you need the extra page to be able to “shift” the image into proper alignment in the allocated region as needed. In AUDK, I resolved this by abstracting existing, buried aligned-allocation code into a MemoryAllocationLibEx class: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70 Without aligned-allocation, you cannot get rid of this extra page. For reference, my image record code is here: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335 > 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. Why does there need to be a protocol? Why should non-Core modules get any information about image topology? A few general words of caution when dealing with PE: - Do not trust the spec. - Do not trust the loader. - Do not trust the converter. - Do *not* trust your intuition. *Everything* is broken, from the producers to the consumers, from the signing to the validation tools, technically, security-wise, and design-wise. Do *not* change things without as little as a collection of Windows and Linux images from various versions and various OpROMs to regression-test with. Otherwise, we will all be in a world of more pain. :) Thanks! Best regards, Marvin -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116736): https://edk2.groups.io/g/devel/message/116736 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] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 7900 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-14 9:57 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Marvin Häuser @ 2024-03-14 14:45 ` Oliver Smith-Denny 2024-03-14 21:57 ` Marvin Häuser 0 siblings, 1 reply; 15+ messages in thread From: Oliver Smith-Denny @ 2024-03-14 14:45 UTC (permalink / raw) To: devel, mhaeuser; +Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe Hi Marvin, Thanks for looking at this. Since sending out the v1, I have done a lot more digging into our PE loader, others, the spec, and differences between MSVC and our ElfConvert tool. I agree that my understanding was flawed in my original comments. On 3/14/2024 2:57 AM, Marvin Häuser wrote: > Good day everyone, > > Sorry for interjecting, but I’d like to avoid premature changes to PE > code that would only make the current mess even worse. > >> On 4. Mar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote: >> >> 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. > > Still no, for some reasons you already mentioned, but also reasons you > didn’t: > - VirtualSize often is not aligned, so they can still trivially mismatch. > - VirtualSize includes zero-initialized data not part of the file. This does not appear to be the case with MSVC built binaries. I am seeing that VirtualSize is the size of the data-initialized part of the section. Whereas on our ElfConverted binaries, what you say is true. The difference concerns me. > - In the old days, VirtualSize = 0 was used to disable loading of debug > data, but SizeOfRawData remained intact, so it could be loaded on demand. > - SizeOfRawData may be unaligned due to bugs. In fact, there is no > reason to trust FileAlignment, it has no real meaning in an UEFI context > (I am not even sure it does on Windows). > >> I see, yes I do believe VirtualSize could be used here instead. > > *Must*. As Ard said, SizeOfRawData is only interesting to know how many > bytes to copy from the file and for *nothing* else. Agreed. I have done further research and v2 uses VirtualSize. I was initially led astray here because the ElfConvert tool sets VirtualSize and SizeOfRawData to the SectionAlignment universally: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 > >> 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 >> > > Sorry, I don’t understand the “however”, this is sound. > >> 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. > > Side note, SizeOfImage may be borked, not only due to bugs, but also due > to malice. There is literally no reason whatsoever to use it - to > validate it, you need to calculate a sane value, and once you have that, > you obviously do not need SizeOfImage anymore. You could reject images > with malformed SizeOfImage, but there likely are various > legitimate-bugged ones out there. This is just an ancient artifact from > before memory safety was a thing. :) Yep, the PE headers are definitely filled with information that can be deduced from other sources and the spec is not very verbose, which is why I definitely want to tread carefully here. For example, it says SizeOfImage is the size of the image loaded into memory, which current implementations seem to take to mean the sum of the headers plus each section's VirtualSize rounded to the SectionAlignment (which the spec does call out that SizeOfImage must be aligned to the SectionAlignment). VirtualSize it says is also the size of the section as loaded into memory, but current implementations (other than our ElfConvert script which I'm currently investigating if we should change that) seem to take this to mean the size of the initialized data in memory, i.e. the "real" data from the image, not the zeroed data to get to the SectionAlignment. But the language in the spec is almost identical between the two. And again, we have a difference in our MSVC and gcc built binaries here. > >> 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. > > No, it can simply be that FileAlignment = SectionAlignment and > SizeOfRawData is aligned and VirtualSize is not. Even when both are > aligned, the former may carry extra optional data (like previously > mentioned debug data), or it might be some weird I/O performance related > padding or something. Agreed, this was misinformed by looking at gcc built binaries. > >> 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). > > NO! :) There are *many* issues surrounding this: > - Sections may overlap (observed with early macOS EFI images, could be > ignored, but needs to be validated across the landscape). > - Sections may have gaps (observed with “old" Linux EFI Shims iirc, > there’ll be plenty of broken images in the wild yet). > - The extra page is not pointless - as you can see, it is allocated when > exceeding EFI page size, which is the only granularity you can > page-allocate at. So, when you require > 4K alignment, you need the > extra page to be able to “shift” the image into proper alignment in the > allocated region as needed. In AUDK, I resolved this by abstracting > existing, buried aligned-allocation code into a MemoryAllocationLibEx > class: > https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70> > Without aligned-allocation, you cannot get rid of this extra page. > > For reference, my image record code is here: > https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdePkg/Library/BaseUefiImageLib/PeSupport.c#L335> Yep, this was me being dumb and not looking a few lines further ahead :) After sending this I looked deeper at this offhand comment and realized the purpose. > >> 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. > > Why does there need to be a protocol? Why should non-Core modules get > any information about image topology? Agreed, only core code needs this. I think I said protocol originally so that the existing ImagePropertiesRecordLib could use it without depending on a core only function, but in a new implementation we would drop the lib. > > A few general words of caution when dealing with PE: > - Do not trust the spec. > - Do not trust the loader. > - Do not trust the converter. > - Do *not* trust your intuition. > > *Everything* is broken, from the producers to the consumers, from the > signing to the validation tools, technically, security-wise, and > design-wise. Do *not* change things without as little as a collection of > Windows and Linux images from various versions and various OpROMs to > regression-test with. Otherwise, we will all be in a world of more pain. :) > I agree with you there. That being said, the current Image record code is, unsurprisingly, broken, and does need to be fixed. Can you review the v2 of this? The only difference is that I changed to VirtualSize. Do you think the code should be updated for the case of VirtualSize == 0? In the current implementation, it will round 0 up to the SectionAlignment, which is what I believe will happen in the PE loader, also, but want to get your thoughts on it. This would still be a short term solution to fix the active bug (ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or we will boot without the benefit of the MAT, if we happen to have an aligned total region but not each entry, testing shows). I do think it is a broken pattern to try to determine the image records after we've loaded them into memory, the PE loader should create them with the factual data of what was loaded. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116756): https://edk2.groups.io/g/devel/message/116756 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] 15+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-14 14:45 ` Oliver Smith-Denny @ 2024-03-14 21:57 ` Marvin Häuser 2024-03-15 22:56 ` Oliver Smith-Denny 0 siblings, 1 reply; 15+ messages in thread From: Marvin Häuser @ 2024-03-14 21:57 UTC (permalink / raw) To: Oliver Smith-Denny Cc: edk2-devel-groups-io, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe [-- Attachment #1: Type: text/plain, Size: 5108 bytes --] > On 14. Mar 2024, at 15:45, Oliver Smith-Denny <osde@linux.microsoft.com> wrote: > > This does not appear to be the case with MSVC built binaries. I am > seeing that VirtualSize is the size of the data-initialized part of the > section. What? :( So you are saying modern MSVC generates PEs that either have non-contiguous sections, or they have sections that contain zero-initialized portions explicitly? Both variants sound badly borked and should be investigated, even if only for resource constraints. Just thinking about it, is there any chance you set ALIGN = FILEALIGN? Because then this will be invoked and your file will be converted to XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612 BaseTools has no concept of what should be a XIP and what should not, so if a file somewhat qualifies, it will just always emit a XIP: https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150 In AUDK I addressed this by tying XIP to reloc stripping, but that is more of a convenient heuristic: https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293 > Whereas on our ElfConverted binaries, what you say is true. The > difference concerns me. Yes, very concerning. Would you mind sharing dumps (section table, etc.) and/or binaries (unless you indeed hit what I outlined above)? I recall MSVC to rather be more aggressive with space-savings, whereas ElfConvert would include section-alignment padding within the file (but not zero-init data). > Agreed. I have done further research and v2 uses VirtualSize. I was > initially led astray here because the ElfConvert tool sets VirtualSize > and SizeOfRawData to the SectionAlignment universally: > > https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 Yes… plenty of more things in the tool to lead people astray. :) > Yep, the PE headers are definitely filled with information that can be > deduced from other sources and the spec is not very verbose, which is > why I definitely want to tread carefully here. For example, it says > SizeOfImage is the size of the image loaded into memory, which > current implementations seem to take to mean the sum of the headers > plus each section's VirtualSize rounded to the SectionAlignment (which > the spec does call out that SizeOfImage must be aligned to the > SectionAlignment). VirtualSize it says is also the size of the section > as loaded into memory, but current implementations (other than our > ElfConvert script which I'm currently investigating if we should > change that) seem to take this to mean the size of the initialized > data in memory, i.e. the "real" data from the image, not the zeroed > data to get to the SectionAlignment. Do you have an example for that? I’m quite certain SizeOfImage should always cover the whole thing. > But the language in the spec > is almost identical between the two. And again, we have a difference > in our MSVC and gcc built binaries here. Do we? :( > I agree with you there. That being said, the current Image record code > is, unsurprisingly, broken, and does need to be fixed. Can you review > the v2 of this? The only difference is that I changed to VirtualSize. LGTM. > Do you think the code should be updated for the case of VirtualSize > == 0? In the current implementation, it will round 0 up to the > SectionAlignment It should not. Well, it will technically round, but it will remain 0. It does not round up to the strictly next boundary, but if the current value is a multiple of the alignment (0 is a multiple of any alignment), it remains unchanged. > , which is what I believe will happen in the PE > loader, also, but want to get your thoughts on it. > > This would still be a short term solution to fix the active bug > (ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or > we will boot without the benefit of the MAT, if we happen to have > an aligned total region but not each entry, testing shows). I do > think it is a broken pattern to try to determine the image records > after we've loaded them into memory, the PE loader should create > them with the factual data of what was loaded. There are many more broken patterns everywhere. Did you notice yet that the API itself is broken from the ground up? Not to keep “advertising” it, but maybe our paper will give you some fun too. :) https://arxiv.org/pdf/2012.05471.pdf Best regards, Marvin > > Thanks, > Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116780): https://edk2.groups.io/g/devel/message/116780 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] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 36075 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-14 21:57 ` Marvin Häuser @ 2024-03-15 22:56 ` Oliver Smith-Denny 2024-03-15 23:45 ` Marvin Häuser 0 siblings, 1 reply; 15+ messages in thread From: Oliver Smith-Denny @ 2024-03-15 22:56 UTC (permalink / raw) To: devel, mhaeuser; +Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/14/2024 2:57 PM, Marvin Häuser wrote: > >> On 14. Mar 2024, at 15:45, Oliver Smith-Denny >> <osde@linux.microsoft.com> wrote: >> >> This does not appear to be the case with MSVC built binaries. I am >> seeing that VirtualSize is the size of the data-initialized part of the >> section. > > What? :( So you are saying modern MSVC generates PEs that either have > non-contiguous sections, or they have sections that contain > zero-initialized portions explicitly? Both variants sound badly borked > and should be investigated, even if only for resource constraints. > I don't think this is what I'm saying. What I am trying to say is that on MSVC, I see PE images getting created that have VirtualSize set to the actual number of initialized bytes in that section (not padded to the section alignment). On ElfConverted binaries, I see the VirtualSize is padded to the section alignment. I've dropped an example below > Just thinking about it, is there any chance you set ALIGN = FILEALIGN? No, the specific case where I was researching this was explicitly setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData is aligned to the file alignment, as expected, but VirtualSize would be the actual size of the data. Again, the troubling thing here for me is that the same binary built with gcc has the VirtualSize aligned to the section alignment. And I have seen other code that loads PE images that relies on VirtualSize not including the padding. The spec is vague here, it says VirtualSize is the size of the section as loaded in memory (which would lead me to believe this should include padding) but it does not explicitly say it should be a multiple of the section alignment (as other fields do). But at a minimum I think we should have different toolchains doing the same behavior here. > Because then this will be invoked and your file will be converted to > XIP: > https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L612> > > BaseTools has no concept of what should be a XIP and what should not, so > if a file somewhat qualifies, it will just always emit a XIP: > https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/GenFw.c#L2150> > > In AUDK I addressed this by tying XIP to reloc stripping, but that is > more of a convenient heuristic: > https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293 <https://github.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/BaseTools/Source/Python/GenFds/EfiSection.py#L293> > >> Whereas on our ElfConverted binaries, what you say is true. The >> difference concerns me. > > Yes, very concerning. Would you mind sharing dumps (section table, etc.) > and/or binaries (unless you indeed hit what I outlined above)? I recall > MSVC to rather be more aggressive with space-savings, whereas ElfConvert > would include section-alignment padding within the file (but not > zero-init data). Correct, ElfConvert sets the file alignment and the section alignment to the same value, based on common-page-size/max-page-size. I will show some examples at the bottom. > >> Agreed. I have done further research and v2 uses VirtualSize. I was >> initially led astray here because the ElfConvert tool sets VirtualSize >> and SizeOfRawData to the SectionAlignment universally: >> >> https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136 <https://github.com/tianocore/edk2/blob/5572b43c6767f7cc46b074ae1fc288f6eccdc65d/BaseTools/Source/C/GenFw/ElfConvert.c#L134-L136> > > Yes… plenty of more things in the tool to lead people astray. :) > >> Yep, the PE headers are definitely filled with information that can be >> deduced from other sources and the spec is not very verbose, which is >> why I definitely want to tread carefully here. For example, it says >> SizeOfImage is the size of the image loaded into memory, which >> current implementations seem to take to mean the sum of the headers >> plus each section's VirtualSize rounded to the SectionAlignment (which >> the spec does call out that SizeOfImage must be aligned to the >> SectionAlignment). VirtualSize it says is also the size of the section >> as loaded into memory, but current implementations (other than our >> ElfConvert script which I'm currently investigating if we should >> change that) seem to take this to mean the size of the initialized >> data in memory, i.e. the "real" data from the image, not the zeroed >> data to get to the SectionAlignment. > > Do you have an example for that? I’m quite certain SizeOfImage should > always cover the whole thing. See below for the VirtualSize examples, I'm confused on your comment on SizeOfImage. I agree that SizeOfImage covers everything as loaded into memory and I have not seen any issues there. > >> But the language in the spec >> is almost identical between the two. And again, we have a difference >> in our MSVC and gcc built binaries here. > > Do we? :( > >> I agree with you there. That being said, the current Image record code >> is, unsurprisingly, broken, and does need to be fixed. Can you review >> the v2 of this? The only difference is that I changed to VirtualSize. > > LGTM. Do you mind adding your RB to v2? And certainly if you have any other comments that is greatly appreciated. > >> Do you think the code should be updated for the case of VirtualSize >> == 0? In the current implementation, it will round 0 up to the >> SectionAlignment > > It should not. Well, it will technically round, but it will remain 0. It > does not round up to the strictly next boundary, but if the current > value is a multiple of the alignment (0 is a multiple of any alignment), > it remains unchanged. > >> , which is what I believe will happen in the PE >> loader, also, but want to get your thoughts on it. >> >> This would still be a short term solution to fix the active bug >> (ARM64 cannot boot a 64k pagesize Linux with a broken MAT table or >> we will boot without the benefit of the MAT, if we happen to have >> an aligned total region but not each entry, testing shows). I do >> think it is a broken pattern to try to determine the image records >> after we've loaded them into memory, the PE loader should create >> them with the factual data of what was loaded. > > There are many more broken patterns everywhere. Did you notice yet that > the API itself is broken from the ground up? Not to keep “advertising” > it, but maybe our paper will give you some fun too. :) > https://arxiv.org/pdf/2012.05471.pdf <https://arxiv.org/pdf/2012.05471.pdf> I'll take a look. It's been on my list to look at for awhile but as always other things come up :) Examples of the differences I see between MSVC and gcc binaries: I originally noticed this on ARM64 on edk2, but wanted to make sure I saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg (edk2 doesn't have VS2022 support and I didn't feel like adding it or reverting back to VS2019). For reference, this is building the current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a. I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin (from VS2022) to examine the PE headers. MSVC selected header values: Main header: 0x3200 size of code 0x2400 size of initialized data 0x0 size of uninitialized data 0x1000 section alignment 0x200 file alignment 0xB000 size of image 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata .text section: 0x30DF virtual size 0x3200 size of raw data .data section: 0x130 virtual size 0x200 size of raw data GCC ElfConverted selected header values: Main header: 0x4000 size of code 0x1000 size of initialized data 0x0 size of uninitialized data 0x1000 section alignment 0x1000 file alignment 0x7000 size of image 3 sections: .data, .text, .reloc .text section: 0x4000 virtual size 0x4000 size of raw data .data section: 0x1000 virtual size 0x1000 size of raw data So my concern here is that ElfConvert takes a different view of VirtualSize, that is should be section aligned, whereas MSVC binaries take VirtualSize to be the actual size without padding. I think the correct thing to do would be change ElfConvert to do what MSVC does (although the spec is vague and so I can understand the confusion). In practice this will tend to work as is, since we are using SizeOfImage to allocate with, but as you have pointed out there are so many edge cases that having a difference here makes me worried we would see something weird with only binaries built by one toolchain. I am curious to hear your thoughts though. This is very easy to reproduce, build any UEFI binary with MSVC and GCC and compare the headers. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116819): https://edk2.groups.io/g/devel/message/116819 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] 15+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-15 22:56 ` Oliver Smith-Denny @ 2024-03-15 23:45 ` Marvin Häuser 2024-03-16 15:47 ` Oliver Smith-Denny 0 siblings, 1 reply; 15+ messages in thread From: Marvin Häuser @ 2024-03-15 23:45 UTC (permalink / raw) To: Oliver Smith-Denny Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe [-- Attachment #1: Type: text/plain, Size: 6047 bytes --] > On 15. Mar 2024, at 23:57, Oliver Smith-Denny <osde@linux.microsoft.com> wrote: > > I don't think this is what I'm saying. What I am trying to say is that > on MSVC, I see PE images getting created that have VirtualSize set to > the actual number of initialized bytes in that section (not padded to > the section alignment). On ElfConverted binaries, I see the VirtualSize > is padded to the section alignment. I've dropped an example below Ah, mismatched terminology. Zero-initialized as Ard and I used it refers to implicitly or explicitly 0-initialized global variables and such, which is not stored in the file, not the padding. So when you mentioned “real data”, I assumed you meant strictly the non-0 data from the file. Same misunderstanding with SizeOfImage, so that’s all fine. Whew. :) > No, the specific case where I was researching this was explicitly > setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs > on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData > is aligned to the file alignment, as expected, but VirtualSize would > be the actual size of the data. Again, the troubling thing here for > me is that the same binary built with gcc has the VirtualSize aligned > to the section alignment. And I have seen other code that loads PE > images that relies on VirtualSize not including the padding. The spec > is vague here, it says VirtualSize is the size of the section as > loaded in memory (which would lead me to believe this should include > padding) but it does not explicitly say it should be a multiple of > the section alignment (as other fields do). But at a minimum I think > we should have different toolchains doing the same behavior here. Well, not rounding to pad is somewhat superior in some scenarios. If you round, you lose the information on what is section data and what is padding, so you might end up treating padding as data for some reason (because it is indistinguishable from mentioned 0-initialized data). This shouldn’t matter too much for executables and libraries, but MSVC/PE have a lot less of a distinction between object file and executable/library concepts (e.g. no distinction between sections and segments). That might be why they do it this way. > See below for the VirtualSize examples, I'm confused on your comment on > SizeOfImage. I agree that SizeOfImage covers everything as loaded into > memory and I have not seen any issues there. See first comment. > Do you mind adding your RB to v2? And certainly if you have any other > comments that is greatly appreciated. Will try to remember tomorrow. :) > Examples of the differences I see between MSVC and gcc binaries: > > I originally noticed this on ARM64 on edk2, but wanted to make sure I > saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg > (edk2 doesn't have VS2022 support and I didn't feel like adding it > or reverting back to VS2019). For reference, this is building the > current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a. > > I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin > (from VS2022) to examine the PE headers. > > MSVC selected header values: > > Main header: > 0x3200 size of code > 0x2400 size of initialized data > 0x0 size of uninitialized data > 0x1000 section alignment > 0x200 file alignment > 0xB000 size of image > > 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata > > .text section: > 0x30DF virtual size > 0x3200 size of raw data > > .data section: > 0x130 virtual size > 0x200 size of raw data > > > GCC ElfConverted selected header values: > > Main header: > 0x4000 size of code > 0x1000 size of initialized data > 0x0 size of uninitialized data > 0x1000 section alignment > 0x1000 file alignment > 0x7000 size of image > > 3 sections: .data, .text, .reloc > > .text section: > 0x4000 virtual size > 0x4000 size of raw data > > .data section: > 0x1000 virtual size > 0x1000 size of raw data > > So my concern here is that ElfConvert takes a > different view of VirtualSize, that is should be > section aligned, whereas MSVC binaries take > VirtualSize to be the actual size without padding. > I think the correct thing to do would be change > ElfConvert to do what MSVC does (although the spec > is vague and so I can understand the confusion). I don’t think it really matters, but it wouldn’t hurt either. Both kinds of binaries are in the wild, so you cannot really leverage any of the choices’ advantages either way. Adjusting to MSVC’s behaviour would be right though, as you can at least properly distinguish between padding and 0-data with new binaries. > In practice this will tend to work as is, since we > are using SizeOfImage to allocate with, but as you > have pointed out there are so many edge cases that > having a difference here makes me worried we would > see something weird with only binaries built by one toolchain. There is plenty of room for that as-is, including that MSVC emits .rdata (and others), but GCC does not, and there is a super awkward heuristic in DxeCore to determine section permissions rather than using the PE values at face value - R-- is literally not respected. Also there had been issues with NASM sections, because PE needs .rdata, but ELF needs .rodata naming there. > I am curious to hear your thoughts though. This is > very easy to reproduce, build any UEFI binary with > MSVC and GCC and compare the headers. Yes, sorry for the confusion, this all looks as expected. Best regards, Marvin > > Thanks, > Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116820): https://edk2.groups.io/g/devel/message/116820 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] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: smime.p7s --] [-- Type: application/pkcs7-signature, Size: 2422 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize 2024-03-15 23:45 ` Marvin Häuser @ 2024-03-16 15:47 ` Oliver Smith-Denny 0 siblings, 0 replies; 15+ messages in thread From: Oliver Smith-Denny @ 2024-03-16 15:47 UTC (permalink / raw) To: devel, mhaeuser; +Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Taylor Beebe On 3/15/2024 4:45 PM, Marvin Häuser wrote: > >> On 15. Mar 2024, at 23:57, Oliver Smith-Denny <osde@linux.microsoft.com> wrote: >> >> I don't think this is what I'm saying. What I am trying to say is that >> on MSVC, I see PE images getting created that have VirtualSize set to >> the actual number of initialized bytes in that section (not padded to >> the section alignment). On ElfConverted binaries, I see the VirtualSize >> is padded to the section alignment. I've dropped an example below > > Ah, mismatched terminology. Zero-initialized as Ard and I used it refers to implicitly or explicitly 0-initialized global variables and such, which is not stored in the file, not the padding. So when you mentioned “real data”, I assumed you meant strictly the non-0 data from the file. Same misunderstanding with SizeOfImage, so that’s all fine. Whew. :) Ah gotcha, thanks I was figuring I was using the wrong terminology :). What you said makes sense and I see your concern based on what I said. > >> No, the specific case where I was researching this was explicitly >> setting /ALIGN:0x10000 and /FILEALIGN:0x1000 for DXE_RUNTIME_DRIVERs >> on ARM64 (a UEFI spec requirement). So I would see the SizeOfRawData >> is aligned to the file alignment, as expected, but VirtualSize would >> be the actual size of the data. Again, the troubling thing here for >> me is that the same binary built with gcc has the VirtualSize aligned >> to the section alignment. And I have seen other code that loads PE >> images that relies on VirtualSize not including the padding. The spec >> is vague here, it says VirtualSize is the size of the section as >> loaded in memory (which would lead me to believe this should include >> padding) but it does not explicitly say it should be a multiple of >> the section alignment (as other fields do). But at a minimum I think >> we should have different toolchains doing the same behavior here. > > Well, not rounding to pad is somewhat superior in some scenarios. If you round, you lose the information on what is section data and what is padding, so you might end up treating padding as data for some reason (because it is indistinguishable from mentioned 0-initialized data). This shouldn’t matter too much for executables and libraries, but MSVC/PE have a lot less of a distinction between object file and executable/library concepts (e.g. no distinction between sections and segments). That might be why they do it this way. > I agree, I've seen other environments that will use VirtualSize to set attributes for that section and then set stricter attributes, like RP on the padded section (if greater than a page). Point being that there are use cases and at least some folks relying on that definition of VirtualSize. >> See below for the VirtualSize examples, I'm confused on your comment on >> SizeOfImage. I agree that SizeOfImage covers everything as loaded into >> memory and I have not seen any issues there. > > See first comment. > >> Do you mind adding your RB to v2? And certainly if you have any other >> comments that is greatly appreciated. > > Will try to remember tomorrow. :) > Thanks! >> Examples of the differences I see between MSVC and gcc binaries: >> >> I originally noticed this on ARM64 on edk2, but wanted to make sure I >> saw it on x64 too, so this is with binaries from Project Mu's QemuQ35Pkg >> (edk2 doesn't have VS2022 support and I didn't feel like adding it >> or reverting back to VS2019). For reference, this is building the >> current top of tree at a4dd5d9785f48302c95fec852338d6bd26dd961a. >> >> I dumped ReportStatusCodeRouterRuntimeDxe.efi from both using dumpbin >> (from VS2022) to examine the PE headers. >> >> MSVC selected header values: >> >> Main header: >> 0x3200 size of code >> 0x2400 size of initialized data >> 0x0 size of uninitialized data >> 0x1000 section alignment >> 0x200 file alignment >> 0xB000 size of image >> >> 6 sections: .data, .pdata, .rdata, .reloc, .text, .xdata >> >> .text section: >> 0x30DF virtual size >> 0x3200 size of raw data >> >> .data section: >> 0x130 virtual size >> 0x200 size of raw data >> >> >> GCC ElfConverted selected header values: >> >> Main header: >> 0x4000 size of code >> 0x1000 size of initialized data >> 0x0 size of uninitialized data >> 0x1000 section alignment >> 0x1000 file alignment >> 0x7000 size of image >> >> 3 sections: .data, .text, .reloc >> >> .text section: >> 0x4000 virtual size >> 0x4000 size of raw data >> >> .data section: >> 0x1000 virtual size >> 0x1000 size of raw data >> >> So my concern here is that ElfConvert takes a >> different view of VirtualSize, that is should be >> section aligned, whereas MSVC binaries take >> VirtualSize to be the actual size without padding. >> I think the correct thing to do would be change >> ElfConvert to do what MSVC does (although the spec >> is vague and so I can understand the confusion). > > I don’t think it really matters, but it wouldn’t hurt either. Both kinds of binaries are in the wild, so you cannot really leverage any of the choices’ advantages either way. Adjusting to MSVC’s behaviour would be right though, as you can at least properly distinguish between padding and 0-data with new binaries. Yeah, I agree. I may take a look at this, but to your point from an earlier email, it may be safer to leave it as is, considering we have all these binaries in the wild already and changes in a broken environment are risky. Thanks, Oliver -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116823): https://edk2.groups.io/g/devel/message/116823 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] 15+ messages in thread
* [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 0 siblings, 1 reply; 15+ 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] 15+ 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 0 siblings, 1 reply; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ 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; 15+ 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] 15+ messages in thread
end of thread, other threads:[~2024-03-16 15:47 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20240227202721.30070-1-osde@...> [not found] ` <20240227202721.30070-2-osde@...> [not found] ` <CAMj1kXGDCsFZmPsKe4GD1XdT+t9SKHVStNSx-FsA_YwBiVh6ng@...> [not found] ` <5d95548d-d25f-4306-a271-a2960cc81ccf@...> [not found] ` <CAMj1kXGsso6BjOajEgqE3AOC1ZSdStQmeYmCoJMQo_tK1QHTWw@...> [not found] ` <531b3cfe-b57c-4b5a-97fd-45e428f97853@...> 2024-03-14 9:57 ` [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Marvin Häuser 2024-03-14 14:45 ` Oliver Smith-Denny 2024-03-14 21:57 ` Marvin Häuser 2024-03-15 22:56 ` Oliver Smith-Denny 2024-03-15 23:45 ` Marvin Häuser 2024-03-16 15:47 ` Oliver Smith-Denny 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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox