lkml.org 
[lkml]   [2012]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] tty/vt: set_get_cmap() check user buffer
    On 03/20/2012 02:07 AM, Michael Gehring wrote:
    > 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):

    FWIW it very much makes sense to me.

    Except you should also remove set_get_cmap now.

    And make sure it really works (test it).

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

    thanks,
    --
    js
    suse labs


    \
     
     \ /
      Last update: 2012-03-20 09:41    [W:0.026 / U:93.576 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site