From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 12DCCAC10BF for ; Thu, 14 Mar 2024 09:57:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zrllnEG21Q0toS+R98CdLjGRgxk2OT6HQltohze4+rE=; c=relaxed/simple; d=groups.io; h=From:Message-Id:Mime-Version:Subject:Date:In-Reply-To:Cc:To:References:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1710410244; v=1; b=O9HxEpIaWuoVAMNUIHIHtVdAjqV9hh8dEzOrvXemo8Wl899cx9o0f2cSViakFo+KbYi2eWty E6VO3KgcQ/xj2ds5ZtQUJK6Js/TFF1bljMZyElsscfxkuxw4SHFhGHenPFZ0dSzRUaKCzSLqvie 7z+cPGLO7TPsPdFImK+6gYJcJfZD/v43zGb/4TdBIik5ziFGfm+i73QYnTEfzqJ+2wRyozC50My 1U4oON+0sgdM1xLCEWLW7FL4KK1dQk7jNSM4JqGQDsArlE9zl3+q9K8NJMcJO4hT2Sacu+viLK9 12gvVwpWEu6OiWJUp+Ca/Hdm1Z+G2iJxlLCHIjL+2G/BA== X-Received: by 127.0.0.2 with SMTP id q2UUYY7687511xgaFnntEJO6; Thu, 14 Mar 2024 02:57:24 -0700 X-Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web10.8922.1710410243249066934 for ; Thu, 14 Mar 2024 02:57:23 -0700 X-Received: from submission (posteo.de [185.67.36.169]) by mout02.posteo.de (Postfix) with ESMTPS id 7AF03240101 for ; Thu, 14 Mar 2024 10:57:20 +0100 (CET) X-Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4TwN8Z40K5z9rxG; Thu, 14 Mar 2024 10:57:18 +0100 (CET) From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-Id: <8DF936BA-F1D0-4513-A8F4-36A2CBEFD84A@posteo.de> Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.500.171.1.1\)) Subject: Re: [edk2-devel][PATCH v1 1/3] MdeModulePkg: ImagePropertiesRecordLib: Use SectionAlignment for CodeSize Date: Thu, 14 Mar 2024 09:57:08 +0000 In-Reply-To: <531b3cfe-b57c-4b5a-97fd-45e428f97853@...> Cc: edk2-devel-groups-io , Leif Lindholm , Ard Biesheuvel , Sami Mujawar , Taylor Beebe To: Oliver Smith-Denny References: <20240227202721.30070-1-osde@...> <20240227202721.30070-2-osde@...> <5d95548d-d25f-4306-a271-a2960cc81ccf@...> <531b3cfe-b57c-4b5a-97fd-45e428f97853@...> Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Thu, 14 Mar 2024 02:57:23 -0700 Reply-To: devel@edk2.groups.io,mhaeuser@posteo.de List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: zTH8D3riWUIROEdA2g58pzKKx7686176AA= Content-Type: multipart/alternative; boundary="Apple-Mail=_FF3B8869-53CE-4D47-86E3-31A8FE2CDA32" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=O9HxEpIa; dmarc=fail reason="SPF not aligned (strict), DKIM not aligned (strict)" header.from=posteo.de (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --Apple-Mail=_FF3B8869-53CE-4D47-86E3-31A8FE2CDA32 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Good day everyone, Sorry for interjecting, but I=E2=80=99d 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 wrote: >=20 > 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 !=3D FileAlignment. Still no, for some reasons you already mentioned, but also reasons you didn= =E2=80=99t: - 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 =3D 0 was used to disable loading of debug d= ata, but SizeOfRawData remained intact, so it could be loaded on demand. - SizeOfRawData may be unaligned due to bugs. In fact, there is no reason t= o 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 byt= es 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). >=20 > The other is that the image loader partially uses VirtualSize when > loading: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1399-L1400 >=20 > However, when determining the size of a loaded image (and therefore the > number of pages to allocate) it will allocate an extra page: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652 >=20 Sorry, I don=E2=80=99t understand the =E2=80=9Chowever=E2=80=9D, this is so= und. > as ImageSize here is: >=20 > https://github.com/tianocore/edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2= c7dea0/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L312 >=20 > 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 Si= zeOfImage, but there likely are various legitimate-bugged ones out there. T= his 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 =3D SectionAlignment and SizeOfRawD= ata is aligned and VirtualSize is not. Even when both are aligned, the form= er 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 igno= red, but needs to be validated across the landscape). - Sections may have gaps (observed with =E2=80=9Cold" Linux EFI Shims iirc,= there=E2=80=99ll be plenty of broken images in the wild yet). - The extra page is not pointless - as you can see, it is allocated when ex= ceeding 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 =E2=80=9Cshift=E2=80=9D the image into proper alignment in the allocate= d region as needed. In AUDK, I resolved this by abstracting existing, burie= d aligned-allocation code into a MemoryAllocationLibEx class: https://githu= b.com/acidanthera/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeMod= ulePkg/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/BaseUefi= ImageLib/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 i= nformation 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 signi= ng 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. O= therwise, we will all be in a world of more pain. :) Thanks! Best regards, Marvin -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --Apple-Mail=_FF3B8869-53CE-4D47-86E3-31A8FE2CDA32 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Good day everyone,

=
Sorry for interjecting, but I=E2=80=99d like to avoid premature change= s to PE code that would only make the current mess even worse.

On 4. M= ar 2024, at 20:24, Oliver Smith-Denny <osde@...> wrote:

I relooked at the spec, you are correct, SizeOfRawData is aligned to<= br>the FileAlignment. So this is only a bug for a file with
SectionAlign= ment !=3D FileAlignment.

St= ill no, for some reasons you already mentioned, but also reasons you didn= =E2=80=99t:
- 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 =3D 0 was u= sed to disable loading of debug data, but SizeOfRawData remained intact, so= it could be loaded on demand.
- SizeOfRawData may be unaligned d= ue to bugs. In fact, there is no reason to trust FileAlignment, it has no r= eal meaning in an UEFI context (I am not even sure it does on Windows).
I see, yes I do believe VirtualSi= ze could be used here instead.

= *Must*. As Ard said, SizeOfRawData is only interesting to know how many byt= es to copy from the file and for *nothing* else.

Two
things give me pause. One is that the PE spec st= ates that SizeOfRawData
is rounded and VirtualSize is not, so that SizeO= fRawData may be greater
than the VirtualSize in some cases (which seems = incorrect).

The other is that the image loader partially uses Virtua= lSize when
loading:

https://github.com/tianocore/edk2/blob/918288= ab5a7c3abe9c58d576ccc0ae32e2c7dea0/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/918288ab5a7c3abe9c58d5= 76ccc0ae32e2c7dea0/MdeModulePkg/Core/Dxe/Image/Image.c#L646-L652


Sorry, I don=E2=80=99t understand= the =E2=80=9Chowever=E2=80=9D, this is sound.

as ImageSize here is:

https://github.com/tianocore/= edk2/blob/918288ab5a7c3abe9c58d576ccc0ae32e2c7dea0/MdePkg/Library/BasePeCof= fLib/BasePeCoff.c#L312

Which according to the spec, SizeOfImage is t= he size of the image as
loaded into memory and must be a multiple of sec= tion alignment.

Side note, = SizeOfImage may be borked, not only due to bugs, but also due to malice. Th= ere 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 nee= d 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 V= irtualSize here, I think
that is the right value to use, the only time w= e would have a
SizeOfRawData that is greater than the VirtualSize would = be if our
FileAlignment is greater than our SectionAlignment, which woul= d be
a misconfiguration.

No, it can simply be that FileAlignment =3D SectionAlignment and SizeOfRaw= Data is aligned and VirtualSize is not. Even when both are aligned, the for= mer may carry extra optional data (like previously mentioned debug data), o= r 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 b= e 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 ima= ge record code is
simpler as well (ensuring we are protecting the right = pages).

NO! :) There are *m= any* issues surrounding this:
- Sections may overlap (observed wi= th early macOS EFI images, could be ignored, but needs to be validated acro= ss the landscape).
- Sections may have gaps (observed with =E2=80= =9Cold" Linux EFI Shims iirc, there=E2=80=99ll be plenty of broken images i= n the wild yet).
- The extra page is not pointless - as you can s= ee, it is allocated when exceeding EFI page size, which is the only granula= rity you can page-allocate at. So, when you require > 4K alignment, you = need the extra page to be able to =E2=80=9Cshift=E2=80=9D the image into pr= oper alignment in the allocated region as needed. In AUDK, I resolved this = by abstracting existing, buried aligned-allocation code into a MemoryAlloca= tionLibEx class: https://github.com/acidanthe= ra/audk/blob/13f99fa7cbde0829ba98e1e4f2a03891a0791ac6/MdeModulePkg/Library/= CommonMemoryAllocationLib/CommonMemoryAllocationLibEx.c#L70
W= ithout aligned-allocation, you cannot get rid of this extra page.


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, t= o
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 tr= ust the spec.
- Do not trust the loader.
- Do not trust= the converter.
- Do *not* trust your intuition.

<= div>*Everything* is broken, from the producers to the consumers, from the s= igning to the validation tools, technically, security-wise, and design-wise= . Do *not* change things without as little as a collection of Windows and L= inux images from various versions and various OpROMs to regression-test wit= h. Otherwise, we will all be in a world of more pain. :)

Thanks!

Best regards,
Marvin
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

View/Reply Online (#116736) | =20 | Mute= This Topic | New Topic
Your Subscriptio= n | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--Apple-Mail=_FF3B8869-53CE-4D47-86E3-31A8FE2CDA32--