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

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

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?

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