From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mout02.posteo.de (mout02.posteo.de [185.67.36.66]) by mx.groups.io with SMTP id smtpd.web12.5508.1628584998238226557 for ; Tue, 10 Aug 2021 01:43:19 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@posteo.de header.s=2017 header.b=FiZ8QSI9; spf=pass (domain: posteo.de, ip: 185.67.36.66, mailfrom: mhaeuser@posteo.de) Received: from submission (posteo.de [89.146.220.130]) by mout02.posteo.de (Postfix) with ESMTPS id 50FD3240104 for ; Tue, 10 Aug 2021 10:43:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=posteo.de; s=2017; t=1628584995; bh=9J/2H0xDUmfkY1Ca0g3CT5JWS/sg/7yhh9GOvUJ3mRc=; h=Subject:To:Cc:From:Date:From; b=FiZ8QSI9mRb4HCGZrHINlBFKveBOCC4tCGUXx32k8AJ8w+qr1A3NLWMxelsRZ8qgJ 3yZvejO1heB/8xn68UCyRwAFUxrbEIydyPMe/Bqtw7El4ama4za00S5D3Y9ZnHgX3r 8uz9b0INFXwlky/JvXRhryifnCjSD/yJ9J5R+YgoB2lm/4eD+sPeldNxeF/1FHQGzB L9R2FkMU7rvw7vrRyIurUE+uH+Ow6hRi48olluEWJIyYqoeA+DrerHCzkJzYvrp2EM AACEx/w7m/NGuxN9SwH7pO4eluJwQ6oqlmDNpoUXgJyRxGeMW/sMcHN6yZkSP7PCQf 0Tn2HNXn0uAKA== Received: from customer (localhost [127.0.0.1]) by submission (posteo.de) with ESMTPSA id 4GkRL952Rzz6tmN; Tue, 10 Aug 2021 10:43:13 +0200 (CEST) Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name To: devel@edk2.groups.io, afish@apple.com, Ray Ni Cc: "Dong, Eric" , "Kumar, Rahul1" , Vitaly Cheptsov References: <252525969122e83d9fb9b83edc95c4f6dfd233b4.1628502434.git.mhaeuser@posteo.de> <34c8c87e673ce08bef460179c5cb7b3ea9271208.1628502434.git.mhaeuser@posteo.de> <170EF66A-C746-4590-A317-0195B53E000A@apple.com> From: =?UTF-8?B?TWFydmluIEjDpHVzZXI=?= Message-ID: <630a8175-b5e6-8762-27f6-61e53a2f6d5d@posteo.de> Date: Tue, 10 Aug 2021 08:43:13 +0000 MIME-Version: 1.0 In-Reply-To: <170EF66A-C746-4590-A317-0195B53E000A@apple.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: quoted-printable On 10/08/2021 06:40, Andrew Fish via groups.io wrote: > > >> On Aug 9, 2021, at 7:43 PM, Ni, Ray > > wrote: >> >> Acked-by: Ray Ni > >> >> I will depend on tool owner to review the tool configuration change=20 >> making sure that the correct section name is chosen for different C=20 >> compilers. >> > > Ray, > > I made a detailed response about Mach-O with Xcode/clang and I don=E2=80= =99t=20 > think patch works. Not sure if it breaks anything, but it puts things=20 > in the .data PE/COFF section. The latter part is true only for Xcode-based toolchains, as far as I am=20 aware now, and this is in fact the way it works right now. > > I=E2=80=99m also worried it is broken for any toolchain that generates EL= F and=20 > use GenFw. I don=E2=80=99t think the GenFw tool creates a PE/COFF .rodata= =20 > section [1] so if things work they will end up in the .data section,=20 > or things might break? Some one who knows that tool better than me=20 > should take a detailed look. You are correct, but the NASM section name semantically translates to an=20 *ELF* .rodata section for usage during linking. The GNU linker scripts=20 merge it into .text later [1], yielding no PE/COFF .rodata section as=20 observed. > > I=E2=80=99m guessing it likely does the correct thing for toolchains that= =20 > generate PE/COFF directly? Behaviour should be fixed for PE-based toolchains, preserved correct for=20 ELF-based toolchains (but inconsistent with PE-based, as we merge=20 .rodata into .text here), and as correct or broken for Xcode-based=20 toolchains as it was before. > > My vote is to not add this feature until we can prove it works=20 > properly on all the toolchains. For Xcode it may be easier to just=20 > dump this stuff in the .text section (see my other mail for more=20 > background). If you can help with designing a solution, I'm more than happy to to=20 submit a V2. Just I don't know Xcode, and I don't run macOS. :) > It looks like we might have to modify GenFw if we want to create a=20 > .rodata section? For now, this patch un-breaks PE-based toolchains. They are broken in=20 the way described in the BZ, but not in a security-critical fashion.=20 Rather we have the worst of both worlds, the additional size of another=20 aligned section, and the downgraded permission as we would observe with=20 a merge into e.g. .text. I have no strong opinion on which route to=20 pick, i.e. dedicated .r(o)data or merging into .text, but I would like=20 it to be consistent in the end, especially across toolchains. Ideally,=20 in my opinion, the platform maintainer can choose whether they want the=20 extra protection (NX .rodata), or the extra space (.text merge). > > It might be possible to cheat and use this concept to force code into=20 > the text section for ELF and Mach-O, but I=E2=80=99m not sure if that hit= s the=20 > correct security bar. But the last thing we want is to claim something=20 > is in a read only section when it is in a read write section. I can check again later, but ELF should be fine, really. Best regards, Marvin [1]=20 https://github.com/tianocore/edk2/blob/d02dbb53cd78de799e6afaa237e98771fb51= 48db/BaseTools/Scripts/GccBase.lds#L25 > > [1] git grep CreateSectionHeader > BaseTools/Source/C/GenFw/Elf32Convert.c:602:*CreateSectionHeader*(".text"= ,=20 > mTextOffset, mDataOffset - mTextOffset, > BaseTools/Source/C/GenFw/Elf32Convert.c:612:*CreateSectionHeader*(".data"= ,=20 > mDataOffset, mHiiRsrcOffset - mDataOffset, > BaseTools/Source/C/GenFw/Elf32Convert.c:622:*CreateSectionHeader*(".rsrc"= ,=20 > mHiiRsrcOffset, mRelocOffset - mHiiRsrcOffset, > BaseTools/Source/C/GenFw/Elf32Convert.c:1107:*CreateSectionHeader*(".relo= c",=20 > mRelocOffset, mCoffOffset - mRelocOffset, > BaseTools/Source/C/GenFw/Elf64Convert.c:929:*CreateSectionHeader*(".text"= ,=20 > mTextOffset, mDataOffset - mTextOffset, > BaseTools/Source/C/GenFw/Elf64Convert.c:939:*CreateSectionHeader*(".data"= ,=20 > mDataOffset, mHiiRsrcOffset - mDataOffset, > BaseTools/Source/C/GenFw/Elf64Convert.c:949:*CreateSectionHeader*(".rsrc"= ,=20 > mHiiRsrcOffset, mRelocOffset - mHiiRsrcOffset, > BaseTools/Source/C/GenFw/Elf64Convert.c:1641:*CreateSectionHeader*(".relo= c",=20 > mRelocOffset, mCoffOffset - mRelocOffset, > BaseTools/Source/C/GenFw/ElfConvert.c:125:*CreateSectionHeader*( > BaseTools/Source/C/GenFw/ElfConvert.h:74:*CreateSectionHeader*( > > Thanks, > > Andrew Fish > >> Thanks, >> Ray >> >>> -----Original Message----- >>> From: Marvin H=C3=A4user > >>> Sent: Monday, August 9, 2021 5:51 PM >>> To:devel@edk2.groups.io >>> Cc: Dong, Eric >;=20 >>> Ni, Ray >; Kumar, Rahul1=20 >>> >; Vitaly=20 >>> Cheptsov >>> > >>> Subject: [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use=20 >>> toolchain-specific rodata section name >>> >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3318=20 >>> >>> >>> Correctly define the read-only data sections with the >>> toolchain-specific section name. This hardens image permission >>> security and may save image space. >>> >>> Cc: Eric Dong > >>> Cc: Ray Ni > >>> Cc: Rahul Kumar = > >>> Cc: Vitaly Cheptsov >> > >>> Signed-off-by: Marvin H=C3=A4user >> > >>> --- >>> UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm | 2 +- >>> UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm =C2=A0| 2 +- >>> 2 files changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm >>> b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm >>> index 5e27cc325012..cfb8bf4a5ae0 100644 >>> --- a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm >>> +++ b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm >>> @@ -6,7 +6,7 @@ >>> ;* >>> >>> ;----------------------------------------------------------------------= -------- >>> >>> >>> >>> - =C2=A0=C2=A0=C2=A0SECTION .rodata >>> >>> + =C2=A0=C2=A0=C2=A0SECTION RODATA_SECTION_NAME >>> >>> >>> >>> ; >>> >>> ; Float control word initial value: >>> >>> diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm >>> b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm >>> index 8485b4713548..3c976a21e391 100644 >>> --- a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm >>> +++ b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm >>> @@ -6,7 +6,7 @@ >>> ;* >>> >>> ;----------------------------------------------------------------------= -------- >>> >>> >>> >>> - =C2=A0=C2=A0=C2=A0SECTION .rodata >>> >>> + =C2=A0=C2=A0=C2=A0SECTION RODATA_SECTION_NAME >>> >>> ; >>> >>> ; Float control word initial value: >>> >>> ; all exceptions masked, double-extended-precision, round-to-nearest >>> >>> -- >>> 2.31.1 >> >> >> > >=20