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

>>> On 16.10.13 at 16:00, Arjan van de Ven <arjan@...ux.intel.com> wrote:
>> +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!

I had pointed out cases in the patch description where I was getting
a warning when I think I shouldn't, and since I pay attention to
warnings this keeps me going back to the sources whenever I didn't
look at the reason long enough and forgot whether it's safe to ignore
these warnings. Warnings are nice and useful, but especially when
they sound dangerous having false positives isn't helpful at all.

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

Iirc I could convince myself that in the cited cases the warnings
were there for no reason.

Jan

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