* [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
@ 2024-03-06 6:03 Shin'ichiro Kawasaki
2024-03-06 6:30 ` Chaitanya Kulkarni
` (4 more replies)
0 siblings, 5 replies; 7+ messages in thread
From: Shin'ichiro Kawasaki @ 2024-03-06 6:03 UTC (permalink / raw
To: linux-nvme
Cc: Daniel Wagner, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Hannes Reinecke,
Shin'ichiro Kawasaki
When nvme_identify_ns() fails, it frees the pointer to the struct
nvme_id_ns before it returns. However, ns_update_nuse() calls kfree()
for the pointer even when nvme_identify_ns() fails. This results in
KASAN double-free, which was observed with blktests nvme/045 with
proposed patches [1] on the kernel v6.8-rc7. Fix the double-free by
skipping kfree() when nvme_identify_ns() fails.
Link: https://lore.kernel.org/linux-block/20240304161303.19681-1-dwagner@suse.de/ [1]
Fixes: a1a825ab6a60 ("nvme: add csi, ms and nuse to sysfs")
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
Changes from v1:
* Removed the goto label and just return on nvme_identify_ns() failure
drivers/nvme/host/sysfs.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index f2832f70e7e0..09fcaa519e5b 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -221,14 +221,11 @@ static int ns_update_nuse(struct nvme_ns *ns)
ret = nvme_identify_ns(ns->ctrl, ns->head->ns_id, &id);
if (ret)
- goto out_free_id;
+ return ret;
ns->head->nuse = le64_to_cpu(id->nuse);
-
-out_free_id:
kfree(id);
-
- return ret;
+ return 0;
}
static ssize_t nuse_show(struct device *dev, struct device_attribute *attr,
--
2.43.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
@ 2024-03-06 6:30 ` Chaitanya Kulkarni
2024-03-06 7:27 ` Daniel Wagner
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Chaitanya Kulkarni @ 2024-03-06 6:30 UTC (permalink / raw
To: Shin'ichiro Kawasaki, linux-nvme@lists.infradead.org
Cc: Daniel Wagner, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Hannes Reinecke
On 3/5/24 22:03, Shin'ichiro Kawasaki wrote:
> When nvme_identify_ns() fails, it frees the pointer to the struct
> nvme_id_ns before it returns. However, ns_update_nuse() calls kfree()
> for the pointer even when nvme_identify_ns() fails. This results in
> KASAN double-free, which was observed with blktests nvme/045 with
> proposed patches [1] on the kernel v6.8-rc7. Fix the double-free by
> skipping kfree() when nvme_identify_ns() fails.
>
> Link: https://lore.kernel.org/linux-block/20240304161303.19681-1-dwagner@suse.de/ [1]
> Fixes: a1a825ab6a60 ("nvme: add csi, ms and nuse to sysfs")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> ---
>
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
-ck
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
2024-03-06 6:30 ` Chaitanya Kulkarni
@ 2024-03-06 7:27 ` Daniel Wagner
2024-03-06 13:04 ` Christoph Hellwig
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Daniel Wagner @ 2024-03-06 7:27 UTC (permalink / raw
To: Shin'ichiro Kawasaki
Cc: linux-nvme, Keith Busch, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Hannes Reinecke
On Wed, Mar 06, 2024 at 03:03:03PM +0900, Shin'ichiro Kawasaki wrote:
> When nvme_identify_ns() fails, it frees the pointer to the struct
> nvme_id_ns before it returns. However, ns_update_nuse() calls kfree()
> for the pointer even when nvme_identify_ns() fails. This results in
> KASAN double-free, which was observed with blktests nvme/045 with
> proposed patches [1] on the kernel v6.8-rc7. Fix the double-free by
> skipping kfree() when nvme_identify_ns() fails.
>
> Link: https://lore.kernel.org/linux-block/20240304161303.19681-1-dwagner@suse.de/ [1]
> Fixes: a1a825ab6a60 ("nvme: add csi, ms and nuse to sysfs")
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Daniel Wagner <dwagner@suse.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
2024-03-06 6:30 ` Chaitanya Kulkarni
2024-03-06 7:27 ` Daniel Wagner
@ 2024-03-06 13:04 ` Christoph Hellwig
2024-03-06 14:00 ` Keith Busch
2024-03-07 7:57 ` Sagi Grimberg
4 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2024-03-06 13:04 UTC (permalink / raw
To: Shin'ichiro Kawasaki
Cc: linux-nvme, Daniel Wagner, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni,
Hannes Reinecke
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
` (2 preceding siblings ...)
2024-03-06 13:04 ` Christoph Hellwig
@ 2024-03-06 14:00 ` Keith Busch
2024-03-06 14:18 ` Christoph Hellwig
2024-03-07 7:57 ` Sagi Grimberg
4 siblings, 1 reply; 7+ messages in thread
From: Keith Busch @ 2024-03-06 14:00 UTC (permalink / raw
To: Shin'ichiro Kawasaki
Cc: linux-nvme, Daniel Wagner, Jens Axboe, Christoph Hellwig,
Sagi Grimberg, Chaitanya Kulkarni, Hannes Reinecke
On Wed, Mar 06, 2024 at 03:03:03PM +0900, Shin'ichiro Kawasaki wrote:
> When nvme_identify_ns() fails, it frees the pointer to the struct
> nvme_id_ns before it returns. However, ns_update_nuse() calls kfree()
> for the pointer even when nvme_identify_ns() fails. This results in
> KASAN double-free, which was observed with blktests nvme/045 with
> proposed patches [1] on the kernel v6.8-rc7. Fix the double-free by
> skipping kfree() when nvme_identify_ns() fails.
Your patch is good and applied for nvme-6.9. I just want to mention we
have a bit of an inconsistency in how the driver handles this pattern:
nvme_identify_ns_nvm() only sets the caller's pointer on success, but
nvme_identify_ns() and nvme_identify_ctrl() set it all the time. If we'd
only set it on success, then this problem wouldn't happen, so a possible
follow up suggestion to prevent the caller from having a pointer to
freed memory:
---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index c4d928585ce35..2baf5786a92fe 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1403,8 +1403,10 @@ static int nvme_identify_ctrl(struct nvme_ctrl *dev, struct nvme_id_ctrl **id)
error = nvme_submit_sync_cmd(dev->admin_q, &c, *id,
sizeof(struct nvme_id_ctrl));
- if (error)
+ if (error) {
kfree(*id);
+ *id = NULL;
+ }
return error;
}
@@ -1533,6 +1535,7 @@ int nvme_identify_ns(struct nvme_ctrl *ctrl, unsigned nsid,
if (error) {
dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
kfree(*id);
+ *id = NULL;
}
return error;
}
--
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 14:00 ` Keith Busch
@ 2024-03-06 14:18 ` Christoph Hellwig
0 siblings, 0 replies; 7+ messages in thread
From: Christoph Hellwig @ 2024-03-06 14:18 UTC (permalink / raw
To: Keith Busch
Cc: Shin'ichiro Kawasaki, linux-nvme, Daniel Wagner, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Chaitanya Kulkarni,
Hannes Reinecke
On Wed, Mar 06, 2024 at 07:00:51AM -0700, Keith Busch wrote:
> On Wed, Mar 06, 2024 at 03:03:03PM +0900, Shin'ichiro Kawasaki wrote:
> > When nvme_identify_ns() fails, it frees the pointer to the struct
> > nvme_id_ns before it returns. However, ns_update_nuse() calls kfree()
> > for the pointer even when nvme_identify_ns() fails. This results in
> > KASAN double-free, which was observed with blktests nvme/045 with
> > proposed patches [1] on the kernel v6.8-rc7. Fix the double-free by
> > skipping kfree() when nvme_identify_ns() fails.
>
> Your patch is good and applied for nvme-6.9. I just want to mention we
> have a bit of an inconsistency in how the driver handles this pattern:
> nvme_identify_ns_nvm() only sets the caller's pointer on success, but
> nvme_identify_ns() and nvme_identify_ctrl() set it all the time. If we'd
> only set it on success, then this problem wouldn't happen, so a possible
> follow up suggestion to prevent the caller from having a pointer to
> freed memory:
This looks like a good idea to me.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
` (3 preceding siblings ...)
2024-03-06 14:00 ` Keith Busch
@ 2024-03-07 7:57 ` Sagi Grimberg
4 siblings, 0 replies; 7+ messages in thread
From: Sagi Grimberg @ 2024-03-07 7:57 UTC (permalink / raw
To: Shin'ichiro Kawasaki, linux-nvme
Cc: Daniel Wagner, Keith Busch, Jens Axboe, Christoph Hellwig,
Chaitanya Kulkarni, Hannes Reinecke
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-07 7:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-06 6:03 [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse() Shin'ichiro Kawasaki
2024-03-06 6:30 ` Chaitanya Kulkarni
2024-03-06 7:27 ` Daniel Wagner
2024-03-06 13:04 ` Christoph Hellwig
2024-03-06 14:00 ` Keith Busch
2024-03-06 14:18 ` Christoph Hellwig
2024-03-07 7:57 ` Sagi Grimberg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).