Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Jens Axboe]

On 10/22/18 5:20 PM, Kyungtae Kim wrote:
> We report a bug found in v4.19-rc2 (v4.19-rc8 as well):
> UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32
> 
> kernel config: https://kt0755.github.io/etc/config_v2-4.19
> repro: https://kt0755.github.io/etc/repro.b4076.c
> 
> Analysis:
> 
> struct floppy_raw_cmd {
>    unsigned char cmd_count;
>    unsigned char cmd[16];
>   ...
> };
> 
> for (i=0; i<raw_cmd->cmd_count; i++)
>     output_byte(raw_cmd->cmd[i])
> 
> In driver/block/floppy.c:1495, the code snippet above is trying to
> write some bytes to the floppy disk controller, depending on "cmd_count".
> As you see "struct floppy_raw_cmd" above, the size of array “cmd” is
> fixed as 16.
> The thing is, there is no boundary check for the index of array "cmd"
> when this is used. Besides, "cmd_count" can be manipulated by raw_cmd_ioctl
> which is derived from ioctl system call.
> We observed that cmd_count is set at line 2540 (or 2111), but that is
> after such a bug arose in our experiment. So by manipulating system call ioctl,
> user program can have illegitimate memory access.
> 
> The following is a simple patch to stop this. (This might not be the
> best.)
> 
> diff --git a/linux-4.19-rc2/drivers/block/floppy.c
> b/linux-4.19-rc2/drivers/block/floppy.c
> index f2b6f4d..a3610c9 100644
> --- a/linux-4.19-rc2/drivers/block/floppy.c
> +++ b/linux-4.19-rc2/drivers/block/floppy.c
> @@ -3149,6 +3149,8 @@ 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;

I think that's a decent way to fix it, but you probably want to
test your patch - it doesn't compile. Send something you've
tested that works.

-- 
Jens Axboe

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Kyungtae Kim]

Thanks. The following should work.

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;
On Tue, Oct 23, 2018 at 6:01 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 10/22/18 5:20 PM, Kyungtae Kim wrote:
> > We report a bug found in v4.19-rc2 (v4.19-rc8 as well):
> > UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32
> >
> > kernel config: https://kt0755.github.io/etc/config_v2-4.19
> > repro: https://kt0755.github.io/etc/repro.b4076.c
> >
> > Analysis:
> >
> > struct floppy_raw_cmd {
> >    unsigned char cmd_count;
> >    unsigned char cmd[16];
> >   ...
> > };
> >
> > for (i=0; i<raw_cmd->cmd_count; i++)
> >     output_byte(raw_cmd->cmd[i])
> >
> > In driver/block/floppy.c:1495, the code snippet above is trying to
> > write some bytes to the floppy disk controller, depending on "cmd_count".
> > As you see "struct floppy_raw_cmd" above, the size of array “cmd” is
> > fixed as 16.
> > The thing is, there is no boundary check for the index of array "cmd"
> > when this is used. Besides, "cmd_count" can be manipulated by raw_cmd_ioctl
> > which is derived from ioctl system call.
> > We observed that cmd_count is set at line 2540 (or 2111), but that is
> > after such a bug arose in our experiment. So by manipulating system call ioctl,
> > user program can have illegitimate memory access.
> >
> > The following is a simple patch to stop this. (This might not be the
> > best.)
> >
> > diff --git a/linux-4.19-rc2/drivers/block/floppy.c
> > b/linux-4.19-rc2/drivers/block/floppy.c
> > index f2b6f4d..a3610c9 100644
> > --- a/linux-4.19-rc2/drivers/block/floppy.c
> > +++ b/linux-4.19-rc2/drivers/block/floppy.c
> > @@ -3149,6 +3149,8 @@ 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;
>
> I think that's a decent way to fix it, but you probably want to
> test your patch - it doesn't compile. Send something you've
> tested that works.
>
> --
> Jens Axboe
>

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Kyungtae Kim]

Corrected.


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;


Thanks,
Kyungtae Kim
On Wed, Oct 24, 2018 at 2:29 AM Kyungtae Kim <kt0755@gmail.com> wrote:
>
> Thanks. The following should work.
>
> 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;
> On Tue, Oct 23, 2018 at 6:01 AM Jens Axboe <axboe@kernel.dk> wrote:
> >
> > On 10/22/18 5:20 PM, Kyungtae Kim wrote:
> > > We report a bug found in v4.19-rc2 (v4.19-rc8 as well):
> > > UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32
> > >
> > > kernel config: https://kt0755.github.io/etc/config_v2-4.19
> > > repro: https://kt0755.github.io/etc/repro.b4076.c
> > >
> > > Analysis:
> > >
> > > struct floppy_raw_cmd {
> > >    unsigned char cmd_count;
> > >    unsigned char cmd[16];
> > >   ...
> > > };
> > >
> > > for (i=0; i<raw_cmd->cmd_count; i++)
> > >     output_byte(raw_cmd->cmd[i])
> > >
> > > In driver/block/floppy.c:1495, the code snippet above is trying to
> > > write some bytes to the floppy disk controller, depending on "cmd_count".
> > > As you see "struct floppy_raw_cmd" above, the size of array “cmd” is
> > > fixed as 16.
> > > The thing is, there is no boundary check for the index of array "cmd"
> > > when this is used. Besides, "cmd_count" can be manipulated by raw_cmd_ioctl
> > > which is derived from ioctl system call.
> > > We observed that cmd_count is set at line 2540 (or 2111), but that is
> > > after such a bug arose in our experiment. So by manipulating system call ioctl,
> > > user program can have illegitimate memory access.
> > >
> > > The following is a simple patch to stop this. (This might not be the
> > > best.)
> > >
> > > diff --git a/linux-4.19-rc2/drivers/block/floppy.c
> > > b/linux-4.19-rc2/drivers/block/floppy.c
> > > index f2b6f4d..a3610c9 100644
> > > --- a/linux-4.19-rc2/drivers/block/floppy.c
> > > +++ b/linux-4.19-rc2/drivers/block/floppy.c
> > > @@ -3149,6 +3149,8 @@ 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;
> >
> > I think that's a decent way to fix it, but you probably want to
> > test your patch - it doesn't compile. Send something you've
> > tested that works.
> >
> > --
> > Jens Axboe
> >

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Jens Axboe]

On 10/24/18 12:33 AM, Kyungtae Kim wrote:
> Corrected.

You'll want to read Documentation/process/submitting-patches.rst as
your patch is lacking in several areas.


-- 
Jens Axboe

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Kyungtae Kim]

I corrected the patch as follows:

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

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


On Wed, Oct 24, 2018 at 5:27 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 10/24/18 12:33 AM, Kyungtae Kim wrote:
> > Corrected.
>
> You'll want to read Documentation/process/submitting-patches.rst as
> your patch is lacking in several areas.
>
>
> --
> Jens Axboe
>

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Jens Axboe]

On 10/26/18 7:22 AM, Kyungtae Kim wrote:
> I corrected the patch as follows:

OK, we're getting there! Please resend as a separate email, so that
the subject line is the patch header, and just the commit message
in the body. I'd fix that up for this one, but you also need to
fix up:

> 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;
> +

This needs to use tabs, not two spaces. Look at the surrounding
code - if yours looks differently, then you should correct that.

-- 
Jens Axboe

Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32

[Denis Efremov]

Hi!

Sorry for the late response. But I think I could add some useful info,
because I also analyzed this report from syzkaller.

I don't think that we could fix this UBSAN warning with this patch. If you look
at the lines right before your check you will find another check of cmd_count
with clarifying comment:

        if (ptr->cmd_count > 33)
                        /* the command may now also take up the space
                         * initially intended for the reply & the
                         * reply count. Needed for long 82078 commands
                         * such as RESTORE, which takes ... 17 command
                         * bytes. Murphy's law #137: When you reserve
                         * 16 bytes for a structure, you'll one day
                         * discover that you really need 17...
                         */
                return -EINVAL;

        for (i = 0; i < 16; i++)
                ptr->reply[i] = 0;
        ptr->resultcode = 0;

And a little bit more details about (from include/uapi/linux/fd.h):
struct floppy_raw_cmd {
...
        unsigned char cmd_count;                                                 
        unsigned char cmd[16];                                                   
        unsigned char reply_count;                                               
        unsigned char reply[16];
...
}

So, cmd[16] + reply_count[1] + reply[16] == 33.

Thus, this behavior is intentional, we could not fix it this way and it's already
a part of UAPI.

But thank you for analyzing the report!

Denis

On 23.10.2018 02:20, Kyungtae Kim wrote:
> We report a bug found in v4.19-rc2 (v4.19-rc8 as well):
> UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32
> 
> kernel config: https://kt0755.github.io/etc/config_v2-4.19
> repro: https://kt0755.github.io/etc/repro.b4076.c
> 
> Analysis:
> 
> struct floppy_raw_cmd {
>    unsigned char cmd_count;
>    unsigned char cmd[16];
>   ...
> };
> 
> for (i=0; i<raw_cmd->cmd_count; i++)
>     output_byte(raw_cmd->cmd[i])
> 
> In driver/block/floppy.c:1495, the code snippet above is trying to
> write some bytes to the floppy disk controller, depending on "cmd_count".
> As you see "struct floppy_raw_cmd" above, the size of array “cmd” is
> fixed as 16.
> The thing is, there is no boundary check for the index of array "cmd"
> when this is used. Besides, "cmd_count" can be manipulated by raw_cmd_ioctl
> which is derived from ioctl system call.
> We observed that cmd_count is set at line 2540 (or 2111), but that is
> after such a bug arose in our experiment. So by manipulating system call
> ioctl,
> user program can have illegitimate memory access.
> 
> The following is a simple patch to stop this. (This might not be the
> best.)
> 
> diff --git a/linux-4.19-rc2/drivers/block/floppy.c
> b/linux-4.19-rc2/drivers/block/floppy.c
> index f2b6f4d..a3610c9 100644
> --- a/linux-4.19-rc2/drivers/block/floppy.c
> +++ b/linux-4.19-rc2/drivers/block/floppy.c
> @@ -3149,6 +3149,8 @@ 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;
> 
> 
> Crash log
> ==================================================================
> 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
> ==================================================================
>