mbox series

[v2,0/2] sd_zbc fixes

Message ID 20220531002812.527368-1-damien.lemoal@opensource.wdc.com
Headers show
Series sd_zbc fixes | expand

Message

Damien Le Moal May 31, 2022, 12:28 a.m. UTC 
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 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     |  4 ++--
 drivers/scsi/sd.h     |  7 ++++---
 drivers/scsi/sd_zbc.c | 26 +++++++++++++-------------
 3 files changed, 19 insertions(+), 18 deletions(-)

Comments

Damien Le Moal May 31, 2022, 8:39 a.m. UTC | #1 
On 5/31/22 17:08, Christoph Hellwig wrote:
> On Tue, May 31, 2022 at 09:28:11AM +0900, Damien Le Moal 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().
> 
> Wouldn't a fix like the one below make more sense?  Until
> sd_revalidate_disk and thus sd_zbc_revalidate_zones is called none of
> the zone information is filled out, and thus we don't need to clear it.

Indeed, very good point. Will send a v3 with that instead of the current fix.

> 
> But at that point we've already initialized the device and thus the
> release will handler deal with all the real cleanup:
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 749316462075e..dabdc0eeb3dca 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);