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

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

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.com@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.
> 
> 
>