* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 [not found] <CAEAjamtML1yMLL0DsV5JkD1H6P0Yg19F2DVq+_c-u09RaCKuDw@mail.gmail.com> @ 2018-10-23 10:01 ` Jens Axboe 2018-10-24 6:29 ` Kyungtae Kim 2019-08-09 13:40 ` Denis Efremov 1 sibling, 1 reply; 7+ messages in thread From: Jens Axboe @ 2018-10-23 10:01 UTC (permalink / raw) To: Kyungtae Kim, jikos Cc: Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 2018-10-23 10:01 ` UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 Jens Axboe @ 2018-10-24 6:29 ` Kyungtae Kim 2018-10-24 6:33 ` Kyungtae Kim 0 siblings, 1 reply; 7+ messages in thread From: Kyungtae Kim @ 2018-10-24 6:29 UTC (permalink / raw) To: axboe Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 2018-10-24 6:29 ` Kyungtae Kim @ 2018-10-24 6:33 ` Kyungtae Kim 2018-10-24 9:27 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Kyungtae Kim @ 2018-10-24 6:33 UTC (permalink / raw) To: axboe Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 > > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 2018-10-24 6:33 ` Kyungtae Kim @ 2018-10-24 9:27 ` Jens Axboe 2018-10-26 13:22 ` Kyungtae Kim 0 siblings, 1 reply; 7+ messages in thread From: Jens Axboe @ 2018-10-24 9:27 UTC (permalink / raw) To: Kyungtae Kim Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 2018-10-24 9:27 ` Jens Axboe @ 2018-10-26 13:22 ` Kyungtae Kim 2018-10-26 14:20 ` Jens Axboe 0 siblings, 1 reply; 7+ messages in thread From: Kyungtae Kim @ 2018-10-26 13:22 UTC (permalink / raw) To: axboe Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 2018-10-26 13:22 ` Kyungtae Kim @ 2018-10-26 14:20 ` Jens Axboe 0 siblings, 0 replies; 7+ messages in thread From: Jens Axboe @ 2018-10-26 14:20 UTC (permalink / raw) To: Kyungtae Kim Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 [not found] <CAEAjamtML1yMLL0DsV5JkD1H6P0Yg19F2DVq+_c-u09RaCKuDw@mail.gmail.com> 2018-10-23 10:01 ` UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 Jens Axboe @ 2019-08-09 13:40 ` Denis Efremov 1 sibling, 0 replies; 7+ messages in thread From: Denis Efremov @ 2019-08-09 13:40 UTC (permalink / raw) To: Kyungtae Kim Cc: axboe, jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel, syzkaller 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 > ================================================================== > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-09 13:40 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CAEAjamtML1yMLL0DsV5JkD1H6P0Yg19F2DVq+_c-u09RaCKuDw@mail.gmail.com> 2018-10-23 10:01 ` UBSAN: Undefined behaviour in drivers/block/floppy.c:1495:32 Jens Axboe 2018-10-24 6:29 ` Kyungtae Kim 2018-10-24 6:33 ` Kyungtae Kim 2018-10-24 9:27 ` Jens Axboe 2018-10-26 13:22 ` Kyungtae Kim 2018-10-26 14:20 ` Jens Axboe 2019-08-09 13:40 ` Denis Efremov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).