Re: [PATCH v2 1/4] scsi: scsi_debug: Do not advance the write pointer of a failed write

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: scsi_debug: ZBC specification violation by returning GOOD status without advancing WP
- [Medium] scsi: scsi_debug: integer overflow in short write check allows incorrect WP advancement
--

commit 2b6fea07514157218eac4ea45ff1ba17e05b5125
Author: Niklas Cassel <cassel@kernel.org>

scsi: scsi_debug: Do not advance the write pointer of a failed write

This commit modifies the write pointer advancement logic for ZBC sequential
write required zones. It ensures the write pointer is only incremented when
all requested data is successfully written.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 6941809dfdb75..3ce2dbdff177c 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -5150,8 +5150,8 @@ static int resp_write_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	if (unlikely(scsi_debug_lbp()))
>  		map_region(sip, lba, num);
>  
> -	/* If ZBC zone then bump its write pointer */
> -	if (sdebug_dev_is_zoned(devip))
> +	/* If ZBC zone then bump its write pointer, if all data was written */
> +	if (sdebug_dev_is_zoned(devip) && ret == num * sdebug_sector_size)

[Severity: Medium]
Could the multiplication num * sdebug_sector_size overflow 32-bit arithmetic
for large num? If the module is loaded with a capacity >= 4GB, an extremely
large num and an empty data-out buffer might cause do_device_access() to
safely return 0. If the multiplication overflows and evaluates to 0, this
check might erroneously pass and incorrectly advance the write pointer.

[Severity: High]
If do_device_access() returns a short count due to a small data-out buffer,
this check prevents advancing the write pointer. However, the function then
proceeds to return 0 (GOOD status) without calling scsi_set_resid(). Does
this violate the ZBC specification, which requires that if a write terminates
with GOOD status, the write pointer must be advanced by the transfer length?

Could this lead to silent data corruption or UNALIGNED WRITE COMMAND errors
on subsequent sequential writes?

>  		zbc_inc_wp(devip, lba, num);
>  	if (meta_data_locked)
>  		sdeb_meta_write_unlock(sip);

[ ... ]

> @@ -5314,8 +5314,8 @@ static int resp_write_scat(struct scsi_cmnd *scp,
>  		 * writes behaviour as possible.
>  		 */
>  		ret = do_device_access(sip, scp, sg_off, lba, num, group, true, true);
> -		/* If ZBC zone then bump its write pointer */
> -		if (sdebug_dev_is_zoned(devip))
> +		/* If ZBC zone then bump its write pointer, if all data was written */
> +		if (sdebug_dev_is_zoned(devip) && ret == num_by)

[Severity: Medium]
Does the num_by variable suffer from a similar 32-bit overflow issue?

Earlier in resp_write_scat(), num_by is calculated as num * lb_size. If
this truncates to 0 on a massive transfer, could this condition falsely
match a 0-byte return from do_device_access(), tricking the driver into
advancing the write pointer anyway?

[Severity: High]
Similar to resp_write_dt0(), if this skips the write pointer advancement for
scattered writes on short buffers, it still continues and eventually returns
0 (GOOD status) at the end of the function. Should this set a residual count
when skipping the write pointer increment to avoid ZBC specification
violations?

>  			zbc_inc_wp(devip, lba, num);
>  		if (unlikely(scsi_debug_lbp()))
>  			map_region(sip, lba, num);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917110316.1104863-6-cassel@kernel.org?part=1