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
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
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 <[email protected]>
Tested-by: Laurence Oberman <[email protected]>
[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