Re: [edk2-devel] static data in dxe_runtime modules

["Laszlo Ersek" <lersek@redhat.com>]

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