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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#44878): https://edk2.groups.io/g/devel/message/44878
Mute This Topic: https://groups.io/mt/32686575/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
