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

[Jiri Slaby <jslaby@suse.cz>]

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