From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=uqEPHf1a; spf=pass (domain: apple.com, ip: 17.151.62.68, mailfrom: afish@apple.com) Received: from nwk-aaemail-lapp03.apple.com (nwk-aaemail-lapp03.apple.com [17.151.62.68]) by groups.io with SMTP; Wed, 07 Aug 2019 10:41:43 -0700 Received: from pps.filterd (nwk-aaemail-lapp03.apple.com [127.0.0.1]) by nwk-aaemail-lapp03.apple.com (8.16.0.27/8.16.0.27) with SMTP id x77HM6JK042831; Wed, 7 Aug 2019 10:41:41 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=sender : from : message-id : content-type : mime-version : subject : date : in-reply-to : cc : to : references; s=20180706; bh=Y301QjJ8Nd0prakI57AKr/5KNEesn5+wyScamwTxgHc=; b=uqEPHf1aPCYzNaRCfFYpt2QQdrRoyZhkSv+kQsr9B14Q6sCgCwVApDDvf4t4C0WDA/fS 2SzXMcKLE4NbfUwTICzszh6f2bYZUA0huCKYFLGB73WOPYydzcoywXCafj1d/X5P2fQO 5yyMiXFRf6dyi/xcowVZ5MQ/ZhVw7NKIVyh2M/aHBHO8M2VzI3fsFax2KdILQHFbk3+b GzfiniCldE3ETGtyOu8uXf/gS1yqjBhwZR72fKYqgUWFDAQ8NXjpjqCzvfTB70N/kIAx SIXpA2gJP2jKpmZJJRw6NYBPwYVVRPaYftiY9Nbfuf95FOmggL1nXvxOkgo1TFx+LqON WQ== Received: from ma1-mtap-s01.corp.apple.com (ma1-mtap-s01.corp.apple.com [17.40.76.5]) by nwk-aaemail-lapp03.apple.com with ESMTP id 2u5tgkb2gw-19 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Wed, 07 Aug 2019 10:41:41 -0700 Received: from nwk-mmpp-sz13.apple.com (nwk-mmpp-sz13.apple.com [17.128.115.216]) by ma1-mtap-s01.corp.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPS id <0PVV004Z5NTCQW70@ma1-mtap-s01.corp.apple.com>; Wed, 07 Aug 2019 10:41:38 -0700 (PDT) Received: from process_milters-daemon.nwk-mmpp-sz13.apple.com by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) id <0PVV00I00NDFYN00@nwk-mmpp-sz13.apple.com>; Wed, 07 Aug 2019 10:41:37 -0700 (PDT) X-Va-A: X-Va-T-CD: 08777febe38bb384cc57fda39d0586b7 X-Va-E-CD: d3bad307e4af6c7675d5029cab479fa4 X-Va-R-CD: 2fcd58ab292acf25a8ea4fedd159c548 X-Va-CD: 0 X-Va-ID: 34471929-6867-490e-a8cd-bf9b4cdbb09e X-V-A: X-V-T-CD: 08777febe38bb384cc57fda39d0586b7 X-V-E-CD: d3bad307e4af6c7675d5029cab479fa4 X-V-R-CD: 2fcd58ab292acf25a8ea4fedd159c548 X-V-CD: 0 X-V-ID: 6dee4f0b-80b4-4685-8344-a90f7bdd0f1c X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-07_04:,, signatures=0 Received: from [17.235.33.179] (unknown [17.235.33.179]) by nwk-mmpp-sz13.apple.com (Oracle Communications Messaging Server 8.0.2.4.20190507 64bit (built May 7 2019)) with ESMTPSA id <0PVV000S5NTBPL10@nwk-mmpp-sz13.apple.com>; Wed, 07 Aug 2019 10:41:37 -0700 (PDT) Sender: afish@apple.com From: "Andrew Fish" Message-id: <0A900AFC-C9A0-4A4C-8EBA-9A6F75B3EE25@apple.com> MIME-version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: Re: [edk2-devel] static data in dxe_runtime modules Date: Wed, 07 Aug 2019 10:41:20 -0700 In-reply-to: <406f2250-41e2-9925-b570-38b99a5f6e41@redhat.com> Cc: Roman Kagan , David Woodhouse To: devel@edk2.groups.io, lersek@redhat.com References: <20190801191621.GB14235@rkaganb.sw.ru> <8d18d4f6-5f33-44e9-2758-46350b43c5ec@redhat.com> <20190805101813.GA27171@rkaganb.sw.ru> <406f2250-41e2-9925-b570-38b99a5f6e41@redhat.com> X-Mailer: Apple Mail (2.3445.104.11) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-08-07_04:,, signatures=0 Content-type: multipart/alternative; boundary="Apple-Mail=_496BE210-2C38-4047-A8D2-182116A1787E" --Apple-Mail=_496BE210-2C38-4047-A8D2-182116A1787E Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=us-ascii > On Aug 7, 2019, at 10:29 AM, Laszlo Ersek wrote: >=20 > ummm... not sure why, but I never got this email in my inbox. I only see > it in my list folder. I see myself addressed on it as: >=20 > Laszlo Ersek via Groups.Io > >=20 > which could be the reason. >=20 > Anyway: >=20 > On 08/05/19 12:18, Roman Kagan wrote: >> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wr= ote: >>> On 08/01/19 21:16, Roman Kagan wrote: >>> This is a serious bug. Thank you for reporting and analyzing it. Can y= ou >>> file it in the TianoCore Bugzilla too, please? >>=20 >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D2053 >>=20 >>> I wonder how far in OpenSSL history this issue goes back. Is this a >>> regression from the latest OpenSSL rebase in edk2? >>=20 >> This is certainly not a recent regression. We've initially caught it >> with Virtuozzo OVMF based on the one from RHEL-7.6, based in turn on >> EDKII as of May 2018. However, the infrastructure that causes the >> problem is there for much longer. >=20 > OK. Thank you for confirming! >=20 >>>> What would be the best way to fix it? >>>>=20 >>>> One option is to audit OpenSSL and make sure it either doesn't put >>>> pointers into static storage or properly cleans them up (and call the >>>> cleanup function in RuntimeCryptLibAddressChangeEvent); then assume t= he >>>> rest of EDKII code is safe in this regard. >>>>=20 >>>> Another is to assume that no static data in dxe_runtime modules shoul= d >>>> survive SetVirtualAddressMap, and thus make >>>> PeCoffLoaderRelocateImageForRuntime reinitialize the modules from >>>> scratch instead of re-applying the relocations only. >>>>=20 >>>> I must admit I don't yet quite understand the full consequences of >>>> either option. Perhaps there are better ones. >>>> Any suggestion is appreciated. >>>=20 >>> If the runtime driver remembers pointer *values* from before >>> SetVirtualAddressMap() -- i.e. it saves pointer values into some >>> storage, for de-referencing at OS runtime --, those stored values have >>> to be converted in the virtual address change event notification funct= ion. >> [...] >>> The "thread_local_storage" array from >>> "CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be >>> exposed to RuntimeCryptLibAddressChangeEvent() somehow. >>>=20 >>> Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* API= s. >>=20 >> I think this would be too awkward, as edk2 has no reason to have any >> visibility into it. >=20 > Edk2 already implements various sets of APIs that "plug" into OpenSSL. >=20 > Not saying that it's optimal, but there is precedence. >=20 >> I'd rather make use of the observation that in reality there's no data >> in OpenSSL that needs to survive SetVirtualAddressMap(). At first I >> started to cook up a fix that involved calling OPENSSL_cleanup() from >> RuntimeCryptLibAddressChangeEvent(), but it soon turned out it didn't >> clean things up to the pristine state so it didn't address the problem. >>=20 >> Moreover I think it's overoptimistic to expect from OpenSSL developers >> the mindset that their code should work seamlessly across relocations a= t >> runtime. >=20 > Well, they do have a UEFI system ID in "Configurations/10-main.conf". >=20 > And I do think OpenSSL developers are interested in robustness over a > number of use cases. After all, the thread-specific key abstraction > exists for this kind of portability in the first place. >=20 >> I don't see what would stop them from introducing another >> pointer variable with global storage further down the road, and nothing >> would allow to even timely spot this new problem. >=20 > Edk2 could deal with this kind of problem a lot better if we timed our > OpenSSL submodule updates to the *start* of every edk2 development > cycle, not to the *end* of it. >=20 >> That's why I think the most reliable solution would be to just reload >> the module completely. If this can't be done for all runtime modules >> then I'd do it for the one(s) linking to OpenSSL. >=20 > I don't think we should special-case how > RuntimeDriverSetVirtualAddressMap() > [MdeModulePkg/Core/RuntimeDxe/Runtime.c] works. UEFI (the specification) > already specifies a general facility for this problem; we should use it. >=20 > I'm convinced that OpenSSL needs to expose a new API for this particular > problem. >=20 This feels right to me too. The reality is any pointer assigned in code ne= eds to get "fixed up" for the new virtual mapping. The other restriction is= non of the EFI Boot Services are available so you can't allocate memory et= c. The code that runs to fixup pointers runs in the physical mapping, but = has access to an API that can convert a physical address to its future virt= ual mapping.=20 If there is an API EFI can call to re-init passing in the old buffers that= may be the easiest thing to maintain. The EFI driver would then have a boo= lean flag and need to call the OpenSSL re-init on the 1st virtual call into= the driver. This would let the OpenSSL code function in a more normal fash= ion. Every thing else I can think of requires the OpenSSL code to keep a li= st of the pointers that need to be fixed up, and that would need to be main= tained by hand.=20 I can help answer any detailed questions about how EFI Runtime functions i= f that would help.=20 Thanks, Andrew Fish > David, can you please offer some thoughts on this? >=20 > Thanks! > Laszlo >=20 >>=20 >> Roman. >>=20 >>=20 >>=20 >=20 >=20 >=20 --Apple-Mail=_496BE210-2C38-4047-A8D2-182116A1787E Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=us-ascii
On Aug 7,= 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:

ummm... not sure why, but I never got t= his email in my inbox. I only see
it in my list folder. I see myself addressed on it as:=

 Laszlo Ersek via Groups.Io <l= ersek=3Dredhat.com@groups.io>

which could be = the reason.

Anyway:

On 08/05/19 12:18, Rom= an Kagan wrote:
On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io= wrote:
On 08/01/19 21:1= 6, Roman Kagan wrote:
This is a serious bug. Thank you for re= porting and analyzing it. Can you
file it in the TianoCore Bu= gzilla too, please?

https://bug= zilla.tianocore.org/show_bug.cgi?id=3D2053

I wonder how far in OpenSSL history t= his issue goes back. Is this a
regression from the latest Ope= nSSL rebase in edk2?

This is cert= ainly not a recent regression.  We've initially caught it
with Virtuozzo OVMF based on the one from RHEL-7.6, based in turn on
EDKII as of May 2018.  However, the infrastructure that caus= es the
problem is there for much longer.

OK. Thank you for conf= irming!

<= blockquote type=3D"cite" class=3D"">What would be the best way to fix it?
One option is to audit OpenSSL and make sure it= either doesn't put
pointers into static storage or properly = cleans them up (and call the
cleanup function in RuntimeCrypt= LibAddressChangeEvent); then assume the
rest of EDKII code is= safe in this regard.

Another is to assume tha= t no static data in dxe_runtime modules should
survive SetVir= tualAddressMap, and thus make
PeCoffLoaderRelocateImageForRun= time reinitialize the modules from
scratch instead of re-appl= ying the relocations only.

I must admit I don'= t yet quite understand the full consequences of
either option= .  Perhaps there are better ones.
Any suggestion is appr= eciated.

If the runtime driver re= members pointer *values* from before
SetVirtualAddressMap() -= - i.e. it saves pointer values into some
storage, for de-refe= rencing at OS runtime --, those stored values have
to be conv= erted in the virtual address change event notification function.
[...]
The "thread_local_storage" array from
"CryptoPkg/Library/Op= ensslLib/openssl/crypto/threads_none.c" has to be
exposed to = RuntimeCryptLibAddressChangeEvent() somehow.

P= erhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.

I think this would be too awkward, a= s edk2 has no reason to have any
visibility into it.

Edk2 alre= ady implements various sets of APIs that "plug" into OpenSSL.

Not saying that it's optimal, but there is precedence.

I'd rather make use of the observation that in reality there= 's no data
in OpenSSL that needs to survive SetVirtualAddress= Map().  At first I
started to cook up a fix that involve= d calling OPENSSL_cleanup() from
RuntimeCryptLibAddressChange= Event(), but it soon turned out it didn't
clean things up to = the pristine state so it didn't address the problem.

Moreover I think it's overoptimistic to expect from OpenSSL develope= rs
the mindset that their code should work seamlessly across = relocations at
runtime.

Well, they do have a UEFI system ID i= n "Configurations/10-main.conf".

And I do think OpenSSL dev= elopers are interested in robustness over a
number of use cases. After all, the thread-specific ke= y abstraction
exists fo= r this kind of portability in the first place.

I don't see what would stop them from introducing another
po= inter variable with global storage further down the road, and nothing
would allow to even timely spot this new problem.

Edk2 could deal wi= th this kind of problem a lot better if we timed our
OpenSSL submodule updates to the *start* of e= very edk2 development
c= ycle, not to the *end* of it.
=
That's why I thi= nk the most reliable solution would be to just reload
the mod= ule completely.  If this can't be done for all runtime modules
then I'd do it for the one(s) linking to OpenSSL.

I don't think we sho= uld special-case how
Ru= ntimeDriverSetVirtualAddressMap()
[MdeModulePkg/Core/RuntimeDxe/Runtime.c] works. UEFI (the spe= cification)
already spe= cifies a general facility for this problem; we should use it.

I'm convinced that OpenSSL needs to expose a new API for this particular=
problem.


This feels right to me too. T= he reality is any pointer assigned in code needs to get "fixed up" for the = new virtual mapping. The other restriction is non of the EFI Boot Services = are available so you can't allocate memory etc.  The code that runs to= fixup pointers runs in the physical mapping, but has access to an API that= can convert a physical address to its future virtual mapping. 
<= div>
If there is an API EFI can call to re-init pa= ssing in the old buffers that may be the easiest thing to maintain. The EFI= driver would then have a boolean flag and need to call the OpenSSL re-init= on the 1st virtual call into the driver. This would let the OpenSSL code f= unction in a more normal fashion. Every thing else I can think of requires = the OpenSSL code to keep a list of the pointers that need to be fixed up, a= nd that would need to be maintained by hand. 

I can help answer any detailed questions about how EFI Runtime = functions if that would help. 

Tha= nks,

Andrew Fish

David, can you pl= ease offer some thoughts on this?

Thanks!
Laszlo

<= br class=3D"">Roman.






--Apple-Mail=_496BE210-2C38-4047-A8D2-182116A1787E--