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

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

On 11/16/2022 3:43 AM, Jonathan Cameron wrote:
> On Tue, 15 Nov 2022 10:01:53 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
> 
>> On 11/15/2022 7:57 AM, Dave Jiang 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;
>>> }
>>>   
>> This is the current change:
>>
>> +       switch (erase->type) {
>> +       case CXL_PMEM_SEC_PASS_MASTER:
>> +               if (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;
> 
>> +               } else if (mdata->security_state & CXL_PMEM_SEC_STATE_USER_PASS_SET) {
>> +                       return -EINVAL;
>> +               }

So while looking at 8.2.9.8.6.3 I stumbled on this line: "When the 
master passphrase is disabled, the device shall return Invalid Input for 
the Passphrase Secure Erase command with the master passphrase". I 
suppose the above would reduce to just else {} instead? And it probably 
wouldn't hurt to have the spec duplicate this line under the passphrase 
secure erase section as well.

> I would add a comment here to say what we aren't faking.  The aim being to show that
> in all the good paths this happens, even though we don't do the other stuff in
> some of them.
> 
> /* Scramble encryption keys so that data is effectively erased */
> 
>> +
>> +               return 0;
>> +       case CXL_PMEM_SEC_PASS_USER:
>> +               if (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);
>> +               }
>> +
> 
> /* Scramble encryption keys so that data is effectively erased */
> here as well for the same reason.
> 
>> +               return 0;
>> +       default:
>> +               fallthrough;
> 
> Might as well return -EINVAL; here and drop the below one.
> 
> Otherwise looks good to me.  We could sprinkle some comments in here to
> hightlight why we have concluded it ought to behave like this.
> If nothing else, I doubt either of us will remember when we look at this
> code in more than a few days time ;)
> 
> Otherwise looks good to me.
> 
> Jonathan
> 
> 
>> +       }
>> +
>> +       return -EINVAL;
>>
>>
>>
>>>>
>>>> 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;
>>>>>        }
>>>>>
>>>>>   
>>>>   
> 
>