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
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 <[email protected]> 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
>
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 <[email protected]> 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 <[email protected]> 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
> >
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
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 <[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 Wed, Oct 24, 2018 at 5:27 AM Jens Axboe <[email protected]> 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
>
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