Re: [PATCH v2] nvme: host: fix double-free of struct nvme_id_ns in ns_update_nuse()

[Keith Busch <kbusch@kernel.org>]

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;
 }
--