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

[Hannes Reinecke <hare@suse.de>]

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.

> 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 ...


Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@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