setup_rw_floppy() writes some bytes of array cmd to the floppy disk
controller, depending on cmd_count.
Although the size of array cmd is fixed like 16, cmd_count can be much
larger through raw_cmd_ioctl().
Noticed there is no bound check for this, thereby leading to invalid
memory access.
This patch adds a bound check for cmd_count when initialized for the
first time.
The crash log is as follows:
UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32
index 16 is out of range for type 'unsigned char [16]'
CPU: 0 PID: 2420 Comm: kworker/u4:3 Not tainted 4.19.0-rc2 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Workqueue: floppy fd_timer_workfn
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xd2/0x148 lib/dump_stack.c:113
ubsan_epilogue+0x12/0x94 lib/ubsan.c:159
__ubsan_handle_out_of_bounds+0x174/0x1b8 lib/ubsan.c:386
setup_rw_floppy+0xbd9/0xe60 drivers/block/floppy.c:1495
seek_floppy drivers/block/floppy.c:1605 [inline]
floppy_ready+0x61a/0x2230 drivers/block/floppy.c:1917
fd_timer_workfn+0x1a/0x20 drivers/block/floppy.c:994
process_one_work+0xa0c/0x1820 kernel/workqueue.c:2153
worker_thread+0x8f/0xd20 kernel/workqueue.c:2296
kthread+0x3a3/0x470 kernel/kthread.c:246
ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:413
Signed-off-by: Kyungtae Kim <[email protected]>
---
drivers/block/floppy.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
index a8cfa01..41160a1 100644
--- a/drivers/block/floppy.c
+++ b/drivers/block/floppy.c
@@ -3146,6 +3146,9 @@ static int raw_cmd_copyin(int cmd, void __user *param,
*/
return -EINVAL;
+ if (ptr->cmd_count > ARRAY_SIZE(ptr->cmd))
+ return -EINVAL;
+
for (i = 0; i < 16; i++)
ptr->reply[i] = 0;
ptr->resultcode = 0;
--
2.7.4
On 10/26/18 8:39 AM, Kyungtae Kim wrote:
> diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> index a8cfa01..41160a1 100644
> --- a/drivers/block/floppy.c
> +++ b/drivers/block/floppy.c
> @@ -3146,6 +3146,9 @@ static int raw_cmd_copyin(int cmd, void __user *param,
> */
> return -EINVAL;
>
> + if (ptr->cmd_count > ARRAY_SIZE(ptr->cmd))
> + return -EINVAL;
> +
> for (i = 0; i < 16; i++)
> ptr->reply[i] = 0;
> ptr->resultcode = 0;
Almost there, the tabs have been turned into spaces. This could be
a mailer issue, what are you using to send out the patch?
--
Jens Axboe
Do you mean mail client?? Well, I'm currently using gmail.
On Fri, Oct 26, 2018 at 10:41 AM Jens Axboe <[email protected]> wrote:
>
> On 10/26/18 8:39 AM, Kyungtae Kim wrote:
>
> > diff --git a/drivers/block/floppy.c b/drivers/block/floppy.c
> > index a8cfa01..41160a1 100644
> > --- a/drivers/block/floppy.c
> > +++ b/drivers/block/floppy.c
> > @@ -3146,6 +3146,9 @@ static int raw_cmd_copyin(int cmd, void __user *param,
> > */
> > return -EINVAL;
> >
> > + if (ptr->cmd_count > ARRAY_SIZE(ptr->cmd))
> > + return -EINVAL;
> > +
> > for (i = 0; i < 16; i++)
> > ptr->reply[i] = 0;
> > ptr->resultcode = 0;
>
> Almost there, the tabs have been turned into spaces. This could be
> a mailer issue, what are you using to send out the patch?
>
> --
> Jens Axboe
>
On 10/26/18 8:51 AM, Kyungtae Kim wrote:
> Do you mean mail client?? Well, I'm currently using gmail.
Yes, email client. I've never sent patches with gmail, so can't
say if that works or not. OK, just checked
Documentation/process/email-clients.rst
and is states that:
"Gmail (Web GUI)
***************
Does not work for sending patches.
Gmail web client converts tabs to spaces automatically."
so I guess that won't work for you. You can use git send-email
with gmail smtp, that's what I do. Or read the above file to figure out
what will work the best for you.
--
Jens Axboe
Let me try to use git as email client.
Thanks,
Kyungtae Kim
On Fri, Oct 26, 2018 at 10:57 AM Jens Axboe <[email protected]> wrote:
>
> On 10/26/18 8:51 AM, Kyungtae Kim wrote:
> > Do you mean mail client?? Well, I'm currently using gmail.
>
> Yes, email client. I've never sent patches with gmail, so can't
> say if that works or not. OK, just checked
>
> Documentation/process/email-clients.rst
>
> and is states that:
>
> "Gmail (Web GUI)
> ***************
>
> Does not work for sending patches.
>
> Gmail web client converts tabs to spaces automatically."
>
> so I guess that won't work for you. You can use git send-email
> with gmail smtp, that's what I do. Or read the above file to figure out
> what will work the best for you.
>
> --
> Jens Axboe
>
On Fri, 2018-10-26 at 10:39 -0400, Kyungtae Kim wrote:
+AD4 setup+AF8-rw+AF8-floppy() writes some bytes of array cmd to the floppy disk
+AD4 controller, depending on cmd+AF8-count.
+AD4 Although the size of array cmd is fixed like 16, cmd+AF8-count can be much
+AD4 larger through raw+AF8-cmd+AF8-ioctl().
+AD4 Noticed there is no bound check for this, thereby leading to invalid
+AD4 memory access.
Against which kernel tree did you prepare this patch? Just above the code
you want to insert I found the following:
if (ptr-+AD4-cmd+AF8-count +AD4 33) ...
Why does that statement compare cmd+AF8-count with 33? Is that comparison correct
or not? Anyway, I don't think it makes sense first to compare cmd+AF8-count against
33 and next to compare it against 16 ...
+AD4 +- if (ptr-+AD4-cmd+AF8-count +AD4 ARRAY+AF8-SIZE(ptr-+AD4-cmd))
+AD4 +- return -EINVAL+ADs
This comparison looks suspicious to me. Almost every comparison of the type
+ACI... +AD4 ARRAY+AF8-SIZE()+ACI I have seen so far was wrong and should be changed into
+ACI... +AD4APQ ARRAY+AF8-SIZE()+ACI instead.
Bart.
Thank you for your reply.
> Against which kernel tree did you prepare this patch? Just above the code
> you want to insert I found the following:
>
> if (ptr->cmd_count > 33) ...
Yes, I saw that code as well. But I find out there is no more
additional code relevant to it.
So I guess, that is for like reserved ones for the future work (e.g., RESTORE).
And such code cannot solve this crash issue because the crash happens
even when cmd_count is less than 33.
> or not? Anyway, I don't think it makes sense first to compare cmd_count against
> 33 and next to compare it against 16 ...
I agree. So far, seems that removing if (ptr->cmd_count > 33) looks better.
Please let me know if you have better ideas.
> This comparison looks suspicious to me. Almost every comparison of the type
> "... > ARRAY_SIZE()" I have seen so far was wrong and should be changed into
> "... >= ARRAY_SIZE()" instead.
Well, let me check this part again.
Thanks,
Kyungtae Kim