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

[Dave Jiang <dave.jiang@intel.com>]

On 11/11/2022 2:37 AM, Jonathan Cameron wrote:
> On Tue, 08 Nov 2022 10:26:30 -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>
> Hi Dave,
> 
> My feedback in previous version was in the wrong place and I think that
> led you to update the wrong error path.
> 
> See inline
> 
> Jonathan
> 
>> ---
>>   tools/testing/cxl/test/mem.c |   59 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 59 insertions(+)
>>
>> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
>> index 90607597b9a4..aa6dda21bc5f 100644
>> --- a/tools/testing/cxl/test/mem.c
>> +++ b/tools/testing/cxl/test/mem.c
>> @@ -362,6 +362,62 @@ 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 &&
>> +	    erase->type != CXL_PMEM_SEC_PASS_MASTER) {
>> +		cmd->return_code = CXL_MBOX_CMD_RC_SECURITY;
>> +		return -ENXIO;
>> +	}
> 
> A stuck my comment in a rather odd location.  I was commenting not
> on the block above, but rather the one below.
> 
> Frozen it's fixed by providing the master pass phrase - so the
> above should just check if frozen.
> 
> The original comment was about the neck block.  Having failed user
> passcode too many times isn't relevant if the one provided this
> time is the master passcode - so add the
> erase->type != CXL_PMEM_SEC_PASS_MASTER to the next if block.

We probably better off do "erase->type == CXL_PMEM_SEC_PASS_USER" to 
check the limit against the user type?

Also while we are here, I think I'm missing a check against the master 
passphrase limit here as well. So probably we should have something like?

         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 (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PLIMIT) {
>> +		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;
>> +	}
>> +
>> +	if (erase->type == CXL_PMEM_SEC_PASS_USER &&
>> +	    mdata->security_state & CXL_PMEM_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);
>> +		return 0;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   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 +526,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;
>>   	}
>>
>>
>