public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* static data in dxe_runtime modules
@ 2019-08-01 19:16 Roman Kagan
  2019-08-02 23:54 ` [edk2-devel] " Andrew Fish
  2019-08-03  2:03 ` Laszlo Ersek
  0 siblings, 2 replies; 15+ messages in thread
From: Roman Kagan @ 2019-08-01 19:16 UTC (permalink / raw)
  To: devel@edk2.groups.io

I'm trying to come up with a solution to the problem that OpenSSL
internally uses static variables ("per-thread" in no-threads config) to
store pointers, which remain unadjusted upon SetVirtualAddressMap and
cause the guest OS crash.

More specifically, trying to set a signed variable leads to the
following call chain:

VariableServiceSetVariable
  ProcessVarWithPk
    VerifyTimeBasedPayloadAndUpdate
      Pkcs7GetSigners
	ASN1_item_d2i
	  asn1_item_embed_d2i
	    asn1_template_ex_d2i
	      asn1_template_noexp_d2i
		asn1_item_embed_d2i
		  asn1_template_ex_d2i
		    asn1_template_noexp_d2i
		      asn1_item_embed_d2i
			asn1_template_ex_d2i
			  asn1_template_noexp_d2i
			    asn1_item_embed_d2i
			      pubkey_cb
				ERR_set_mark
				  ERR_get_state

The latter performs one-time initialization if needed, and then returns
a pointer maintained in a static array using CRYPTO_THREAD_get_local().
If a signed variable is set before SetVirtualAddressMap and (another
one) after it, the pointer is initialized while in the old mapping and
dereferenced while in the new one, crashing the OS.

The real-world reproducer: install Windows Server 2016 in a VM with
OVMF, shut it down, replace the varstore with a fresh template copy,
boot Windows, try to update, say, "dbx" from within Windows => BSOD.
The reason is that the Windows bootloader stores a secure variable
"CurrentPolicy" if it isn't there, triggering the above callchain while
still in identity mapping.
(This problem isn't widely noticed because during the installation the
bootloader stores CurrentPolicy, but the OS is soon rebooted, before it
attempts to update dbx; upon that CurrentPolicy remains in the varstore
and on further boots the bootloader skips setting it.)

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 RuntimeCryptLibAddressChangeEvent); then assume the
rest of EDKII code is safe in this regard.

Another is to assume that no static data in dxe_runtime modules should
survive SetVirtualAddressMap, and thus make
PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
scratch instead of re-applying 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 appreciated.

Thanks,
Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-01 19:16 static data in dxe_runtime modules Roman Kagan
@ 2019-08-02 23:54 ` Andrew Fish
  2019-08-03  2:03 ` Laszlo Ersek
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Fish @ 2019-08-02 23:54 UTC (permalink / raw)
  To: Roman Kagan, devel

[-- Attachment #1: Type: text/plain, Size: 1004 bytes --]

Roman,

Sorry for the top post, as my email client seems to have eating your email.

1) In C static scopes the visibility of the symbol from a compiler linker point of view, but the static and non-static globals are going to be stored in the same section of the image. On macOS clang the only difference is the static global does not also have a .globl assembly directive to make the label visible outside the scope of the module.

In general Runtime Drivers will gather data that is only available at boot time and store this data in globals, so you can't clear out part of the image on fix-up as that would break a lot of other stuff.

2) The corrrect answer would be to fixup any pointers needed at runtime via RuntimeCryptLibAddressChangeEvent(). On option would be to have an array of the addresses of the static variables and have RuntimeCryptLibAddressChangeEvent() walk that and fixup the Physical addresses to virtual addresses by calling EfiConvertPointer().

Thanks,

Andrew Fish

[-- Attachment #2: Type: text/html, Size: 1157 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-01 19:16 static data in dxe_runtime modules Roman Kagan
  2019-08-02 23:54 ` [edk2-devel] " Andrew Fish
@ 2019-08-03  2:03 ` Laszlo Ersek
  2019-08-05 10:18   ` Roman Kagan
  1 sibling, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-03  2:03 UTC (permalink / raw)
  To: Roman Kagan, devel

On 08/01/19 21:16, Roman Kagan wrote:
> I'm trying to come up with a solution to the problem that OpenSSL
> internally uses static variables ("per-thread" in no-threads config) to
> store pointers, which remain unadjusted upon SetVirtualAddressMap and
> cause the guest OS crash.
> 
> More specifically, trying to set a signed variable leads to the
> following call chain:
> 
> VariableServiceSetVariable
>   ProcessVarWithPk
>     VerifyTimeBasedPayloadAndUpdate
>       Pkcs7GetSigners
> 	ASN1_item_d2i
> 	  asn1_item_embed_d2i
> 	    asn1_template_ex_d2i
> 	      asn1_template_noexp_d2i
> 		asn1_item_embed_d2i
> 		  asn1_template_ex_d2i
> 		    asn1_template_noexp_d2i
> 		      asn1_item_embed_d2i
> 			asn1_template_ex_d2i
> 			  asn1_template_noexp_d2i
> 			    asn1_item_embed_d2i
> 			      pubkey_cb
> 				ERR_set_mark
> 				  ERR_get_state
> 
> The latter performs one-time initialization if needed, and then returns
> a pointer maintained in a static array using CRYPTO_THREAD_get_local().
> If a signed variable is set before SetVirtualAddressMap and (another
> one) after it, the pointer is initialized while in the old mapping and
> dereferenced while in the new one, crashing the OS.
> 
> The real-world reproducer: install Windows Server 2016 in a VM with
> OVMF, shut it down, replace the varstore with a fresh template copy,
> boot Windows, try to update, say, "dbx" from within Windows => BSOD.
> The reason is that the Windows bootloader stores a secure variable
> "CurrentPolicy" if it isn't there, triggering the above callchain while
> still in identity mapping.
> (This problem isn't widely noticed because during the installation the
> bootloader stores CurrentPolicy, but the OS is soon rebooted, before it
> attempts to update dbx; upon that CurrentPolicy remains in the varstore
> and on further boots the bootloader skips setting it.)

This is a serious bug. Thank you for reporting and analyzing it. Can you
file it in the TianoCore Bugzilla too, please?

I think it's of general interest; virtualization is one way to reproduce
it, but it's not exclusive. Loss or wipign of UEFI variables can occur
on physical hardware too.

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

> 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 RuntimeCryptLibAddressChangeEvent); then assume the
> rest of EDKII code is safe in this regard.
> 
> Another is to assume that no static data in dxe_runtime modules should
> survive SetVirtualAddressMap, and thus make
> PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
> scratch instead of re-applying 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 appreciated.

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 function.

If your runtime driver says

  Ptr = &mGlobalVariable;
  Ptr->Field = 1;

at runtime, you don't have to do anything. If your driver says

  mPtr = &mGlobalVariable;

at boot time, and then at OS runtime it does

  mPtr->Field = 1;

then "mPtr" has to be converted, with EfiConvertPointer() in
"MdePkg/Library/UefiRuntimeLib/RuntimeLib.c".

PE/COFF massaging is unneeded.

The "thread_local_storage" array from
"CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be
exposed to RuntimeCryptLibAddressChangeEvent() somehow.

Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-03  2:03 ` Laszlo Ersek
@ 2019-08-05 10:18   ` Roman Kagan
  2019-08-07 17:29     ` Laszlo Ersek
  0 siblings, 1 reply; 15+ messages in thread
From: Roman Kagan @ 2019-08-05 10:18 UTC (permalink / raw)
  To: Laszlo Ersek via Groups.Io; +Cc: devel@edk2.groups.io

On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
> On 08/01/19 21:16, Roman Kagan wrote:
> This is a serious bug. Thank you for reporting and analyzing it. Can you
> file it in the TianoCore Bugzilla too, please?

https://bugzilla.tianocore.org/show_bug.cgi?id=2053

> I wonder how far in OpenSSL history this issue goes back. Is this a
> regression from the latest OpenSSL rebase in edk2?

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.

> > 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 RuntimeCryptLibAddressChangeEvent); then assume the
> > rest of EDKII code is safe in this regard.
> > 
> > Another is to assume that no static data in dxe_runtime modules should
> > survive SetVirtualAddressMap, and thus make
> > PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
> > scratch instead of re-applying 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 appreciated.
> 
> 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 function.
[...]
> The "thread_local_storage" array from
> "CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be
> exposed to RuntimeCryptLibAddressChangeEvent() somehow.
> 
> Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.

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

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.

Moreover I think it's overoptimistic to expect from OpenSSL developers
the mindset that their code should work seamlessly across relocations at
runtime.  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.

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.

Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-05 10:18   ` Roman Kagan
@ 2019-08-07 17:29     ` Laszlo Ersek
  2019-08-07 17:41       ` Andrew Fish
  0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-07 17:29 UTC (permalink / raw)
  To: Roman Kagan, devel, David Woodhouse

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:

  Laszlo Ersek via Groups.Io <lersek=redhat.com@groups.io>

which could be the reason.

Anyway:

On 08/05/19 12:18, Roman Kagan wrote:
> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>> On 08/01/19 21:16, Roman Kagan wrote:
>> This is a serious bug. Thank you for reporting and analyzing it. Can you
>> file it in the TianoCore Bugzilla too, please?
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2053
> 
>> I wonder how far in OpenSSL history this issue goes back. Is this a
>> regression from the latest OpenSSL rebase in edk2?
> 
> 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.

OK. Thank you for confirming!

>>> 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 RuntimeCryptLibAddressChangeEvent); then assume the
>>> rest of EDKII code is safe in this regard.
>>>
>>> Another is to assume that no static data in dxe_runtime modules should
>>> survive SetVirtualAddressMap, and thus make
>>> PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
>>> scratch instead of re-applying 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 appreciated.
>>
>> 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 function.
> [...]
>> The "thread_local_storage" array from
>> "CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be
>> exposed to RuntimeCryptLibAddressChangeEvent() somehow.
>>
>> Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.
> 
> I think this would be too awkward, as edk2 has no reason to have any
> visibility into it.

Edk2 already 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 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.
> 
> Moreover I think it's overoptimistic to expect from OpenSSL developers
> the mindset that their code should work seamlessly across relocations at
> runtime.

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

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.

> 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.

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.

> 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.

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.

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

David, can you please offer some thoughts on this?

Thanks!
Laszlo

> 
> Roman.
> 
> 
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-07 17:29     ` Laszlo Ersek
@ 2019-08-07 17:41       ` Andrew Fish
  2019-08-08 17:39         ` Laszlo Ersek
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Fish @ 2019-08-07 17:41 UTC (permalink / raw)
  To: devel, lersek; +Cc: Roman Kagan, David Woodhouse

[-- Attachment #1: Type: text/plain, Size: 5598 bytes --]



> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> 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:
> 
>  Laszlo Ersek via Groups.Io <lersek=redhat.com@groups.io <mailto:lersek=redhat.com@groups.io>>
> 
> which could be the reason.
> 
> Anyway:
> 
> On 08/05/19 12:18, Roman Kagan wrote:
>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>> On 08/01/19 21:16, Roman Kagan wrote:
>>> This is a serious bug. Thank you for reporting and analyzing it. Can you
>>> file it in the TianoCore Bugzilla too, please?
>> 
>> https://bugzilla.tianocore.org/show_bug.cgi?id=2053
>> 
>>> I wonder how far in OpenSSL history this issue goes back. Is this a
>>> regression from the latest OpenSSL rebase in edk2?
>> 
>> 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.
> 
> OK. Thank you for confirming!
> 
>>>> 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 RuntimeCryptLibAddressChangeEvent); then assume the
>>>> rest of EDKII code is safe in this regard.
>>>> 
>>>> Another is to assume that no static data in dxe_runtime modules should
>>>> survive SetVirtualAddressMap, and thus make
>>>> PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
>>>> scratch instead of re-applying 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 appreciated.
>>> 
>>> 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 function.
>> [...]
>>> The "thread_local_storage" array from
>>> "CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be
>>> exposed to RuntimeCryptLibAddressChangeEvent() somehow.
>>> 
>>> Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.
>> 
>> I think this would be too awkward, as edk2 has no reason to have any
>> visibility into it.
> 
> Edk2 already 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 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.
>> 
>> Moreover I think it's overoptimistic to expect from OpenSSL developers
>> the mindset that their code should work seamlessly across relocations at
>> runtime.
> 
> Well, they do have a UEFI system ID in "Configurations/10-main.conf".
> 
> 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.
> 
>> 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.
> 
> 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.
> 
>> 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.
> 
> 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.
> 
> I'm convinced that OpenSSL needs to expose a new API for this particular
> problem.
> 

This feels right to me too. The 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. 

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 boolean 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 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, and that would need to be maintained by hand. 

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

Thanks,

Andrew Fish

> David, can you please offer some thoughts on this?
> 
> Thanks!
> Laszlo
> 
>> 
>> Roman.
>> 
>> 
>> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 35455 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-07 17:41       ` Andrew Fish
@ 2019-08-08 17:39         ` Laszlo Ersek
  2019-08-09 16:07           ` Roman Kagan
       [not found]           ` <15B94CD6CF07DEE2.13696@groups.io>
  0 siblings, 2 replies; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-08 17:39 UTC (permalink / raw)
  To: Andrew Fish, Roman Kagan; +Cc: devel, David Woodhouse

On 08/07/19 19:41, Andrew Fish wrote:
> 
> 
>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> 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:
>>
>>  Laszlo Ersek via Groups.Io <lersek=redhat.com@groups.io <mailto:lersek=redhat.com@groups.io>>
>>
>> which could be the reason.
>>
>> Anyway:
>>
>> On 08/05/19 12:18, Roman Kagan wrote:
>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>>> On 08/01/19 21:16, Roman Kagan wrote:
>>>> This is a serious bug. Thank you for reporting and analyzing it. Can you
>>>> file it in the TianoCore Bugzilla too, please?
>>>
>>> https://bugzilla.tianocore.org/show_bug.cgi?id=2053
>>>
>>>> I wonder how far in OpenSSL history this issue goes back. Is this a
>>>> regression from the latest OpenSSL rebase in edk2?
>>>
>>> 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.
>>
>> OK. Thank you for confirming!
>>
>>>>> 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 RuntimeCryptLibAddressChangeEvent); then assume the
>>>>> rest of EDKII code is safe in this regard.
>>>>>
>>>>> Another is to assume that no static data in dxe_runtime modules should
>>>>> survive SetVirtualAddressMap, and thus make
>>>>> PeCoffLoaderRelocateImageForRuntime reinitialize the modules from
>>>>> scratch instead of re-applying 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 appreciated.
>>>>
>>>> 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 function.
>>> [...]
>>>> The "thread_local_storage" array from
>>>> "CryptoPkg/Library/OpensslLib/openssl/crypto/threads_none.c" has to be
>>>> exposed to RuntimeCryptLibAddressChangeEvent() somehow.
>>>>
>>>> Perhaps OpenSSL should allow edk2 to bring its own CRYPTO_THREAD_* APIs.
>>>
>>> I think this would be too awkward, as edk2 has no reason to have any
>>> visibility into it.
>>
>> Edk2 already 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 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.
>>>
>>> Moreover I think it's overoptimistic to expect from OpenSSL developers
>>> the mindset that their code should work seamlessly across relocations at
>>> runtime.
>>
>> Well, they do have a UEFI system ID in "Configurations/10-main.conf".
>>
>> 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.
>>
>>> 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.
>>
>> 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.
>>
>>> 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.
>>
>> 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.
>>
>> I'm convinced that OpenSSL needs to expose a new API for this particular
>> problem.
>>
> 
> This feels right to me too. The 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. 
> 
> 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 boolean 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 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, and that would need to be maintained by hand. 
> 
> I can help answer any detailed questions about how EFI Runtime functions if that would help. 

Thanks, Andrew!

Roman: I've been pondering why I've never seen this issue myself. I
think now I know why.

(1.1) The issue is specific to the following library instance:

  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

That is the lib instance to which RuntimeCryptLibAddressChangeEvent()
belongs to.

It is also the lib instance through wich OpenSSL is linked into
DXE_RUNTIME_DRIVER modules -- see the dependency on the OpensslLib class.

(1.2) When you build the OvmfPkg and ArmVirtPkg platforms with

  -D SECURE_BOOT_ENABLE

the following DXE_RUNTIME_DRIVER will be included:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

(1.3) This runtime driver depends on the AuthVariableLib class. With
SECURE_BOOT_ENABLE, the DSC files in question resolve AuthVariableLib to

  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

(1.4) That library instance depends on the BaseCryptLib class. Given
that VariableRuntimeDxe is a DXE_RUNTIME_DRIVER, the following
BaseCryptLib resolution from the DSC files takes effect:

  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

And that's how you end up with an instance of OpenSSL that
- is statically linked into a DXE_RUNTIME_DRIVER,
- and is therefore subject to SetVirtualAddressMap().


(2) The problem with building ArmVirtPkg and OvmfPkg platforms with *just*

  -D SECURE_BOOT_ENABLE

is that those builds do not protect the variable driver, or the
underlying variable storage (in pflash), from tampering by the OS. There
is no hardware-based barrier between the authenticated UEFI variables
and the OS -- the OS can overwrite the variable store with direct
hardware (pflash) access, and that is a problem for Secure Boot.

This is why SECURE_BOOT_ENABLE is not production-ready in ArmVirtPkg,
and also why SECURE_BOOT_ENABLE is production-ready in OVMF *only* when
it's combined with:

  -D SMM_REQUIRE

So let's see what happens when SMM_REQUIRE is *also* set in OVMF.

(2.1) The variable driver is split in two halves,

- a non-privileged DXE_RUNTIME_DRIVER part, which only formats request
buffers for, and parses response buffers from, the privileged half,

- and a privileged half, which runs in SMM, and validates and serves (or
rejects) requests from the non-privileged half.

The halves are:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf

(2.2) The unprivileged half is still a DXE_RUNTIME_DRIVER, but it does
not depend on the AuthVariableLib class. The privileged half is a
DXE_SMM_DRIVER, and it is the one to depend on AuthVariableLib.

In other words, the authentication of UEFI variable updates is moved
into SMM.

(2.3) The same AuthVariableLib instance, that is,

  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

continues to depend on the BaseCryptLib class. However, this time we're
discussing a DXE_SMM_DRIVER. For that, BaseCryptLib is resolved as
follows, in the OVMF DSC files:

  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf

(2.4) That library instance pulls in OpenSSL again (via the OpensslLib
class dependency) -- but now OpenSSL will exist in SMRAM, and run in SMM
at OS runtime, as part of the "VariableSmm" driver.

Importantly, because the code runs in SMM, it is not subject to
SetVirtualAddressMap() / runtime relocation. SMM uses its own page
tables, which
- live in SMRAM,
- implement identity-mapping,
- implement a number of access protection schemes.

Thus, whatever pointer values OpenSSL creates initially, when
VariableSmm is first dispatched, those pointers will point into SMRAM.
And the same pointer values remain valid (into SMRAM) regardless of
when, or if, SetVirtualAddressMap() is called by the OS (or OS bootloader).

In other words, the problem doesn't exist when OpenSSL (with the rest of
the variable driver stack) is protected with SMM, as pointers into SMRAM
remain valid "forever", after the initial SMM driver dispatch.

Thanks,
Laszlo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-08 17:39         ` Laszlo Ersek
@ 2019-08-09 16:07           ` Roman Kagan
       [not found]           ` <15B94CD6CF07DEE2.13696@groups.io>
  1 sibling, 0 replies; 15+ messages in thread
From: Roman Kagan @ 2019-08-09 16:07 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Andrew Fish, devel@edk2.groups.io, David Woodhouse

On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
> On 08/07/19 19:41, Andrew Fish wrote:
> >> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >> 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:
> >>
> >>  Laszlo Ersek via Groups.Io <lersek=redhat.com@groups.io <mailto:lersek=redhat.com@groups.io>>

I've adjusted my mail config to honor the 'reply-to:' header, should be
ok now.

> >> On 08/05/19 12:18, Roman Kagan wrote:
> >>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
> >>>> On 08/01/19 21:16, Roman Kagan wrote:
> >> I'm convinced that OpenSSL needs to expose a new API for this particular
> >> problem.

Since, as you point out below, the problem only affects the essentially
broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
saving time and effort and sticking to the hack-ish approach proposed in
the bugzilla issue, which is to iterate over "thread-local" pointers and
EfiConvertPointer() on each.  (As long as it fixes the problem of
course; I'll test and report back.)  So we should be good without a new
API from OpenSSL.

> In other words, the problem doesn't exist when OpenSSL (with the rest of
> the variable driver stack) is protected with SMM, as pointers into SMRAM
> remain valid "forever", after the initial SMM driver dispatch.

Makes perfect sense.  We happen to build this broken configuration for
some historical reasons, I'm failing to recall exactly which.  Will try
to get rid of it.

Thanks,
Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
       [not found]           ` <15B94CD6CF07DEE2.13696@groups.io>
@ 2019-08-12 18:43             ` Roman Kagan
  2019-08-13  9:10               ` Laszlo Ersek
  0 siblings, 1 reply; 15+ messages in thread
From: Roman Kagan @ 2019-08-12 18:43 UTC (permalink / raw)
  To: devel@edk2.groups.io; +Cc: Laszlo Ersek, Andrew Fish, David Woodhouse

On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
> > On 08/07/19 19:41, Andrew Fish wrote:
> > >> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> > >> On 08/05/19 12:18, Roman Kagan wrote:
> > >>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
> > >>>> On 08/01/19 21:16, Roman Kagan wrote:
> > >> I'm convinced that OpenSSL needs to expose a new API for this particular
> > >> problem.
> 
> Since, as you point out below, the problem only affects the essentially
> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
> saving time and effort and sticking to the hack-ish approach proposed in
> the bugzilla issue, which is to iterate over "thread-local" pointers and
> EfiConvertPointer() on each.  (As long as it fixes the problem of
> course; I'll test and report back.)

It doesn't :(  It just gets slightly further and hits another static
pointer variable which is not part of the thread-local array:

...
  Pkcs7Verify
    EVP_add_digest
      OBJ_NAME_add

this one uses a few static pointer variables that are also initialized
on demand and become stale upon SetVirtualAddressMap().

> So we should be good without a new API from OpenSSL.

> > In other words, the problem doesn't exist when OpenSSL (with the rest of
> > the variable driver stack) is protected with SMM, as pointers into SMRAM
> > remain valid "forever", after the initial SMM driver dispatch.
> 
> Makes perfect sense.  We happen to build this broken configuration for
> some historical reasons, I'm failing to recall exactly which.  Will try
> to get rid of it.

We appear to have some i440fx-based VMs with SecureBoot in the field
(dunno about their origin) and those don't allow SMM.

Thanks,
Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-12 18:43             ` Roman Kagan
@ 2019-08-13  9:10               ` Laszlo Ersek
  2019-08-13 11:23                 ` Roman Kagan
  0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-13  9:10 UTC (permalink / raw)
  To: Roman Kagan, devel, Andrew Fish, David Woodhouse

On 08/12/19 20:43, Roman Kagan wrote:
> On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
>> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
>>> On 08/07/19 19:41, Andrew Fish wrote:
>>>>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>> On 08/05/19 12:18, Roman Kagan wrote:
>>>>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>>>>>> On 08/01/19 21:16, Roman Kagan wrote:
>>>>> I'm convinced that OpenSSL needs to expose a new API for this particular
>>>>> problem.
>>
>> Since, as you point out below, the problem only affects the essentially
>> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
>> saving time and effort and sticking to the hack-ish approach proposed in
>> the bugzilla issue, which is to iterate over "thread-local" pointers and
>> EfiConvertPointer() on each.  (As long as it fixes the problem of
>> course; I'll test and report back.)
> 
> It doesn't :(  It just gets slightly further and hits another static
> pointer variable which is not part of the thread-local array:
> 
> ...
>   Pkcs7Verify
>     EVP_add_digest
>       OBJ_NAME_add
> 
> this one uses a few static pointer variables that are also initialized
> on demand and become stale upon SetVirtualAddressMap().

So it looks like the issue can't be solved without making OpenSSL aware
of this use case.

>> So we should be good without a new API from OpenSSL.
> 
>>> In other words, the problem doesn't exist when OpenSSL (with the rest of
>>> the variable driver stack) is protected with SMM, as pointers into SMRAM
>>> remain valid "forever", after the initial SMM driver dispatch.
>>
>> Makes perfect sense.  We happen to build this broken configuration for
>> some historical reasons, I'm failing to recall exactly which.  Will try
>> to get rid of it.
> 
> We appear to have some i440fx-based VMs with SecureBoot in the field
> (dunno about their origin) and those don't allow SMM.

Indeed i440fx has no support for SMM/SMRAM -- but that only means that
Secure Boot cannot be made actually secure on i440fx.

Red Hat used to offer an OVMF binary that ran on i440fx too, and
included the Secure Boot feature without SMM. But the package containing
that binary was always marked as Tech Preview. When we exited Tech
Preview with OVMF, we removed said binary.

Thanks
Laszlo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-13  9:10               ` Laszlo Ersek
@ 2019-08-13 11:23                 ` Roman Kagan
  2019-08-14 15:16                   ` Laszlo Ersek
  0 siblings, 1 reply; 15+ messages in thread
From: Roman Kagan @ 2019-08-13 11:23 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: devel@edk2.groups.io, Andrew Fish, David Woodhouse

On Tue, Aug 13, 2019 at 11:10:27AM +0200, Laszlo Ersek wrote:
> On 08/12/19 20:43, Roman Kagan wrote:
> > On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
> >> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
> >>> On 08/07/19 19:41, Andrew Fish wrote:
> >>>>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> >>>>> On 08/05/19 12:18, Roman Kagan wrote:
> >>>>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
> >>>>>>> On 08/01/19 21:16, Roman Kagan wrote:
> >>>>> I'm convinced that OpenSSL needs to expose a new API for this particular
> >>>>> problem.
> >>
> >> Since, as you point out below, the problem only affects the essentially
> >> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
> >> saving time and effort and sticking to the hack-ish approach proposed in
> >> the bugzilla issue, which is to iterate over "thread-local" pointers and
> >> EfiConvertPointer() on each.  (As long as it fixes the problem of
> >> course; I'll test and report back.)
> > 
> > It doesn't :(  It just gets slightly further and hits another static
> > pointer variable which is not part of the thread-local array:
> > 
> > ...
> >   Pkcs7Verify
> >     EVP_add_digest
> >       OBJ_NAME_add
> > 
> > this one uses a few static pointer variables that are also initialized
> > on demand and become stale upon SetVirtualAddressMap().
> 
> So it looks like the issue can't be solved without making OpenSSL aware
> of this use case.

Is reloading the module from scratch ruled out completely?

I'd try to cook up a patch for that unless there's a strong no-go.

Thanks,
Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-13 11:23                 ` Roman Kagan
@ 2019-08-14 15:16                   ` Laszlo Ersek
  2019-08-14 16:26                     ` Andrew Fish
  0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-14 15:16 UTC (permalink / raw)
  To: Roman Kagan, devel, Andrew Fish, David Woodhouse

On 08/13/19 13:23, Roman Kagan wrote:
> On Tue, Aug 13, 2019 at 11:10:27AM +0200, Laszlo Ersek wrote:
>> On 08/12/19 20:43, Roman Kagan wrote:
>>> On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
>>>> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
>>>>> On 08/07/19 19:41, Andrew Fish wrote:
>>>>>>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>>> On 08/05/19 12:18, Roman Kagan wrote:
>>>>>>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>>>>>>>> On 08/01/19 21:16, Roman Kagan wrote:
>>>>>>> I'm convinced that OpenSSL needs to expose a new API for this particular
>>>>>>> problem.
>>>>
>>>> Since, as you point out below, the problem only affects the essentially
>>>> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
>>>> saving time and effort and sticking to the hack-ish approach proposed in
>>>> the bugzilla issue, which is to iterate over "thread-local" pointers and
>>>> EfiConvertPointer() on each.  (As long as it fixes the problem of
>>>> course; I'll test and report back.)
>>>
>>> It doesn't :(  It just gets slightly further and hits another static
>>> pointer variable which is not part of the thread-local array:
>>>
>>> ...
>>>   Pkcs7Verify
>>>     EVP_add_digest
>>>       OBJ_NAME_add
>>>
>>> this one uses a few static pointer variables that are also initialized
>>> on demand and become stale upon SetVirtualAddressMap().
>>
>> So it looks like the issue can't be solved without making OpenSSL aware
>> of this use case.
> 
> Is reloading the module from scratch ruled out completely?

Not my place to say authoritatively, but:
- it would be a first, as much as I can say,
- it would duplicate (in purpose) an existing facility.

Personally I'd expect it to be rejected, but it's not up to me. If
you're willing to "build one to (possibly) throw away", that could be
the most direct way to get authoritative (= maintainer) feedback.

Thanks
Laszlo

> I'd try to cook up a patch for that unless there's a strong no-go.
> 
> Thanks,
> Roman.
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-14 15:16                   ` Laszlo Ersek
@ 2019-08-14 16:26                     ` Andrew Fish
  2019-08-16 15:22                       ` Laszlo Ersek
  2019-08-16 17:00                       ` Roman Kagan
  0 siblings, 2 replies; 15+ messages in thread
From: Andrew Fish @ 2019-08-14 16:26 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: Roman Kagan, devel, David Woodhouse

[-- Attachment #1: Type: text/plain, Size: 3958 bytes --]



> On Aug 14, 2019, at 8:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> On 08/13/19 13:23, Roman Kagan wrote:
>> On Tue, Aug 13, 2019 at 11:10:27AM +0200, Laszlo Ersek wrote:
>>> On 08/12/19 20:43, Roman Kagan wrote:
>>>> On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
>>>>> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
>>>>>> On 08/07/19 19:41, Andrew Fish wrote:
>>>>>>>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>>>> On 08/05/19 12:18, Roman Kagan wrote:
>>>>>>>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>>>>>>>>> On 08/01/19 21:16, Roman Kagan wrote:
>>>>>>>> I'm convinced that OpenSSL needs to expose a new API for this particular
>>>>>>>> problem.
>>>>> 
>>>>> Since, as you point out below, the problem only affects the essentially
>>>>> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
>>>>> saving time and effort and sticking to the hack-ish approach proposed in
>>>>> the bugzilla issue, which is to iterate over "thread-local" pointers and
>>>>> EfiConvertPointer() on each.  (As long as it fixes the problem of
>>>>> course; I'll test and report back.)
>>>> 
>>>> It doesn't :(  It just gets slightly further and hits another static
>>>> pointer variable which is not part of the thread-local array:
>>>> 
>>>> ...
>>>>  Pkcs7Verify
>>>>    EVP_add_digest
>>>>      OBJ_NAME_add
>>>> 
>>>> this one uses a few static pointer variables that are also initialized
>>>> on demand and become stale upon SetVirtualAddressMap().
>>> 
>>> So it looks like the issue can't be solved without making OpenSSL aware
>>> of this use case.
>> 
>> Is reloading the module from scratch ruled out completely?
> 
> Not my place to say authoritatively, but:
> - it would be a first, as much as I can say,
> - it would duplicate (in purpose) an existing facility.
> 
> Personally I'd expect it to be rejected, but it's not up to me. If
> you're willing to "build one to (possibly) throw away", that could be
> the most direct way to get authoritative (= maintainer) feedback.
> 

Laszlo,

I thunk it is more likely to get rejected as it would not work. 

Every runtime driver I've every seen usually works like this:
1) Loads as an EFI driver and uses EFI Boot Services in its constructor (gBS, gDS, AllocatePool(), etc.)
2) You use the EFI Boot Service to register the ExitBootServices Event. 
3) SetVirtualAddressMap event fires and converts all the pointers 
4) After all the ExitBootServices events have been processed the DXE Runtime driver re-relocates the Runtime Driver
5) The next code that runs is a call from the kernel virtual mapping, and the system is at runtime 

It is important to remember that when gRT->SetVirtualAddressMap() is called by the OS Loader (or early kernel) that gBS->ExitBootServices() has already been called. So by the time you get 3) almost all of EFI is gone. The only services that remain are gRT. Note you find the location of gRT by using the gST passed into the entry point of the driver. So here is lies the problem the entry point is passed a Handle (EFI Boot Services concept) and a pointer to gST (another boot services table). So you can't really reload a module and expect it to work. 

In EFI the transition from Boot Service time to Runtime already requires a lot of coding discipline to not call services at runtime that will go away after ExitBootServices. Having C code that can be called in Physical mode, and then with a virtual mapping is a very unnatural and what EFI does today is the minimum required to make things work. 

Also remember dumping more into EFI runtime is stealing memory and usually kernel virtual address space from the OS. 

Thanks,

Andrew Fish


> Thanks
> Laszlo
> 
>> I'd try to cook up a patch for that unless there's a strong no-go.
>> 
>> Thanks,
>> Roman.


[-- Attachment #2: Type: text/html, Size: 13808 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-14 16:26                     ` Andrew Fish
@ 2019-08-16 15:22                       ` Laszlo Ersek
  2019-08-16 17:00                       ` Roman Kagan
  1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2019-08-16 15:22 UTC (permalink / raw)
  To: Andrew Fish; +Cc: Roman Kagan, devel, David Woodhouse

On 08/14/19 18:26, Andrew Fish wrote:
> 
> 
>> On Aug 14, 2019, at 8:16 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 08/13/19 13:23, Roman Kagan wrote:
>>> On Tue, Aug 13, 2019 at 11:10:27AM +0200, Laszlo Ersek wrote:
>>>> On 08/12/19 20:43, Roman Kagan wrote:
>>>>> On Fri, Aug 09, 2019 at 04:07:00PM +0000, Roman Kagan via Groups.Io wrote:
>>>>>> On Thu, Aug 08, 2019 at 07:39:14PM +0200, Laszlo Ersek wrote:
>>>>>>> On 08/07/19 19:41, Andrew Fish wrote:
>>>>>>>>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>>>>>>>> On 08/05/19 12:18, Roman Kagan wrote:
>>>>>>>>>> On Sat, Aug 03, 2019 at 04:03:04AM +0200, Laszlo Ersek via Groups.Io wrote:
>>>>>>>>>>> On 08/01/19 21:16, Roman Kagan wrote:
>>>>>>>>> I'm convinced that OpenSSL needs to expose a new API for this particular
>>>>>>>>> problem.
>>>>>>
>>>>>> Since, as you point out below, the problem only affects the essentially
>>>>>> broken configuration (SECURE_BOOT_ENABLE && !SMM_REQUIRE), I'm fine with
>>>>>> saving time and effort and sticking to the hack-ish approach proposed in
>>>>>> the bugzilla issue, which is to iterate over "thread-local" pointers and
>>>>>> EfiConvertPointer() on each.  (As long as it fixes the problem of
>>>>>> course; I'll test and report back.)
>>>>>
>>>>> It doesn't :(  It just gets slightly further and hits another static
>>>>> pointer variable which is not part of the thread-local array:
>>>>>
>>>>> ...
>>>>>  Pkcs7Verify
>>>>>    EVP_add_digest
>>>>>      OBJ_NAME_add
>>>>>
>>>>> this one uses a few static pointer variables that are also initialized
>>>>> on demand and become stale upon SetVirtualAddressMap().
>>>>
>>>> So it looks like the issue can't be solved without making OpenSSL aware
>>>> of this use case.
>>>
>>> Is reloading the module from scratch ruled out completely?
>>
>> Not my place to say authoritatively, but:
>> - it would be a first, as much as I can say,
>> - it would duplicate (in purpose) an existing facility.
>>
>> Personally I'd expect it to be rejected, but it's not up to me. If
>> you're willing to "build one to (possibly) throw away", that could be
>> the most direct way to get authoritative (= maintainer) feedback.
>>
> 
> Laszlo,
> 
> I thunk it is more likely to get rejected as it would not work. 
> 
> Every runtime driver I've every seen usually works like this:
> 1) Loads as an EFI driver and uses EFI Boot Services in its constructor (gBS, gDS, AllocatePool(), etc.)
> 2) You use the EFI Boot Service to register the ExitBootServices Event. 
> 3) SetVirtualAddressMap event fires and converts all the pointers 
> 4) After all the ExitBootServices events have been processed the DXE Runtime driver re-relocates the Runtime Driver
> 5) The next code that runs is a call from the kernel virtual mapping, and the system is at runtime 
> 
> It is important to remember that when gRT->SetVirtualAddressMap() is called by the OS Loader (or early kernel) that gBS->ExitBootServices() has already been called. So by the time you get 3) almost all of EFI is gone. The only services that remain are gRT. Note you find the location of gRT by using the gST passed into the entry point of the driver. So here is lies the problem the entry point is passed a Handle (EFI Boot Services concept) and a pointer to gST (another boot services table). So you can't really reload a module and expect it to work. 
> 
> In EFI the transition from Boot Service time to Runtime already requires a lot of coding discipline to not call services at runtime that will go away after ExitBootServices. Having C code that can be called in Physical mode, and then with a virtual mapping is a very unnatural and what EFI does today is the minimum required to make things work. 
> 
> Also remember dumping more into EFI runtime is stealing memory and usually kernel virtual address space from the OS. 

Thank you for the detailed explanation!
Laszlo

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [edk2-devel] static data in dxe_runtime modules
  2019-08-14 16:26                     ` Andrew Fish
  2019-08-16 15:22                       ` Laszlo Ersek
@ 2019-08-16 17:00                       ` Roman Kagan
  1 sibling, 0 replies; 15+ messages in thread
From: Roman Kagan @ 2019-08-16 17:00 UTC (permalink / raw)
  To: devel@edk2.groups.io, afish@apple.com; +Cc: Laszlo Ersek, David Woodhouse

On Wed, Aug 14, 2019 at 09:26:47AM -0700, Andrew Fish via Groups.Io wrote:
> Every runtime driver I've every seen usually works like this:
> 1) Loads as an EFI driver and uses EFI Boot Services in its constructor (gBS, gDS, AllocatePool(), etc.)
> 2) You use the EFI Boot Service to register the ExitBootServices Event. 
> 3) SetVirtualAddressMap event fires and converts all the pointers 
> 4) After all the ExitBootServices events have been processed the DXE Runtime driver re-relocates the Runtime Driver
> 5) The next code that runs is a call from the kernel virtual mapping, and the system is at runtime 
> 
> It is important to remember that when gRT->SetVirtualAddressMap() is
> called by the OS Loader (or early kernel) that gBS->ExitBootServices()
> has already been called. So by the time you get 3) almost all of EFI
> is gone. The only services that remain are gRT. Note you find the
> location of gRT by using the gST passed into the entry point of the
> driver. So here is lies the problem the entry point is passed a Handle
> (EFI Boot Services concept) and a pointer to gST (another boot
> services table). So you can't really reload a module and expect it to
> work. 

Indeed.  My hope was that the particular VariableRuntimeDxe module was
just a bunch of self-contained code that accesses all the data it needs
through the arguments it's passed.  Having now looked at the code, this
assumption is certainly wrong: the module depends on a lot of private
data that is initialized at boot services time.

Thanks and sorry for the noise,
Roman.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-08-16 17:00 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-01 19:16 static data in dxe_runtime modules Roman Kagan
2019-08-02 23:54 ` [edk2-devel] " Andrew Fish
2019-08-03  2:03 ` Laszlo Ersek
2019-08-05 10:18   ` Roman Kagan
2019-08-07 17:29     ` Laszlo Ersek
2019-08-07 17:41       ` Andrew Fish
2019-08-08 17:39         ` Laszlo Ersek
2019-08-09 16:07           ` Roman Kagan
     [not found]           ` <15B94CD6CF07DEE2.13696@groups.io>
2019-08-12 18:43             ` Roman Kagan
2019-08-13  9:10               ` Laszlo Ersek
2019-08-13 11:23                 ` Roman Kagan
2019-08-14 15:16                   ` Laszlo Ersek
2019-08-14 16:26                     ` Andrew Fish
2019-08-16 15:22                       ` Laszlo Ersek
2019-08-16 17:00                       ` Roman Kagan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox