On 07/14/2017 10:58 PM, Linus Torvalds wrote: > 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). > 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 >
