gpu: vga: limit kmalloc'ed memory size

PATCH In review Vasiliy Kulikov 2 participants 5 messages Nov 22, 2010 – Nov 24, 2010

Subsystems (3)

DRM drivers DRM misc Kernel janitors

lore

Share & tools

lore

Vasiliy Kulikov

author

2010-11-22 17:11 UTC #0 lore raw

PATCH v2 1/1 gpu: vga: limit kmalloc'ed memory size

count is not checked before kmalloc() call. Too big value would generate stack dump. To prevent this limit 'count' maximum value. 1024 looks OK - the data should be the string of tens of bytes.

Signed-off-by Vasiliy Kulikov <[email protected]>

---
Compile tested only.

v1 had incorrect comment text, as Dan Rosenberg noticed.

 drivers/gpu/vga/vgaarb.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/gpu/vga/vgaarb.c +2 -0

index c380c65..09e3090 100644
@@ -836,6 +836,8 @@ static ssize_t vga_arb_write(struct file *file, const char __user * buf,
 	int ret_val;
 	int i;
 
+	if (count > 1024)
+		count = 1024;
 
 	kbuf = kmalloc(count + 1, GFP_KERNEL);
 	if (!kbuf)

Signature · 1 line

1.7.0.4

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?

The code should be using strndup_user() anyway. And perhaps strndup_user() needs __GFP_NOWARN treatment.

Vasiliy Kulikov

author

2010-11-23 19:08 UTC #2 lore raw

Quoted 4 lines · Andrew Morton

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]  [<ffffffff8106079f>] warn_slowpath_common+0x7f/0xc0
[13896.002059]  [<ffffffff810607fa>] warn_slowpath_null+0x1a/0x20
[13896.002063]  [<ffffffff811081bb>] __alloc_pages_slowpath+0x44b/0x590
[13896.002067]  [<ffffffff8112aaae>] ? lazy_max_pages+0x1e/0x30
[13896.002071]  [<ffffffff81104f74>] ? __probe_kernel_write+0x44/0x70
[13896.002075]  [<ffffffff8110849a>] __alloc_pages_nodemask+0x19a/0x1f0
[13896.002080]  [<ffffffff8113a30a>] alloc_pages_current+0x9a/0x100
[13896.002084]  [<ffffffffa01cb020>] ? hello_init+0x0/0xa0 [test]
[13896.002088]  [<ffffffff8110736e>] __get_free_pages+0xe/0x50
[13896.002091]  [<ffffffffa01cb04f>] hello_init+0x2f/0xa0 [test]
[13896.002095]  [<ffffffffa01cb020>] ? hello_init+0x0/0xa0 [test]
[13896.002099]  [<ffffffff8100204c>] do_one_initcall+0x3c/0x1a0
[13896.002104]  [<ffffffff8109bceb>] sys_init_module+0xbb/0x200
[13896.002109]  [<ffffffff8100a0f2>] system_call_fastpath+0x16/0x1b
[13896.002112] ---[ end trace b991655c592a1d6e ]---
[13896.002114] p = (null)

Quoted 2 lines

And perhaps
strndup_user() needs __GFP_NOWARN treatment.

strndup_user() silently return ERR_PTR(-EINVAL) if string is too long.

Quoted 1 line

The code should be using strndup_user() anyway.

Smth like this?

drivers/gpu/vga/vgaarb.c +3 ~~-12~~

index 09e3090..3afd249 100644
@@ -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;

Signature · 2 lines

Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments

Andrew Morton

2010-11-23 20:46 UTC #3 lore raw

Quoted 26 lines · On Tue, 23 Nov 2010 22:08:28 +0300 Vasiliy Kulikov <[email protected]> wrote:

Quoted 2 lines

And perhaps
strndup_user() needs __GFP_NOWARN treatment.

strndup_user() silently return ERR_PTR(-EINVAL) if string is too long.

Quoted 1 line

The code should be using strndup_user() anyway.

Smth like this?

drivers/gpu/vga/vgaarb.c +0 -9

index 09e3090..3afd249 100644
@@ -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);

drivers/gpu/vga/vgaarb.c static ssize_t vga_arb_write(struct file *file, const char __user * buf, · line 845

Quoted 10 lines

drivers/gpu/vga/vgaarb.c +3 -3

-               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;

What I'm suggesting is that we simply do

	kbuf = strndup_user(buf, count);

and make strndup_user() do the right thing if `count' turned out to be crazy large. THis way we don't have to sprinkle decisions about "crazy largeness" all over the kernel.

And the way in which I suggest that strndup_user() decides whether the length is too great is to try to kmalloc that amount of memory. If it succeeds then fine, proceed. If it fails then return an error, probably ENOMEM. And that attempt to invoke kmalloc() shouldn't spew a warning.

Vasiliy Kulikov

author

2010-11-24 18:33 UTC #4 lore raw

Andrew,

Quoted 11 lines · Andrew Morton

What I'm suggesting is that we simply do
	kbuf = strndup_user(buf, count);
and make strndup_user() do the right thing if `count' turned out to be crazy large. THis way we don't have to sprinkle decisions about "crazy largeness" all over the kernel.
And the way in which I suggest that strndup_user() decides whether the length is too great is to try to kmalloc that amount of memory. If it succeeds then fine, proceed.

I don't think that it is a good idea - the process would have an ability to allocate too much system memory bypassing any limits. Assuming that the kernel would only double the memory is not right - even if the process is limited in physical memory it may pass address of e.g. mapped file.

Also this specific driver is happy with very low limit of copied string.

Quoted 2 lines

If it fails then return an error,
probably ENOMEM.

It is already done in strndup_user().

Quoted 2 lines

And that attempt to invoke kmalloc() shouldn't spew a warning.

It is not obvious for me to change strndup_user's behaviour, I'm not familiar with this code.

Signature · 2 lines

Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments