Re: [PATCH] x86: unify copy_from_user() checking

[Jan Beulich]

>>> On 16.10.13 at 16:00, Arjan van de Ven <arjan@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/