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

["Roman Kagan" <rkagan@virtuozzo.com>]

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.

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

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

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.

Roman.