subject:"\[PATCH v2\] drivers\/gpu\/vga\: allocate vga_arb

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

2016-11-15 Thread Daniel Vetter

On Mon, Nov 14, 2016 at 10:11:47AM +0100, Dmitry Vyukov wrote:
> On Fri, Oct 14, 2016 at 3:22 PM, Dmitry Vyukov  wrote:
> > Size of kmalloc() in vga_arb_write() is controlled by user.
> > Too large kmalloc() size triggers WARNING message on console.
> > Allocate the buffer on stack to avoid the WARNING.
> > The string must be small (e.g "target PCI:domain:bus:dev.fn").
> >
> > Signed-off-by: Dmitry Vyukov 
> > Reviewed-by: Ville SyrjÃ¤lÃ¤ 
> > Cc: Dave Airlie 
> > Cc: Ville SyrjÃ¤lÃ¤ 
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: syzkaller at googlegroups.com
> 
> Ping, this is still not merged.
> David, please take this to dri tree.

It's applied to drm-misc already.
-Daniel

> 
> 
> > ---
> >
> > Changes since v1:
> >  - removed another kfree(kbuf)
> >
> > I've sent a patch that adds __GFP_NOWARN to the kmalloc a while ago:
> > https://groups.google.com/forum/#!msg/syzkaller/navdAwe4iCo/mZDhODsnAAAJ
> >
> > Ville suggested to allocate the buffer on stack as it must be small:
> >
> > "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. "
> >
> > 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: GB  4.5.0-rc1+ #283
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
> >   880069eff670 8299a06d 
> >  8800658a4740 864985a0 880069eff6b0 8134fcf9
> >  8166de32 864985a0 0bb7 024040c0
> > Call Trace:
> >  [< inline >] __dump_stack lib/dump_stack.c:15
> >  [] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
> >  [] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
> >  [] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
> >  [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
> >  [] __alloc_pages_nodemask+0x7d2/0x1760 
> > mm/page_alloc.c:3253
> >  [] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
> >  [< inline >] alloc_pages include/linux/gfp.h:459
> >  [] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
> >  [] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
> >  [] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
> >  [< inline >] kmalloc_large include/linux/slab.h:395
> >  [] __kmalloc+0x2f4/0x340 mm/slub.c:3557
> >  [< inline >] kmalloc include/linux/slab.h:468
> >  [] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
> >  [] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
> >  [] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
> >  [] vfs_writev+0x86/0xc0 fs/read_write.c:886
> >  [< inline >] SYSC_writev fs/read_write.c:919
> >  [] SyS_writev+0x111/0x2b0 fs/read_write.c:911
> >  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> > arch/x86/entry/entry_64.S:185
> > ---
> >  drivers/gpu/vga/vgaarb.c | 15 ---
> >  1 file changed, 4 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> > index 1887f19..77657a8 100644
> > --- a/drivers/gpu/vga/vgaarb.c
> > +++ b/drivers/gpu/vga/vgaarb.c
> > @@ -1022,21 +1022,16 @@ static ssize_t vga_arb_write(struct file *file, 
> > const char __user *buf,
> >
> > unsigned int io_state;
> >
> > -   char *kbuf, *curr_pos;
> > +   char kbuf[64], *curr_pos;
> > size_t remaining = count;
> >
> > int ret_val;
> > int i;
> >
> > -
> > -   kbuf = kmalloc(count + 1, GFP_KERNEL);
> > -   if (!kbuf)
> > -   return -ENOMEM;
> > -
> > -   if (copy_from_user(kbuf, buf, count)) {
> > -   kfree(kbuf);
> > +   if (count >= sizeof(kbuf))
> > +   return -EINVAL;
> > +   if (copy_from_user(kbuf, buf, count))
> > return -EFAULT;
> > -   }
> > curr_pos = kbuf;
> > kbuf[count] = '\0'; /* Just to make sure... */
> >
> > @@ -1259,11 +1254,9 @@ static ssize_t vga_arb_write(struct file *file, 
> > const char __user *buf,
> > goto done;
> > }
> > /* If we got here, the message written is not part of the protocol! 
> > */
> > -   kfree(kbuf);
> > return -EPROTO;
> >
> >  done:
> > -   kfree(kbuf);
> > return ret_val;
> >  }
> >
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

2016-11-14 Thread Dmitry Vyukov

On Fri, Oct 14, 2016 at 3:22 PM, Dmitry Vyukov  wrote:
> Size of kmalloc() in vga_arb_write() is controlled by user.
> Too large kmalloc() size triggers WARNING message on console.
> Allocate the buffer on stack to avoid the WARNING.
> The string must be small (e.g "target PCI:domain:bus:dev.fn").
>
> Signed-off-by: Dmitry Vyukov 
> Reviewed-by: Ville SyrjÃ¤lÃ¤ 
> Cc: Dave Airlie 
> Cc: Ville SyrjÃ¤lÃ¤ 
> Cc: dri-devel at lists.freedesktop.org
> Cc: syzkaller at googlegroups.com

Ping, this is still not merged.
David, please take this to dri tree.


> ---
>
> Changes since v1:
>  - removed another kfree(kbuf)
>
> I've sent a patch that adds __GFP_NOWARN to the kmalloc a while ago:
> https://groups.google.com/forum/#!msg/syzkaller/navdAwe4iCo/mZDhODsnAAAJ
>
> Ville suggested to allocate the buffer on stack as it must be small:
>
> "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. "
>
> 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: GB  4.5.0-rc1+ #283
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>   880069eff670 8299a06d 
>  8800658a4740 864985a0 880069eff6b0 8134fcf9
>  8166de32 864985a0 0bb7 024040c0
> Call Trace:
>  [< inline >] __dump_stack lib/dump_stack.c:15
>  [] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
>  [] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
>  [] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
>  [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
>  [] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
>  [] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
>  [< inline >] alloc_pages include/linux/gfp.h:459
>  [] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
>  [] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
>  [] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
>  [< inline >] kmalloc_large include/linux/slab.h:395
>  [] __kmalloc+0x2f4/0x340 mm/slub.c:3557
>  [< inline >] kmalloc include/linux/slab.h:468
>  [] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
>  [] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
>  [] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
>  [] vfs_writev+0x86/0xc0 fs/read_write.c:886
>  [< inline >] SYSC_writev fs/read_write.c:919
>  [] SyS_writev+0x111/0x2b0 fs/read_write.c:911
>  [] entry_SYSCALL_64_fastpath+0x16/0x7a
> arch/x86/entry/entry_64.S:185
> ---
>  drivers/gpu/vga/vgaarb.c | 15 ---
>  1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
> index 1887f19..77657a8 100644
> --- a/drivers/gpu/vga/vgaarb.c
> +++ b/drivers/gpu/vga/vgaarb.c
> @@ -1022,21 +1022,16 @@ static ssize_t vga_arb_write(struct file *file, const 
> char __user *buf,
>
> unsigned int io_state;
>
> -   char *kbuf, *curr_pos;
> +   char kbuf[64], *curr_pos;
> size_t remaining = count;
>
> int ret_val;
> int i;
>
> -
> -   kbuf = kmalloc(count + 1, GFP_KERNEL);
> -   if (!kbuf)
> -   return -ENOMEM;
> -
> -   if (copy_from_user(kbuf, buf, count)) {
> -   kfree(kbuf);
> +   if (count >= sizeof(kbuf))
> +   return -EINVAL;
> +   if (copy_from_user(kbuf, buf, count))
> return -EFAULT;
> -   }
> curr_pos = kbuf;
> kbuf[count] = '\0'; /* Just to make sure... */
>
> @@ -1259,11 +1254,9 @@ static ssize_t vga_arb_write(struct file *file, const 
> char __user *buf,
> goto done;
> }
> /* If we got here, the message written is not part of the protocol! */
> -   kfree(kbuf);
> return -EPROTO;
>
>  done:
> -   kfree(kbuf);
> return ret_val;
>  }
>
> --
> 2.8.0.rc3.226.g39d4020
>

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

2016-10-14 Thread Dmitry Vyukov

Size of kmalloc() in vga_arb_write() is controlled by user.
Too large kmalloc() size triggers WARNING message on console.
Allocate the buffer on stack to avoid the WARNING.
The string must be small (e.g "target PCI:domain:bus:dev.fn").

Signed-off-by: Dmitry Vyukov 
Reviewed-by: Ville SyrjÃ¤lÃ¤ 
Cc: Dave Airlie 
Cc: Ville SyrjÃ¤lÃ¤ 
Cc: dri-devel at lists.freedesktop.org
Cc: syzkaller at googlegroups.com

---

Changes since v1:
 - removed another kfree(kbuf)

I've sent a patch that adds __GFP_NOWARN to the kmalloc a while ago:
https://groups.google.com/forum/#!msg/syzkaller/navdAwe4iCo/mZDhODsnAAAJ

Ville suggested to allocate the buffer on stack as it must be small:

"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. "

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: GB  4.5.0-rc1+ #283
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  880069eff670 8299a06d 
 8800658a4740 864985a0 880069eff6b0 8134fcf9
 8166de32 864985a0 0bb7 024040c0
Call Trace:
 [< inline >] __dump_stack lib/dump_stack.c:15
 [] dump_stack+0x6f/0xa2 lib/dump_stack.c:50
 [] warn_slowpath_common+0xd9/0x140 kernel/panic.c:482
 [] warn_slowpath_null+0x29/0x30 kernel/panic.c:515
 [< inline >] __alloc_pages_slowpath mm/page_alloc.c:2999
 [] __alloc_pages_nodemask+0x7d2/0x1760 mm/page_alloc.c:3253
 [] alloc_pages_current+0xe9/0x450 mm/mempolicy.c:2090
 [< inline >] alloc_pages include/linux/gfp.h:459
 [] alloc_kmem_pages+0x16/0x100 mm/page_alloc.c:3433
 [] kmalloc_order+0x1f/0x80 mm/slab_common.c:1008
 [] kmalloc_order_trace+0x1f/0x140 mm/slab_common.c:1019
 [< inline >] kmalloc_large include/linux/slab.h:395
 [] __kmalloc+0x2f4/0x340 mm/slub.c:3557
 [< inline >] kmalloc include/linux/slab.h:468
 [] vga_arb_write+0xd4/0xe40 drivers/gpu/vga/vgaarb.c:926
 [] do_loop_readv_writev+0x141/0x1e0 fs/read_write.c:719
 [] do_readv_writev+0x5f8/0x6e0 fs/read_write.c:849
 [] vfs_writev+0x86/0xc0 fs/read_write.c:886
 [< inline >] SYSC_writev fs/read_write.c:919
 [] SyS_writev+0x111/0x2b0 fs/read_write.c:911
 [] entry_SYSCALL_64_fastpath+0x16/0x7a
arch/x86/entry/entry_64.S:185
---
 drivers/gpu/vga/vgaarb.c | 15 ---
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c
index 1887f19..77657a8 100644
--- a/drivers/gpu/vga/vgaarb.c
+++ b/drivers/gpu/vga/vgaarb.c
@@ -1022,21 +1022,16 @@ static ssize_t vga_arb_write(struct file *file, const 
char __user *buf,

unsigned int io_state;

-   char *kbuf, *curr_pos;
+   char kbuf[64], *curr_pos;
size_t remaining = count;

int ret_val;
int i;

-
-   kbuf = kmalloc(count + 1, GFP_KERNEL);
-   if (!kbuf)
-   return -ENOMEM;
-
-   if (copy_from_user(kbuf, buf, count)) {
-   kfree(kbuf);
+   if (count >= sizeof(kbuf))
+   return -EINVAL;
+   if (copy_from_user(kbuf, buf, count))
return -EFAULT;
-   }
curr_pos = kbuf;
kbuf[count] = '\0'; /* Just to make sure... */

@@ -1259,11 +1254,9 @@ static ssize_t vga_arb_write(struct file *file, const 
char __user *buf,
goto done;
}
/* If we got here, the message written is not part of the protocol! */
-   kfree(kbuf);
return -EPROTO;

 done:
-   kfree(kbuf);
return ret_val;
 }

-- 
2.8.0.rc3.226.g39d4020

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

[PATCH v2] drivers/gpu/vga: allocate vga_arb_write() buffer on stack

3 matches

Site Navigation

Mail list logo

Footer information