linux-kernel - Re: [PATCH] x86: unify copy_from

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:	Wed, 16 Oct 2013 07:00:23 -0700
From:	Arjan van de Ven <arjan@...ux.intel.com>
To:	Jan Beulich <JBeulich@...e.com>
CC:	mingo@...e.hu, tglx@...utronix.de, hpa@...or.com,
	linux@...ck-us.net, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86: unify copy_from_user() checking

>
> It also puzzles me that similar checking isn't done for copy_to_user():
> While not resulting in (kernel) buffer overflows, size mistakes there
> would still lead to information leaks.

at the time I went for the highest payoff only; e.g. the mistake of copying
to a fixed size kernel buffer.

Adding the other direction is a good idea of course.

>
> Signed-off-by: Jan Beulich <jbeulich@...e.com>
> Cc: Arjan van de Ven <arjan@...ux.intel.com>
> Cc: Guenter Roeck <linux@...ck-us.net>

> +static inline unsigned long __must_check copy_from_user(void *to,
> +					  const void __user *from,
> +					  unsigned long n)
> +{
> +	int sz = __compiletime_object_size(to);
> +
> +	might_fault();
> +	if (likely(sz == -1 || sz >= n))
> +		n = _copy_from_user(to, from, n);
> +	else if(__builtin_constant_p(n))
> +		copy_from_user_overflow();

this part I am not so sure about.
the original intent was that even if n is not constant, the compiler must still be able
to prove that it is smaller than sz using the range tracking feature in gcc!
In fact, that was the whole point.
The code (at the time, they're all fixed) found cases where the checks done to "n" were off by one
etc...

by requiring "n" to be constant for these checks you remove that layer of checking.

if you have found cases where this matters... maybe you found a new security issue...

so while I like the cleanup part of your patch.... not convinced on this part yet

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