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