lkml.org 
[lkml]   [2017]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [GIT PULL] Please pull NFS client changes for Linux 4.13
From
Date


On 07/14/2017 10:58 PM, Linus Torvalds wrote:
> On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
> <aryabinin@virtuozzo.com> wrote:
>>
>>> yet when I look at the generated code for __ip_map_lookup, I see
>>>
>>> movl $32, %edx #,
>>> movq %r13, %rsi # class,
>>> leaq 48(%rax), %rdi #, tmp126
>>> call strscpy #
>>>
>>> what's the bug here? Look at that third argume8nt - %rdx. It is
>>> initialized to 32.
>>
>> It's not a compiler bug, it's a bug in our strcpy().
>> Whoever wrote this strcpy() into strscpy() code apparently didn't read carefully
>> enough gcc manual about __builtin_object_size().
>>
>> Summary from https://gcc.gnu.org/onlinedocs/gcc/Object-Size-Checking.html :
>>
>> __builtin_object_size(ptr, type) returns a constant number of bytes from 'ptr' to the end of the object 'ptr'
>> pointer points to. "type" is an integer constant from 0 to 3. If the least significant bit is clear, objects
>> are whole variables, if it is set, a closest surrounding subobject is considered the object a pointer points to.
>> The second bit determines if maximum or minimum of remaining bytes is computed.
>>
>> We have type = 0 in strcpy(), so the least significant bit is clear. So the 'ptr' is considered as a pointer to the whole
>> variable i.e. pointer to struct ip_map ip;
>> And the number of bytes from 'ip.m_class' to the end of the ip object is exactly 32.
>>
>> I suppose that changing the type to 1 should fix this bug.
>
> Oh, that absolutely needs to be done.
>
> Because that "strcpy() -> strscpy()" conversion really depends on that
> size being the right size (well, in this case minimal safe size) for
> the actual accesses, exactly because "strscpy()" is perfectly willing
> to write *past* the end of the destination string within that given
> size limit (ie it reads and writes in the same 8-byte chunks).
>
> So if you have a small target string that is contained in a big
> object, then the "hardened" strcpy() code can actually end up
> overwriting things past the end of the strring, even if the string
> itself were to have fit in the buffer.
>
> I note that every single use in string.h is buggy, and it worries me
> that __compiletime_object_size() does this too. The only user of that
> seems to be check_copy_size(), and now I'm a bit worried what that bug
> may have hidden.
>
> I find "hardening" code that adds bugs to be particularly bad and
> ugly, the same way that I absolutely *hate* debugging code that turns
> out to make debugging impossible (we had that with the "better" stack
> tracing code that caused kernel panics to kill the machine entirely
> rather than show the backtrace, and I'm still bitter about it a decade
> after the fact).
>

A have some more news to make you even more "happier" :)
strcpy() choose to copy 32-bytes instead of smaller 5-bytes because it has one more bug :)

GCC couldn't determine size of class (which is 5-byte string):
strcpy(ip.m_class, class);

So, p_size = 32 and q_size = -1, this "if (p_size == (size_t)-1 && q_size == (size_t)-1)" is false
(because of bogus '&&', obviously we should have '||' here)

and since (32 < (size_t)-1)
if (strscpy(p, q, p_size < q_size ? p_size : q_size) < 0)

we end up with 32-bytes strscpy().

Enjoy :)


> There is something actively *evil* about it. Daniel, Kees, please jump on this.
>
> Andrey, thanks for noticing this thing,
>
> Linus
>

\
 
 \ /
  Last update: 2017-07-14 22:28    [W:0.133 / U:6.860 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site