[edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Kubacki, Michael A]

Hello,

I would appreciate any feedback you may have for this proposal.

Overview
--------------
This is a proposal to reduce SMM usage when using VariableSmmRuntimeDxe with VariableSmm. It will do so by eliminating SMM usage for the vast majority of runtime service GetVariable ( ) and GetNextVariableName ( ) invocations. Most UEFI variable usage in typical systems after the variable store is initialized (e.g. manufacturing boots) is due to GetVariable ( ) and GetNextVariableName ( ) not SetVariable ( ). GetVariable ( ) calls can regularly exceed 100 per boot while SetVariable ( ) calls typically remain less than 10 per boot. By focusing on the common case, the majority of overhead associated with SMM can be avoided while still using existing and proven code for operations such as variable authentication that require an isolated execution environment.

 * Advantage: Reduces overall system SMM usage
 * Disadvantage: Requires more Runtime data memory usage

Initial Performance Observations
----------------------------------------------
 * With these proposed changes, an Intel Atom based SoC saw GetVariable ( ) time for an existing variable reduce from ~220us to ~5us.

Major Changes
---------------------
 1. Two UEFI variable caches will be maintained.
     a. "Runtime Cache" - Maintained in VariableSmmRuntimeDxe. Used to serve runtime service GetVariable ( ) and GetNextVariableName ( ) callers.
     b. "SMM cache" - Maintained in VariableSmm to service SMM GetVariable ( ) and GetNextVariableName ( ) callers.
         i. A cache in SMRAM retained so SMM modules do not operate on data outside SMRAM.
 2. A new UEFI variable read and write flow will be used as described below.

At any given time, the two caches would be coherent. On a variable write, the runtime cache is only updated after validation in SMM and, in the case of a non-volatile UEFI variable, the variable must also be successfully written to non-volatile storage. 

Primary Concern
-----------------------
The primary item that I believe warrants feedback is whether there are substantial concerns with keeping a variable store cache that is used to serve UEFI Runtime Services callers in a buffer of EfiRuntimeServicesData type. 

Proof-of-Concept Implementation
----------------------------------------------
The implementation is available in the following commit - check the commit message for some more details. 
https://github.com/makubacki/edk2/commit/d812d43412a26e44581d283382596a863c1ae825

Please note this is "POC" level code quality and there will be cleanup of lock interfaces used and some other minor changes. Please feel free to leave any comments on the changes. This code was tested with the master branch of edk2 on an Intel Whiskey Lake U reference validation platform.

Why Keep SMM on Variable Writes
------------------------------------------------
 * SMM provides a fairly ubiquitous isolated execution environment in x86 for authenticated UEFI variables.
 * BIOS region SPI flash write restrictions to SMM in platforms today can be retained.

Today's UEFI Variable Cache
--------------------------------------
 * Maintained in SMRAM via VariableSmm.
 * A "write-through" cache of variable data in the form of a UEFI variable store.
 * Non-volatile and volatile variables are maintained in separate buffers (variable stores).

Runtime & SMM Cache Coherency
----------------------------------------------
The non-volatile cache should always accurately reflect non-volatile storage contents (done today) and the "SMM cache" and "Runtime cache" should always be coherent on access. The runtime cache is updated by VariableSmm.

Updating both caches from within a SMM SetVariable ( ) operation is fairly straightforward but a race condition can occur if an SMI occurs during the execution of runtime code reading from the runtime cache. To handle this case, a runtime cache read lock is introduced that explicitly moves pending updates from SMM to the runtime cache if an SMM update occurs while the runtime cache is locked. Note that is not expected a Runtime services call will interrupt SMM processing since all cores rendezvous in SMM. 

New Key Elements for Coherence
---------------------------------------------
Runtime DXE (VariableSmmRuntimeDxe)
 1. RuntimeCacheReadLock - A global lock used to lock read access to the runtime cache.
 2. RuntimeCachePendingUpdate - A global flag used to notify runtime code of a pending cache update in SMM.

SMM (VariableSmm)
 1. FlushRuntimeCachePendingUpdate SMI - A SW SMI handler that synchronizes the runtime cache buffer with the SMM cache buffer.

Proposed Runtime DXE Read Flow
----------------------------------------------
 1. Wait for RuntimeCacheReadLock to be free
 2. Acquire RuntimeCacheReadLock
 3. If RuntimeCachePendingUpdate flag is set then:
     3.a. Trigger FlushRuntimeCachePendingUpdate SMI
     3.b. Verify RuntimeCachePendingUpdate flag is cleared
 4. Perform read from RuntimeCache
 5. Release RuntimeCacheReadLock

Proposed FlushRuntimeCachePendingUpdate SMI
-------------------------------------------------------------------
 1. If RuntimeCachePendingUpdate flag is not set:
     1.a. Return
 2. Copy the data at RuntimeCachePendingOffset of RuntimeCachePendingLength to RuntimeCache
 3. Clear the RuntimeCachePendingUpdate flag

Proposed SMM Write Flow
-------------------------------------
 1. Perform variable authentication and non-volatile write. If either fail, return an error to the caller.
 2. If RuntimeCacheReadLock is set then:
     2.a. Set RuntimeCachePendingUpdate flag
     2.b. Update RuntimeCachePendingOffset and RuntimeCachePendingLength to cover the a superset of the pending chunk (for simplicity, the entire variable store is currently synchronized).
3. Else:
     3.a. Update RuntimeCache
4. Update SmmCache
     - Note: RT read cannot occur during SMI processing since all cores are locked in SMM.

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Johnson, Michael]

[-- Attachment #1: Type: text/plain, Size: 753 bytes --]

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.

[-- Attachment #2: Type: text/html, Size: 781 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Kubacki, Michael A]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.

[-- Attachment #2: Type: text/html, Size: 4059 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Johnson, Michael]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com>; devel@edk2.groups.io
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.

[-- Attachment #2: Type: text/html, Size: 5753 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 3757 bytes --]

Hey, from security perspective, I am not clear what is difference on below 2 scenario – TPM or read-modify-write.

Whenever we return some data from SMM, we are in ring0, any program in ring0 may modify the variable content in the communication buffer.
If we assume ring0 is malicious, then the malicious code may let one AP keep looping to monitor the communication data, when BSP call get (authenticated) variable. Once communication buffer is updated and status is filled to EFI_SUCCESS, the AP may modify the communication buffer, then the BSP will return *modified* data to caller. Or an interrupt handler in BSP may also modify the communication buffer before the data is returned to caller. This race condition exists today.

I think putting cache buffer to SMM just raise the BAR, but *NOT* a security solution, because SMM communication buffer in reserved memory is same as cache buffer.

Thank you
Yao Jiewen

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Johnson, Michael
Sent: Saturday, September 7, 2019 5:52 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.


[-- Attachment #2: Type: text/html, Size: 8263 bytes --]

Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Laszlo Ersek]

On 09/05/19 21:54, Kubacki, Michael A wrote:

> Proof-of-Concept Implementation
> ----------------------------------------------
> The implementation is available in the following commit - check the commit message for some more details. 
> https://github.com/makubacki/edk2/commit/d812d43412a26e44581d283382596a863c1ae825
> 
> Please note this is "POC" level code quality and there will be cleanup of lock interfaces used and some other minor changes. Please feel free to leave any comments on the changes.

First some meta thoughts:

- If this code is meant for edk2 ultimately, please keep the discussion
on the mailing list, and/or in a TianoCore bugzilla.

- The size of this feature is significant. According to the github
webui, "19 changed files with 2,083 additions and 1,072 deletions".

A feature of this size must absolutely be broken up into a fine-grained
patch series (assuming the feature targets the edk2 master branch). It's
not only that such a huge patch is basically unreviewable: if someone
runs into an issue after the feature is committed, they will need the
ability to bisect the regression to a well isolated, self contained
modification. Then the experts around the feature have a much better
chance at root causing the issue from the patch that the bug reporter
has identified. An all-or-nothing patch is not bisectable.

- Combining the above two points into one, I'd suggest splitting the
feature into small patches, and posting it as an RFC series to the list.
(Assuming the initial reaction to the feature is interest -- I think it
is.) Admittedly, this is a lot of work.

Some more on-topic comments:

> Why Keep SMM on Variable Writes
> ------------------------------------------------
>  * SMM provides a fairly ubiquitous isolated execution environment in x86 for authenticated UEFI variables.
>  * BIOS region SPI flash write restrictions to SMM in platforms today can be retained.

Can you confirm that the runtime DXE code would not read flash, only the
cache in EfiRuntimeServicesData memory?

> Today's UEFI Variable Cache
> --------------------------------------
>  * Maintained in SMRAM via VariableSmm.
>  * A "write-through" cache of variable data in the form of a UEFI variable store.
>  * Non-volatile and volatile variables are maintained in separate buffers (variable stores).

I'm unclear on how the items on this list should be interpreted. Are
these items from today that we keep, or items that we change?

For example, it's quite beneficial that NV and V variables are
maintained in separate buffers -- the sizing can be different, and
that's good. I believe we shouldn't change that.

Thanks!
Laszlo

Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Kubacki, Michael A]

I completely understand the need for granular breakup of changes for code review and future maintenance. I would not send this as a single patch on the mailing list for formal code review. Due to the size of the change, the main point here was to initially focus feedback on the high-level approach and design sparing the review of implementation details for an actual code review. Though I understand for those interested, it is much easier to digest the code in a clean patch series so I will send that RFC series to the list once I incorporate the feedback already received.

I replied elsewhere inline.

Thanks,
Michael

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Monday, September 9, 2019 8:32 AM
> To: devel@edk2.groups.io; Kubacki, Michael A
> <michael.a.kubacki@intel.com>
> Subject: Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction
> 
> On 09/05/19 21:54, Kubacki, Michael A wrote:
> 
> > Proof-of-Concept Implementation
> > ----------------------------------------------
> > The implementation is available in the following commit - check the commit
> message for some more details.
> >
> https://github.com/makubacki/edk2/commit/d812d43412a26e44581d283382
> 596
> > a863c1ae825
> >
> > Please note this is "POC" level code quality and there will be cleanup of lock
> interfaces used and some other minor changes. Please feel free to leave any
> comments on the changes.
> 
> First some meta thoughts:
> 
> - If this code is meant for edk2 ultimately, please keep the discussion on the
> mailing list, and/or in a TianoCore bugzilla.
> 
> - The size of this feature is significant. According to the github webui, "19
> changed files with 2,083 additions and 1,072 deletions".
> 
> A feature of this size must absolutely be broken up into a fine-grained patch
> series (assuming the feature targets the edk2 master branch). It's not only
> that such a huge patch is basically unreviewable: if someone runs into an
> issue after the feature is committed, they will need the ability to bisect the
> regression to a well isolated, self contained modification. Then the experts
> around the feature have a much better chance at root causing the issue from
> the patch that the bug reporter has identified. An all-or-nothing patch is not
> bisectable.
> 
> - Combining the above two points into one, I'd suggest splitting the feature
> into small patches, and posting it as an RFC series to the list.
> (Assuming the initial reaction to the feature is interest -- I think it
> is.) Admittedly, this is a lot of work.
> 
> Some more on-topic comments:
> 
> > Why Keep SMM on Variable Writes
> > ------------------------------------------------
> >  * SMM provides a fairly ubiquitous isolated execution environment in x86
> for authenticated UEFI variables.
> >  * BIOS region SPI flash write restrictions to SMM in platforms today can be
> retained.
> 
> Can you confirm that the runtime DXE code would not read flash, only the
> cache in EfiRuntimeServicesData memory?

Yes, that is correct.
> 
> > Today's UEFI Variable Cache
> > --------------------------------------
> >  * Maintained in SMRAM via VariableSmm.
> >  * A "write-through" cache of variable data in the form of a UEFI variable
> store.
> >  * Non-volatile and volatile variables are maintained in separate buffers
> (variable stores).
> 
> I'm unclear on how the items on this list should be interpreted. Are these
> items from today that we keep, or items that we change?
> 
> For example, it's quite beneficial that NV and V variables are maintained in
> separate buffers -- the sizing can be different, and that's good. I believe we
> shouldn't change that.

These points just summarize today's operation for the unaware. I agree the two buffers should be separate and there's no plan to change that.
> 
> Thanks!
> Laszlo

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Nate DeSimone]

[-- Attachment #1: Type: text/plain, Size: 4671 bytes --]

Hi All,

There is a security issue with regard to the way VarCheckLib works. There are plenty of usages of VarCheckLib that are intended to prevent ring0 from reading a variable after ReadyToBoot() is called. If we assume a malicious operating system, then having a ring0 buffered version of the variable that VarCheckLib is attempting to prevent the OS from reading would provide a backdoor for the OS to read that protected variable’s contents.

Thanks,
Nate

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
Sent: Sunday, September 8, 2019 3:36 PM
To: devel@edk2.groups.io; Johnson, Michael <michael.johnson@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hey, from security perspective, I am not clear what is difference on below 2 scenario – TPM or read-modify-write.

Whenever we return some data from SMM, we are in ring0, any program in ring0 may modify the variable content in the communication buffer.
If we assume ring0 is malicious, then the malicious code may let one AP keep looping to monitor the communication data, when BSP call get (authenticated) variable. Once communication buffer is updated and status is filled to EFI_SUCCESS, the AP may modify the communication buffer, then the BSP will return *modified* data to caller. Or an interrupt handler in BSP may also modify the communication buffer before the data is returned to caller. This race condition exists today.

I think putting cache buffer to SMM just raise the BAR, but *NOT* a security solution, because SMM communication buffer in reserved memory is same as cache buffer.

Thank you
Yao Jiewen

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Johnson, Michael
Sent: Saturday, September 7, 2019 5:52 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.


[-- Attachment #2: Type: text/html, Size: 9715 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Yao, Jiewen]

[-- Attachment #1: Type: text/plain, Size: 5318 bytes --]

Nate
I believe this SMI reduction work only handle GetVariable.

VarCheckLib only handles SetVariable.
VarCheckLib does not handle GetVaraible.

Thank you
Yao Jiewen

From: Desimone, Nathaniel L
Sent: Wednesday, September 11, 2019 10:43 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Johnson, Michael <michael.johnson@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hi All,

There is a security issue with regard to the way VarCheckLib works. There are plenty of usages of VarCheckLib that are intended to prevent ring0 from reading a variable after ReadyToBoot() is called. If we assume a malicious operating system, then having a ring0 buffered version of the variable that VarCheckLib is attempting to prevent the OS from reading would provide a backdoor for the OS to read that protected variable’s contents.

Thanks,
Nate

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Yao, Jiewen
Sent: Sunday, September 8, 2019 3:36 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hey, from security perspective, I am not clear what is difference on below 2 scenario – TPM or read-modify-write.

Whenever we return some data from SMM, we are in ring0, any program in ring0 may modify the variable content in the communication buffer.
If we assume ring0 is malicious, then the malicious code may let one AP keep looping to monitor the communication data, when BSP call get (authenticated) variable. Once communication buffer is updated and status is filled to EFI_SUCCESS, the AP may modify the communication buffer, then the BSP will return *modified* data to caller. Or an interrupt handler in BSP may also modify the communication buffer before the data is returned to caller. This race condition exists today.

I think putting cache buffer to SMM just raise the BAR, but *NOT* a security solution, because SMM communication buffer in reserved memory is same as cache buffer.

Thank you
Yao Jiewen

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Johnson, Michael
Sent: Saturday, September 7, 2019 5:52 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.


[-- Attachment #2: Type: text/html, Size: 11348 bytes --]

Re: [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Laszlo Ersek]

On 09/09/19 20:03, Kubacki, Michael A wrote:
> I completely understand the need for granular breakup of changes for code review and future maintenance. I would not send this as a single patch on the mailing list for formal code review. Due to the size of the change, the main point here was to initially focus feedback on the high-level approach and design sparing the review of implementation details for an actual code review. Though I understand for those interested, it is much easier to digest the code in a clean patch series so I will send that RFC series to the list once I incorporate the feedback already received.

Thank you!

> I replied elsewhere inline.

For those answers as well.

Laszlo

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Kubacki, Michael A]

[-- Attachment #1: Type: text/plain, Size: 5819 bytes --]

Jiewen, you’re right, SetVariable ( ) will continue to go through SW SMI. I agree it should not be an issue for this work. Nate, can you please confirm we can close the open?

From: Yao, Jiewen
Sent: Tuesday, September 10, 2019 8:32 PM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io; Johnson, Michael <michael.johnson@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Nate
I believe this SMI reduction work only handle GetVariable.

VarCheckLib only handles SetVariable.
VarCheckLib does not handle GetVaraible.

Thank you
Yao Jiewen

From: Desimone, Nathaniel L
Sent: Wednesday, September 11, 2019 10:43 AM
To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Johnson, Michael <michael.johnson@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hi All,

There is a security issue with regard to the way VarCheckLib works. There are plenty of usages of VarCheckLib that are intended to prevent ring0 from reading a variable after ReadyToBoot() is called. If we assume a malicious operating system, then having a ring0 buffered version of the variable that VarCheckLib is attempting to prevent the OS from reading would provide a backdoor for the OS to read that protected variable’s contents.

Thanks,
Nate

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Yao, Jiewen
Sent: Sunday, September 8, 2019 3:36 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hey, from security perspective, I am not clear what is difference on below 2 scenario – TPM or read-modify-write.

Whenever we return some data from SMM, we are in ring0, any program in ring0 may modify the variable content in the communication buffer.
If we assume ring0 is malicious, then the malicious code may let one AP keep looping to monitor the communication data, when BSP call get (authenticated) variable. Once communication buffer is updated and status is filled to EFI_SUCCESS, the AP may modify the communication buffer, then the BSP will return *modified* data to caller. Or an interrupt handler in BSP may also modify the communication buffer before the data is returned to caller. This race condition exists today.

I think putting cache buffer to SMM just raise the BAR, but *NOT* a security solution, because SMM communication buffer in reserved memory is same as cache buffer.

Thank you
Yao Jiewen

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Johnson, Michael
Sent: Saturday, September 7, 2019 5:52 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.


[-- Attachment #2: Type: text/html, Size: 12224 bytes --]

Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

[Nate DeSimone]

[-- Attachment #1: Type: text/plain, Size: 6282 bytes --]

Hi Jiewen,

Looking at the code yes you are right, sorry I forgot what exactly VarCheckLib hooked in to versus didn’t hook into. The other one that same to mind was the EDKII_VARIABLE_LOCK_PROTOCOL, but again, that only affects writes not reads.

Since the current variable driver does not have any confidentiality feature I think this should be fine from a security standpoint. However, if we ever add a feature to make certain variables confidential then this will need to be revisited.

Thanks,
Nate

From: Yao, Jiewen
Sent: Tuesday, September 10, 2019 8:32 PM
To: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io; Johnson, Michael <michael.johnson@intel.com>; Kubacki, Michael A <michael.a.kubacki@intel.com>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Nate
I believe this SMI reduction work only handle GetVariable.

VarCheckLib only handles SetVariable.
VarCheckLib does not handle GetVaraible.

Thank you
Yao Jiewen

From: Desimone, Nathaniel L
Sent: Wednesday, September 11, 2019 10:43 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hi All,

There is a security issue with regard to the way VarCheckLib works. There are plenty of usages of VarCheckLib that are intended to prevent ring0 from reading a variable after ReadyToBoot() is called. If we assume a malicious operating system, then having a ring0 buffered version of the variable that VarCheckLib is attempting to prevent the OS from reading would provide a backdoor for the OS to read that protected variable’s contents.

Thanks,
Nate

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Yao, Jiewen
Sent: Sunday, September 8, 2019 3:36 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Hey, from security perspective, I am not clear what is difference on below 2 scenario – TPM or read-modify-write.

Whenever we return some data from SMM, we are in ring0, any program in ring0 may modify the variable content in the communication buffer.
If we assume ring0 is malicious, then the malicious code may let one AP keep looping to monitor the communication data, when BSP call get (authenticated) variable. Once communication buffer is updated and status is filled to EFI_SUCCESS, the AP may modify the communication buffer, then the BSP will return *modified* data to caller. Or an interrupt handler in BSP may also modify the communication buffer before the data is returned to caller. This race condition exists today.

I think putting cache buffer to SMM just raise the BAR, but *NOT* a security solution, because SMM communication buffer in reserved memory is same as cache buffer.

Thank you
Yao Jiewen

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Johnson, Michael
Sent: Saturday, September 7, 2019 5:52 AM
To: Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Yes - both things I bring up are just *different* ring 0 accesses than are (easily) allowed today, so are not fundamentally new.  Generally we either trust all of ring 0 or none of it, so neither is a showstopper.

Reading back from real varstore/SMM if the variable has the auth attribute removes any interesting vectors so all that changes is a bad ring 0 agent can go through memory instead of the RT API, which is not threatening.

I have no problems if write and auth-read come from SMM and all else comes from cache.

From: Kubacki, Michael A
Sent: Friday, September 6, 2019 2:48 PM
To: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: RE: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

My understanding is both of your points return to the issue of a ring 0 entity potentially modifying the runtime cache. As the SetVariable ( ) API is already accessible to ring 0, the variables could similarly be updated today so that should not be an issue. You have a good point for authenticated variables where the update is authenticated in SMM so the variable data should continue to be returned from SMM.

How about if the variable has the authenticated attribute set, those are sent to GetVariable ( ) in SMM? This should be relatively rare with the most common case likely being secure boot related keys.

From: Johnson, Michael <michael.johnson@intel.com<mailto:michael.johnson@intel.com>>
Sent: Thursday, September 5, 2019 1:59 PM
To: Kubacki; Kubacki, Michael A <michael.a.kubacki@intel.com<mailto:michael.a.kubacki@intel.com>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: Re: [edk2-devel] [edk2-rfc] [edk2-devel] UEFI Variable SMI Reduction

Your primary concern is my primary concern.  I can think of two scenarios where a runtime memory varstore would hurt.

The less severe one is that any variables measured into a TPM could appear to be modified when read back so that if/when some entity wants to verify or unseal something, they would be unable to match the TPM's PCR values and unable to verify/unseal.  This turns access to runtime EFI memory into a denial of service for TPM-based post-boot software.

The more worrying possibility is if somebody decides to use a read-modify-write pattern for some variable they have an interest in and thus end up defeating the security of the variable write method.  Today a read-modify-write is safe, but after this change it would not be.


[-- Attachment #2: Type: text/html, Size: 13027 bytes --]