[PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Kyungtae Kim]

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 <kt0755@gmail.com>
---
 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

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Jens Axboe]

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

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Kyungtae Kim]

Do you mean mail client?? Well, I'm currently using gmail.

On Fri, Oct 26, 2018 at 10:41 AM Jens Axboe <axboe@kernel.dk> 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
>

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[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

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Kyungtae Kim]

Let me try to use git as email client.


Thanks,
Kyungtae Kim
On Fri, Oct 26, 2018 at 10:57 AM Jens Axboe <axboe@kernel.dk> 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
>

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Bart Van Assche]

On Fri, 2018-10-26 at 10:39 -0400, Kyungtae Kim wrote:
> 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.

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) ...

Why does that statement compare cmd_count with 33? Is that comparison correct
or not? Anyway, I don't think it makes sense first to compare cmd_count against
33 and next to compare it against 16 ...

> +       if (ptr->cmd_count > ARRAY_SIZE(ptr->cmd))
> +               return -EINVAL;

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.

Bart.

Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()

[Kyungtae Kim]

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