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

[Sagi Grimberg <sagi@grimberg.me>]

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?