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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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
>