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
