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 3 potential issue(s) to consider:

New issues:
- [High] Unbounded loops with preemption disabled, combined with excessive kernel logging and wrap-around logic errors, cause CPU lockups and state corruption.
- [Medium] The debugfs write handler passes the raw user-provided count to `memdup_user_nul()` without capping it, risking kernel allocator warnings.

Pre-existing issues:
- [High] Out-of-bounds memory read in `map_state()` when Logical Block Provisioning (LBP) and `virtual_gb` are used together.
--

commit 9a79e34fe4a44a1c0a0765e0674426353e4ac98e
Author: Christoph Hellwig <hch@lst.de>

scsi_debug: add support to corrupt data and/or reftag

This commit adds a new debugfs file "corrupt" to inject data and/or reftag
corruptions into scsi_debug devices. This enables testing file system and
protection information mechanisms via random bit errors or misplaced writes.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 3346562275f80..959177adfc9a8 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
[ ... ]
> +static ssize_t sdebug_corrupt_write(struct file *file, const char __user *ubuf,
> +		size_t count, loff_t *ppos)
> +{
[ ... ]
> +	u64 lba = 0;
> +	u32 num = 1;
> +	u32 nr_bit_errors = 0;
> +	s32 reftag_adjust = 0;
> +
> +	buf = memdup_user_nul(ubuf, count);

[Severity: Medium]
Could passing the raw VFS count directly to memdup_user_nul() trigger page
allocator warnings? 

Since count can be up to ~2GB (MAX_RW_COUNT), and memdup_user_nul()
internally uses kmalloc_track_caller() without the __GFP_NOWARN flag, a
large write request might cause loud warning stack traces or allocation
failures.

[ ... ]
> +static int corrupt_lbas(struct sdebug_dev_info *devip, u64 lba, u32 num,
> +		u32 nr_bit_errors, s32 reftag_adjust)
> +{
[ ... ]
> +	if (scsi_debug_lbp() &&
> +	    (!map_state(sip, lba, &num_mapped) || num > num_mapped)) {

[Severity: High]
This is a pre-existing issue, but does calling map_state() with an
un-folded lba cause an out-of-bounds read if virtual_gb > 0?

When map_state() uses test_bit() on lba_to_map_index(lba), it doesn't
verify if the index exceeds the allocated map_size. If lba goes up to
sdebug_capacity, it can easily exceed the backing store size. This vector
was already reachable via block I/O (like GET LBA STATUS) but this patch
now exposes it directly via debugfs.

[ ... ]
> +	/*
> +	 * 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++) {
> +			u8 *p = sip->storep + (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;
> +		}
> +	}

[Severity: High]
Can these nested loops cause CPU soft lockups?

Since nr_bit_errors and num are parsed from user input without strict upper
bounds, large values might stall the CPU while holding sdeb_data_lock(),
which acquires a rwlock in read mode and disables preemption.

Also, if num is larger than sdebug_store_sectors, the wrap-around logic 
will visit the same physical backing memory multiple times. If a sector is
visited an even number of times, does this accidentally undo the bit flips?

> +	if (reftag_adjust) {
> +		pr_info("adjusting reftag by %d sectors for LBAs %llu:%llu\n",
> +			reftag_adjust, lba, lba + num - 1);
> +		for (i = 0; i < num; i++) {
> +			struct t10_pi_tuple *sdt = dif_store(sip, lba + i);
> +
> +			be32_add_cpu(&sdt->ref_tag, reftag_adjust);
> +		}
> +	}
> +	sdeb_data_unlock(sip, false);

[Severity: High]
Similar to the wrap-around logic above, if num exceeds sdebug_store_sectors,
will dif_store() wrap around and redundantly apply reftag_adjust multiple
times to the exact same tuple?

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