* [RFC PATCH 1/3] scsi: qedf: do not touch __user pointer in qedf_dbg_stop_io_on_error_cmd_read() directly
2023-07-24 12:02 [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Oleksandr Natalenko
@ 2023-07-24 12:02 ` Oleksandr Natalenko
2023-07-24 12:02 ` [RFC PATCH 2/3] scsi: qedf: do not touch __user pointer in qedf_dbg_debug_cmd_read() directly Oleksandr Natalenko
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Natalenko @ 2023-07-24 12:02 UTC (permalink / raw)
To: linux-kernel
Cc: linux-scsi, Saurav Kashyap, Javed Hasan,
GR-QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, Jozef Bacik, Laurence Oberman, Rob Evers
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RFC PATCH 2/3] scsi: qedf: do not touch __user pointer in qedf_dbg_debug_cmd_read() directly
2023-07-24 12:02 [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Oleksandr Natalenko
2023-07-24 12:02 ` [RFC PATCH 1/3] scsi: qedf: do not touch __user pointer in qedf_dbg_stop_io_on_error_cmd_read() directly Oleksandr Natalenko
@ 2023-07-24 12:02 ` Oleksandr Natalenko
2023-07-24 12:02 ` [RFC PATCH 3/3] scsi: qedf: do not touch __user pointer in qedf_dbg_fp_int_cmd_read() directly Oleksandr Natalenko
2023-07-24 13:03 ` [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Laurence Oberman
3 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Natalenko @ 2023-07-24 12:02 UTC (permalink / raw)
To: linux-kernel
Cc: linux-scsi, Saurav Kashyap, Javed Hasan,
GR-QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, Jozef Bacik, Laurence Oberman, Rob Evers
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [RFC PATCH 3/3] scsi: qedf: do not touch __user pointer in qedf_dbg_fp_int_cmd_read() directly
2023-07-24 12:02 [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Oleksandr Natalenko
2023-07-24 12:02 ` [RFC PATCH 1/3] scsi: qedf: do not touch __user pointer in qedf_dbg_stop_io_on_error_cmd_read() directly Oleksandr Natalenko
2023-07-24 12:02 ` [RFC PATCH 2/3] scsi: qedf: do not touch __user pointer in qedf_dbg_debug_cmd_read() directly Oleksandr Natalenko
@ 2023-07-24 12:02 ` Oleksandr Natalenko
2023-07-24 13:03 ` [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Laurence Oberman
3 siblings, 0 replies; 6+ messages in thread
From: Oleksandr Natalenko @ 2023-07-24 12:02 UTC (permalink / raw)
To: linux-kernel
Cc: linux-scsi, Saurav Kashyap, Javed Hasan,
GR-QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, Jozef Bacik, Laurence Oberman, Rob Evers
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH 0/3] scsi: qedf: sanitise uaccess
2023-07-24 12:02 [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Oleksandr Natalenko
` (2 preceding siblings ...)
2023-07-24 12:02 ` [RFC PATCH 3/3] scsi: qedf: do not touch __user pointer in qedf_dbg_fp_int_cmd_read() directly Oleksandr Natalenko
@ 2023-07-24 13:03 ` Laurence Oberman
2023-07-24 16:27 ` Laurence Oberman
3 siblings, 1 reply; 6+ messages in thread
From: Laurence Oberman @ 2023-07-24 13:03 UTC (permalink / raw)
To: Oleksandr Natalenko, linux-kernel
Cc: linux-scsi, Saurav Kashyap, Javed Hasan,
GR-QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, Jozef Bacik, Rob Evers
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [RFC PATCH 0/3] scsi: qedf: sanitise uaccess
2023-07-24 13:03 ` [RFC PATCH 0/3] scsi: qedf: sanitise uaccess Laurence Oberman
@ 2023-07-24 16:27 ` Laurence Oberman
0 siblings, 0 replies; 6+ messages in thread
From: Laurence Oberman @ 2023-07-24 16:27 UTC (permalink / raw)
To: Oleksandr Natalenko, linux-kernel
Cc: linux-scsi, Saurav Kashyap, Javed Hasan,
GR-QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, Jozef Bacik, Rob Evers
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
^ permalink raw reply [flat|nested] 6+ messages in thread