Messages in this thread Patch in this message | | | Date | Tue, 20 Mar 2012 02:07:39 +0100 | From | Michael Gehring <> | Subject | Re: [PATCH] tty/vt: set_get_cmap() check user buffer |
| |
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)
| |