答复: 答复: 答复: [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?
> >