Re: [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries, and Implementation for VariableLock Alternative


Yao, Jiewen
 

Add devel@edk2.groups.io.

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Friday, February 7, 2020 6:13 AM
To: brbarkel@microsoft.com; rfc@edk2.groups.io
Cc: edk2-devel@lists.01.org
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries,
and Implementation for VariableLock Alternative

Thanks, Bret.

Comments below.

-----Original Message-----
From: brbarkel via [] <brbarkel=microsoft.com@[]>
Sent: Friday, February 7, 2020 5:49 AM
To: Yao; Yao, Jiewen <jiewen.yao@intel.com>; rfc@edk2.groups.io
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] VariablePolicy - Protocol, Libraries,
and Implementation for VariableLock Alternative

Sorry, Jiewen. I missed this email when it came in...

1. We have 2 variable related protocol - EDKII_VARIABLE_LOCK_PROTOCOL
and EDKII_VAR_CHECK_PROTOCOL. Do you want to deprecate both? Or only
deprecate EDKII_VARIABLE_LOCK_PROTOCOL?

I think we would recommend deprecating both. Is there much consumption of
the VarCheck protocol? A platform could always opt to include both (or all 3!),
but they wouldn't be able to immediately tell which engine rejected the
SetVariable.
[Jiewen] Right. That is my understanding- deprecate both.
I saw you only mention to deprecate VarLock, but not VarCheck. That is why I
get confused.

I don’t recommend we have 3 engines. Too burden to maintain the code.

One possible option is to implement VarLockProtocol and VarCheckProtocol on
top of VarPolicyProtocol. (like a thunk)
As such, there is no impact on the existing private platform code, with the
benefit that one central engine controls everything.
Later, we can check platform one by one. After the last one is migrated, we can
remove VarLock and VarCheck thunk.




2. The Function - DumpVariablePolicy() - makes me confused in the
beginning.
In my thought, "Dump" means to show some debug message. But here, you
want to return the policy. Can we change the name to GetVariablePolicy()?

I see your point about possible confusion. I think we would try to clarify this in
the function documentation. Due to the code-first approach we've taken on
this,
we already have partners who have implemented the policy with
DumpVariablePolicy as the name and it doesn't seem like a huge problem.

3. The function - DisableVariablePolicy(). Does it disable current policy
engine?
Does it disable any future policy engine? Does it block RegisterVariablePolicy()
call?

Disable turns off the enforcement. You should still be able to register new
policies for auditing purposes (they will still be returned by
DumpVariablePolicy),
but no SetVariables will be rejected.

4. The function - LockVariablePolicy() - Can it lock the DisableVariablePolicy()
call?

Correct. Once the interface is locked, it cannot be disabled. Locking should be
performed at EndOfDxe or ReadyToBoot or wherever the platform TCB is.

5. The use case "In MFG Mode Variable Policy Engine is disabled, thus these
VPD variables can be created. These variables are locked with lock policy type
LockNow, so that these variables can't be tampered with in Customer Mode."

Correct. The MfgMode is just a suggestion and is entirely up to the platform.
Our
MfgMode (which has not been open-sourced yet, but we're working on it) will
call DisableVariablePolicy because our threat model indicates that
compromising
MfgMode is a total compromise (there are many other attack vectors once
MfgMode is compromised). As such, we protect it extensively. But there is
nothing in the core or extra code that would automatically disable the policy
engine for any platform that didn't want that. It is up to the platform, however,
to make sure they lock the policy engine (similar to locking SMM).
[Jiewen] OK, if my understanding is correct, the DisablePolicy() should only be
called in some special environment, but NOT a normal feature involved in the
normal boot.

Then I am feeling we should separate the DisablePolicy from this protocol,
because this interface is very dangerous. I don’t want any driver call it by
mistake.

Can we split the VARIABLE_POLICY to VARIABLE_POLICY (normal boot usage) +
VARIABLE_POLICY_CONTROL (very special usage)

Like below:

typedef struct {
UINT64 Revision;
REGISTER_VARIABLE_POLICY RegisterVariablePolicy;
DUMP_VARIABLE_POLICY DumpVariablePolicy;
LOCK_VARIABLE_POLICY LockVariablePolicy;
} _VARIABLE_POLICY_PROTOCOL;

typedef struct {
UINT64 Revision;
DISABLE_VARIABLE_POLICY DisableVariablePolicy;
IS_VARIABLE_POLICY_ENABLED IsVariablePolicyEnabled;
} _VARIABLE_POLICY_CONTROL_PROTOCOL;

VARIABLE_POLICY_PROTOCOL is always installed.

In normal boot, the VARIABLE_POLICY_CONTROL_PROTOCOL is NOT installed.
As such, the policy is always enforced.
The VARIABLE_POLICY_CONTROL_PROTOCOL is only installed in some special
mode, controlled by PCD - maybe.
A platform may choose to:
1) Use static PCD to always disable VARIABLE_POLICY_CONTROL_PROTOCOL
installation.
2) Use dynamic PCD to control VARIABLE_POLICY_CONTROL_PROTOCOL
installation only in special mode.


Thank you
Yao Jiewen

Join rfc@edk2.groups.io to automatically receive all group messages.