Re: [PATCH] scsi: scsi_debug: negate wrapped memcmp() result

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] VERIFY command with BYTCHK=3 bypasses vnum boundary checks, leading to an out-of-bounds read in comp_write_worker().
--

--- Patch [1]: [PATCH] scsi: scsi_debug: negate wrapped memcmp() result ---
commit d60dbbd2f90d919b4ace8e2a0080c2edd89e096f
Author: Xu Rao <raoxu@uniontech.com>

scsi: scsi_debug: negate wrapped memcmp() result
    
This commit fixes a logic error in comp_write_worker() where the result of
the wrapped segment memcmp() was not being negated. It ensures that equal
data correctly results in a true value, matching the behavior of the first
segment comparison.

> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -4264,8 +4264,8 @@ static bool comp_write_worker(struct sdeb_store_info *sip, u64 lba, u32 num,
>  	if (!res)
>  		return res;
>  	if (rest)
> -		res = memcmp(fsp, arr + ((num - rest) * lb_size),
> -			     rest * lb_size);
> +		res = !memcmp(fsp, arr + ((num - rest) * lb_size),
> +			      rest * lb_size);

[Severity: High]
This isn't a bug introduced by this patch, but can this second memcmp()
perform an out-of-bounds read into unmapped vmalloc memory when triggered
from resp_verify()?

Looking at resp_verify(), when a verify command is issued and is_bytchk3
is true, a_num is hardcoded to 1 before checking the device access
boundaries:

drivers/scsi/scsi_debug.c:resp_verify() {
    ...
    a_num = is_bytchk3 ? 1 : vnum;
    /* Treat following check like one for read (i.e. no write) access */
    ret = check_device_access_params(scp, lba, a_num, false);
    ...
}

Because check_device_access_params() validates a_num rather than vnum, a
large vnum can completely bypass the sdebug_store_sectors limit. If
scsi_debug is loaded with a small dev_size_mb (e.g. 1), a vnum up to 8192
can still pass the kcalloc() allocation.

When comp_write_worker() later calculates the remainder:

    if (block + num > store_blks)
        rest = block + num - store_blks;

If num (which comes from vnum) exceeds store_blks, rest will also exceed
store_blks. This would cause the memcmp() operation here to read
rest * lb_size bytes from fsp, crossing into unmapped vmalloc guard pages
and potentially causing an oops.

>  	if (!res)
>  		return res;
>  	if (compare_only)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/E5AD42E9C0E18633+20260803095328.3445311-1-raoxu@uniontech.com?part=1