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 08:57:38 -0700
Dave Jiang <dave.jiang@intel.com> 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;
> }

Let's do this for now, but also gather up a set of questions / clarifications
to take to CXL SSWG.  Can gather that on linux-cxl as discussing public
stuff only, then one of us can have the pleasure of seeking clarifications
in SSWG / possibly leading to future spec changes / Errata.

Jonathan


> 
> > 
> > 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;
> >>   	}
> >>
> >>  
> >