[PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[Zhang Qilong]

If dhchap_secret is not consistent with options or
nvme_auth_generate_key() fails, we should free the
memory of dhchap_secret to avoid memleak.

Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
---
 drivers/nvme/host/core.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index da55ce45ac70..e06d1b3961fe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3748,13 +3748,16 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 		int ret;
 
 		ret = nvme_auth_generate_key(dhchap_secret, &ctrl->host_key);
-		if (ret)
+		if (ret) {
+			kfree(dhchap_secret);
 			return ret;
+		}
 		kfree(opts->dhchap_secret);
 		opts->dhchap_secret = dhchap_secret;
 		/* Key has changed; re-authentication with new key */
 		nvme_auth_reset(ctrl);
-	}
+	} else
+		kfree(dhchap_secret);
 	/* Start re-authentication */
 	dev_info(ctrl->device, "re-authenticating controller\n");
 	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
-- 
2.25.1

Re: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[Sagi Grimberg]

Hey Zhang,

> If dhchap_secret is not consistent with options or
> nvme_auth_generate_key() fails, we should free the
> memory of dhchap_secret to avoid memleak.
> 
> Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>

Please have a look at nvme-6.2, if there is still a leak, please
send a patch against it.

Thanks.

> ---
>   drivers/nvme/host/core.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index da55ce45ac70..e06d1b3961fe 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3748,13 +3748,16 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>   		int ret;
>   
>   		ret = nvme_auth_generate_key(dhchap_secret, &ctrl->host_key);
> -		if (ret)
> +		if (ret) {
> +			kfree(dhchap_secret);
>   			return ret;
> +		}
>   		kfree(opts->dhchap_secret);
>   		opts->dhchap_secret = dhchap_secret;
>   		/* Key has changed; re-authentication with new key */
>   		nvme_auth_reset(ctrl);
> -	}
> +	} else
> +		kfree(dhchap_secret);
>   	/* Start re-authentication */
>   	dev_info(ctrl->device, "re-authenticating controller\n");
>   	queue_work(nvme_wq, &ctrl->dhchap_auth_work);

答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[zhangqilong]

> 
> > If dhchap_secret is not consistent with options or
> > nvme_auth_generate_key() fails, we should free the memory of
> > dhchap_secret to avoid memleak.
> >
> > Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
> > Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
> 
> Please have a look at nvme-6.2, if there is still a leak, please send a patch
> against it.
> 

Hi
     I have checked it,  there is still a leak at nvme-6.2.

Thanks.

> Thanks.
> 
> > ---
> >   drivers/nvme/host/core.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index
> > da55ce45ac70..e06d1b3961fe 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3748,13 +3748,16 @@ static ssize_t
> nvme_ctrl_dhchap_secret_store(struct device *dev,
> >   		int ret;
> >
> >   		ret = nvme_auth_generate_key(dhchap_secret, &ctrl-
> >host_key);
> > -		if (ret)
> > +		if (ret) {
> > +			kfree(dhchap_secret);
> >   			return ret;
> > +		}
> >   		kfree(opts->dhchap_secret);
> >   		opts->dhchap_secret = dhchap_secret;
> >   		/* Key has changed; re-authentication with new key */
> >   		nvme_auth_reset(ctrl);
> > -	}
> > +	} else
> > +		kfree(dhchap_secret);
> >   	/* Start re-authentication */
> >   	dev_info(ctrl->device, "re-authenticating controller\n");
> >   	queue_work(nvme_wq, &ctrl->dhchap_auth_work);

Re: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[Sagi Grimberg]

On 11/21/22 13:33, zhangqilong wrote:
>>
>>> If dhchap_secret is not consistent with options or
>>> nvme_auth_generate_key() fails, we should free the memory of
>>> dhchap_secret to avoid memleak.
>>>
>>> Fixes: f50fff73d620 ("nvme: implement In-Band authentication")
>>> Signed-off-by: Zhang Qilong <zhangqilong3@huawei.com>
>>
>> Please have a look at nvme-6.2, if there is still a leak, please send a patch
>> against it.
>>
> 
> Hi
>       I have checked it,  there is still a leak at nvme-6.2.

Cool. Thanks

> 
> Thanks.
> 
>> Thanks.
>>
>>> ---
>>>    drivers/nvme/host/core.c | 7 +++++--
>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c index
>>> da55ce45ac70..e06d1b3961fe 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -3748,13 +3748,16 @@ static ssize_t
>> nvme_ctrl_dhchap_secret_store(struct device *dev,
>>>    		int ret;
>>>
>>>    		ret = nvme_auth_generate_key(dhchap_secret, &ctrl-
>>> host_key);
>>> -		if (ret)
>>> +		if (ret) {
>>> +			kfree(dhchap_secret);
>>>    			return ret;
>>> +		}
>>>    		kfree(opts->dhchap_secret);
>>>    		opts->dhchap_secret = dhchap_secret;
>>>    		/* Key has changed; re-authentication with new key */
>>>    		nvme_auth_reset(ctrl);
>>> -	}
>>> +	} else
>>> +		kfree(dhchap_secret);

Perhaps lets change the check above to strncmp directly against buf and
allocate inside the clause.

>>>    	/* Start re-authentication */
>>>    	dev_info(ctrl->device, "re-authenticating controller\n");
>>>    	queue_work(nvme_wq, &ctrl->dhchap_auth_work);
> 

答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[zhangqilong]

> >>>    drivers/nvme/host/core.c | 7 +++++--
> >>>    1 file changed, 5 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >>> index da55ce45ac70..e06d1b3961fe 100644
> >>> --- a/drivers/nvme/host/core.c
> >>> +++ b/drivers/nvme/host/core.c
> >>> @@ -3748,13 +3748,16 @@ static ssize_t
> >> nvme_ctrl_dhchap_secret_store(struct device *dev,
> >>>    		int ret;
> >>>
> >>>    		ret = nvme_auth_generate_key(dhchap_secret, &ctrl-
> host_key);
> >>> -		if (ret)
> >>> +		if (ret) {
> >>> +			kfree(dhchap_secret);
> >>>    			return ret;
> >>> +		}
> >>>    		kfree(opts->dhchap_secret);
> >>>    		opts->dhchap_secret = dhchap_secret;
> >>>    		/* Key has changed; re-authentication with new key */
> >>>    		nvme_auth_reset(ctrl);
> >>> -	}
> >>> +	} else
> >>> +		kfree(dhchap_secret);
> 
> Perhaps lets change the check above to strncmp directly against buf and
> allocate inside the clause.
> 

Good suggestion and I have updated fixes like:

--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -3730,7 +3730,6 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
 {
        struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
        struct nvmf_ctrl_options *opts = ctrl->opts;
-       char *dhchap_secret;

        if (!ctrl->opts->dhchap_secret)
                return -EINVAL;
@@ -3739,17 +3738,20 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
        if (memcmp(buf, "DHHC-1:", 7))
                return -EINVAL;

-       dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
-       if (!dhchap_secret)
-               return -ENOMEM;
-       memcpy(dhchap_secret, buf, count);
        nvme_auth_stop(ctrl);
-       if (strcmp(dhchap_secret, opts->dhchap_secret)) {
+       if (strncmp(buf, opts->dhchap_secret, count)) {
                int ret;
+               char *dhchap_secret;

+               dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
+               if (!dhchap_secret)
+                       return -ENOMEM;
+               memcpy(dhchap_secret, buf, count);
                ret = nvme_auth_generate_key(dhchap_secret, &ctrl->host_key);
-               if (ret)
+               if (ret) {
+                       kfree(dhchap_secret);
                        return ret;
+               }
                kfree(opts->dhchap_secret);

Do you think is it that ok?

Re: 答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[Sagi Grimberg]

On 11/21/22 13:54, zhangqilong wrote:
>>>>>     drivers/nvme/host/core.c | 7 +++++--
>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>> index da55ce45ac70..e06d1b3961fe 100644
>>>>> --- a/drivers/nvme/host/core.c
>>>>> +++ b/drivers/nvme/host/core.c
>>>>> @@ -3748,13 +3748,16 @@ static ssize_t
>>>> nvme_ctrl_dhchap_secret_store(struct device *dev,
>>>>>     		int ret;
>>>>>
>>>>>     		ret = nvme_auth_generate_key(dhchap_secret, &ctrl-
>> host_key);
>>>>> -		if (ret)
>>>>> +		if (ret) {
>>>>> +			kfree(dhchap_secret);
>>>>>     			return ret;
>>>>> +		}
>>>>>     		kfree(opts->dhchap_secret);
>>>>>     		opts->dhchap_secret = dhchap_secret;
>>>>>     		/* Key has changed; re-authentication with new key */
>>>>>     		nvme_auth_reset(ctrl);
>>>>> -	}
>>>>> +	} else
>>>>> +		kfree(dhchap_secret);
>>
>> Perhaps lets change the check above to strncmp directly against buf and
>> allocate inside the clause.
>>
> 
> Good suggestion and I have updated fixes like:
> 
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -3730,7 +3730,6 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>   {
>          struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
>          struct nvmf_ctrl_options *opts = ctrl->opts;
> -       char *dhchap_secret;
> 
>          if (!ctrl->opts->dhchap_secret)
>                  return -EINVAL;
> @@ -3739,17 +3738,20 @@ static ssize_t nvme_ctrl_dhchap_secret_store(struct device *dev,
>          if (memcmp(buf, "DHHC-1:", 7))
>                  return -EINVAL;
> 
> -       dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> -       if (!dhchap_secret)
> -               return -ENOMEM;
> -       memcpy(dhchap_secret, buf, count);
>          nvme_auth_stop(ctrl);
> -       if (strcmp(dhchap_secret, opts->dhchap_secret)) {
> +       if (strncmp(buf, opts->dhchap_secret, count)) {
>                  int ret;
> +               char *dhchap_secret;
> 
> +               dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> +               if (!dhchap_secret)
> +                       return -ENOMEM;
> +               memcpy(dhchap_secret, buf, count);

Maybe kmemdup instead?

>                  ret = nvme_auth_generate_key(dhchap_secret, &ctrl->host_key);
> -               if (ret)
> +               if (ret) {
> +                       kfree(dhchap_secret);
>                          return ret;
> +               }
>                  kfree(opts->dhchap_secret);
> 
> Do you think is it that ok?
> 

答复: 答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()

[zhangqilong]

> On 11/21/22 13:54, zhangqilong wrote:
> >>>>>     drivers/nvme/host/core.c | 7 +++++--
> >>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >>>>> index da55ce45ac70..e06d1b3961fe 100644
> >>>>> --- a/drivers/nvme/host/core.c
> >>>>> +++ b/drivers/nvme/host/core.c
> >>>>> @@ -3748,13 +3748,16 @@ static ssize_t
> >>>> nvme_ctrl_dhchap_secret_store(struct device *dev,
> >>>>>     		int ret;
> >>>>>
> >>>>>     		ret = nvme_auth_generate_key(dhchap_secret,
> &ctrl-
> >> host_key);
> >>>>> -		if (ret)
> >>>>> +		if (ret) {
> >>>>> +			kfree(dhchap_secret);
> >>>>>     			return ret;
> >>>>> +		}
> >>>>>     		kfree(opts->dhchap_secret);
> >>>>>     		opts->dhchap_secret = dhchap_secret;
> >>>>>     		/* Key has changed; re-authentication with new key
> */
> >>>>>     		nvme_auth_reset(ctrl);
> >>>>> -	}
> >>>>> +	} else
> >>>>> +		kfree(dhchap_secret);
> >>
> >> Perhaps lets change the check above to strncmp directly against buf
> >> and allocate inside the clause.
> >>
> >
> > Good suggestion and I have updated fixes like:
> >
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -3730,7 +3730,6 @@ static ssize_t
> nvme_ctrl_dhchap_secret_store(struct device *dev,
> >   {
> >          struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> >          struct nvmf_ctrl_options *opts = ctrl->opts;
> > -       char *dhchap_secret;
> >
> >          if (!ctrl->opts->dhchap_secret)
> >                  return -EINVAL;
> > @@ -3739,17 +3738,20 @@ static ssize_t
> nvme_ctrl_dhchap_secret_store(struct device *dev,
> >          if (memcmp(buf, "DHHC-1:", 7))
> >                  return -EINVAL;
> >
> > -       dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> > -       if (!dhchap_secret)
> > -               return -ENOMEM;
> > -       memcpy(dhchap_secret, buf, count);
> >          nvme_auth_stop(ctrl);
> > -       if (strcmp(dhchap_secret, opts->dhchap_secret)) {
> > +       if (strncmp(buf, opts->dhchap_secret, count)) {
> >                  int ret;
> > +               char *dhchap_secret;
> >
> > +               dhchap_secret = kzalloc(count + 1, GFP_KERNEL);
> > +               if (!dhchap_secret)
> > +                       return -ENOMEM;
> > +               memcpy(dhchap_secret, buf, count);
> 
> Maybe kmemdup instead?

OK, It looks good. I will update in version v2.

Thanks.
> 
> >                  ret = nvme_auth_generate_key(dhchap_secret, &ctrl->host_key);
> > -               if (ret)
> > +               if (ret) {
> > +                       kfree(dhchap_secret);
> >                          return ret;
> > +               }
> >                  kfree(opts->dhchap_secret);
> >
> > Do you think is it that ok?
> >