linux-kernel - Re: [PATCH v2] x86: fix ktermios-termio conversion

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]

Date:	Tue, 12 May 2009 11:04:30 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Ingo Molnar <mingo@...e.hu>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org,
	Remis Lima Baima <remis.developer@...glemail.com>
Subject: Re: [PATCH v2] x86: fix ktermios-termio conversion

On Tuesday 12 May 2009, Ingo Molnar wrote:
> Hm, that looks a bit ugly and it also adds 150 bytes of bloat to the 
> kernel:
> 
> drivers/char/tty_ioctl.o:
> 
>    text    data     bss     dec     hex filename
>    4704       0       0    4704    1260 tty_ioctl.o.before
>    4841       0       0    4841    12e9 tty_ioctl.o.after

It's easy to go back to a macro (or replace it by another inline
function if you prefer that stylistically) to make it look
nicer.

Regarding the bloat, I'm still looking for a better version.
The code I posted is basically what is in asm-generic/termios.h
traditionally, so at least I'm reasonably confident that it
is correct, while the x86 version currently fails to return
-EFAULT on incorrect pointers and misses c_line changes.

I now tried this version, which theoretically should be
fairly compact on most architectures:

static inline int set_low_termios_bits(unsigned int *termios,
			const short __user *termio)
{
	unsigned short tmp;
	int ret;

	ret = __get_user(tmp, termio);
	*termios = (0xffff0000 & *termios) | tmp;

	return ret;
}

/*
 * Translate a "termio" structure into a "termios". Ugh.
 */
static inline int user_termio_to_kernel_termios(struct ktermios *termios,
						const struct termio __user *termio)
{
	if (access_ok(VERIFY_READ, termio, sizeof (*termio)))
		return -EFAULT;

	return set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) |
	       set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) |
	       set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) |
	       set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) |
	       __get_user(termios->c_line, &termio->c_line) |
	       (__copy_from_user(termios->c_cc, termio->c_cc, NCC)
				? -EFAULT : 0);
}

/*
 * Translate a "termios" structure into a "termio". Ugh.
 */
static inline int kernel_termios_to_user_termio(struct termio __user *termio,
						struct ktermios *termios)
{
	if (access_ok(VERIFY_WRITE, termio, sizeof (*termio)))
		return -EFAULT;

	return __put_user(termios->c_iflag, &termio->c_iflag) |
	       __put_user(termios->c_oflag, &termio->c_oflag) |
	       __put_user(termios->c_cflag, &termio->c_cflag) |
	       __put_user(termios->c_lflag, &termio->c_lflag) |
	       __put_user(termios->c_line,  &termio->c_line) |
	       (__copy_to_user(termio->c_cc, termios->c_cc, NCC)
				? -EFAULT : 0);
}

Unfortunately, this is even more code on x86, where get_user/put_user
is out-of-line, while __get_user/__put_user is inline and produces
fixup code.
The best I could come up with is somewhat slower but also shorter:

static inline int set_low_termios_bits(unsigned int *termios,
			const short __user *termio)
{
	unsigned short tmp;
	int ret;

	ret = get_user(tmp, termio);
	*termios = (0xffff0000 & *termios) | tmp;

	return ret;
}

/*
 * Translate a "termio" structure into a "termios". Ugh.
 */
static inline int user_termio_to_kernel_termios(struct ktermios *termios,
						const struct termio __user *termio)
{
	return (set_low_termios_bits(&termios->c_iflag, &termio->c_iflag) ||
		set_low_termios_bits(&termios->c_oflag, &termio->c_oflag) ||
		set_low_termios_bits(&termios->c_cflag, &termio->c_cflag) ||
		set_low_termios_bits(&termios->c_lflag, &termio->c_lflag) ||
		get_user(termios->c_line, &termio->c_line) ||
		copy_from_user(termios->c_cc, termio->c_cc, NCC))
				? -EFAULT : 0;
}

/*
 * Translate a "termios" structure into a "termio". Ugh.
 */
static inline int kernel_termios_to_user_termio(struct termio __user *termio,
						struct ktermios *termios)
{
	return put_user(termios->c_iflag, &termio->c_iflag) ||
	       put_user(termios->c_oflag, &termio->c_oflag) ||
	       put_user(termios->c_cflag, &termio->c_cflag) ||
	       put_user(termios->c_lflag, &termio->c_lflag) ||
	       put_user(termios->c_line,  &termio->c_line) ||
	       (copy_to_user(termio->c_cc, termios->c_cc, NCC))
				? -EFAULT : 0;
}

If you think that looks better, I can send another patch. I have
not tested this code functionally though.

	Arnd <><
--
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/