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; Wed, 07 Aug 2019 10:29:04 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1E313308212A; Wed, 7 Aug 2019 17:29:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 14F9E19C69; Wed, 7 Aug 2019 17:29:02 +0000 (UTC) Subject: Re: [edk2-devel] static data in dxe_runtime modules To: Roman Kagan , devel@edk2.groups.io, David Woodhouse References: <20190801191621.GB14235@rkaganb.sw.ru> <8d18d4f6-5f33-44e9-2758-46350b43c5ec@redhat.com> <20190805101813.GA27171@rkaganb.sw.ru> From: "Laszlo Ersek" Message-ID: <406f2250-41e2-9925-b570-38b99a5f6e41@redhat.com> Date: Wed, 7 Aug 2019 19:29:02 +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: <20190805101813.GA27171@rkaganb.sw.ru> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Wed, 07 Aug 2019 17:29:04 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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. > > >