* [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
@ 2016-02-04 15:49 Dmitry Vyukov
2016-02-04 16:32 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2016-02-04 15:49 UTC (permalink / raw)
To: daniel, airlied, dri-devel
Cc: linux-kernel, syzkaller, kcc, glider, Dmitry Vyukov
Size of kmalloc() in vga_arb_write() is controlled by user.
Too large kmalloc() size triggers WARNING message on console.
Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
---
Example WARNING:
WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
__alloc_pages_nodemask+0x7d2/0x1760()
Modules linked in:
CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
Call Trace:
[< inline >] __dump_stack lib/dump_stack.c:15
[<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
[<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
[<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
[< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
[<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
[<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
[< inline >] alloc_pages include/linux/gfp.h:459
[<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
[<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
[<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
[< inline >] kmalloc_large include/linux/slab.h:395
[<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
[< inline >] kmalloc include/linux/slab.h:468
[<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
[<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
[<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
[<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
[< inline >] SYSC_writev fs/read_write.c:919
[<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
[<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---
drivers/gpu/vga/vgaarb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index f17cb04..d73b85b 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
int i;
- kbuf = kmalloc(count + 1, GFP_KERNEL);
+ kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
if (!kbuf)
return -ENOMEM;
--
2.7.0.rc3.207.g0ac5344
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
2016-02-04 15:49 [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc Dmitry Vyukov
@ 2016-02-04 16:32 ` Ville Syrjälä
2016-02-04 16:37 ` Dmitry Vyukov
0 siblings, 1 reply; 8+ messages in thread
From: Ville Syrjälä @ 2016-02-04 16:32 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: daniel, airlied, dri-devel, kcc, syzkaller, glider, linux-kernel
On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
> Size of kmalloc() in vga_arb_write() is controlled by user.
> Too large kmalloc() size triggers WARNING message on console.
>
> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> ---
> Example WARNING:
>
> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
> __alloc_pages_nodemask+0x7d2/0x1760()
> Modules linked in:
> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
> Call Trace:
> [< inline >] __dump_stack lib/dump_stack.c:15
> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
> [< inline >] alloc_pages include/linux/gfp.h:459
> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
> [< inline >] kmalloc_large include/linux/slab.h:395
> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
> [< inline >] kmalloc include/linux/slab.h:468
> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
> [< inline >] SYSC_writev fs/read_write.c:919
> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> ---
> drivers/gpu/vga/vgaarb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index f17cb04..d73b85b 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
> int i;
>
>
> - kbuf = kmalloc(count + 1, GFP_KERNEL);
> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
I don't really see why it does this user controlled malloc in the
first place. The max legth of the string it will actually handle looks
well bounded, so it could just use some fixed length buffer (on stack
even).
> if (!kbuf)
> return -ENOMEM;
>
> --
> 2.7.0.rc3.207.g0ac5344
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
2016-02-04 16:32 ` Ville Syrjälä
@ 2016-02-04 16:37 ` Dmitry Vyukov
2016-02-04 16:59 ` Ville Syrjälä
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2016-02-04 16:37 UTC (permalink / raw)
To: Ville Syrjälä
Cc: daniel, airlied, dri-devel, Kostya Serebryany, syzkaller,
Alexander Potapenko, LKML
On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
>> Size of kmalloc() in vga_arb_write() is controlled by user.
>> Too large kmalloc() size triggers WARNING message on console.
>>
>> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
>>
>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> ---
>> Example WARNING:
>>
>> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
>> __alloc_pages_nodemask+0x7d2/0x1760()
>> Modules linked in:
>> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
>> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
>> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
>> Call Trace:
>> [< inline >] __dump_stack lib/dump_stack.c:15
>> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
>> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
>> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
>> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
>> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
>> [< inline >] alloc_pages include/linux/gfp.h:459
>> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
>> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
>> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
>> [< inline >] kmalloc_large include/linux/slab.h:395
>> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
>> [< inline >] kmalloc include/linux/slab.h:468
>> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
>> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
>> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
>> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
>> [< inline >] SYSC_writev fs/read_write.c:919
>> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
>> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
>> arch/x86/entry/entry_64.S:185
>> ---
>> drivers/gpu/vga/vgaarb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>> index f17cb04..d73b85b 100644
>> --- a/drivers/gpu/vga/vgaarb.c
>> +++ b/drivers/gpu/vga/vgaarb.c
>> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>> int i;
>>
>>
>> - kbuf = kmalloc(count + 1, GFP_KERNEL);
>> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
>
> I don't really see why it does this user controlled malloc in the
> first place. The max legth of the string it will actually handle looks
> well bounded, so it could just use some fixed length buffer (on stack
> even).
What would be the right limit on data len?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
2016-02-04 16:37 ` Dmitry Vyukov
@ 2016-02-04 16:59 ` Ville Syrjälä
0 siblings, 0 replies; 8+ messages in thread
From: Ville Syrjälä @ 2016-02-04 16:59 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: LKML, dri-devel, Kostya Serebryany, syzkaller,
Alexander Potapenko
On Thu, Feb 04, 2016 at 05:37:49PM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
> >> Size of kmalloc() in vga_arb_write() is controlled by user.
> >> Too large kmalloc() size triggers WARNING message on console.
> >>
> >> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
> >>
> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> >> ---
> >> Example WARNING:
> >>
> >> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
> >> __alloc_pages_nodemask+0x7d2/0x1760()
> >> Modules linked in:
> >> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
> >> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
> >> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
> >> Call Trace:
> >> [< inline >] __dump_stack lib/dump_stack.c:15
> >> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
> >> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
> >> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
> >> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
> >> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
> >> [< inline >] alloc_pages include/linux/gfp.h:459
> >> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
> >> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
> >> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
> >> [< inline >] kmalloc_large include/linux/slab.h:395
> >> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
> >> [< inline >] kmalloc include/linux/slab.h:468
> >> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
> >> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
> >> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
> >> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
> >> [< inline >] SYSC_writev fs/read_write.c:919
> >> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
> >> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
> >> arch/x86/entry/entry_64.S:185
> >> ---
> >> drivers/gpu/vga/vgaarb.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> >> index f17cb04..d73b85b 100644
> >> --- a/drivers/gpu/vga/vgaarb.c
> >> +++ b/drivers/gpu/vga/vgaarb.c
> >> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
> >> int i;
> >>
> >>
> >> - kbuf = kmalloc(count + 1, GFP_KERNEL);
> >> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
> >
> > I don't really see why it does this user controlled malloc in the
> > first place. The max legth of the string it will actually handle looks
> > well bounded, so it could just use some fixed length buffer (on stack
> > even).
>
>
> What would be the right limit on data len?
From the looks of things the longest command could be the
"target PCI:domain:bus:dev.fn" thing. Even assuming something silly like
having 10 characters for each domain,bus,dev,fn that would still be only
55 bytes. So based on that even something like 64 bytes should be more
than enough AFAICS.
The other thing that strikes me as bit odd in this code is that it
just ignores whatever data is left over after it's done parsing the
string. But it returns the full count to userspace, indicating it
ate all of it. I guess that's fairly sane when userspace just uses a
fixed size buffer and checks that the kernel consumed it all. But
maybe there should be an actual check to see that there's a '\0'
or maybe <any amount of whitespace>+'\0' after the parsed string.
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
@ 2016-02-04 16:59 ` Ville Syrjälä
0 siblings, 0 replies; 8+ messages in thread
From: Ville Syrjälä @ 2016-02-04 16:59 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: daniel, airlied, dri-devel, Kostya Serebryany, syzkaller,
Alexander Potapenko, LKML
On Thu, Feb 04, 2016 at 05:37:49PM +0100, Dmitry Vyukov wrote:
> On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
> >> Size of kmalloc() in vga_arb_write() is controlled by user.
> >> Too large kmalloc() size triggers WARNING message on console.
> >>
> >> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
> >>
> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> >> ---
> >> Example WARNING:
> >>
> >> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
> >> __alloc_pages_nodemask+0x7d2/0x1760()
> >> Modules linked in:
> >> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
> >> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
> >> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
> >> Call Trace:
> >> [< inline >] __dump_stack lib/dump_stack.c:15
> >> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
> >> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
> >> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
> >> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
> >> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
> >> [< inline >] alloc_pages include/linux/gfp.h:459
> >> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
> >> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
> >> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
> >> [< inline >] kmalloc_large include/linux/slab.h:395
> >> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
> >> [< inline >] kmalloc include/linux/slab.h:468
> >> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
> >> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
> >> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
> >> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
> >> [< inline >] SYSC_writev fs/read_write.c:919
> >> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
> >> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
> >> arch/x86/entry/entry_64.S:185
> >> ---
> >> drivers/gpu/vga/vgaarb.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> >> index f17cb04..d73b85b 100644
> >> --- a/drivers/gpu/vga/vgaarb.c
> >> +++ b/drivers/gpu/vga/vgaarb.c
> >> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
> >> int i;
> >>
> >>
> >> - kbuf = kmalloc(count + 1, GFP_KERNEL);
> >> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
> >
> > I don't really see why it does this user controlled malloc in the
> > first place. The max legth of the string it will actually handle looks
> > well bounded, so it could just use some fixed length buffer (on stack
> > even).
>
>
> What would be the right limit on data len?
>From the looks of things the longest command could be the
"target PCI:domain:bus:dev.fn" thing. Even assuming something silly like
having 10 characters for each domain,bus,dev,fn that would still be only
55 bytes. So based on that even something like 64 bytes should be more
than enough AFAICS.
The other thing that strikes me as bit odd in this code is that it
just ignores whatever data is left over after it's done parsing the
string. But it returns the full count to userspace, indicating it
ate all of it. I guess that's fairly sane when userspace just uses a
fixed size buffer and checks that the kernel consumed it all. But
maybe there should be an actual check to see that there's a '\0'
or maybe <any amount of whitespace>+'\0' after the parsed string.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
2016-02-04 16:59 ` Ville Syrjälä
(?)
@ 2016-02-04 18:31 ` Dmitry Vyukov
2016-02-04 21:13 ` Dave Airlie
-1 siblings, 1 reply; 8+ messages in thread
From: Dmitry Vyukov @ 2016-02-04 18:31 UTC (permalink / raw)
To: syzkaller
Cc: Daniel Vetter, airlied, dri-devel, Kostya Serebryany,
Alexander Potapenko, LKML
On Thu, Feb 4, 2016 at 5:59 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Feb 04, 2016 at 05:37:49PM +0100, Dmitry Vyukov wrote:
>> On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
>> >> Size of kmalloc() in vga_arb_write() is controlled by user.
>> >> Too large kmalloc() size triggers WARNING message on console.
>> >>
>> >> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
>> >>
>> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>> >> ---
>> >> Example WARNING:
>> >>
>> >> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
>> >> __alloc_pages_nodemask+0x7d2/0x1760()
>> >> Modules linked in:
>> >> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
>> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> >> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
>> >> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
>> >> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
>> >> Call Trace:
>> >> [< inline >] __dump_stack lib/dump_stack.c:15
>> >> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>> >> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
>> >> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
>> >> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
>> >> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
>> >> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
>> >> [< inline >] alloc_pages include/linux/gfp.h:459
>> >> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
>> >> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
>> >> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
>> >> [< inline >] kmalloc_large include/linux/slab.h:395
>> >> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
>> >> [< inline >] kmalloc include/linux/slab.h:468
>> >> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
>> >> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
>> >> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
>> >> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
>> >> [< inline >] SYSC_writev fs/read_write.c:919
>> >> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
>> >> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
>> >> arch/x86/entry/entry_64.S:185
>> >> ---
>> >> drivers/gpu/vga/vgaarb.c | 2 +-
>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>> >> index f17cb04..d73b85b 100644
>> >> --- a/drivers/gpu/vga/vgaarb.c
>> >> +++ b/drivers/gpu/vga/vgaarb.c
>> >> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>> >> int i;
>> >>
>> >>
>> >> - kbuf = kmalloc(count + 1, GFP_KERNEL);
>> >> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
>> >
>> > I don't really see why it does this user controlled malloc in the
>> > first place. The max legth of the string it will actually handle looks
>> > well bounded, so it could just use some fixed length buffer (on stack
>> > even).
>>
>>
>> What would be the right limit on data len?
>
> From the looks of things the longest command could be the
> "target PCI:domain:bus:dev.fn" thing. Even assuming something silly like
> having 10 characters for each domain,bus,dev,fn that would still be only
> 55 bytes. So based on that even something like 64 bytes should be more
> than enough AFAICS.
David, what do you think? I can allocate char kbuf[64] on stack.
> The other thing that strikes me as bit odd in this code is that it
> just ignores whatever data is left over after it's done parsing the
> string. But it returns the full count to userspace, indicating it
> ate all of it. I guess that's fairly sane when userspace just uses a
> fixed size buffer and checks that the kernel consumed it all. But
> maybe there should be an actual check to see that there's a '\0'
> or maybe <any amount of whitespace>+'\0' after the parsed string.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
2016-02-04 18:31 ` Dmitry Vyukov
@ 2016-02-04 21:13 ` Dave Airlie
0 siblings, 0 replies; 8+ messages in thread
From: Dave Airlie @ 2016-02-04 21:13 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: LKML, dri-devel, Kostya Serebryany, syzkaller,
Alexander Potapenko
On 5 February 2016 at 04:31, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Feb 4, 2016 at 5:59 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Thu, Feb 04, 2016 at 05:37:49PM +0100, Dmitry Vyukov wrote:
>>> On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>> > On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
>>> >> Size of kmalloc() in vga_arb_write() is controlled by user.
>>> >> Too large kmalloc() size triggers WARNING message on console.
>>> >>
>>> >> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
>>> >>
>>> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>> >> ---
>>> >> Example WARNING:
>>> >>
>>> >> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
>>> >> __alloc_pages_nodemask+0x7d2/0x1760()
>>> >> Modules linked in:
>>> >> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
>>> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>> >> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
>>> >> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
>>> >> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
>>> >> Call Trace:
>>> >> [< inline >] __dump_stack lib/dump_stack.c:15
>>> >> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>>> >> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
>>> >> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
>>> >> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
>>> >> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
>>> >> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
>>> >> [< inline >] alloc_pages include/linux/gfp.h:459
>>> >> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
>>> >> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
>>> >> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
>>> >> [< inline >] kmalloc_large include/linux/slab.h:395
>>> >> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
>>> >> [< inline >] kmalloc include/linux/slab.h:468
>>> >> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
>>> >> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
>>> >> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
>>> >> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
>>> >> [< inline >] SYSC_writev fs/read_write.c:919
>>> >> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
>>> >> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
>>> >> arch/x86/entry/entry_64.S:185
>>> >> ---
>>> >> drivers/gpu/vga/vgaarb.c | 2 +-
>>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>>> >> index f17cb04..d73b85b 100644
>>> >> --- a/drivers/gpu/vga/vgaarb.c
>>> >> +++ b/drivers/gpu/vga/vgaarb.c
>>> >> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>>> >> int i;
>>> >>
>>> >>
>>> >> - kbuf = kmalloc(count + 1, GFP_KERNEL);
>>> >> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
>>> >
>>> > I don't really see why it does this user controlled malloc in the
>>> > first place. The max legth of the string it will actually handle looks
>>> > well bounded, so it could just use some fixed length buffer (on stack
>>> > even).
>>>
>>>
>>> What would be the right limit on data len?
>>
>> From the looks of things the longest command could be the
>> "target PCI:domain:bus:dev.fn" thing. Even assuming something silly like
>> having 10 characters for each domain,bus,dev,fn that would still be only
>> 55 bytes. So based on that even something like 64 bytes should be more
>> than enough AFAICS.
>
> David, what do you think? I can allocate char kbuf[64] on stack.
>
>
>> The other thing that strikes me as bit odd in this code is that it
>> just ignores whatever data is left over after it's done parsing the
>> string. But it returns the full count to userspace, indicating it
>> ate all of it. I guess that's fairly sane when userspace just uses a
>> fixed size buffer and checks that the kernel consumed it all. But
>> maybe there should be an actual check to see that there's a '\0'
>> or maybe <any amount of whitespace>+'\0' after the parsed string.
Yeah Ville is probably right, we could just allocate 64 bytes and dump
anything greater than that, and stuck \0 on the end.
Dave.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
@ 2016-02-04 21:13 ` Dave Airlie
0 siblings, 0 replies; 8+ messages in thread
From: Dave Airlie @ 2016-02-04 21:13 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: syzkaller, Daniel Vetter, Dave Airlie, dri-devel,
Kostya Serebryany, Alexander Potapenko, LKML
On 5 February 2016 at 04:31, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Thu, Feb 4, 2016 at 5:59 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Thu, Feb 04, 2016 at 05:37:49PM +0100, Dmitry Vyukov wrote:
>>> On Thu, Feb 4, 2016 at 5:32 PM, Ville Syrjälä
>>> <ville.syrjala@linux.intel.com> wrote:
>>> > On Thu, Feb 04, 2016 at 04:49:49PM +0100, Dmitry Vyukov wrote:
>>> >> Size of kmalloc() in vga_arb_write() is controlled by user.
>>> >> Too large kmalloc() size triggers WARNING message on console.
>>> >>
>>> >> Use GFP_USER | __GFP_NOWARN for this kmalloc() to not scare admins.
>>> >>
>>> >> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>>> >> ---
>>> >> Example WARNING:
>>> >>
>>> >> WARNING: CPU: 2 PID: 29322 at mm/page_alloc.c:2999
>>> >> __alloc_pages_nodemask+0x7d2/0x1760()
>>> >> Modules linked in:
>>> >> CPU: 2 PID: 29322 Comm: syz-executor Tainted: G B 4.5.0-rc1+ #283
>>> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>> >> 00000000ffffffff ffff880069eff670 ffffffff8299a06d 0000000000000000
>>> >> ffff8800658a4740 ffffffff864985a0 ffff880069eff6b0 ffffffff8134fcf9
>>> >> ffffffff8166de32 ffffffff864985a0 0000000000000bb7 00000000024040c0
>>> >> Call Trace:
>>> >> [< inline >] __dump_stack lib/dump_stack.c:15
>>> >> [<ffffffff8299a06d>] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>>> >> [<ffffffff8134fcf9>] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
>>> >> [<ffffffff8134ff29>] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
>>> >> [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
>>> >> [<ffffffff8166de32>] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
>>> >> [<ffffffff81745c99>] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
>>> >> [< inline >] alloc_pages include/linux/gfp.h:459
>>> >> [<ffffffff81669bb6>] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
>>> >> [<ffffffff816c20af>] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
>>> >> [<ffffffff816c212f>] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
>>> >> [< inline >] kmalloc_large include/linux/slab.h:395
>>> >> [<ffffffff81756b24>] __kmalloc+0x2f4/0x340 mm/slub.c:3557
>>> >> [< inline >] kmalloc include/linux/slab.h:468
>>> >> [<ffffffff832c65a4>] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
>>> >> [<ffffffff817a9831>] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
>>> >> [<ffffffff817ad698>] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
>>> >> [<ffffffff817ad8b6>] vfs_writev+0x86/0xc0 fs/read_write.c:886
>>> >> [< inline >] SYSC_writev fs/read_write.c:919
>>> >> [<ffffffff817b0a21>] SyS_writev+0x111/0x2b0 fs/read_write.c:911
>>> >> [<ffffffff86359636>] entry_SYSCALL_64_fastpath+0x16/0x7a
>>> >> arch/x86/entry/entry_64.S:185
>>> >> ---
>>> >> drivers/gpu/vga/vgaarb.c | 2 +-
>>> >> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
>>> >> index f17cb04..d73b85b 100644
>>> >> --- a/drivers/gpu/vga/vgaarb.c
>>> >> +++ b/drivers/gpu/vga/vgaarb.c
>>> >> @@ -923,7 +923,7 @@ static ssize_t vga_arb_write(struct file *file, const char __user *buf,
>>> >> int i;
>>> >>
>>> >>
>>> >> - kbuf = kmalloc(count + 1, GFP_KERNEL);
>>> >> + kbuf = kmalloc(count + 1, GFP_USER | __GFP_NOWARN);
>>> >
>>> > I don't really see why it does this user controlled malloc in the
>>> > first place. The max legth of the string it will actually handle looks
>>> > well bounded, so it could just use some fixed length buffer (on stack
>>> > even).
>>>
>>>
>>> What would be the right limit on data len?
>>
>> From the looks of things the longest command could be the
>> "target PCI:domain:bus:dev.fn" thing. Even assuming something silly like
>> having 10 characters for each domain,bus,dev,fn that would still be only
>> 55 bytes. So based on that even something like 64 bytes should be more
>> than enough AFAICS.
>
> David, what do you think? I can allocate char kbuf[64] on stack.
>
>
>> The other thing that strikes me as bit odd in this code is that it
>> just ignores whatever data is left over after it's done parsing the
>> string. But it returns the full count to userspace, indicating it
>> ate all of it. I guess that's fairly sane when userspace just uses a
>> fixed size buffer and checks that the kernel consumed it all. But
>> maybe there should be an actual check to see that there's a '\0'
>> or maybe <any amount of whitespace>+'\0' after the parsed string.
Yeah Ville is probably right, we could just allocate 64 bytes and dump
anything greater than that, and stuck \0 on the end.
Dave.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-04 21:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-04 15:49 [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc Dmitry Vyukov
2016-02-04 16:32 ` Ville Syrjälä
2016-02-04 16:37 ` Dmitry Vyukov
2016-02-04 16:59 ` Ville Syrjälä
2016-02-04 16:59 ` Ville Syrjälä
2016-02-04 18:31 ` Dmitry Vyukov
2016-02-04 21:13 ` Dave Airlie
2016-02-04 21:13 ` Dave Airlie
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.