* [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
@ 2022-11-21 6:21 Zhang Qilong
2022-11-21 9:51 ` Sagi Grimberg
0 siblings, 1 reply; 7+ messages in thread
From: Zhang Qilong @ 2022-11-21 6:21 UTC (permalink / raw)
To: hare, kbusch, axboe, hch, sagi; +Cc: linux-nvme
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 6:21 [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store() Zhang Qilong
@ 2022-11-21 9:51 ` Sagi Grimberg
2022-11-21 11:33 ` 答复: " zhangqilong
0 siblings, 1 reply; 7+ messages in thread
From: Sagi Grimberg @ 2022-11-21 9:51 UTC (permalink / raw)
To: Zhang Qilong, hare, kbusch, axboe, hch; +Cc: linux-nvme
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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 9:51 ` Sagi Grimberg
@ 2022-11-21 11:33 ` zhangqilong
2022-11-21 11:39 ` Sagi Grimberg
0 siblings, 1 reply; 7+ messages in thread
From: zhangqilong @ 2022-11-21 11:33 UTC (permalink / raw)
To: Sagi Grimberg, hare@suse.de, kbusch@kernel.org, axboe@fb.com,
hch@lst.de
Cc: linux-nvme@lists.infradead.org
>
> > 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);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 11:33 ` 答复: " zhangqilong
@ 2022-11-21 11:39 ` Sagi Grimberg
2022-11-21 11:54 ` 答复: " zhangqilong
0 siblings, 1 reply; 7+ messages in thread
From: Sagi Grimberg @ 2022-11-21 11:39 UTC (permalink / raw)
To: zhangqilong, hare@suse.de, kbusch@kernel.org, axboe@fb.com,
hch@lst.de
Cc: linux-nvme@lists.infradead.org
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);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* 答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 11:39 ` Sagi Grimberg
@ 2022-11-21 11:54 ` zhangqilong
2022-11-21 14:23 ` Sagi Grimberg
0 siblings, 1 reply; 7+ messages in thread
From: zhangqilong @ 2022-11-21 11:54 UTC (permalink / raw)
To: Sagi Grimberg, hare@suse.de, kbusch@kernel.org, axboe@fb.com,
hch@lst.de
Cc: linux-nvme@lists.infradead.org
> >>> 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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: 答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 11:54 ` 答复: " zhangqilong
@ 2022-11-21 14:23 ` Sagi Grimberg
2022-11-21 15:09 ` 答复: " zhangqilong
0 siblings, 1 reply; 7+ messages in thread
From: Sagi Grimberg @ 2022-11-21 14:23 UTC (permalink / raw)
To: zhangqilong, hare@suse.de, kbusch@kernel.org, axboe@fb.com,
hch@lst.de
Cc: linux-nvme@lists.infradead.org
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?
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* 答复: 答复: 答复: [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store()
2022-11-21 14:23 ` Sagi Grimberg
@ 2022-11-21 15:09 ` zhangqilong
0 siblings, 0 replies; 7+ messages in thread
From: zhangqilong @ 2022-11-21 15:09 UTC (permalink / raw)
To: Sagi Grimberg, hare@suse.de, kbusch@kernel.org, axboe@fb.com,
hch@lst.de
Cc: linux-nvme@lists.infradead.org
> 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?
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-11-21 15:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-21 6:21 [PATCH] nvme: fix memleak in nvme_ctrl_dhchap_secret_store() Zhang Qilong
2022-11-21 9:51 ` Sagi Grimberg
2022-11-21 11:33 ` 答复: " zhangqilong
2022-11-21 11:39 ` Sagi Grimberg
2022-11-21 11:54 ` 答复: " zhangqilong
2022-11-21 14:23 ` Sagi Grimberg
2022-11-21 15:09 ` 答复: " zhangqilong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox