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

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

On Wed, 16 Nov 2022 14:54:02 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 11/16/2022 3:43 AM, Jonathan Cameron wrote:
> > 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;
> >> +               }  
> 
> So while looking at 8.2.9.8.6.3 I stumbled on this line: "When the 
> master passphrase is disabled, the device shall return Invalid Input for 
> the Passphrase Secure Erase command with the master passphrase". I 
> suppose the above would reduce to just else {} instead?

Good spot. Agreed, this one is just an else.  Definitely a case for a reference
to the spec though!

> And it probably 
> wouldn't hurt to have the spec duplicate this line under the passphrase 
> secure erase section as well.

Would be nice :)