On Fri, 2017-07-14 at 13:50 -0700, Linus Torvalds wrote: > On Fri, Jul 14, 2017 at 1:38 PM, Daniel Micay <danielmicay@xxxxxxxxx> > wrote: > > > > If strscpy treats the count parameter as a *guarantee* of the dest > > size > > rather than a limit, > > No, it's a *limit*. > > And by a *limit*, I mean that we know that we can access both source > and destination within that limit. FORTIFY_SOURCE needs to be able to pass a limit without implying that there's a minimum. That's the distinction I was trying to make. It's wrong to use anything where it's interpreted as a minimum here. Using __builtin_object_size(ptr, 0) vs. __builtin_object_size(ptr, 1) doesn't avoid the problem. __builtin_object_size(ptr, 1) returns a maximum among the possible buffer sizes just like 0. It's just stricter, i.e. catches intra-object overflow, which isn't desirable for the first take since it will cause compatibility issues. There's code using memcpy, copy_to_user, etc. to read / write multiple fields with a pointer to the first one passed as the source / destination. > > My initial patch used strlcpy there, because I wasn't aware of > > strscpy > > before it was suggested: > > Since I'm looking at this, I note that the "strlcpy()" code is > complete garbage too, and has that same > > p_size == (size_t)-1 && q_size == (size_t)-1 > > check which is wrong. Of course, in strlcpy, q_size is never actually > *used*, so the whole check seems bogus. That check is only an optimization. __builtin_object_size always returns a constant, and it's (size_t)-1 when no limit could be found. The reason q_size isn't used is because it doesn't yet prevent read overflow. The commit message mentions that among the current limitations along with __builtin_object_size(ptr, 1). > But no, strlcpy() is complete garbage, and should never be used. It is > truly a shit interface, and anybody who uses it is by definition > buggy. > > Why? Because the return value of "strlcpy()" is defined to be ignoring > the limit, so you FUNDAMENTALLY must not use that thing on untrusted > source strings. > > But since the whole *point* of people using it is for untrusted > sources, it by definition is garbage. > > Ergo: don't use strlcpy(). It's unbelievable crap. It's wrong. There's > a reason we defined "strscpy()" as the way to do safe copies > (strncpy(), of course, is broken for both lack of NUL termination > _and_ for excessive NUL termination when a NUL did exist). Sure, it doesn't prevent read overflow (but it's not worse than strcpy, which is the purpose) which is why I said this: "The fortified string functions should place a limit on reads from the source. For strcat/strcpy, these could just be a variant of strlcat / strlcpy using the size limit as a bound on both the source and destination, with the return value only reporting whether truncation occurred rather than providing the source length. It would be an easier API for developers to use too and not that it really matters but it would be more efficient for the case where truncation is intended." That's why strscpy was suggested and I switched to that + updated the commit message to only mention strcat, but it's wrong to use it here because __builtin_object_size(p, 0) / __builtin_object_size(p, 1) are only a guaranteed maximum length with no minimum guarantee.
