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