Re: [PATCH v2 1/2] scsi: core: Add KUnit tests for scsi_logging.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Data race and out-of-bounds memory write in SCSI logging test capture.
- [Medium] NULL pointer dereference due to a TOCTOU data race on the global `scsi_logging_test_dev_printk` pointer.
- [Low] Global suppression of actual SCSI system logs during KUnit test execution.
--

commit 33967e88f5091c2f971d2edac33840afe7e6e925
Author: Bart Van Assche <bvanassche@acm.org>
    
scsi: core: Add KUnit tests for scsi_logging.c
    
This commit introduces KUnit tests for SCSI logging functions by
intercepting dev_printk() when CONFIG_SCSI_LOGGING_KUNIT_TEST is enabled.
It captures log outputs into a buffer to verify standard and formatted
SCSI command outputs.

> diff --git a/drivers/scsi/scsi_logging.c b/drivers/scsi/scsi_logging.c
> index 3cd0d30740855..3bff4dea4c6ac 100644
> --- a/drivers/scsi/scsi_logging.c
> +++ b/drivers/scsi/scsi_logging.c
> @@ -15,6 +15,32 @@
>  #include <scsi/scsi_eh.h>
>  #include <scsi/scsi_dbg.h>
>  
> +#if IS_ENABLED(CONFIG_SCSI_LOGGING_KUNIT_TEST)
> +static void (*scsi_logging_test_dev_printk)(const char *level,
> +					    const struct device *dev,
> +					    const char *fmt, va_list args);
> +
> +static void scsi_logging_dev_printk(const char *level, const struct device *dev,
> +				    const char *fmt, ...)
> +{
> +	va_list args;
> +
> +	va_start(args, fmt);
> +	if (unlikely(scsi_logging_test_dev_printk)) {
> +		scsi_logging_test_dev_printk(level, dev, fmt, args);

[Severity: Medium]
Could this result in a NULL pointer dereference? The global pointer
scsi_logging_test_dev_printk is checked for truthiness and then invoked
without synchronization or READ_ONCE(). If a background SCSI event triggers
a log concurrently with the test teardown in scsi_logging_test_exit()
(which clears the pointer), a race window exists where the pointer might be
evaluated as non-NULL but cleared before invocation.

> +	} else {
> +		struct va_format vaf = { .fmt = fmt, .va = &args };
> +
> +		_dev_printk(level, dev, "%pV", &vaf);
> +	}

[Severity: Low]
Does this branch inadvertently suppress all actual SCSI system logs during
the test execution? Because the hook intercepts all dev_printk() calls in
scsi_logging.c and doesn't filter for the test-specific dev structure,
background hardware logs might be swallowed while the test is running.

> +	va_end(args);
> +}

[ ... ]

> diff --git a/drivers/scsi/scsi_logging_test.c b/drivers/scsi/scsi_logging_test.c
> new file mode 100644
> index 0000000000000..1dcebc383052a
> --- /dev/null
> +++ b/drivers/scsi/scsi_logging_test.c
> @@ -0,0 +1,422 @@
[ ... ]
> +static struct captured_dev_printk captured_logs[MAX_CAPTURED_LINES];
> +static int captured_count;
> +
> +static void test_capture_dev_printk(const char *level, const struct device *dev,
> +				    const char *fmt, va_list args)
> +{
> +	if (captured_count < MAX_CAPTURED_LINES) {
> +		captured_logs[captured_count].level = level;
> +		captured_logs[captured_count].dev = dev;

[Severity: Medium]
Is there a risk of an out-of-bounds memory write here if background SCSI
events occur concurrently? The bounds check on the shared captured_count
variable is made locklessly, creating a TOCTOU race window. Multiple
threads or an interrupt on the same CPU could evaluate the bounds check
simultaneously and then proceed to write into the array.

> +		vscnprintf(captured_logs[captured_count].msg,
> +			   sizeof(captured_logs[captured_count].msg), fmt,
> +			   args);
> +		captured_count++;

[Severity: Medium]
This increment further contributes to the race, as concurrent executions
would increment captured_count beyond MAX_CAPTURED_LINES after writing out
of bounds.

> +	}
> +}
> +
> +static void scsi_logging_test_reset(void)
> +{
> +	captured_count = 0;
> +	memset(captured_logs, 0, sizeof(captured_logs));
> +}
> +
> +static int scsi_logging_test_init(struct kunit *test)
> +{
> +	scsi_logging_test_reset();
> +	scsi_logging_test_dev_printk = test_capture_dev_printk;
> +	return 0;
> +}
> +
> +static void scsi_logging_test_exit(struct kunit *test)
> +{
> +	scsi_logging_test_dev_printk = NULL;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788198780.git.bvanassche@acm.org?part=1