From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 02 Aug 2019 19:03:07 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 468BF30EA186; Sat, 3 Aug 2019 02:03:06 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-36.ams2.redhat.com [10.36.116.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3CE605C207; Sat, 3 Aug 2019 02:03:04 +0000 (UTC) Subject: Re: [edk2-devel] static data in dxe_runtime modules To: Roman Kagan , devel@edk2.groups.io References: <20190801191621.GB14235@rkaganb.sw.ru> From: "Laszlo Ersek" Message-ID: <8d18d4f6-5f33-44e9-2758-46350b43c5ec@redhat.com> Date: Sat, 3 Aug 2019 04:03:04 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190801191621.GB14235@rkaganb.sw.ru> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Sat, 03 Aug 2019 02:03:06 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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