Re: [GIT PULL] Please pull NFS client changes for Linux 4.13

[Linus Torvalds]

On Fri, Jul 14, 2017 at 12:43 PM, Andrey Ryabinin
<aryabinin@xxxxxxxxxxxxx> 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).

There is something actively *evil* about it. Daniel, Kees, please jump on this.

Andrey, thanks for noticing this thing,

                          Linus