PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[Robert T. Johnson]

Since sprite is a user pointer, reading sprite->mask or sprite->image.data 
requires unsafe dereferences.  Let me know if you have any questions or if 
I've made a mistake.

Best,
Rob

--- linux-2.6.7-rc3-full/drivers/video/fbmem.c.orig	Wed Jun  9 13:09:30 2004
+++ linux-2.6.7-rc3-full/drivers/video/fbmem.c	Wed Jun  9 13:07:06 2004
@@ -901,7 +901,9 @@ fb_cursor(struct fb_info *info, struct f
 {
 	struct fb_cursor cursor;
 	int err;
-	
+	char *mask;
+	const char *image_data;
+
 	if (copy_from_user(&cursor, sprite, sizeof(struct fb_cursor)))
 		return -EFAULT;
 
@@ -920,18 +922,20 @@ fb_cursor(struct fb_info *info, struct f
 		    (cursor.image.width != info->cursor.image.width))
 			cursor.set |= FB_CUR_SETSIZE;
 		
+		image_data = cursor.image.data;
 		cursor.image.data = kmalloc(size, GFP_KERNEL);
 		if (!cursor.image.data)
 			return -ENOMEM;
 		
+		mask = cursor.mask;
 		cursor.mask = kmalloc(size, GFP_KERNEL);
 		if (!cursor.mask) {
 			kfree(cursor.image.data);
 			return -ENOMEM;
 		}
 		
-		if (copy_from_user(cursor.image.data, sprite->image.data, size) ||
-		    copy_from_user(cursor.mask, sprite->mask, size)) { 
+		if (copy_from_user(cursor.image.data, image_data, size) ||
+		    copy_from_user(cursor.mask, mask, size)) { 
 			kfree(cursor.image.data);
 			kfree(cursor.mask);
 			return -EFAULT;

Re: PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[viro]

On Wed, Jun 09, 2004 at 03:46:39PM -0700, Robert T. Johnson wrote:
> Since sprite is a user pointer, reading sprite->mask or sprite->image.data 
> requires unsafe dereferences.  Let me know if you have any questions or if 
> I've made a mistake.

Nice catch.  IMO drivers/video/* is too damn scary to get in there and look
for now - as long as there are less nasty areas to deal with ;-)

BTW, had con_font_op() shown up in your checks?  It _does_ avoid dereferencing
userland pointers in a very similar scenario, but proving that is not something
I'd wish on any code.  Short version of the story:
	* console_font_op ->data can be a userland pointer.  It is obtained
from ioctls on many paths; all end up passing the (kernel) pointer to
structure to con_font_op().
	* a lot of code in drivers uses struct console_font_op.  And
dereferences ->data.
	* con_font_op() checks op->op and if it is KD_FONT_OP_[SG]ET, op->data
gets moved to kernel.  In any case, op is passed to the same method -
->con_font_op().  In some cases - with kernel pointer in ->data, in some -
with userland one.
	* ->con_font_op() in drivers does not dereference op->data unless
op->op is one of those two.

Oh, and to make life even funnier, struct console_font_op is also misused to
store current font.

I wonder what did cqual say about that one - it sure as hell should raise a lot
of red flags and the last item (none of the drivers dereference ->data unless
->op is one of KD_FONT_OP_{S,G}ET) is going to be hell on anything short of
full AI.  sparse does *not* figure out that it's safe and raises hell over
->data not being declared as userland pointer while getting copy_..._user()
on it.

	FWIW, I think we should reduxe mixing of ioctl and kernel structures.
console_font_op is a particulary obnoxious example, but lots of the stuff
in drivers/video is not much better.


-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite!  GUADEC: The world's #1 Open Source Desktop Event.
GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org

Re: PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[Robert T. Johnson]

On Wed, 2004-06-09 at 18:24, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Wed, Jun 09, 2004 at 03:46:39PM -0700, Robert T. Johnson wrote:
> > Since sprite is a user pointer, reading sprite->mask or sprite->image.data 
> > requires unsafe dereferences.  Let me know if you have any questions or if 
> > I've made a mistake.
> 
> Nice catch.  IMO drivers/video/* is too damn scary to get in there and look
> for now - as long as there are less nasty areas to deal with ;-)
> 
> BTW, had con_font_op() shown up in your checks?  It _does_ avoid dereferencing
> userland pointers in a very similar scenario, but proving that is not something
> I'd wish on any code.  Short version of the story:

Yep.  I remember that code.  CQual does complain about it.

A similar situation arises in tty code, which passes around a flag
"from_user" to indicate whether another pointer is a user pointer or
kernel pointer.  See, for example, drivers/char/pty.c:pty_write(). 
CQual complains about that, too.  How about sparse?

We describe two ways of fixing this kind of code in the "Working around
false positives" section of that paper.  The simpler workaround will
probably work for sparse.  Basically rewrite the function

static int pty_write(struct tty_struct * tty, int from_user,
		       const unsigned char *buf, int count);

as

static int pty_write(struct tty_struct * tty, int from_user,
		       const unsigned char __user   *ubuf, 
                       const unsigned char __kernel *kbuf,
                       int count)

(In this case, you can even eliminate the from_user flag by just
checking which of ubuf and kbuf is non-NULL).

The slicker work-around involves passing a function pointer for
accessing the buffer, like in the example I gave in the other email. 
This works for cqual, but probably wouldn't help sparse.

> I wonder what did cqual say about that one - it sure as hell should raise a lot
> of red flags and the last item (none of the drivers dereference ->data unless
> ->op is one of KD_FONT_OP_{S,G}ET) is going to be hell on anything short of
> full AI.  sparse does *not* figure out that it's safe and raises hell over
> ->data not being declared as userland pointer while getting copy_..._user()
> on it.

I agree it would be difficult.  I have some vague ideas how to prove
this safe automatically, but nothing I plan to implement any time soon.

> 	FWIW, I think we should reduxe mixing of ioctl and kernel structures.
> console_font_op is a particulary obnoxious example, but lots of the stuff
> in drivers/video is not much better.

I think the key point is to make pointers either be always user pointers
or always kernel pointers.  Their type shouldn't depend on some other
flag.

I fear that completely separating ioctl and kernel data structures would
result in lots of redundant structure definitions, which will lead to
code maintainence problems and their own host of bugs.  Would it be
better to just design a bug finding tool that's capable of keeping track
of different structure instances separately?

Best,
Rob




-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite!  GUADEC: The world's #1 Open Source Desktop Event.
GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org

Re: PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[viro]

On Wed, Jun 09, 2004 at 08:50:33PM -0700, Robert T. Johnson wrote:
> A similar situation arises in tty code, which passes around a flag
> "from_user" to indicate whether another pointer is a user pointer or
> kernel pointer.  See, for example, drivers/char/pty.c:pty_write(). 
> CQual complains about that, too.  How about sparse?

Of course - we have the same pointer passed both to memcpy() and
copy_from_user().  Since they expect different address spaces for
their arguments, no annotaion will avoid tripping a warning.

> static int pty_write(struct tty_struct * tty, int from_user,
> 		       const unsigned char __user   *ubuf, 
>                        const unsigned char __kernel *kbuf,
>                        int count)

Eeek.  You do realize that it doesn't fix the real problem, don't you?
Paper over it, but that's it - and we are trading one constraint for
another ("at most one of those two arguments is non-NULL") and _still_
have locking implications from hell.

That warning is actually a sign of real problem - it is not a false
positive in a sense that tty_driver ->write() is a broken API and
is ripe with e.g. locking bugs.

> The slicker work-around involves passing a function pointer for
> accessing the buffer, like in the example I gave in the other email. 
> This works for cqual, but probably wouldn't help sparse.

<shrug> just pass opaque data around as unsigned long.  Ugly, but will
work.  And no, I'm not happy about the idea of doing polymorphic types -
C is not particulary well suited for that.  It's still papering over
the problem instead of fixing it.

In case of tty ->write() the real question is different - if you take
a look at the actual instances of that method, you'll see that they
have serious code duplication between the kernel/userland cases (not
a surprise) and the only difference besides the way we copy is in the
locking done on these paths.

It's actually worth investigating - I'm fairly sure that we'll find
	a) shitloads of SMP problems on the "kernel" side of things (aka. "we
don't need no stinkin' locking, memcpy won't block")
	b) very distinct possibility of making the locking mechanism used
on "userland" path generic and driven by n_tty.c - along with copying from
userland to kernel.  Note that n_tty.c is the only caller of ->write() with
userland pointers; everything else is kernel-only.

So I suspect that it in the long run the proper fix will be to sanitize
the locking and move all copy_from_user() to the (only) caller.

> > 	FWIW, I think we should reduxe mixing of ioctl and kernel structures.
> > console_font_op is a particulary obnoxious example, but lots of the stuff
> > in drivers/video is not much better.
> 
> I think the key point is to make pointers either be always user pointers
> or always kernel pointers.  Their type shouldn't depend on some other
> flag.
> 
> I fear that completely separating ioctl and kernel data structures would
> result in lots of redundant structure definitions, which will lead to
> code maintainence problems and their own host of bugs.  Would it be
> better to just design a bug finding tool that's capable of keeping track
> of different structure instances separately?

I doubt it.  Most of the ioctl data structures do not survive past the
decoding; fb layer is ugly that way, but that's a local problem and it
can be fixed.

Keep in mind that anything containing userland pointers needs to be explicitly
dealt with on 32/64 platforms - otherwise 32bit code won't be able to issue
that ioctl anyway.

	Besides, kernel data structures should not be tied by ABI stability
requirements - and ioctl arguments have to.  Which leads to far worse bug
potential than explict decoding.


-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite!  GUADEC: The world's #1 Open Source Desktop Event.
GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org

Re: PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[Robert T. Johnson]

On Wed, 2004-06-09 at 21:15, viro@parcelfarce.linux.theplanet.co.uk
wrote:
> On Wed, Jun 09, 2004 at 08:50:33PM -0700, Robert T. Johnson wrote:
> > static int pty_write(struct tty_struct * tty, int from_user,
> > 		       const unsigned char __user   *ubuf, 
> >                        const unsigned char __kernel *kbuf,
> >                        int count)
> 
> So I suspect that it in the long run the proper fix will be to sanitize
> the locking and move all copy_from_user() to the (only) caller.

I agree this is the ideal fix.  I can see advantages and disadvantages
to all the approaches.  I'm not familiar with the locking issues, so I
can't comment on that.

> > I fear that completely separating ioctl and kernel data structures would
> > result in lots of redundant structure definitions, which will lead to
> > code maintainence problems and their own host of bugs.  Would it be
> > better to just design a bug finding tool that's capable of keeping track
> > of different structure instances separately?
> 
> I doubt it.  Most of the ioctl data structures do not survive past the
> decoding; fb layer is ugly that way, but that's a local problem and it
> can be fixed.
> 
> Keep in mind that anything containing userland pointers needs to be explicitly
> dealt with on 32/64 platforms - otherwise 32bit code won't be able to issue
> that ioctl anyway.
> 
> 	Besides, kernel data structures should not be tied by ABI stability
> requirements - and ioctl arguments have to.  Which leads to far worse bug
> potential than explict decoding.

These are design issues outside the scope of what I'm doing, but they
are important.  I'll try to keep these considerations in mind as I
continue to improve cqual.  Thanks for the helpful feedback.

Best,
Rob




-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite!  GUADEC: The world's #1 Open Source Desktop Event.
GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org

Re: PATCH: 2.6.7-rc3 drivers/video/fbmem.c: user/kernel pointer bugs

[Geert Uytterhoeven]

On Thu, 9 Jun 2004, Robert T. Johnson wrote:
> Since sprite is a user pointer, reading sprite->mask or sprite->image.data
> requires unsafe dereferences.  Let me know if you have any questions or if
> I've made a mistake.

I sent this one (untested, except for a compile test) to linux-fbdev-devel a
few days ago.

It also makes cursor.mask const, just like image.data.

--- linux-2.6.7-rc1/drivers/video/fbmem.c.orig	2004-05-24 11:38:04.000000000 +0200
+++ linux-2.6.7-rc1/drivers/video/fbmem.c	2004-06-07 22:38:42.000000000 +0200
@@ -916,26 +916,30 @@ fb_cursor(struct fb_info *info, struct f

 	if (cursor.set & FB_CUR_SETSHAPE) {
 		int size = ((cursor.image.width + 7) >> 3) * cursor.image.height;
+		char *data, *mask;
+
 		if ((cursor.image.height != info->cursor.image.height) ||
 		    (cursor.image.width != info->cursor.image.width))
 			cursor.set |= FB_CUR_SETSIZE;

-		cursor.image.data = kmalloc(size, GFP_KERNEL);
-		if (!cursor.image.data)
+		data = kmalloc(size, GFP_KERNEL);
+		if (!data)
 			return -ENOMEM;

-		cursor.mask = kmalloc(size, GFP_KERNEL);
-		if (!cursor.mask) {
-			kfree(cursor.image.data);
+		mask = kmalloc(size, GFP_KERNEL);
+		if (!mask) {
+			kfree(data);
 			return -ENOMEM;
 		}

-		if (copy_from_user(cursor.image.data, sprite->image.data, size) ||
-		    copy_from_user(cursor.mask, sprite->mask, size)) {
-			kfree(cursor.image.data);
-			kfree(cursor.mask);
+		if (copy_from_user(data, sprite->image.data, size) ||
+		    copy_from_user(mask, sprite->mask, size)) {
+			kfree(data);
+			kfree(mask);
 			return -EFAULT;
 		}
+		cursor.image.data = data;
+		cursor.mask = mask;
 	}
 	info->cursor.set = cursor.set;
 	info->cursor.rop = cursor.rop;
--- linux-2.6.7-rc1/include/linux/fb.h.orig	2004-05-24 11:14:01.000000000 +0200
+++ linux-2.6.7-rc1/include/linux/fb.h	2004-06-07 22:38:01.000000000 +0200
@@ -375,7 +375,7 @@ struct fb_cursor {
 	__u16 set;		/* what to set */
 	__u16 enable;		/* cursor on/off */
 	__u16 rop;		/* bitop operation */
-	char *mask;		/* cursor mask bits */
+	const char *mask;	/* cursor mask bits */
 	struct fbcurpos hot;	/* cursor hot spot */
 	struct fb_image	image;	/* Cursor image */
 };

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


-------------------------------------------------------
This SF.Net email is sponsored by: GNOME Foundation
Hackers Unite!  GUADEC: The world's #1 Open Source Desktop Event.
GNOME Users and Developers European Conference, 28-30th June in Norway
http://2004/guadec.org