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

[Hannes Reinecke]

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.
Consequently, if a non-default value is already set it won't be overwritten.
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().
Hence I fear we have to go with your suggestion.

Will be preparing a pull request.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare at suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer