Re: [PATCH 2/2] scsi: scsi_debug: generate protection information for writes without it

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 2 related pre-existing issues it does not
introduce:
- [Critical] scsi: scsi_debug: LBP and protection state corruption on
  partial writes [1]
- [Critical] scsi: scsi_debug: write protection bypass in
  resp_comp_write() [2]
--

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

scsi: scsi_debug: generate protection information for writes without it

This commit generates T10 protection information in scsi_debug for write
commands submitted without protection scatterlists, and extends the metadata
write lock in write handlers to cover DIX updates.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 7d50650396377..2c7afdd2846b1 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> @@ -5195,6 +5221,9 @@ static int __resp_write_dt0(struct scsi_cmnd *scp,
>  	ret = do_device_access(sip, scp, 0, lba, num, group, true, atomic);
>  	if (ret < 0)
>  		*scsi_status = DID_ERROR << 16;
> +	else if (!scsi_prot_sg_count(scp))
> +		dif_generate(sip, lba, DIV_ROUND_UP(ret, sdebug_sector_size),
> +			     false);
>  
>  	if (unlikely(lbp))
>  		map_region(sip, lba, num);

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failed or short
write in __resp_write_dt0() and resp_write_scat() corrupt the LBP and DIX
metadata [1]?

While dif_generate() uses the actual byte count returned by
do_device_access(), map_region() right below it (and __resp_write_dt0_dix()
or prot_verify_write() before the transfer) still operates on the full
requested sector count num.  If do_device_access() fails or transfers fewer
bytes than requested, unwritten sectors are marked as mapped in
sip->map_storep and have their protection tags overwritten in
sip->dif_storep, desynchronizing the metadata from the backing store and
causing subsequent reads of those sectors to fail verification.

[ ... ]
> @@ -5753,6 +5786,7 @@ static int resp_comp_write(struct scsi_cmnd *scp,
>  		retval = check_condition_result;
>  		goto cleanup_unlock;
>  	}
> +	dif_generate(sip, lba, num, false);
>  
>  	/* Cover sip->map_storep (which map_region() sets) with the meta lock */
>  	if (scsi_debug_lbp())

[Severity: Critical]
This problem wasn't introduced by this patch, but does resp_comp_write()
bypass device write protection by passing false for the write argument to
check_device_access_params() [2]?

drivers/scsi/scsi_debug.c:resp_comp_write() {
	...
	ret = check_device_access_params(scp, lba, num, false);
	if (ret)
		return ret;
	...
}

Because write is false, check_device_access_params() skips the sdebug_wp
check, allowing COMPARE AND WRITE to modify data and protection metadata on
a write-protected device.

[1] https://sashiko.dev/#/bug/linux-5ff253b7-5155-4709-ae8b-55ff4a1eb98f
[2] https://sashiko.dev/#/bug/linux-0b24b1f8-28c0-4efa-915b-66147893a0f1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-scsi-debug-pi-v1-0-539829902b3a@columbia.edu?part=2