Re: [PATCH] scsi_debug: add support to corrupt data and/or reftag

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unbounded nested loops in `corrupt_lbas()` executed with preemption disabled allow users to cause CPU lockups.
- [High] Unconditional dereference of `sip` in `corrupt_lbas()` when initialized with fake I/O (`fake_rw=1`).
- [Medium] Flawed map state validation in `corrupt_lbas()` rejects valid wrapping corruption requests.

Pre-existing issues:
- [High] Pre-existing out-of-bounds array access in `map_state()` when using large virtual LBAs.
- [High] Toggling `fake_rw` from 1 to 0 at runtime causes a pre-existing NULL pointer dereference in LBP operations.
--

--- Patch [1]: [PATCH] scsi_debug: add support to corrupt data and/or reftag ---
commit b674d4afbf6311bc89a7d69aaa16a3947e8d3546
Author: Christoph Hellwig <hch@lst.de>
scsi_debug: add support to corrupt data and/or reftag

Add a new debugfs file to inject corruptions of the data and/or reftag.
This will be used to detect that protection information and/or file
system checksumming can detect random bit errors or misplaced writes.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 3346562275f80..8750e07759306 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c

[ ... ]

> @@ -4862,6 +4952,77 @@ static int resp_write_tape(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>  	return 0;
>  }
>  
> +static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
> +		u32 nr_bit_errors, s32 reftag_adjust)
> +{
> +	struct sdeb_store_info *sip = devip2sip(devip, false);
> +	bool meta_data_locked = false;
> +	u8 *fsp = sip->storep;

[Severity: High]
Does this unconditionally dereference sip if devip2sip() returns NULL?

If the scsi_debug module is loaded or toggled with fake_rw=1, devip2sip()
explicitly returns NULL. The sip->storep assignment then dereferences it
before any validation occurs, which will cause a kernel panic.

> +	u32 block, num_mapped, b, i;
> +	int error = 0;
> +
> +	if (sdebug_dev_is_zoned(devip) ||
> +	    sdebug_dix ||
> +	    scsi_debug_lbp())  {
> +		sdeb_meta_write_lock(sip);
> +		meta_data_locked = true;
> +	}
> +
> +	if (num > sdebug_capacity || lba > sdebug_capacity - num) {
> +		pr_err("logical blocks out of bounds: %llu:%u", lba, num);
> +		error = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	if (scsi_debug_lbp() &&
> +	    (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {

[Severity: High]
This is a pre-existing issue, but can map_state() perform an out-of-bounds
array access when called with large virtual LBAs?

If sdebug_virtual_gb is configured, sdebug_capacity exceeds the physical
sdebug_store_sectors. A command passing a large LBA generates an index
larger than map_size:

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

This causes test_bit and subsequent bit operations to access memory outside
the allocated sip->map_storep array.

[Severity: Medium]
Does this check incorrectly reject valid corruption requests that cross the
physical storage boundary?

If a user requests corruption across the physical store boundary (for example,
an lba near sdebug_store_sectors with a num crossing it), map_state() limits
the returned contiguous blocks to the physical boundary:

drivers/scsi/scsi_debug.c:map_state() {
    ...
    end = min_t(sector_t, sdebug_store_sectors, map_index_to_lba(next));
    *num = end - lba;
    ...
}

This causes the num > num_mapped condition to trigger and abort the request,
even though the underlying data loops correctly use div_u64_rem to wrap
around the physical bounds.

[Severity: High]
This is a pre-existing issue, but does toggling fake_rw from 1 to 0 at
runtime cause a NULL pointer dereference in LBP operations?

If fake_rw is toggled from 1 to 0 via sysfs, fake_rw_store() calls
sdebug_add_store() before updating sdebug_fake_rw to 0:

drivers/scsi/scsi_debug.c:fake_rw_store() {
    ...
    idx = sdebug_add_store();
    ...
    sdebug_fake_rw = n;
    ...
}

Inside sdebug_add_store(), scsi_debug_lbp() returns false because
sdebug_fake_rw is still 1, so sip->map_storep remains unallocated. Once
the transition completes, fake_rw is 0 and LBP becomes active. Any
subsequent command using LBP calls map_state(), which crashes when
dereferencing the NULL sip->map_storep.

> +		pr_err("can't modify unmapped logical blocks: %llu:%u",
> +			lba, num);
> +		error = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * For each possible bit error, flip the same bit in each corrupted
> +	 * block to simulate repeated patterns like errors in cables.
> +	 */
> +	sdeb_data_lock(sip, false);
> +	for (b = 0; b < nr_bit_errors; b++) {
> +		unsigned int bit;
> +
> +		bit = get_random_u32_below(sdebug_sector_size * BITS_PER_BYTE);
> +		pr_info("flipping bit %u in LBAs %llu:%llu\n",
> +				bit, lba, lba + num - 1);
> +
> +		div_u64_rem(lba, sdebug_store_sectors, &block);
> +		for (i = 0; i < num; i++) {

[Severity: High]
Can these unbounded nested loops cause a CPU lockup since they run with
preemption disabled?

A user can write large values for num and nr_bit_errors into the corrupt
debugfs file. The sdeb_data_lock() function calls read_lock(), which
disables preemption. The function then enters these loops, potentially
stalling the CPU for an extended period of time if the inputs are large.

> +			u8 *p = fsp + (block * sdebug_sector_size);
> +
> +			sdeb_data_sector_lock(sip, true);
> +			p[bit / BITS_PER_BYTE] ^= (1 << (bit % BITS_PER_BYTE));
> +			sdeb_data_sector_unlock(sip, true);
> +			if (++block >= sdebug_store_sectors)
> +				block = 0;
> +		}
> +	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819065850.1358861-2-hch@lst.de?part=1