[RFC PATCH 0/3] scsi: qedf: sanitise uaccess

[RFC PATCH 0/3] scsi: qedf: sanitise uaccess

[Oleksandr Natalenko]

qedf driver, debugfs part of it specifically, touches __user pointers
directly for printing out info to userspace via sprintf(), which may
cause crash like this:

BUG: unable to handle kernel paging request at 00007ffd1d6b43a0
IP: [<ffffffffaa7a882a>] string.isra.7+0x6a/0xf0
Oops: 0003 [#1] SMP
Call Trace:
 [<ffffffffaa7a9f31>] vsnprintf+0x201/0x6a0
 [<ffffffffaa7aa556>] sprintf+0x56/0x80
 [<ffffffffc04227ed>] qedf_dbg_stop_io_on_error_cmd_read+0x6d/0x90 [qedf]
 [<ffffffffaa65bb2f>] vfs_read+0x9f/0x170
 [<ffffffffaa65cb82>] SyS_pread64+0x92/0xc0

Avoid this by preparing the info in a kernel buffer first, either
allocated on stack for small printouts, or via vmalloc() for big ones,
and then copying it to the userspace properly.

I'm not sure how big the vmalloc()'ed buffer should be, and also whether
vmalloc()'ing it directly in the _read() function is a good idea, hence
RFC prefix.

The qedf_dbg_stop_io_on_error_cmd_read()-related patch is actually tested,
the rest is compile-tested only.

Oleksandr Natalenko (3):
  scsi: qedf: do not touch __user pointer in
    qedf_dbg_stop_io_on_error_cmd_read() directly
  scsi: qedf: do not touch __user pointer in qedf_dbg_debug_cmd_read()
    directly
  scsi: qedf: do not touch __user pointer in qedf_dbg_fp_int_cmd_read()
    directly

 drivers/scsi/qedf/qedf_dbg.h     |  2 ++
 drivers/scsi/qedf/qedf_debugfs.c | 35 +++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 14 deletions(-)

--
2.41.0

[RFC PATCH 1/3] scsi: qedf: do not touch __user pointer in qedf_dbg_stop_io_on_error_cmd_read() directly

[Oleksandr Natalenko]

The qedf_dbg_stop_io_on_error_cmd_read() function invokes sprintf()
directly on a __user pointer, which may crash the kernel.

Avoid doing that by using a small on-stack buffer for sprintf()
and then calling simple_read_from_buffer() which does a proper
copy_to_user() call.

Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
---
 drivers/scsi/qedf/qedf_debugfs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index a3ed681c8ce3f..4d1b99569d490 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -185,18 +185,17 @@ qedf_dbg_stop_io_on_error_cmd_read(struct file *filp, char __user *buffer,
 				   size_t count, loff_t *ppos)
 {
 	int cnt;
+	char cbuf[7];
 	struct qedf_dbg_ctx *qedf_dbg =
 				(struct qedf_dbg_ctx *)filp->private_data;
 	struct qedf_ctx *qedf = container_of(qedf_dbg,
 	    struct qedf_ctx, dbg_ctx);
 
 	QEDF_INFO(qedf_dbg, QEDF_LOG_DEBUGFS, "entered\n");
-	cnt = sprintf(buffer, "%s\n",
+	cnt = sprintf(cbuf, "%s\n",
 	    qedf->stop_io_on_error ? "true" : "false");
 
-	cnt = min_t(int, count, cnt - *ppos);
-	*ppos += cnt;
-	return cnt;
+	return simple_read_from_buffer(buffer, count, ppos, cbuf, cnt);
 }
 
 static ssize_t
-- 
2.41.0

[RFC PATCH 2/3] scsi: qedf: do not touch __user pointer in qedf_dbg_debug_cmd_read() directly

[Oleksandr Natalenko]

The qedf_dbg_debug_cmd_read() function invokes sprintf()
directly on a __user pointer, which may crash the kernel.

Avoid doing that by using a small on-stack buffer for sprintf()
and then calling simple_read_from_buffer() which does a proper
copy_to_user() call.

Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
---
 drivers/scsi/qedf/qedf_debugfs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index 4d1b99569d490..f910af0029a2c 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -138,15 +138,14 @@ qedf_dbg_debug_cmd_read(struct file *filp, char __user *buffer, size_t count,
 			loff_t *ppos)
 {
 	int cnt;
+	char cbuf[35];
 	struct qedf_dbg_ctx *qedf_dbg =
 				(struct qedf_dbg_ctx *)filp->private_data;
 
 	QEDF_INFO(qedf_dbg, QEDF_LOG_DEBUGFS, "debug mask=0x%x\n", qedf_debug);
-	cnt = sprintf(buffer, "debug mask = 0x%x\n", qedf_debug);
+	cnt = sprintf(cbuf, "debug mask = 0x%x\n", qedf_debug);
 
-	cnt = min_t(int, count, cnt - *ppos);
-	*ppos += cnt;
-	return cnt;
+	return simple_read_from_buffer(buffer, count, ppos, cbuf, cnt);
 }
 
 static ssize_t
-- 
2.41.0

[RFC PATCH 3/3] scsi: qedf: do not touch __user pointer in qedf_dbg_fp_int_cmd_read() directly

[Oleksandr Natalenko]

The qedf_dbg_fp_int_cmd_read() function invokes sprintf()
directly on a __user pointer, which may crash the kernel.

Avoid doing that by vmalloc()'ating a buffer for scnprintf()
and then calling simple_read_from_buffer() which does a proper
copy_to_user() call.

Signed-off-by: Oleksandr Natalenko <oleksandr@redhat.com>
---
 drivers/scsi/qedf/qedf_dbg.h     |  2 ++
 drivers/scsi/qedf/qedf_debugfs.c | 21 +++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/qedf/qedf_dbg.h b/drivers/scsi/qedf/qedf_dbg.h
index f4d81127239eb..5ec2b817c694a 100644
--- a/drivers/scsi/qedf/qedf_dbg.h
+++ b/drivers/scsi/qedf/qedf_dbg.h
@@ -59,6 +59,8 @@ extern uint qedf_debug;
 #define QEDF_LOG_NOTICE	0x40000000	/* Notice logs */
 #define QEDF_LOG_WARN		0x80000000	/* Warning logs */
 
+#define QEDF_DEBUGFS_LOG_LEN (2 * PAGE_SIZE)
+
 /* Debug context structure */
 struct qedf_dbg_ctx {
 	unsigned int host_no;
diff --git a/drivers/scsi/qedf/qedf_debugfs.c b/drivers/scsi/qedf/qedf_debugfs.c
index f910af0029a2c..6db996b73fe39 100644
--- a/drivers/scsi/qedf/qedf_debugfs.c
+++ b/drivers/scsi/qedf/qedf_debugfs.c
@@ -8,6 +8,7 @@
 #include <linux/uaccess.h>
 #include <linux/debugfs.h>
 #include <linux/module.h>
+#include <linux/vmalloc.h>
 
 #include "qedf.h"
 #include "qedf_dbg.h"
@@ -98,7 +99,9 @@ static ssize_t
 qedf_dbg_fp_int_cmd_read(struct file *filp, char __user *buffer, size_t count,
 			 loff_t *ppos)
 {
+	ssize_t ret;
 	size_t cnt = 0;
+	char *cbuf;
 	int id;
 	struct qedf_fastpath *fp = NULL;
 	struct qedf_dbg_ctx *qedf_dbg =
@@ -108,19 +111,25 @@ qedf_dbg_fp_int_cmd_read(struct file *filp, char __user *buffer, size_t count,
 
 	QEDF_INFO(qedf_dbg, QEDF_LOG_DEBUGFS, "entered\n");
 
-	cnt = sprintf(buffer, "\nFastpath I/O completions\n\n");
+	cbuf = vmalloc(QEDF_DEBUGFS_LOG_LEN);
+	if (!cbuf)
+		return 0;
+
+	cnt += scnprintf(cbuf + cnt, QEDF_DEBUGFS_LOG_LEN - cnt, "\nFastpath I/O completions\n\n");
 
 	for (id = 0; id < qedf->num_queues; id++) {
 		fp = &(qedf->fp_array[id]);
 		if (fp->sb_id == QEDF_SB_ID_NULL)
 			continue;
-		cnt += sprintf((buffer + cnt), "#%d: %lu\n", id,
-			       fp->completions);
+		cnt += scnprintf(cbuf + cnt, QEDF_DEBUGFS_LOG_LEN - cnt,
+				 "#%d: %lu\n", id, fp->completions);
 	}
 
-	cnt = min_t(int, count, cnt - *ppos);
-	*ppos += cnt;
-	return cnt;
+	ret = simple_read_from_buffer(buffer, count, ppos, cbuf, cnt);
+
+	vfree(cbuf);
+
+	return ret;
 }
 
 static ssize_t
-- 
2.41.0

Re: [RFC PATCH 0/3] scsi: qedf: sanitise uaccess

[Laurence Oberman]

On Mon, 2023-07-24 at 14:02 +0200, Oleksandr Natalenko wrote:
> qedf driver, debugfs part of it specifically, touches __user pointers
> directly for printing out info to userspace via sprintf(), which may
> cause crash like this:
> 
> BUG: unable to handle kernel paging request at 00007ffd1d6b43a0
> IP: [<ffffffffaa7a882a>] string.isra.7+0x6a/0xf0
> Oops: 0003 [#1] SMP
> Call Trace:
>  [<ffffffffaa7a9f31>] vsnprintf+0x201/0x6a0
>  [<ffffffffaa7aa556>] sprintf+0x56/0x80
>  [<ffffffffc04227ed>] qedf_dbg_stop_io_on_error_cmd_read+0x6d/0x90
> [qedf]
>  [<ffffffffaa65bb2f>] vfs_read+0x9f/0x170
>  [<ffffffffaa65cb82>] SyS_pread64+0x92/0xc0
> 
> Avoid this by preparing the info in a kernel buffer first, either
> allocated on stack for small printouts, or via vmalloc() for big
> ones,
> and then copying it to the userspace properly.
> 
> I'm not sure how big the vmalloc()'ed buffer should be, and also
> whether
> vmalloc()'ing it directly in the _read() function is a good idea,
> hence
> RFC prefix.
> 
> The qedf_dbg_stop_io_on_error_cmd_read()-related patch is actually
> tested,
> the rest is compile-tested only.
> 
> Oleksandr Natalenko (3):
>   scsi: qedf: do not touch __user pointer in
>     qedf_dbg_stop_io_on_error_cmd_read() directly
>   scsi: qedf: do not touch __user pointer in
> qedf_dbg_debug_cmd_read()
>     directly
>   scsi: qedf: do not touch __user pointer in
> qedf_dbg_fp_int_cmd_read()
>     directly
> 
>  drivers/scsi/qedf/qedf_dbg.h     |  2 ++
>  drivers/scsi/qedf/qedf_debugfs.c | 35 +++++++++++++++++++-----------
> --
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> --
> 2.41.0
> 

I will test the series, the one patch was already tested.
This was reproduced in our LAB
Will report back after testing

Regards
Laurence

Re: [RFC PATCH 0/3] scsi: qedf: sanitise uaccess

[Laurence Oberman]

On Mon, 2023-07-24 at 09:03 -0400, Laurence Oberman wrote:
> On Mon, 2023-07-24 at 14:02 +0200, Oleksandr Natalenko wrote:
> > qedf driver, debugfs part of it specifically, touches __user
> > pointers
> > directly for printing out info to userspace via sprintf(), which
> > may
> > cause crash like this:
> > 
> > BUG: unable to handle kernel paging request at 00007ffd1d6b43a0
> > IP: [<ffffffffaa7a882a>] string.isra.7+0x6a/0xf0
> > Oops: 0003 [#1] SMP
> > Call Trace:
> >  [<ffffffffaa7a9f31>] vsnprintf+0x201/0x6a0
> >  [<ffffffffaa7aa556>] sprintf+0x56/0x80
> >  [<ffffffffc04227ed>] qedf_dbg_stop_io_on_error_cmd_read+0x6d/0x90
> > [qedf]
> >  [<ffffffffaa65bb2f>] vfs_read+0x9f/0x170
> >  [<ffffffffaa65cb82>] SyS_pread64+0x92/0xc0
> > 
> > Avoid this by preparing the info in a kernel buffer first, either
> > allocated on stack for small printouts, or via vmalloc() for big
> > ones,
> > and then copying it to the userspace properly.
> > 
> > I'm not sure how big the vmalloc()'ed buffer should be, and also
> > whether
> > vmalloc()'ing it directly in the _read() function is a good idea,
> > hence
> > RFC prefix.
> > 
> > The qedf_dbg_stop_io_on_error_cmd_read()-related patch is actually
> > tested,
> > the rest is compile-tested only.
> > 
> > Oleksandr Natalenko (3):
> >   scsi: qedf: do not touch __user pointer in
> >     qedf_dbg_stop_io_on_error_cmd_read() directly
> >   scsi: qedf: do not touch __user pointer in
> > qedf_dbg_debug_cmd_read()
> >     directly
> >   scsi: qedf: do not touch __user pointer in
> > qedf_dbg_fp_int_cmd_read()
> >     directly
> > 
> >  drivers/scsi/qedf/qedf_dbg.h     |  2 ++
> >  drivers/scsi/qedf/qedf_debugfs.c | 35 +++++++++++++++++++---------
> > --
> > --
> >  2 files changed, 23 insertions(+), 14 deletions(-)
> > 
> > --
> > 2.41.0
> > 
> 
> I will test the series, the one patch was already tested.
> This was reproduced in our LAB
> Will report back after testing
> 
> Regards
> Laurence

For the series:  Against 6.5.0-rc3
Makes sense to me and tested.

Reviewed-by: Laurence Oberman <loberman@redhat.com>
Tested-by: Laurence Oberman <loberman@redhat.com>


[root@segstorage5 host2]# ls
clear_stats  debug  driver_stats  fp_int  io_trace  offload_stats 
stop_io_on_error
[root@segstorage5 host2]# cat stop_io_on_error
false
[root@segstorage5 host2]# cat debug
debug mask = 0x2
[root@segstorage5 host2]# cat fp_int

Fastpath I/O completions

#0: 792
#1: 1242
#2: 1151
#3: 978
#4: 775
#5: 855
#6: 899
#7: 643
#8: 801
#9: 1013
#10: 956
#11: 678
#12: 703
#13: 817
#14: 932
#15: 614