linux-kernel - Re: [PATCH] tty/vt: set_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120319234839.GA26931@kroah.com>
Date:	Mon, 19 Mar 2012 16:48:39 -0700
From:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:	Michael Gehring <mg@...e.org>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] tty/vt: set_get_cmap() check user buffer

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?

> 
> Another side effect of this: calling the PIO_CMAP ioctl with an invalid
> buffer will zero the default colormap and the palette for all vts (all
> colors set to black).
> 
> Use access_ok() and return -EFAULT when appropriate.
> 
> Signed-off-by: Michael Gehring <mg@...e.org>
> ---
>  drivers/tty/vt/vt.c |   15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> 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)
>  
>      WARN_CONSOLE_UNLOCKED();
>  
> +    if (!access_ok(set ? VERIFY_READ : VERIFY_WRITE, arg, 3 * 16))
> +	return -EFAULT;
> +
>      for (i = 0; i < 16; i++)
>  	if (set) {
> -	    get_user(default_red[i], arg++);
> -	    get_user(default_grn[i], arg++);
> -	    get_user(default_blu[i], arg++);
> +	    __get_user(default_red[i], arg++);
> +	    __get_user(default_grn[i], arg++);
> +	    __get_user(default_blu[i], arg++);
>  	} else {
> -	    put_user(default_red[i], arg++);
> -	    put_user(default_grn[i], arg++);
> -	    put_user(default_blu[i], arg++);
> +	    __put_user(default_red[i], arg++);
> +	    __put_user(default_grn[i], arg++);
> +	    __put_user(default_blu[i], arg++);

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.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/