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    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site