Re: [PATCH v4 12/18] tools/testing/cxl: Add "passphrase secure erase" opcode support

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 15 Nov 2022 10:01:53 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 11/15/2022 7:57 AM, Dave Jiang wrote:
> > 
> > 
> > On 11/15/2022 3:08 AM, Jonathan Cameron wrote:  
> >> On Mon, 14 Nov 2022 13:34:14 -0700
> >> Dave Jiang <dave.jiang@intel.com> wrote:
> >>  
> >>> Add support to emulate a CXL mem device support the "passphrase secure
> >>> erase" operation.
> >>>
> >>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> >> The logic in here gives me a headache but I'm not sure it's correct 
> >> yet...
> >>
> >> If you can figure out what is supposed to happen if this is called
> >> with Passphrase Type == master before the master passphrase has been set
> >> then you are doing better than me.
> >>
> >> Unlike for the User passphrase, where the language " .. and the user 
> >> passphrase
> >> is not currently set or is not supported by the device, this value is 
> >> ignored."
> >> to me implies we wipe the device and clear the non existent user pass 
> >> phrase,
> >> the not set master passphrase case isn't covered as far as I can see.
> >>
> >> The user passphrase question raises a futher question (see inline)
> >>
> >> Thoughts?  
> > 
> > Guess this is what happens when you bolt on master passphrase support 
> > after defining the spec without its existence, and then move it to a 
> > different spec and try to maintain compatibility between the two in 
> > order to not fork the hardware/firmware....
> > 
> > Should we treat the no passphrase set instance the same as sending a 
> > Secure Erase (Opcode 4401h)? And then the only case left is no master 
> > pass set but user pass is set.
> > 
> > if (!master_pass_set && pass_type_master) {
> >      if (user_pass_set)
> >          return -EINVAL;
> >      else
> >          secure_erase;
> > }
> >  
> This is the current change:
> 
> +       switch (erase->type) {
> +       case CXL_PMEM_SEC_PASS_MASTER:
> +               if (mdata->security_state & CXL_PMEM_SEC_STATE_MASTER_PASS_SET) {
> +                       if (memcmp(mdata->master_pass, erase->pass,
> +                                  NVDIMM_PASSPHRASE_LEN)) {
> +                               master_plimit_check(mdata);
> +                               cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> +                               return -ENXIO;
> +                       }
> +                       mdata->master_limit = 0;
> +                       mdata->user_limit = 0;
> +                       mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> +                       memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> +                       mdata->security_state &= ~CXL_PMEM_SEC_STATE_LOCKED;

> +               } else if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {
> +                       return -EINVAL;
> +               }
I would add a comment here to say what we aren't faking.  The aim being to show that
in all the good paths this happens, even though we don't do the other stuff in
some of them.

/* Scramble encryption keys so that data is effectively erased */

> +
> +               return 0;
> +       case CXL_PMEM_SEC_PASS_USER:
> +               if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {
> +                       if (memcmp(mdata->user_pass, erase->pass,
> +                                  NVDIMM_PASSPHRASE_LEN)) {
> +                               user_plimit_check(mdata);
> +                               cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> +                               return -ENXIO;
> +                       }
> +                       mdata->user_limit = 0;
> +                       mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> +                       memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> +               }
> +

/* Scramble encryption keys so that data is effectively erased */
here as well for the same reason.

> +               return 0;
> +       default:
> +               fallthrough;

Might as well return -EINVAL; here and drop the below one.

Otherwise looks good to me.  We could sprinkle some comments in here to
hightlight why we have concluded it ought to behave like this.
If nothing else, I doubt either of us will remember when we look at this
code in more than a few days time ;)

Otherwise looks good to me.

Jonathan


> +       }
> +
> +       return -EINVAL;
> 
> 
> 
> >>
> >> Other than that some suggestions inline but nothing functional, so up 
> >> to you.
> >> Either way
> >>
> >> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >>  
> >>> ---
> >>>   tools/testing/cxl/test/mem.c |   65 
> >>> ++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 65 insertions(+)
> >>>
> >>> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> >>> index 90607597b9a4..fc28f7cc147a 100644
> >>> --- a/tools/testing/cxl/test/mem.c
> >>> +++ b/tools/testing/cxl/test/mem.c
> >>> @@ -362,6 +362,68 @@ static int mock_unlock_security(struct 
> >>> cxl_dev_state *cxlds, struct cxl_mbox_cmd
> >>>       return 0;
> >>>   }
> >>> +static int mock_passphrase_secure_erase(struct cxl_dev_state *cxlds,
> >>> +                    struct cxl_mbox_cmd *cmd)
> >>> +{
> >>> +    struct cxl_mock_mem_pdata *mdata = dev_get_platdata(cxlds->dev);
> >>> +    struct cxl_pass_erase *erase;
> >>> +
> >>> +    if (cmd->size_in != sizeof(*erase))
> >>> +        return -EINVAL;
> >>> +
> >>> +    if (cmd->size_out != 0)
> >>> +        return -EINVAL;
> >>> +
> >>> +    erase = cmd->payload_in;
> >>> +    if (mdata->security_state & CXL_PMEM_SEC_STATE_FROZEN) {
> >>> +        cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> >>> +        return -ENXIO;
> >>> +    }
> >>> +
> >>> +    if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PLIMIT &&
> >>> +        erase->type == CXL_PMEM_SEC_PASS_USER) {
> >>> +        cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> >>> +        return -ENXIO;
> >>> +    }
> >>> +
> >>> +    if (mdata->security_state & CXL_PMEM_SEC_STATE_MASTER_PLIMIT &&
> >>> +        erase->type == CXL_PMEM_SEC_PASS_MASTER) {
> >>> +        cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> >>> +        return -ENXIO;
> >>> +    }
> >>> +
> >>> +    if (erase->type == CXL_PMEM_SEC_PASS_MASTER &&
> >>> +        mdata->security_state & CXL_PMEM_SEC_STATE_MASTER_PASS_SET) {
> >>> +        if (memcmp(mdata->master_pass, erase->pass, 
> >>> NVDIMM_PASSPHRASE_LEN)) {
> >>> +            master_plimit_check(mdata);
> >>> +            cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> >>> +            return -ENXIO;
> >>> +        }
> >>> +        mdata->master_limit = 0;
> >>> +        mdata->user_limit = 0;
> >>> +        mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> >>> +        memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> >>> +        mdata->security_state &= ~CXL_PMEM_SEC_STATE_LOCKED;
> >>> +        return 0;
> >>> +    }  
> >> What to do if the masterpass phrase isn't set?
> >> Even if we return 0, I'd slightly prefer to see that done locally so 
> >> refactor as
> >>     if (erase->type == CXL_PMEM_SEC_PASS_MASTER) {
> >>         if (!(mdata->security_state & 
> >> CXL_PMEM_SEC_STATATE_MASTER_PASS_SET)) {
> >>             return 0; /* ? */
> >>         if (memcmp)...
> >>     } else { /* CXL_PMEM_SEC_PASS_USER */ //or make it a switch.
> >>  
> >>> +
> >>> +    if (erase->type == CXL_PMEM_SEC_PASS_USER &&
> >>> +        mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {  
> >>
> >> Given we aren't actually scrambling the encryption keys (as we don't 
> >> have any ;)
> >> it doesn't make a functional difference, but to line up with the spec, 
> >> I would
> >> consider changing this to explicitly have the path for no user 
> >> passphrase set.
> >>
> >>     if (erase->type == CXL_PMEM_SEC_PASS_USER) {
> >>         if (mdata->security_state & CXL_MEM_SEC_STATE_USER_PASS_SET) {
> >>                 if (memcmp(mdata->user_pass, erase->pass, 
> >> NVDIMM_PASSPHRASE_LEN)) {
> >>                 user_plimit_check(mdata);
> >>                 cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> >>                 return -ENXIO;
> >>               }
> >>
> >>             mdata->user_limit = 0;
> >>             mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> >>             memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> >>         }
> >>         /* Change encryption keys */
> >>         return 0;
> >>     }
> >>  
> >>> +        if (memcmp(mdata->user_pass, erase->pass, 
> >>> NVDIMM_PASSPHRASE_LEN)) {
> >>> +            user_plimit_check(mdata);
> >>> +            cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> >>> +            return -ENXIO;
> >>> +        }
> >>> +
> >>> +        mdata->user_limit = 0;
> >>> +        mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> >>> +        memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> >>> +        return 0;
> >>> +    }
> >>> +
> >>> +    return 0;  
> >>
> >> With above changes you can never reach here.
> >>  
> >>> +}
> >>> +
> >>>   static int mock_get_lsa(struct cxl_dev_state *cxlds, struct 
> >>> cxl_mbox_cmd *cmd)
> >>>   {
> >>>       struct cxl_mbox_get_lsa *get_lsa = cmd->payload_in;
> >>> @@ -470,6 +532,9 @@ static int cxl_mock_mbox_send(struct 
> >>> cxl_dev_state *cxlds, struct cxl_mbox_cmd *
> >>>       case CXL_MBOX_OP_UNLOCK:
> >>>           rc = mock_unlock_security(cxlds, cmd);
> >>>           break;
> >>> +    case CXL_MBOX_OP_PASSPHRASE_SECURE_ERASE:
> >>> +        rc = mock_passphrase_secure_erase(cxlds, cmd);
> >>> +        break;
> >>>       default:
> >>>           break;
> >>>       }
> >>>
> >>>  
> >>