[PATCH 06/11] nvme: Implement In-Band authentication

[Sagi Grimberg]

On 6/22/22 13:26, Hannes Reinecke wrote:
> On 6/22/22 11:58, Sagi Grimberg wrote:
>>
>>
>> On 6/22/22 12:20, Hannes Reinecke wrote:
>>> On 6/22/22 11:09, Sagi Grimberg wrote:
>>>>
>>>>>>>>> Looks like if I pass a malformed ctrl key to nvme connect I am 
>>>>>>>>> able to
>>>>>>>>> crash the system:
>>>>>>>>
>>>>>>>> This was what I used in this:
>>>>>>>> $ nvme connect -a 192.168.123.1 -t tcp  -s 8009 -n testnqn1 -S 
>>>>>>>> "DHHC-1:00:QpxVGpctx5J+4SeW2MClUI8rfZO3WdP1llImvsPsx7e3TK+I:" -C 
>>>>>>>> "DHHC-1:00:Jc/My1o0qtLCWRp+sHhAVafdfaS7YQOMYhk9zSmlatobqB8C:"
>>>>>>>>
>>>>>>>> The dhchap_ctrl_key is the offending string...
>>>>>>>>
>>>>>>> Right. Should be fixed with the attached patch.
>>>>>>> Can you check?
>>>>>>
>>>>>> Yes, that works.
>>>>>
>>>>> So, to summarize: With this one and the tentative patch I've sent 
>>>>> earlier:
>>>>>
>>>>> diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
>>>>> index 53184ac76240..a03f41fa146e 100644
>>>>> --- a/drivers/nvme/host/auth.c
>>>>> +++ b/drivers/nvme/host/auth.c
>>>>> @@ -842,6 +842,10 @@ int nvme_auth_negotiate(struct nvme_ctrl 
>>>>> *ctrl, int qid)
>>>>>                  dev_warn(ctrl->device, "qid %d: no key\n", qid);
>>>>>                  return -ENOKEY;
>>>>>          }
>>>>> +       if (ctrl->opts->dhchap_ctrl_secret && !ctrl->ctrl_key) {
>>>>> +               dev_warn(ctrl->device, "qid %d: invalid ctrl 
>>>>> key\n", qid);
>>>>> +               return -ENOKEY;
>>>>> +       }
>>>>>
>>>>>          mutex_lock(&ctrl->dhchap_auth_mutex);
>>>>>          /* Check if the context is already queued */
>>>>>
>>>>>
>>>>>
>>>>> your issues are resolved?
>>>>> Just to clarify before I sent another round of patches ...
>>>>
>>>> No. I don't understand why this patch is needed. As I noted
>>>> earlier, when I pass a wrong ctrl key I fail to connect do
>>>> to authentication error. Also when I pass a malformed ctrl key.
>>>>
>>>> So what does this patch fix? Can you give me an example that should
>>>> have failed before and only with this patch it fails?
>>>>
>>> Reasoning is as follows:
>>> When an invalid controller key is passed via 
>>> 'ctrl->opts->dhchap_ctrl_secret' we'll end up with an empty 
>>> 'ctrl->ctrl_key' in nvme_auth_init_ctrl() (which is what caused the 
>>> original oops).
>>> But the negotiation will still start, and, seeing that 
>>> 'ctrl->ctrl_key' is empty, will _not_ attempt controller authentication.
>>> But as a controller key is passed from userland we should have 
>>> attempted it, and hence an error is in order.
>>
>> Yes, I see it now.
>> And the patch seems to work.
>>
>>>> And for the other issue, please read it again, I think it is related
>>>> to nvme-cli and not this patchset.
>>>
>>> Hmm. Okay, I'll check, and possibly have to create another blktest 
>>> for this ...
>>
>> Looks like a libnvme fix makes the behavior go away:
>> -- 
>> diff --git a/src/nvme/fabrics.c b/src/nvme/fabrics.c
>> index 53199a29e264..2dd863fb8b4c 100644
>> --- a/src/nvme/fabrics.c
>> +++ b/src/nvme/fabrics.c
>> @@ -596,8 +596,11 @@ int nvmf_add_ctrl(nvme_host_t h, nvme_ctrl_t c,
>>                                          nvme_ctrl_get_host_iface(c),
>>                                          nvme_ctrl_get_trsvcid(c),
>>                                          NULL);
>> -               if (fc)
>> +               if (fc) {
>>                          cfg = merge_config(c, nvme_ctrl_get_config(fc));
>> +                       if (fc->dhchap_key)
>> +                               nvme_ctrl_set_dhchap_key(c, 
>> fc->dhchap_key);
>> +               }
>>          }
>>
>>          nvme_ctrl_set_discovered(c, true);
>> -- 
>>
>> Looks like when we scan to see an existing controller in the config,
>> we find it, but merge_config does not touch the ctrl dhchap_key.
>>
>> Maybe it would be better to just add the dhchap_key to the ctrl cfg
>> and have merge_config merge it as well? Or we can live with dhchap_key
>> merged explicitly?
> 
> Hmm. That runs slightly afoul of the merge logic.
> 'merge_config' will overwrite any value _not_ being the default.

There is no ctrl dhchap_key default though...

> Consequently, if a non-default value is already set it won't be 
> overwritten.

Makes sense.

> Which means that if the 'cfg' structure already contained a controller 
> secret (which could've been used, say, for discovery) we won't overwrite 
> it in merge_config().

We probably shouldn't override it this way either if it was explicitly
set by the user no?

> Hence I fear we have to go with your suggestion.

Whatever you choose. Don't really care...

> 
> Will be preparing a pull request.
> 

Thanks