* [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
@ 2018-10-26 14:39 Kyungtae Kim
2018-10-26 14:41 ` Jens Axboe
2018-10-26 15:21 ` Bart Van Assche
0 siblings, 2 replies; 7+ messages in thread
From: Kyungtae Kim @ 2018-10-26 14:39 UTC (permalink / raw)
To: Jens Axboe
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 14:39 [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy() Kyungtae Kim
@ 2018-10-26 14:41 ` Jens Axboe
2018-10-26 14:51 ` Kyungtae Kim
2018-10-26 15:21 ` Bart Van Assche
1 sibling, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2018-10-26 14:41 UTC (permalink / raw)
To: Kyungtae Kim
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 14:41 ` Jens Axboe
@ 2018-10-26 14:51 ` Kyungtae Kim
2018-10-26 14:57 ` Jens Axboe
0 siblings, 1 reply; 7+ messages in thread
From: Kyungtae Kim @ 2018-10-26 14:51 UTC (permalink / raw)
To: Jens Axboe
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 14:51 ` Kyungtae Kim
@ 2018-10-26 14:57 ` Jens Axboe
2018-10-26 15:06 ` Kyungtae Kim
0 siblings, 1 reply; 7+ messages in thread
From: Jens Axboe @ 2018-10-26 14:57 UTC (permalink / raw)
To: Kyungtae Kim
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 14:57 ` Jens Axboe
@ 2018-10-26 15:06 ` Kyungtae Kim
0 siblings, 0 replies; 7+ messages in thread
From: Kyungtae Kim @ 2018-10-26 15:06 UTC (permalink / raw)
To: Jens Axboe
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 14:39 [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy() Kyungtae Kim
2018-10-26 14:41 ` Jens Axboe
@ 2018-10-26 15:21 ` Bart Van Assche
2018-10-26 15:49 ` Kyungtae Kim
1 sibling, 1 reply; 7+ messages in thread
From: Bart Van Assche @ 2018-10-26 15:21 UTC (permalink / raw)
To: Kyungtae Kim, Jens Axboe
Cc: jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block, linux-kernel
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy()
2018-10-26 15:21 ` Bart Van Assche
@ 2018-10-26 15:49 ` Kyungtae Kim
0 siblings, 0 replies; 7+ messages in thread
From: Kyungtae Kim @ 2018-10-26 15:49 UTC (permalink / raw)
To: bvanassche
Cc: Jens Axboe, jikos, Byoungyoung Lee, DaeRyong Jeong, linux-block,
linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-26 15:49 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-26 14:39 [PATCH] floppy: Avoid memory access beyond the array bounds in setup_rw_floppy() Kyungtae Kim
2018-10-26 14:41 ` Jens Axboe
2018-10-26 14:51 ` Kyungtae Kim
2018-10-26 14:57 ` Jens Axboe
2018-10-26 15:06 ` Kyungtae Kim
2018-10-26 15:21 ` Bart Van Assche
2018-10-26 15:49 ` Kyungtae Kim
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).