Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Dmitry Vyukov <dvyukov@google.com>
Cc: daniel@ffwll.ch, airlied@linux.ie,
	dri-devel@lists.freedesktop.org, kcc@google.com,
	syzkaller@googlegroups.com, glider@google.com,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers/gpu/vga: use __GFP_NOWARN for user-controlled kmalloc
Date: Thu, 4 Feb 2016 18:32:44 +0200	[thread overview]
Message-ID: <20160204163244.GQ23290@intel.com> (raw)
In-Reply-To: <1454600989-123018-1-git-send-email-dvyukov@google.com>

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