Re: [PATCH] scsi: fnic: use scnprintf in fnic_get_debug_info()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: fnic: cumulative return in fnic_get_debug_info() causes OOB read
--

commit 6c596234ba5fa29d815cf93a8c722b4691009e37
Author: Haoran Wang <haoranwangsec@gmail.com>

scsi: fnic: use scnprintf in fnic_get_debug_info()

This commit replaces snprintf() with scnprintf() in fnic_get_debug_info()
to prevent buffer overflow and out-of-bounds reads when formatting fnic
debugfs statistics.

> diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c
> index a3f583703664d..7eeb9c8b8999f 100644
> --- a/drivers/scsi/fnic/fnic_trace.c
> +++ b/drivers/scsi/fnic/fnic_trace.c
> @@ -469,35 +469,35 @@ int fnic_get_debug_info(struct stats_debug_info *info, struct fnic *fnic)
>  	struct fnic_tport_s *tport, *next;
>  	unsigned long flags;
>  
> -	len += snprintf(info->debug_buffer + len, buf_size - len,
> +	len += scnprintf(info->debug_buffer + len, buf_size - len,
>  					"------------------------------------------\n"
>  					"\t\t Debug Info\n"
>  					"------------------------------------------\n");

[ ... ]

> @@ -505,7 +505,7 @@ int fnic_get_debug_info(struct stats_debug_info *info, struct fnic *fnic)
>  
>  	spin_lock_irqsave(&fnic->fnic_lock, flags);
>  	list_for_each_entry_safe(tport, next, &iport->tport_list, links) {
> -		len += snprintf(info->debug_buffer + len, buf_size - len,
> +		len += scnprintf(info->debug_buffer + len, buf_size - len,
>  		"tport fcid:0x%x state:%d flags:0x%x inflight:%d retry_counter:%d\n",
>  		tport->fcid, tport->state, tport->flags,
>  		atomic_read(&tport->in_flight),

[Severity: High]
Does debug->buffer_len still overflow debug->buf_size and trigger the
out-of-bounds read in simple_read_from_buffer() even after switching to
scnprintf()?

In drivers/scsi/fnic/fnic_trace.c, fnic_get_debug_info() initializes len
to the existing info->buffer_len and returns the cumulative offset len at
the end of the function:

drivers/scsi/fnic/fnic_trace.c:fnic_get_debug_info() {
	int buf_size = info->buf_size;
	int len = info->buffer_len;
	...
	return len;
}

However, in drivers/scsi/fnic/fnic_debugfs.c, fnic_stats_debugfs_open()
adds the return value of fnic_get_debug_info() to debug->buffer_len using
+= rather than assigning it directly:

drivers/scsi/fnic/fnic_debugfs.c:fnic_stats_debugfs_open() {
	...
	debug->buffer_len = fnic_get_stats_data(debug, fnic_stats);
	debug->buffer_len += fnic_get_debug_info(debug, fnic);
}

Because fnic_get_debug_info() returns info->buffer_len plus the newly
appended bytes, using += counts the length returned by
fnic_get_stats_data() twice. Since fnic_get_stats_data() already writes
more than 4 KB into the 2 * PAGE_SIZE vmalloc() buffer even when all
counters are zero, debug->buffer_len still exceeds debug->buf_size when
userspace opens the stats debugfs file (even with zero target ports).

When userspace subsequently reads the file through
fnic_stats_debugfs_read() -> simple_read_from_buffer(), won't passing this
inflated debug->buffer_len still read past the end of debug->debug_buffer?
Should fnic_get_debug_info() return len - info->buffer_len (or should
fnic_stats_debugfs_open() use = instead of +=)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179241615575.5916.9521598583158892732@whr?part=1