[PATCH 0/2] sd_zbc fixes

[PATCH 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.

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

 drivers/scsi/sd.h     |  3 ++-
 drivers/scsi/sd_zbc.c | 15 ++++++++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.36.1

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

[Damien Le Moal]

If sd_probe() sees an error before sdkp->device is initialized,
sd_zbc_release_disk() is called, which causes a NULL pointer dereference
when sd_is_zoned() is called. Avoid this by also testing if a scsi disk
device pointer is set in sd_is_zoned().

Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
Fixes: 89d947561077 ("sd: Implement support for ZBC device")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/sd.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 2abad54fd23f..b90b96e8834e 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -236,7 +236,8 @@ static inline void sd_dif_config_host(struct scsi_disk *disk)
 
 static inline int sd_is_zoned(struct scsi_disk *sdkp)
 {
-	return sdkp->zoned == 1 || sdkp->device->type == TYPE_ZBC;
+	return sdkp->zoned == 1 ||
+		(sdkp->device && sdkp->device->type == TYPE_ZBC);
 }
 
 #ifdef CONFIG_BLK_DEV_ZONED
-- 
2.36.1

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

[Damien Le Moal]

Make sure to always clear 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).

This change also makes sure that the sdkp rev_mutex is never used while
not being initialized by gating sd_zbc_clear_zone_info() cleanup code
with a check on the zone_wp_update_buf field which is never NULL when
rev_mutex has been initialized.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/sd_zbc.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 5b9fad70aa88..6245205b1159 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp)
 
 static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
 {
+	if (!sdkp->zone_wp_update_buf)
+		return;
+
 	/* Serialize against revalidate zones */
 	mutex_lock(&sdkp->rev_mutex);
 
@@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
 
 void sd_zbc_release_disk(struct scsi_disk *sdkp)
 {
-	if (sd_is_zoned(sdkp))
-		sd_zbc_clear_zone_info(sdkp);
+	sd_zbc_clear_zone_info(sdkp);
 }
 
 static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
@@ -914,12 +916,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, clear the disk zone information in
+		 * case the device type changed.
 		 */
+		sd_zbc_clear_zone_info(sdkp);
 		return 0;
+	}
 
 	/* READ16/WRITE16 is mandatory for ZBC disks */
 	sdkp->device->use_16_for_rw = 1;
-- 
2.36.1

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

[Dongliang Mu]

On Mon, May 30, 2022 at 9:43 AM Damien Le Moal
<damien.lemoal@opensource.wdc.com> wrote:
>
> If sd_probe() sees an error before sdkp->device is initialized,
> sd_zbc_release_disk() is called, which causes a NULL pointer dereference
> when sd_is_zoned() is called. Avoid this by also testing if a scsi disk
> device pointer is set in sd_is_zoned().
>
> Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
> Fixes: 89d947561077 ("sd: Implement support for ZBC device")
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/scsi/sd.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 2abad54fd23f..b90b96e8834e 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -236,7 +236,8 @@ static inline void sd_dif_config_host(struct scsi_disk *disk)
>
>  static inline int sd_is_zoned(struct scsi_disk *sdkp)
>  {
> -       return sdkp->zoned == 1 || sdkp->device->type == TYPE_ZBC;
> +       return sdkp->zoned == 1 ||
> +               (sdkp->device && sdkp->device->type == TYPE_ZBC);
>  }
>

Tested-by: Dongliang Mu <mudongliangabcd@gmail.com>


>  #ifdef CONFIG_BLK_DEV_ZONED
> --
> 2.36.1
>

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

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[Johannes Thumshirn]

On 30/05/2022 03:43, Damien Le Moal wrote:
> Make sure to always clear 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).
> 
> This change also makes sure that the sdkp rev_mutex is never used while
> not being initialized by gating sd_zbc_clear_zone_info() cleanup code
> with a check on the zone_wp_update_buf field which is never NULL when
> rev_mutex has been initialized.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>  drivers/scsi/sd_zbc.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
> index 5b9fad70aa88..6245205b1159 100644
> --- a/drivers/scsi/sd_zbc.c
> +++ b/drivers/scsi/sd_zbc.c
> @@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp)
>  
>  static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
>  {
> +	if (!sdkp->zone_wp_update_buf)
> +		return;
> +
>  	/* Serialize against revalidate zones */
>  	mutex_lock(&sdkp->rev_mutex);
>  
> @@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
>  
>  void sd_zbc_release_disk(struct scsi_disk *sdkp)
>  {
> -	if (sd_is_zoned(sdkp))
> -		sd_zbc_clear_zone_info(sdkp);
> +	sd_zbc_clear_zone_info(sdkp);
>  }

Now sd_zbc_release_disk() has become a simple rename of sd_zbc_clear_zone_info().
I think it can go and we can use sd_zbc_clear_zone_info() in the callers instead.

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

[Damien Le Moal]

On 5/30/22 16:48, Johannes Thumshirn wrote:
> On 30/05/2022 03:43, Damien Le Moal wrote:
>> Make sure to always clear 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).
>>
>> This change also makes sure that the sdkp rev_mutex is never used while
>> not being initialized by gating sd_zbc_clear_zone_info() cleanup code
>> with a check on the zone_wp_update_buf field which is never NULL when
>> rev_mutex has been initialized.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/scsi/sd_zbc.c | 15 ++++++++++-----
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
>> index 5b9fad70aa88..6245205b1159 100644
>> --- a/drivers/scsi/sd_zbc.c
>> +++ b/drivers/scsi/sd_zbc.c
>> @@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp)
>>  
>>  static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
>>  {
>> +	if (!sdkp->zone_wp_update_buf)
>> +		return;
>> +
>>  	/* Serialize against revalidate zones */
>>  	mutex_lock(&sdkp->rev_mutex);
>>  
>> @@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
>>  
>>  void sd_zbc_release_disk(struct scsi_disk *sdkp)
>>  {
>> -	if (sd_is_zoned(sdkp))
>> -		sd_zbc_clear_zone_info(sdkp);
>> +	sd_zbc_clear_zone_info(sdkp);
>>  }
> 
> Now sd_zbc_release_disk() has become a simple rename of sd_zbc_clear_zone_info().
> I think it can go and we can use sd_zbc_clear_zone_info() in the callers instead.

Yes, I thought of that, but I wanted to keep the name to make it clear
that the "main" caller is scsi_disk_release(). But if you insist, we can
get rid of it :)

-- 
Damien Le Moal
Western Digital Research