On Fri, 2017-07-14 at 12:58 -0700, 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). > > There is something actively *evil* about it. Daniel, Kees, please jump > on this. > > Andrey, thanks for noticing this thing, > > Linus The issue is the usage of strscpy then, not the __builtin_object_size type parameter. The type is set 0 rather than 1 to be more lenient by not detecting intra-object overflow, which is going to come later. If strscpy treats the count parameter as a *guarantee* of the dest size rather than a limit, it's wrong to use it there, whether or not the type parameter for __builtin_object_size is 0 or 1 since it can still return a larger size. It's a limit with no guaranteed minimum. My initial patch used strlcpy there, because I wasn't aware of strscpy before it was suggested: http://www.openwall.com/lists/kernel-hardening/2017/05/04/11 I was wrong to move it to strscpy. It could be switched back to strlcpy again unless the kernel considers the count parameter to be a guarantee that could be leveraged in the future. Using the fortified strlen + memcpy would provide the improvement that strscpy was meant to provide there over strlcpy.
