[PATCH v3 0/2] sd_zbc fixes

[PATCH v3 0/2] sd_zbc fixes

[Damien Le Moal]

A couple of patches to fix 2 issues with the zbc code:
* A potential NULL pointer dereference in sd_is_zoned(), if that
  function is called when sdkp->device is not yet set (e.g. if an error
  happen early in sd_probe()).
* Make sure that sdkp zone information memory is never leaked.

Changes from v2:
* Simplified patch 1 fix as suggested by Christoph.

Changes from v1:
* Added reviewed-by and tested-by tags in patch 1
* Changed patch 2 to rename sd_zbc_clear_zone_info() to
  sd_zbc_free_zone_info() and remove sd_zbc_release_disk().

Damien Le Moal (2):
  scsi: sd: Fix potential NULL pointer dereference
  scsi: sd_zbc: prevent zone information memory leak

 drivers/scsi/sd.c     |  3 +--
 drivers/scsi/sd.h     |  4 ++--
 drivers/scsi/sd_zbc.c | 26 +++++++++++++-------------
 3 files changed, 16 insertions(+), 17 deletions(-)

-- 
2.36.1

[PATCH v3 1/2] scsi: sd: Fix potential NULL pointer dereference

[Damien Le Moal]

If sd_probe() sees an early error before sdkp->device is initialized,
sd_zbc_release_disk() is called. This causes a NULL pointer dereference
when sd_is_zoned() is called inside that function. Avoid this by
removing the call to sd_zbc_release_disk() in sd_probe() error path.

This chnage is safe and does not result in zone information memory
leakage because the zone information for a zoned disk is allocated only
when sd_revalidate_disk() is called, at which point sdkp->disk_dev is
fully set, resulting in sd_disk_release() being called when needed to
cleanup a disk zone information using sd_zbc_release_disk().

Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
Suggested-by: Christoph Hellwig <hch@lst.de>
Fixes: 89d947561077 ("sd: Implement support for ZBC device")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/sd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 749316462075..dabdc0eeb3dc 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3542,7 +3542,6 @@ static int sd_probe(struct device *dev)
  out_put:
 	put_disk(gd);
  out_free:
-	sd_zbc_release_disk(sdkp);
 	kfree(sdkp);
  out:
 	scsi_autopm_put_device(sdp);
-- 
2.36.1

Re: [PATCH v3 1/2] scsi: sd: Fix potential NULL pointer dereference

[Christoph Hellwig]

On Wed, Jun 01, 2022 at 03:25:43PM +0900, Damien Le Moal wrote:
> If sd_probe() sees an early error before sdkp->device is initialized,
> sd_zbc_release_disk() is called. This causes a NULL pointer dereference
> when sd_is_zoned() is called inside that function. Avoid this by
> removing the call to sd_zbc_release_disk() in sd_probe() error path.
> 
> This chnage is safe and does not result in zone information memory
> leakage because the zone information for a zoned disk is allocated only
> when sd_revalidate_disk() is called, at which point sdkp->disk_dev is
> fully set, resulting in sd_disk_release() being called when needed to
> cleanup a disk zone information using sd_zbc_release_disk().

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v3 2/2] scsi: sd_zbc: prevent zone information memory leak

[Damien Le Moal]

Make sure to always free a scsi disk zone information, even for regular
disks. This ensures that there is no memory leak, even in the case of a
zoned disk changing type to a regular disk (e.g. with a reformat using
the FORMAT WITH PRESET command or other vendor proprietary command).

To do this, rename sd_zbc_clear_zone_info() to sd_zbc_free_zone_info()
and remove sd_zbc_release_disk(). A call to sd_zbc_free_zone_info() is
added to sd_zbc_read_zones() for drives for which sd_is_zoned() returns
false. Furthermore, sd_zbc_free_zone_info() code make s sure that the
sdkp rev_mutex is never used while not being initialized by gating the
cleanup code with a a check on the zone_wp_update_buf field as it is
never NULL when rev_mutex has been initialized.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 drivers/scsi/sd.c     |  2 +-
 drivers/scsi/sd.h     |  4 ++--
 drivers/scsi/sd_zbc.c | 26 +++++++++++++-------------
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index dabdc0eeb3dc..be812987aa3f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3578,7 +3578,7 @@ static void scsi_disk_release(struct device *dev)
 	struct scsi_disk *sdkp = to_scsi_disk(dev);
 
 	ida_free(&sd_index_ida, sdkp->index);
-	sd_zbc_release_disk(sdkp);
+	sd_zbc_free_zone_info(sdkp);
 	put_device(&sdkp->device->sdev_gendev);
 	free_opal_dev(sdkp->opal_dev);
 
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 2abad54fd23f..5eea762f84d1 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -241,7 +241,7 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
 
 #ifdef CONFIG_BLK_DEV_ZONED
 
-void sd_zbc_release_disk(struct scsi_disk *sdkp);
+void sd_zbc_free_zone_info(struct scsi_disk *sdkp);
 int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]);
 int sd_zbc_revalidate_zones(struct scsi_disk *sdkp);
 blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
@@ -256,7 +256,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
 
 #else /* CONFIG_BLK_DEV_ZONED */
 
-static inline void sd_zbc_release_disk(struct scsi_disk *sdkp) {}
+static inline void sd_zbc_free_zone_info(struct scsi_disk *sdkp) {}
 
 static inline int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 {
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 5b9fad70aa88..6acc4f406eb8 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -786,8 +786,11 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp)
 	return 0;
 }
 
-static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
+void sd_zbc_free_zone_info(struct scsi_disk *sdkp)
 {
+	if (!sdkp->zone_wp_update_buf)
+		return;
+
 	/* Serialize against revalidate zones */
 	mutex_lock(&sdkp->rev_mutex);
 
@@ -802,12 +805,6 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
 	mutex_unlock(&sdkp->rev_mutex);
 }
 
-void sd_zbc_release_disk(struct scsi_disk *sdkp)
-{
-	if (sd_is_zoned(sdkp))
-		sd_zbc_clear_zone_info(sdkp);
-}
-
 static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
 {
 	struct scsi_disk *sdkp = scsi_disk(disk);
@@ -914,12 +911,15 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 	u32 zone_blocks = 0;
 	int ret;
 
-	if (!sd_is_zoned(sdkp))
+	if (!sd_is_zoned(sdkp)) {
 		/*
-		 * Device managed or normal SCSI disk,
-		 * no special handling required
+		 * Device managed or normal SCSI disk, no special handling
+		 * required. Nevertheless, free the disk zone information in
+		 * case the device type changed.
 		 */
+		sd_zbc_free_zone_info(sdkp);
 		return 0;
+	}
 
 	/* READ16/WRITE16 is mandatory for ZBC disks */
 	sdkp->device->use_16_for_rw = 1;
@@ -928,11 +928,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
 	if (!blk_queue_is_zoned(q)) {
 		/*
 		 * This can happen for a host aware disk with partitions.
-		 * The block device zone information was already cleared
-		 * by blk_queue_set_zoned(). Only clear the scsi disk zone
+		 * The block device zone model was already cleared by
+		 * blk_queue_set_zoned(). Only free the scsi disk zone
 		 * information and exit early.
 		 */
-		sd_zbc_clear_zone_info(sdkp);
+		sd_zbc_free_zone_info(sdkp);
 		return 0;
 	}
 
-- 
2.36.1

Re: [PATCH v3 2/2] scsi: sd_zbc: prevent zone information memory leak

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 0/2] sd_zbc fixes

[Martin K. Petersen]

Damien,

> A couple of patches to fix 2 issues with the zbc code:
> * A potential NULL pointer dereference in sd_is_zoned(), if that
>   function is called when sdkp->device is not yet set (e.g. if an error
>   happen early in sd_probe()).
> * Make sure that sdkp zone information memory is never leaked.

Applied to 5.19/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering