From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasiliy Kulikov Date: Tue, 23 Nov 2010 19:08:28 +0000 Subject: Re: [PATCH v2] gpu: vga: limit kmalloc'ed memory size Message-Id: <20101123190828.GA27159@albatros> List-Id: References: <1290445864-13657-1-git-send-email-segoon@openwall.com> <20101122100915.5bf966fe.akpm@linux-foundation.org> In-Reply-To: <20101122100915.5bf966fe.akpm@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Andrew Morton Cc: kernel-janitors@vger.kernel.org, Dave Airlie , Tiago Vignatti , Mike Travis , "H. Peter Anvin" , linux-kernel@vger.kernel.org On Mon, Nov 22, 2010 at 10:09 -0800, Andrew Morton wrote: > Arguably we should just let the allocation attempt pass through to > kmalloc() and let kmalloc() return an error if it was too large. Possibly > we need a __GFP_NOWARN in there somewhere. Please send us that stack > dump? void *p = kmalloc(5L*1024*1024, GFP_KERNEL); pr_err("p = %p\n", p); [13896.001970] ------------[ cut here ]------------ [13896.001979] WARNING: at /build/buildd/linux-lts-backport-maverick-2.6.35/mm/page_alloc.c:1968 __alloc_pages_slowpath+0x44b/0x590() [13896.001983] Hardware name: Q310 [13896.001984] Modules linked in: test(+) easy_slow_down_manager rfcomm sco bridge stp bnep l2cap vboxnetadp vboxnetflt vboxdrv nfsd exportfs nfs lockd fscache nfs_acl auth_rpcgss sunrpc samsung_backlight binfmt_misc ppdev input_polldev lp parport snd_seq_dummy uvcvideo videodev v4l1_compat v4l2_compat_ioctl32 iTCO_vendor_support dm_crypt usb_storage aes_x86_64 aes_generic vfat fat nls_cp437 nls_iso8859_1 usblp joydev arc4 btusb bluetooth snd_hda_intel snd_hda_codec snd_hwdep snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq snd_timer snd_seq_device nouveau ath5k ttm mac80211 drm_kms_helper ath snd cfg80211 video output drm soundcore led_class psmouse snd_page_alloc sky2 serio_raw i2c_algo_bit intel_agp [last unloaded: test] [13896.002045] Pid: 27627, comm: insmod Not tainted 2.6.35-22-generic #34~lucid1-Ubuntu [13896.002047] Call Trace: [13896.002055] [] warn_slowpath_common+0x7f/0xc0 [13896.002059] [] warn_slowpath_null+0x1a/0x20 [13896.002063] [] __alloc_pages_slowpath+0x44b/0x590 [13896.002067] [] ? lazy_max_pages+0x1e/0x30 [13896.002071] [] ? __probe_kernel_write+0x44/0x70 [13896.002075] [] __alloc_pages_nodemask+0x19a/0x1f0 [13896.002080] [] alloc_pages_current+0x9a/0x100 [13896.002084] [] ? hello_init+0x0/0xa0 [test] [13896.002088] [] __get_free_pages+0xe/0x50 [13896.002091] [] hello_init+0x2f/0xa0 [test] [13896.002095] [] ? hello_init+0x0/0xa0 [test] [13896.002099] [] do_one_initcall+0x3c/0x1a0 [13896.002104] [] sys_init_module+0xbb/0x200 [13896.002109] [] system_call_fastpath+0x16/0x1b [13896.002112] ---[ end trace b991655c592a1d6e ]--- [13896.002114] p = (null) > And perhaps > strndup_user() needs __GFP_NOWARN treatment. strndup_user() silently return ERR_PTR(-EINVAL) if string is too long. > The code should be using strndup_user() anyway. Smth like this? diff --git a/drivers/gpu/vga/vgaarb.c b/drivers/gpu/vga/vgaarb.c index 09e3090..3afd249 100644 --- a/drivers/gpu/vga/vgaarb.c +++ b/drivers/gpu/vga/vgaarb.c @@ -836,19 +836,10 @@ static ssize_t vga_arb_write(struct file *file, const char __user * buf, int ret_val; int i; - if (count > PAGE_SIZE) - count = PAGE_SIZE; - - kbuf = kmalloc(count + 1, GFP_KERNEL); - if (!kbuf) - return -ENOMEM; - - if (copy_from_user(kbuf, buf, count)) { - kfree(kbuf); - return -EFAULT; - } + kbuf = strndup_user(buf, PAGE_SIZE); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); curr_pos = kbuf; - kbuf[count] = '\0'; /* Just to make sure... */ if (strncmp(curr_pos, "lock ", 5) = 0) { curr_pos += 5; -- Vasiliy Kulikov http://www.openwall.com - bringing security into open computing environments