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

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

On 08/07/19 19:41, Andrew Fish wrote:
> 
> 
>> On Aug 7, 2019, at 10:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> 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 <mailto: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.
>>
> 
> This feels right to me too. The reality is any pointer assigned in code needs to get "fixed up" for the new virtual mapping. The other restriction is non of the EFI Boot Services are available so you can't allocate memory etc.  The code that runs to fixup pointers runs in the physical mapping, but has access to an API that can convert a physical address to its future virtual mapping. 
> 
> If there is an API EFI can call to re-init passing in the old buffers that may be the easiest thing to maintain. The EFI driver would then have a boolean flag and need to call the OpenSSL re-init on the 1st virtual call into the driver. This would let the OpenSSL code function in a more normal fashion. Every thing else I can think of requires the OpenSSL code to keep a list of the pointers that need to be fixed up, and that would need to be maintained by hand. 
> 
> I can help answer any detailed questions about how EFI Runtime functions if that would help. 

Thanks, Andrew!

Roman: I've been pondering why I've never seen this issue myself. I
think now I know why.

(1.1) The issue is specific to the following library instance:

  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

That is the lib instance to which RuntimeCryptLibAddressChangeEvent()
belongs to.

It is also the lib instance through wich OpenSSL is linked into
DXE_RUNTIME_DRIVER modules -- see the dependency on the OpensslLib class.

(1.2) When you build the OvmfPkg and ArmVirtPkg platforms with

  -D SECURE_BOOT_ENABLE

the following DXE_RUNTIME_DRIVER will be included:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf

(1.3) This runtime driver depends on the AuthVariableLib class. With
SECURE_BOOT_ENABLE, the DSC files in question resolve AuthVariableLib to

  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

(1.4) That library instance depends on the BaseCryptLib class. Given
that VariableRuntimeDxe is a DXE_RUNTIME_DRIVER, the following
BaseCryptLib resolution from the DSC files takes effect:

  CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

And that's how you end up with an instance of OpenSSL that
- is statically linked into a DXE_RUNTIME_DRIVER,
- and is therefore subject to SetVirtualAddressMap().


(2) The problem with building ArmVirtPkg and OvmfPkg platforms with *just*

  -D SECURE_BOOT_ENABLE

is that those builds do not protect the variable driver, or the
underlying variable storage (in pflash), from tampering by the OS. There
is no hardware-based barrier between the authenticated UEFI variables
and the OS -- the OS can overwrite the variable store with direct
hardware (pflash) access, and that is a problem for Secure Boot.

This is why SECURE_BOOT_ENABLE is not production-ready in ArmVirtPkg,
and also why SECURE_BOOT_ENABLE is production-ready in OVMF *only* when
it's combined with:

  -D SMM_REQUIRE

So let's see what happens when SMM_REQUIRE is *also* set in OVMF.

(2.1) The variable driver is split in two halves,

- a non-privileged DXE_RUNTIME_DRIVER part, which only formats request
buffers for, and parses response buffers from, the privileged half,

- and a privileged half, which runs in SMM, and validates and serves (or
rejects) requests from the non-privileged half.

The halves are:

  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.inf
  MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf

(2.2) The unprivileged half is still a DXE_RUNTIME_DRIVER, but it does
not depend on the AuthVariableLib class. The privileged half is a
DXE_SMM_DRIVER, and it is the one to depend on AuthVariableLib.

In other words, the authentication of UEFI variable updates is moved
into SMM.

(2.3) The same AuthVariableLib instance, that is,

  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

continues to depend on the BaseCryptLib class. However, this time we're
discussing a DXE_SMM_DRIVER. For that, BaseCryptLib is resolved as
follows, in the OVMF DSC files:

  CryptoPkg/Library/BaseCryptLib/SmmCryptLib.inf

(2.4) That library instance pulls in OpenSSL again (via the OpensslLib
class dependency) -- but now OpenSSL will exist in SMRAM, and run in SMM
at OS runtime, as part of the "VariableSmm" driver.

Importantly, because the code runs in SMM, it is not subject to
SetVirtualAddressMap() / runtime relocation. SMM uses its own page
tables, which
- live in SMRAM,
- implement identity-mapping,
- implement a number of access protection schemes.

Thus, whatever pointer values OpenSSL creates initially, when
VariableSmm is first dispatched, those pointers will point into SMRAM.
And the same pointer values remain valid (into SMRAM) regardless of
when, or if, SetVirtualAddressMap() is called by the OS (or OS bootloader).

In other words, the problem doesn't exist when OpenSSL (with the rest of
the variable driver stack) is protected with SMM, as pointers into SMRAM
remain valid "forever", after the initial SMM driver dispatch.

Thanks,
Laszlo