Re: [PATCH] tty/vt: set_get_cmap() check user buffer

[Michael Gehring <mg@ebfe.org>]

On Mon, Mar 19, 2012 at 04:48:39PM -0700, Greg Kroah-Hartman wrote:
> On Tue, Mar 20, 2012 at 12:34:01AM +0100, Michael Gehring wrote:
> > set_get_cmap() ignores the result of {get,put}_user(), causing ioctl(vt,
> > {G,P}IO_CMAP, 0xdeadbeef) to silently fail.
> 
> Why not just check each return value, failing only if/when a specific
> write fails?

For the 'set' case, that would result in a partially updated default
colormap.

> > diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
> > index e716839..176b2a1 100644
> > --- a/drivers/tty/vt/vt.c
> > +++ b/drivers/tty/vt/vt.c
> > @@ -3897,15 +3897,18 @@ static int set_get_cmap(unsigned char __user *arg, int set)
... 
> What's to keep this userspace buffer from becoming invalid after the
> check?  For some reason I thought we couldn't check beforehand like
> this, but I can't recall why at this specific moment.
> 
> And ugh, why do we have a function that does two things, like this?  The
> only thing we are "saving" is a single for loop by doing things this
> way, splitting it out into a set/get function, would make more sense in
> the end.


How about something like this (untested):


diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index e716839..f0a881d 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -3928,24 +3928,53 @@ static int set_get_cmap(unsigned char __user *arg, int set)
 
 int con_set_cmap(unsigned char __user *arg)
 {
-	int rc;
+	int i, j, k;
+	unsigned char colormap[3*16];
+
+	if (copy_from_user(colormap, arg, sizeof(colormap)))
+		return -EFAULT;
 
 	console_lock();
-	rc = set_get_cmap (arg,1);
+
+	for (i = k = 0; i < 16; i++) {
+		default_red[i] = colormap[k++];
+		default_grn[i] = colormap[k++];
+		default_blu[i] = colormap[k++];
+	}
+
+	for (i = 0; i < MAX_NR_CONSOLES; i++) {
+		if (!vc_cons_allocated(i))
+			continue;
+		for (j = k = 0; j < 16; j++) {
+		    vc_cons[i].d->vc_palette[k++] = default_red[j];
+		    vc_cons[i].d->vc_palette[k++] = default_grn[j];
+		    vc_cons[i].d->vc_palette[k++] = default_blu[j];
+		}
+		set_palette(vc_cons[i].d);
+	}
+
 	console_unlock();
 
-	return rc;
+	return 0;
 }
 
 int con_get_cmap(unsigned char __user *arg)
 {
-	int rc;
+	int i, k;
+	unsigned char colormap[3*16];
 
 	console_lock();
-	rc = set_get_cmap (arg,0);
+	for (i = k = 0; i < 16; i++) {
+		colormap[k++] = default_red[i];
+		colormap[k++] = default_grn[i];
+		colormap[k++] = default_blu[i];
+	}
 	console_unlock();
 
-	return rc;
+	if (copy_to_user(arg, colormap, sizeof(colormap)))
+		return -EFAULT;
+
+	return 0;
 }
 
 void reset_palette(struct vc_data *vc)