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

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

[Haoran Wang]

fnic_get_debug_info() formats the fnic debugfs statistics buffer with
eight `len += snprintf(info->debug_buffer + len, buf_size - len, ...)`
calls into an 8192-byte vmalloc buffer (buf_size = 2*PAGE_SIZE).
snprintf returns the number of bytes that would have been written and
does not clamp on truncation, so once the buffer fills, `len` keeps
growing past buf_size.

The last call site is a loop over iport->tport_list with no
`len >= buf_size` guard.

    list_for_each_entry_safe(tport, next, &iport->tport_list, links) {
        len += snprintf(info->debug_buffer + len, buf_size - len, ...);
    }

tport entries are created one per GPN_FT target port with no upper
bound (fdls_create_tport).  With a typical stats header (~4-6 KB) and
roughly 46 or more target ports, `len` crosses 8192.  The next
iteration then computes `buf_size - len` as a negative int, which is
promoted to a huge size_t, and vsnprintf writes the full line past the
8192-byte vmalloc allocation into the guard page, causing a kernel
oops on the debugfs open() path.  The inflated `return len` is also
stored as buffer_len and later drives an out-of-bounds read in
simple_read_from_buffer().

The sibling fnic_get_stats_data() in the same file already uses
scnprintf everywhere; fnic_get_debug_info() using snprintf is a porting
oversight.

Replace all eight snprintf calls with scnprintf.  scnprintf returns
min(would-be, size-1), so `len` is bounded to buf_size-1 and
`buf_size - len` stays >= 1 (never negative).

Signed-off-by: Haoran Wang <haoranwangsec@gmail.com>
---
 drivers/scsi/fnic/fnic_trace.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c
index 4ed57ea1f..51e7c973c 100644
--- a/drivers/scsi/fnic/fnic_trace.c
+++ b/drivers/scsi/fnic/fnic_trace.c
@@ -468,35 +468,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");
-	len += snprintf(info->debug_buffer + len, buf_size - len,
+	len += scnprintf(info->debug_buffer + len, buf_size - len,
 					"fnic Name:%s number:%d Role:%s State:%s\n",
 					fnic->name, fnic->fnic_num,
 					fnic_role_to_str(fnic->role),
 					fnic_state_to_str(fnic->state));
 	len +=
-		snprintf(info->debug_buffer + len, buf_size - len,
+		scnprintf(info->debug_buffer + len, buf_size - len,
 			 "iport State:%d Flags:0x%x vlan_id:%d fcid:0x%x\n",
 			 iport->state, iport->flags, iport->vlan_id, iport->fcid);
 	len +=
-		snprintf(info->debug_buffer + len, buf_size - len,
+		scnprintf(info->debug_buffer + len, buf_size - len,
 			 "usefip:%d fip_state:%d fip_flogi_retry:%d\n",
 			 iport->usefip, iport->fip.state, iport->fip.flogi_retry);
 	len +=
-		snprintf(info->debug_buffer + len, buf_size - len,
+		scnprintf(info->debug_buffer + len, buf_size - len,
 				 "fpma %02x:%02x:%02x:%02x:%02x:%02x",
 				 iport->fpma[5], iport->fpma[4], iport->fpma[3],
 				 iport->fpma[2], iport->fpma[1], iport->fpma[0]);
 	len +=
-		snprintf(info->debug_buffer + len, buf_size - len,
+		scnprintf(info->debug_buffer + len, buf_size - len,
 				"fcfmac %02x:%02x:%02x:%02x:%02x:%02x\n",
 				iport->fcfmac[5], iport->fcfmac[4], iport->fcfmac[3],
 				iport->fcfmac[2], iport->fcfmac[1], iport->fcfmac[0]);
 	len +=
-		snprintf(info->debug_buffer + len, buf_size - len,
+		scnprintf(info->debug_buffer + len, buf_size - len,
 		 "fabric state:%d flags:0x%x retry_counter:%d e_d_tov:%d r_a_tov:%d\n",
 		 iport->fabric.state, iport->fabric.flags,
 		 iport->fabric.retry_counter, iport->e_d_tov,
@@ -504,7 +504,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),

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

[sashiko-bot]

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