Re: [PATCH v2 13/19] tools/testing/cxl: Add "passphrase secure erase" opcode support

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

On Wed, 21 Sep 2022 08:32:46 -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>
> ---
>  tools/testing/cxl/test/mem.c |   56 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index 840378d239bf..a0a58156c15a 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
> @@ -356,6 +356,59 @@ static int mock_unlock_security(struct cxl_dev_state *cxlds, struct cxl_mbox_cmd
>  	return 0;
>  }
>  
> +static int mock_passphrase_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;
> +
> +	if (mdata->security_state & CXL_PMEM_SEC_STATE_FROZEN) {
> +		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> +		return -ENXIO;
> +	}
> +

I think we need to check also that the passphrase supplied is not the
master one in which case the lockout on user passphrase shouldn't matter.

> +	if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PLIMIT) {
> +		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
> +		return -ENXIO;
> +	}
> +
> +	erase = cmd->payload_in;
> +	if (erase->type == CXL_PMEM_SEC_PASS_MASTER &&
> +	    mdata->security_state & CXL_PMEM_SEC_STATE_MASTER_PASS_SET &&
> +	    memcmp(mdata->master_pass, erase->pass, NVDIMM_PASSPHRASE_LEN)) {
> +		if (++mdata->master_limit == PASS_TRY_LIMIT)

It's harmless, but I'm not sure I like the adding to this when we've already
hit the limit.  Maybe only increment if not?

> +			mdata->security_state |= CXL_PMEM_SEC_STATE_MASTER_PLIMIT;
> +		cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> +		return -ENXIO;
> +	}
> +
> +	if (erase->type == CXL_PMEM_SEC_PASS_USER &&
> +	    mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET &&
> +	    memcmp(mdata->user_pass, erase->pass, NVDIMM_PASSPHRASE_LEN)) {
> +		if (++mdata->user_limit == PASS_TRY_LIMIT)
> +			mdata->security_state |= CXL_PMEM_SEC_STATE_USER_PLIMIT;
> +		cmd->return_code = CXL_MBOX_CMD_RC_PASSPHRASE;
> +		return -ENXIO;
> +	}
> +
> +	if (erase->type == CXL_PMEM_SEC_PASS_USER) {
> +		mdata->security_state &= ~CXL_PMEM_SEC_STATE_USER_PASS_SET;
> +		mdata->user_limit = 0;

I think it would be more logical to set this to zero as part of the password
testing block above rather than down here.

I also 'think' the user passphrase is wiped even if the secure erase was
done with the master key. 
"The user passphrase shall be disabled after secure erase, but the master passphrase, if set, shall 
be unchanged" doesn't say anything about only if the user passphrase was the one used to
perform the erase.

> +		memset(mdata->user_pass, 0, NVDIMM_PASSPHRASE_LEN);
> +	} else if (erase->type == CXL_PMEM_SEC_PASS_MASTER) {
> +		mdata->master_limit = 0;
> +	}
> +
> +	mdata->security_state &= ~CXL_PMEM_SEC_STATE_LOCKED;
> +
> +	return 0;
> +}
> +