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....@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. > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#45029): https://edk2.groups.io/g/devel/message/45029 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] -=-=-=-=-=-=-=-=-=-=-=-
