On Mon, 14 Nov 2022 13:34:14 -0700 Dave Jiang <[email protected]> wrote:
> Add support to emulate a CXL mem device support the "passphrase secure > erase" operation. > > Signed-off-by: Dave Jiang <[email protected]> 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? Other than that some suggestions inline but nothing functional, so up to you. Either way Reviewed-by: Jonathan Cameron <[email protected]> > --- > 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; > } > >
