lkml.org 
[lkml]   [2012]   [Mar]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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)

\
 
 \ /
  Last update: 2012-03-20 02:11    [W:0.041 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site