Re: [PATCH 1/2] scsi: scsi_debug: wrap around the store when zeroing a reset zone

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] scsi: scsi_debug: Fixes trailer cites f0d1cf9378bd instead of
  2d62253eb1b6
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] scsi: scsi_debug: out-of-bounds read in map_state() [1]
--

commit fffdafa6f420e79758f4780228f5ace2090a7a92
Author: Tal Zussman <tz2294@columbia.edu>

> scsi: scsi_debug: wrap around the store when zeroing a reset zone
>
> zbc_rwp_zone() zeroes the blocks of a zone that is reset, using the
> start of the zone as an offset into the store. With virtual_gb set, the
> device and its zones can extend past the end of the store, which is
> only dev_size_mb in size. Writes wrap around in do_device_access(), but
> the memset() doesn't, so resetting a zone that has been written to can
> write past the end of the store.
>
> On a device whose fifth zone starts exactly at the end of the store,
> write to that zone and reset it:
>
>   # modprobe scsi_debug zbc=managed sector_size=4096 dev_size_mb=16 \
>         virtual_gb=1 zone_size_mb=4
>   # dd if=/dev/urandom of=/dev/sda bs=1M count=1 seek=16 oflag=direct
>   # blkzone reset -o 32768 -c 1 /dev/sda
>
> The memset() starts at the first byte past the store, in the guard page
> after the vmalloc'ed area, for the 1 MiB that was written:
>
> BUG: unable to handle page fault for address: ffffc90002c00000
> Oops: Oops: 0002 [#1] SMP KASAN NOPTI
> RIP: 0010:memset_orig+0x33/0xb0
> RDX: 0000000000100000 RSI: 0000000000000000 RDI: ffffc90002c00000
> Call Trace:
>  zbc_rwp_zone+0x1a7/0x2a0 [scsi_debug]
>  resp_rwp_zone+0x19c/0x450 [scsi_debug]
>  schedule_resp+0x2dd/0xbc0 [scsi_debug]
>  scsi_debug_queuecommand+0x1286/0x2950 [scsi_debug]
>  scsi_queue_rq+0x1d74/0x3200
>  blk_mq_dispatch_rq_list+0x3ad/0x23c0
>
> The command never completes, so the reset returns EIO to userspace and
> the device is unusable afterwards.
>
> Zero the blocks in a helper that wraps around at the end of the store,
> and that stops after one pass over it for a zone that is larger than
> the store.
>
> Fixes: f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands")
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

[Severity: Low]
In commit f0d1cf9378bd ("scsi: scsi_debug: Add ZBC zone commands"),
zbc_rwp_zone() only reset z_wp and z_cond without touching sip->storep.
The non-wrapping memset() was added later by commit 2d62253eb1b6 ("scsi:
scsi_debug: Zero clear zones at reset write pointer"), so should the Fixes
tag point there instead?

  Fixes: 2d62253eb1b6 ("scsi: scsi_debug: Zero clear zones at reset write pointer")

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index a922c8318a2fb..1c27ea0f9b694 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6612,6 +6612,29 @@ static int resp_finish_zone(struct scsi_cmnd *scp,
>  	return res;
>  }
>  
> +/*
> + * Zero the blocks of a zone that is being reset. The store can be smaller
> + * than the device, so wrap around at its end like do_device_access() does.
> + */
> +static void zbc_zero_blocks(struct sdeb_store_info *sip, sector_t lba,
> +			    sector_t num)
> +{
> +	/* One pass over the store zeroes all of it */
> +	num = min_t(sector_t, num, sdebug_store_sectors);
> +
> +	while (num) {
> +		sector_t block = lba;
> +		u32 n;
> +
> +		block = sector_div(block, sdebug_store_sectors);
> +		n = min_t(sector_t, num, sdebug_store_sectors - block);
> +		memset(sip->storep + block * sdebug_sector_size, 0,
> +		       (size_t)n * sdebug_sector_size);
> +		lba += n;
> +		num -= n;
> +	}
> +}
> +
>  static void zbc_rwp_zone(struct sdebug_dev_info *devip,
>  			 struct sdeb_zone_state *zsp)
>  {
> @@ -6629,8 +6652,7 @@ static void zbc_rwp_zone(struct sdebug_dev_info *devip,
>  		devip->nr_closed--;
>  
>  	if (zsp->z_wp > zsp->z_start)
> -		memset(sip->storep + zsp->z_start * sdebug_sector_size, 0,
> -		       (zsp->z_wp - zsp->z_start) * sdebug_sector_size);
> +		zbc_zero_blocks(sip, zsp->z_start, zsp->z_wp - zsp->z_start);
>  
>  	zsp->z_non_seq_resource = false;
>  	zsp->z_wp = zsp->z_start;

[Severity: Critical]
This problem wasn't introduced by this patch, but can map_state() read past
the end of sip->map_storep when virtual_gb is set [1]?

drivers/scsi/scsi_debug.c:map_state() {
    ...
	index = lba_to_map_index(lba);
	mapped = test_bit(index, sip->map_storep);
    ...
}

sip->map_storep is sized for sdebug_store_sectors (map_size), whereas
resp_get_lba_status() validates the LBA against the larger virtual
sdebug_capacity before calling map_state().  A GET LBA STATUS command for
an LBA above sdebug_store_sectors produces an index past map_size, causing
test_bit() to read out of bounds of the vcalloc-allocated bitmap and
potentially panic or disclose kernel memory.

[1] https://sashiko.dev/#/bug/linux-be69a5e8-c8ca-4d89-b06c-a23fccc84744

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-scsi-debug-zone-reset-v1-0-28cb9184b5ea@columbia.edu?part=1