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.
Previous submission is an RFC: [1]. There are no code changes since
then. The RFC prefix is dropped. The Tested-by tag from Laurence is
added.
There's similar submission from Saurav [2], but we agreed I could nack
it and proceed with my one.
[1] https://lore.kernel.org/linux-scsi/[email protected]/
[2] https://lore.kernel.org/linux-scsi/[email protected]/
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
On pátek 28. července 2023 8:58:16 CEST 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.
>
> Previous submission is an RFC: [1]. There are no code changes since
> then. The RFC prefix is dropped. The Tested-by tag from Laurence is
> added.
>
> There's similar submission from Saurav [2], but we agreed I could nack
> it and proceed with my one.
>
> [1] https://lore.kernel.org/linux-scsi/[email protected]/
> [2] https://lore.kernel.org/linux-scsi/[email protected]/
>
> 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(-)
>
>
Oops, I forgot to add:
Reviewed-by: Laurence Oberman <[email protected]>
as per [1].
My ask to the maintainer to add it if the submission is accepted, or let me know if I should do a v2 instead.
[1] https://lore.kernel.org/linux-scsi/[email protected]/
--
Oleksandr Natalenko (post-factum)
Principal Software Maintenance Engineer
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.
Fixes: 61d8658b4a ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/linux-scsi/[email protected]/
Cc: Saurav Kashyap <[email protected]>
Cc: Rob Evers <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jozef Bacik <[email protected]>
Cc: Laurence Oberman <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Tested-by: Laurence Oberman <[email protected]>
Signed-off-by: Oleksandr Natalenko <[email protected]>
---
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
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.
Fixes: 61d8658b4a ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
Link: https://lore.kernel.org/lkml/[email protected]/
Link: https://lore.kernel.org/linux-scsi/[email protected]/
Cc: Saurav Kashyap <[email protected]>
Cc: Rob Evers <[email protected]>
Cc: Johannes Thumshirn <[email protected]>
Cc: Jozef Bacik <[email protected]>
Cc: Laurence Oberman <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Tested-by: Laurence Oberman <[email protected]>
Signed-off-by: Oleksandr Natalenko <[email protected]>
---
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
Looks good,
Reviewed-by: Johannes Thumshirn <[email protected]>
From: Oleksandr Natalenko
> Sent: 28 July 2023 07:58
>
> 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.
>
> Fixes: 61d8658b4a ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
...
> 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");
You've made cbuf[] exactly just big enough.
If anyone breathes on this code it could overflow.
You really should use scnprintf() for safety.
>
> - cnt = min_t(int, count, cnt - *ppos);
> - *ppos += cnt;
> - return cnt;
> + return simple_read_from_buffer(buffer, count, ppos, cbuf, cnt);
Or just:
if (gedf->stop_on_error)
return simple_read_from_buffer(buffer, count, ppos, "true\n", 5);
return simple_read_from_buffer(buffer, count, ppos, "false\n", 6);
David
> }
>
> static ssize_t
> --
> 2.41.0
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Hello.
On pátek 28. července 2023 17:23:25 CEST David Laight wrote:
> From: Oleksandr Natalenko
> > Sent: 28 July 2023 07:58
> >
> > 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.
> >
> > Fixes: 61d8658b4a ("scsi: qedf: Add QLogic FastLinQ offload FCoE driver framework.")
> ...
> > 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");
>
> You've made cbuf[] exactly just big enough.
> If anyone breathes on this code it could overflow.
> You really should use scnprintf() for safety.
OK, I'll do scnprintf(cbuf, sizeof(cbuf), ...) in the next version of the submission.
Thanks.
> >
> > - cnt = min_t(int, count, cnt - *ppos);
> > - *ppos += cnt;
> > - return cnt;
> > + return simple_read_from_buffer(buffer, count, ppos, cbuf, cnt);
>
> Or just:
> if (gedf->stop_on_error)
> return simple_read_from_buffer(buffer, count, ppos, "true\n", 5);
> return simple_read_from_buffer(buffer, count, ppos, "false\n", 6);
>
> David
>
>
> > }
> >
> > static ssize_t
> > --
> > 2.41.0
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
>
--
Oleksandr Natalenko (post-factum)
Principal Software Maintenance Engineer