Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240002 > unrolled thread

Re: [PATCH] string: Improve the generic strlcpy() implementation

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 00:30 +0200
    Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-06 10:00 +0200
    Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-06 10:10 +0200
      Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-07 00:10 +0200
        Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-07 09:20 +0200
          Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-07 11:10 +0200
            Re: [PATCH] string: Improve the generic strlcpy() implementation Linus Torvalds <torvalds@linux-foundation.org> - 2015-10-07 11:30 +0200
              Re: [PATCH] string: Improve the generic strlcpy() implementation Ingo Molnar <mingo@kernel.org> - 2015-10-08 10:50 +0200
                Re: [PATCH] string: Improve the generic strlcpy() implementation Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 10:20 +0200
                  [RFC 0/3] eliminate potential race in string() (was: [PATCH] string: Improve the generic strlcpy() implementation) Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
                    [RFC 1/3] lib/vsprintf.c: pull out padding code from dentry_name() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
                    [RFC 3/3] lib/vsprintf.c: eliminate potential race in string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
                    [RFC 2/3] lib/vsprintf.c: move string() below widen_string() Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-09 11:20 +0200
                    Re: [RFC 0/3] eliminate potential race in string() (was: [PATCH]  string: Improve the generic strlcpy() implementation) Ingo Molnar <mingo@kernel.org> - 2015-10-10 09:50 +0200

#1240002 — Re: [PATCH] string: Improve the generic strlcpy() implementation

On Mon, Oct 05 2015, Ingo Molnar <mingo@kernel.org> wrote:

> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> So I finally pulled it. I like the patch, I like the new interface,
>> but despite that I wasn't really sure if I wanted to pull it in - thus
>> the long delay of me just seeing this in my list of pending pulls for
>> almost a month, but never really getting to the point where I decided
>> I want to commit to it.
>
> Interesting. I noticed that strscpy() says this in its comments:
>
>  * In addition, the implementation is robust to the string changing out
>  * from underneath it, unlike the current strlcpy() implementation.
>
> The strscpy() interface is very nice, but shouldn't we also fix this strlcpy() 
> unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call 
> sites?

How about every single occurence of %s in a format string? vsnprintf
also has that "issue", but has it actually ever been a problem? The
window for something bad to happen is probably also much larger in the
printf case, and especially when when some %p extension is used and/or
the vsnprintf user is kasprintf() (where we 'replay' the formatting,
having hopefully obtained a correct-sized buffer).

In fact, writing this, it occurs to me that we should probably check the
return value of the second vsnprintf call in kasprintf and compare to
the first, issuing a warning if they don't match.

I'm not against making strlcpy more robust, but I think the theoretical
race is far more likely to manifest through a member of the printf
family.

Note that, unless one cares for performance or worries about 2G+
length strings, strlcpy could just be 'return snprintf(dst, len, "%s",
src);', which would give the "check for insanely large/negative len" for
free [though not giving strlen(src) as return value - but the caller is much
more likely to be tripped up by no copying having taken place anyway]. 

> Another problem is that strlcpy() will also happily do bad stuff if we pass
> it a negative size. Instead of that we will from now on print a (one time)
> warning and return safely.

Well, not too sure about that 'safely'. If the caller somehow managed to
compute an insanely large (remaining) capacity in the buffer and has
that in a size_t variable, then proceeds to comparing the return
value to the supposed buffer size to check for overflow, he will think
that everything is fine and proceed to using likely uninitialized
contents of his buffer.

I think a return value of 0 might be slightly better. Assuming the
caller has the capacity in a signed variable (so it only became huge by
being converted to size_t) and makes a signed comparison with the return
value, both 0 and strlen() triggers an overflow check, so we wouldn't be
worse off in that case. Clearly the same is true if the return value is
not used at all. If the return value is used mindlessly for advancing
dst and decrementing the capacity, staying put is probably better.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240220

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Mon, Oct 05 2015, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> So I finally pulled it. I like the patch, I like the new interface,
> >> but despite that I wasn't really sure if I wanted to pull it in - thus
> >> the long delay of me just seeing this in my list of pending pulls for
> >> almost a month, but never really getting to the point where I decided
> >> I want to commit to it.
> >
> > Interesting. I noticed that strscpy() says this in its comments:
> >
> >  * In addition, the implementation is robust to the string changing out
> >  * from underneath it, unlike the current strlcpy() implementation.
> >
> > The strscpy() interface is very nice, but shouldn't we also fix this strlcpy() 
> > unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call 
> > sites?
> 
> How about every single occurence of %s in a format string? vsnprintf
> also has that "issue", but has it actually ever been a problem? The
> window for something bad to happen is probably also much larger in the
> printf case, and especially when when some %p extension is used and/or
> the vsnprintf user is kasprintf() (where we 'replay' the formatting,
> having hopefully obtained a correct-sized buffer).
>
> In fact, writing this, it occurs to me that we should probably check the
> return value of the second vsnprintf call in kasprintf and compare to
> the first, issuing a warning if they don't match.
> 
> I'm not against making strlcpy more robust, but I think the theoretical
> race is far more likely to manifest through a member of the printf
> family.
> 
> Note that, unless one cares for performance or worries about 2G+
> length strings, strlcpy could just be 'return snprintf(dst, len, "%s",
> src);', which would give the "check for insanely large/negative len" for
> free [though not giving strlen(src) as return value - but the caller is much
> more likely to be tripped up by no copying having taken place anyway]. 
> 
> > Another problem is that strlcpy() will also happily do bad stuff if we pass
> > it a negative size. Instead of that we will from now on print a (one time)
> > warning and return safely.
> 
> Well, not too sure about that 'safely'. If the caller somehow managed to compute 
> an insanely large (remaining) capacity in the buffer and has that in a size_t 
> variable, then proceeds to comparing the return value to the supposed buffer 
> size to check for overflow, he will think that everything is fine and proceed to 
> using likely uninitialized contents of his buffer.
> 
> I think a return value of 0 might be slightly better. Assuming the caller has 
> the capacity in a signed variable (so it only became huge by being converted to 
> size_t) and makes a signed comparison with the return value, both 0 and strlen() 
> triggers an overflow check, so we wouldn't be worse off in that case. Clearly 
> the same is true if the return value is not used at all. If the return value is 
> used mindlessly for advancing dst and decrementing the capacity, staying put is 
> probably better.

Ok, I can certainly change the return value to 0, but note that the (insane!) 
return value of strlcpy() gets used in only about 0.8% of the cases:

  triton:~/tip> git grep -w strlcpy | wc -l
  2097
  triton:~/tip> git grep -w strlcpy | grep -w if | wc -l
  11

... so this all is pretty theoretical I think, and we could as well just migrate 
all those standalone strlcpy() users that don't check the return code over to 
strscpy()!

This would probably speed up all those usecases, so it's a nice optimization.

Then we could convert the remaining ~20 call sites and mark strlcpy() as a working 
but deprecated API.

Linus, would you object to such patches, if it's done in a relatively painless 
fashion: not propagated into -next but generated automatically late in the merge 
window or right after -rc1 or so.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240230

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Mon, Oct 05 2015, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Linus Torvalds <torvalds@linux-foundation.org> wrote:
> >
> >> So I finally pulled it. I like the patch, I like the new interface, but 
> >> despite that I wasn't really sure if I wanted to pull it in - thus the long 
> >> delay of me just seeing this in my list of pending pulls for almost a month, 
> >> but never really getting to the point where I decided I want to commit to it.
> >
> > Interesting. I noticed that strscpy() says this in its comments:
> >
> >  * In addition, the implementation is robust to the string changing out
> >  * from underneath it, unlike the current strlcpy() implementation.
> >
> > The strscpy() interface is very nice, but shouldn't we also fix this strlcpy() 
> > unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call 
> > sites?
> 
> How about every single occurence of %s in a format string? vsnprintf also has 
> that "issue", but has it actually ever been a problem? The window for something 
> bad to happen is probably also much larger in the printf case, and especially 
> when when some %p extension is used and/or the vsnprintf user is kasprintf() 
> (where we 'replay' the formatting, having hopefully obtained a correct-sized 
> buffer).
> 
> In fact, writing this, it occurs to me that we should probably check the return 
> value of the second vsnprintf call in kasprintf and compare to the first, 
> issuing a warning if they don't match.
> 
> I'm not against making strlcpy more robust, but I think the theoretical race is 
> far more likely to manifest through a member of the printf family.

So the printf family is generally less frequently used in ABI output than string 
copies, but yeah, since there are 15,000 s[n]printf() calls in the kernel it's 
more likely to be an issue not just by virtue of timing, but also by sheer mass of 
usage, statistically.

So I'd improve it all in the following order:

  - fix the strscpy() uninitialized use

  - base strlcpy() on strscpy() via the patch I sent. This makes all users faster 
    and eliminates the theoretical race.

  - phase out 'simple' strlcpy() uses via an automated patch. This gets rid of 
    2,000 strlcpy() call sites in a single pass.

  - phase out the remaining two dozen or so 'complex' strlcpy() uses one by one.

  - mark strlcpy() deprecated, add checkpatch warning.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241027

On Tue, Oct 06 2015, Ingo Molnar <mingo@kernel.org> wrote:

> * Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
>
>> 
>> I'm not against making strlcpy more robust, but I think the theoretical race is 
>> far more likely to manifest through a member of the printf family.
>
> So the printf family is generally less frequently used in ABI output than string 
> copies,

Huh? snprintf and friends are often used just to copy or concatenate
strings (essentially, any format string containing no specifiers other
than %s does exactly that) - even if a str*() function could do the same
thing. I see no reason to believe this wouldn't also be done in cases
where the final string ends up being presented to userspace.

> but yeah, since there are 15,000 s[n]printf() calls in the kernel it's 
> more likely to be an issue not just by virtue of timing, but also by sheer mass of 
> usage, statistically.

Yes.

> So I'd improve it all in the following order:
>
>   - fix the strscpy() uninitialized use
>
>   - base strlcpy() on strscpy() via the patch I sent. This makes all users faster 
>     and eliminates the theoretical race.

I'm not so sure about that part. I'd strongly suspect that the vast
majority of strings handled by strlcpy (and in the future strscpy) are
shorter than 32 bytes, so is all the word_at_a_time and pre/post
alignment yoga really worth it? strscpy is 299 bytes - that's a lot of
instruction cache lines, and it will almost always be cache cold. This
isn't an argument against basing strlcpy on strscpy (that would likely
just make the former a little smaller), but I'd like to see numbers
(cycle counts, distribution of input lengths, ...)  before I believe in
the performance argument.

>   - phase out 'simple' strlcpy() uses via an automated patch. This gets rid of 
>     2,000 strlcpy() call sites in a single pass.

That seems to be exactly the kind of mass-conversion Linus referred to
above. Also, you can't really do it if strscpy keeps it __must_check
annotation, as you'd then introduce 2000+ warnings...

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241186

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Tue, Oct 06 2015, Ingo Molnar <mingo@kernel.org> wrote:
> 
> > * Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> >
> >> 
> >> I'm not against making strlcpy more robust, but I think the theoretical race is 
> >> far more likely to manifest through a member of the printf family.
> >
> > So the printf family is generally less frequently used in ABI output than string 
> > copies,
> 
> Huh? snprintf and friends are often used just to copy or concatenate
> strings (essentially, any format string containing no specifiers other
> than %s does exactly that) - even if a str*() function could do the same
> thing. I see no reason to believe this wouldn't also be done in cases
> where the final string ends up being presented to userspace.

But 'format string' usually means 'human output', and most of our ABI
pertains to system calls where we rarely form human output, we typically
generate programmatically actionable data structures.

We have procfs and sysfs as well, where format strings are indeed dominant, but 
are you sure this race exists in snprintf() in that form? I.e. can the return 
value of snprintf() be different from the true length of the output string, if the 
source string is modified in parallel?

> > So I'd improve it all in the following order:
> >
> >   - fix the strscpy() uninitialized use
> >
> >   - base strlcpy() on strscpy() via the patch I sent. This makes all users faster 
> >     and eliminates the theoretical race.
> 
> I'm not so sure about that part. I'd strongly suspect that the vast
> majority of strings handled by strlcpy (and in the future strscpy) are
> shorter than 32 bytes, so is all the word_at_a_time and pre/post
> alignment yoga really worth it?

That's a good question, I'll measure it.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241247

On Wed, Oct 07 2015, Ingo Molnar <mingo@kernel.org> wrote:

> We have procfs and sysfs as well, where format strings are indeed dominant, but 
> are you sure this race exists in snprintf() in that form? I.e. can the return 
> value of snprintf() be different from the true length of the output string, if the 
> source string is modified in parallel?

Well, if truncation has happened the return value is different
(larger). But assuming the output buffer is large enough, the 'compute
strlen, then do copying, potentially copying a nul byte which wasn't
there moments before' is pretty obvious:

lib/vsprintf.c:

static noinline_for_stack
char *string(char *buf, char *end, const char *s, struct printf_spec spec)
{
	int len, i;

	if ((unsigned long)s < PAGE_SIZE)
		s = "(null)";

	len = strnlen(s, spec.precision);

	if (!(spec.flags & LEFT)) {
		while (len < spec.field_width--) {
			if (buf < end)
				*buf = ' ';
			++buf;
		}
	}
	for (i = 0; i < len; ++i) {
		if (buf < end)
			*buf = *s;
		++buf; ++s;
	}
	while (len < spec.field_width--) {
		if (buf < end)
			*buf = ' ';
		++buf;
	}

	return buf;
}

(spec.precision is an s16 which by default is set to -1, so for the
usual case of plain %s the upper bound in strnlen is (size_t)-1,
effectively infinity). If it wasn't for the field width padding it would
probably not be that hard to fix.

But, to rephrase an earlier question: Can anyone point to an instance
where the strlcpy source or a %s argument to a printf function can
actually change under us? I'd like to see if one can intentionally
trigger the potential race, but I suspect that the vast majority cannot
have a problem - maybe someone has an idea of specific places that are
worth looking at.

>> > So I'd improve it all in the following order:
>> >
>> >   - fix the strscpy() uninitialized use
>> >
>> >   - base strlcpy() on strscpy() via the patch I sent. This makes all users faster 
>> >     and eliminates the theoretical race.
>> 
>> I'm not so sure about that part. I'd strongly suspect that the vast
>> majority of strings handled by strlcpy (and in the future strscpy) are
>> shorter than 32 bytes, so is all the word_at_a_time and pre/post
>> alignment yoga really worth it?
>
> That's a good question, I'll measure it.

Here's a few pseudo-datapoints. About half the strlcpy instances (just
from lazy grepping) has a string literal as src, and those only have
about 1/8th chance of being aligned. A quick skim through a small
vmlinux showed 34 calls of strlcpy where %rsi was easily seen to be a
literal address, of which 7 were aligned. That's 1/5, but the sample
size is rather small (also, the 1/8 is admittedly an underestimate,
since gcc seems to put long enough literals in rodata.str1.8).

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241288

On Wed, Oct 7, 2015 at 10:04 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
>
> Well, if truncation has happened the return value is different
> (larger). But assuming the output buffer is large enough, the 'compute
> strlen, then do copying, potentially copying a nul byte which wasn't
> there moments before' is pretty obvious:
[snip]

So I really refuse to worry about the snprintf() family of functions
wrt this race. I don't think it was hugely important for strlcpy()
either - more of a "quality of implementation" issue rather than
anything fundamental - but for snprintf and friends it's an almost
unavoidable issue because of how snprintf works.

Saying that 'strlcpy()' and 'snprintf("%s")' are equivalent is true
only in the loosest sense. Yes, they return the same return value.
Yes, the result string should be the same. But the two are completely
different despite that.

snprintf() has to handle all the *other* cases than just "%s",
including right-justification, string precision handling, etc etc. It
is effectively impossible to do without doing "strlen()" on the source
of the string beforehand. As a result, snprintf() is fundamentally
always going to be racy wrt the string changing during the call.

So the simple end result is that we shouldn't worry about it, and if
you are doing snprintf() on a changing string, you should just be
aware of it. We *do* actually do that, for things like "current->comm"
that really can change while being printed out. We just don't care
deeply, and have in fact been removing locks in this area, because the
end result is still guaranteed to be NUL-terminated etc.

Can we get odd truncated printouts in the (very very very unlikely)
case that the string is being changed? Yes. We just don't care.

With strlcpy(), the situation is different at least in the sense that
we *can* write a source-modification-safe version. Of course, the end
result will still be undefined, but at least the resulting string
length in the destination can be made to not disagree violently with
the return value.

Do we care? Probably not. If you do strlcpy() on strings that change
without using locking, it's either a serialization bug, or you really
don't care very deeply about the end result anyway (ie it's something
like the "current->comm" issue). But just from a quality of
implementation standpoint, I think it would be good to just do the
RightThing(tm) anyway.

That's particularly true since we should be able to do it trivially by
just implementing strlcpy() using strscpy() plus the overflow fixup.
But let's wait with that until people are happy about the state of
strscpy. There's absolutely no rush, and in fact the one thing I
absolutely wanted to avoid was to have the introduction of strscpy()
resulting in pointless churn elsewhere, so let's do the *opposite* of
rushing into this, and just say :"ok, some day we should do this, just
in case"

> Here's a few pseudo-datapoints. About half the strlcpy instances (just
> from lazy grepping) has a string literal as src, and those only have
> about 1/8th chance of being aligned.

Hmm. I think gcc actually tends to align string literals - at least on
architectures where unaligned accesses tend to be more expensive and
we do the whole SLOW_UNALIGNEd handling etc.

I don't think gcc does it on x86, but on x86 we don't much care.
Somebody on ppc or ARM might want to check.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1242085

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> So I really refuse to worry about the snprintf() family of functions wrt this 
> race. I don't think it was hugely important for strlcpy() either - more of a 
> "quality of implementation" issue rather than anything fundamental - but for 
> snprintf and friends it's an almost unavoidable issue because of how snprintf 
> works.
> 
> Saying that 'strlcpy()' and 'snprintf("%s")' are equivalent is true only in the 
> loosest sense. Yes, they return the same return value. Yes, the result string 
> should be the same. But the two are completely different despite that.
> 
> snprintf() has to handle all the *other* cases than just "%s", including 
> right-justification, string precision handling, etc etc. It is effectively 
> impossible to do without doing "strlen()" on the source of the string 
> beforehand. As a result, snprintf() is fundamentally always going to be racy wrt 
> the string changing during the call.
> 
> So the simple end result is that we shouldn't worry about it, and if you are 
> doing snprintf() on a changing string, you should just be aware of it. We *do* 
> actually do that, for things like "current->comm" that really can change while 
> being printed out. We just don't care deeply, and have in fact been removing 
> locks in this area, because the end result is still guaranteed to be 
> NUL-terminated etc.
> 
> Can we get odd truncated printouts in the (very very very unlikely) case that 
> the string is being changed? Yes. We just don't care.

I do agree mostly, but I think we should still try to achieve the following two 
properties, if possible sanely+cheaply+cleanly:

 - the printed string should not contain spurious \0 bytes even if the %s source
   'races'. [I think this is true currently.]

 - the return code should correctly represent what snprintf did to the target
   string. [This might not be the case currently. But I'm not sure!]

Because that's a real concern I think: snprintf() return is used frequently to 
iterate over buffers, and it should correctly and reliably represent what it did, 
regardless of what the source buffer does - because snprintf obviously knows what 
it did to the output buffer, it has full, race-free control over it.

Whether left-alignment and other formatting details were calculated correctly, 
etc. is a secondary concern and cannot be guaranteed, but we should at least 
guarantee that we generated a single string, that we did nothing else, and that we 
correctly returned its length.

Agreed?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243112

On Thu, Oct 08 2015, Ingo Molnar <mingo@kernel.org> wrote:

> * Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
>> So I really refuse to worry about the snprintf() family of functions wrt this 
>> race. I don't think it was hugely important for strlcpy() either - more of a 
>> "quality of implementation" issue rather than anything fundamental - but for 
>> snprintf and friends it's an almost unavoidable issue because of how snprintf 
>> works.
>>
[snip]
>> 
>> Can we get odd truncated printouts in the (very very very unlikely) case that 
>> the string is being changed? Yes. We just don't care.
>
> I do agree mostly, but I think we should still try to achieve the following two 
> properties, if possible sanely+cheaply+cleanly:
>
>  - the printed string should not contain spurious \0 bytes even if the %s source
>    'races'. [I think this is true currently.]

Sorry, no, that's not true currently.

>  - the return code should correctly represent what snprintf did to the target
>    string. [This might not be the case currently. But I'm not sure!]

It does, in fact, represent "the number of characters, excluding the
trailing nul byte, that would have been written if the output buffer is
big enough" - but in some cases some of those bytes may happen to be
'\0'.

[The %s race is the only way I can see spurious \0, but \0 can also
legitimately be put in the output using %c, or maybe these days also
with some %p extension.]

> Because that's a real concern I think: snprintf() return is used frequently to 
> iterate over buffers, and it should correctly and reliably represent what it did, 
> regardless of what the source buffer does - because snprintf obviously knows what 
> it did to the output buffer, it has full, race-free control over it.
>
> Whether left-alignment and other formatting details were calculated correctly, 
> etc. is a secondary concern and cannot be guaranteed, but we should at least 
> guarantee that we generated a single string, that we did nothing else, and that we 
> correctly returned its length.
>
> Agreed?

No. More precisely, I don't agree with left-alignment etc. being a
secondary concern.

It's hard not to agree with the overall "let's make it more robust if it
can be done sanely+cheaply+cleanly". I was a bit skeptical about whether
those three requirements could be met, since we'd have to do
byte-by-byte traversal of the string, maybe-copying it to the output as
we go along, but then right-alignment would require us to do a memmove,
but not before we've done some complicated bookkeeping
exercise. However, now that I read the source again, it seems that Al
Viro already did that exercise when he added dentry(). So maybe it's
doable without a net increase in LOC.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243171 — [RFC 0/3] eliminate potential race in string() (was: [PATCH] string: Improve the generic strlcpy() implementation)

On Fri, Oct 09 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> It's hard not to agree with the overall "let's make it more robust if it
> can be done sanely+cheaply+cleanly". I was a bit skeptical about whether
> those three requirements could be met, since we'd have to do
> byte-by-byte traversal of the string, maybe-copying it to the output as
> we go along, but then right-alignment would require us to do a memmove,
> but not before we've done some complicated bookkeeping
> exercise. However, now that I read the source again, it seems that Al
> Viro already did that exercise when he added dentry(). So maybe it's
> doable without a net increase in LOC.

Something like this. The net increase is because I added a
comment. Passes the new printf test suite, but I'm not sure that's
thorough enough yet - still, it's better than nothing. There's also this
small bonus:

$ scripts/bloat-o-meter /tmp/vsprintf.o.{old,new}
add/remove: 1/0 grow/shrink: 0/2 up/down: 178/-245 (-67)
function                                     old     new   delta
widen_string.isra                              -     178    +178
string.isra                                  186     109     -77
dentry_name.isra                             358     190    -168


Rasmus Villemoes (3):
  lib/vsprintf.c: pull out padding code from dentry_name()
  lib/vsprintf.c: move string() below widen_string()
  lib/vsprintf.c: eliminate potential race in string()

 lib/vsprintf.c | 98 +++++++++++++++++++++++++++++++---------------------------
 1 file changed, 52 insertions(+), 46 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243174 — [RFC 1/3] lib/vsprintf.c: pull out padding code from dentry_name()

Pull out the logic in dentry_name() which handles field width space
padding, in preparation for reusing it from string(). Rename the
widen() helper to move_right(), since it is used for handling the
!(flags & LEFT) case.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b43b99..83b77796ac7e 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -538,7 +538,7 @@ char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 	return buf;
 }
 
-static void widen(char *buf, char *end, unsigned len, unsigned spaces)
+static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
 	if (buf >= end)	/* nowhere to put anything */
@@ -556,6 +556,35 @@ static void widen(char *buf, char *end, unsigned len, unsigned spaces)
 	memset(buf, ' ', spaces);
 }
 
+/*
+ * Handle field width padding for a string.
+ * @buf: current buffer position
+ * @n: length of string
+ * @end: end of output buffer
+ * @spec: for field width and flags
+ * Returns: new buffer position after padding.
+ */
+static noinline_for_stack
+char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
+{
+	unsigned spaces;
+
+	if (n >= spec.field_width)
+		return buf;
+	/* we want to pad the sucker */
+	spaces = spec.field_width - n;
+	if (!(spec.flags & LEFT)) {
+		move_right(buf - n, end, n, spaces);
+		return buf + spaces;
+	}
+	while (spaces--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+	return buf;
+}
+
 static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
@@ -597,20 +626,7 @@ char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_sp
 			*buf = c;
 	}
 	rcu_read_unlock();
-	if (n < spec.field_width) {
-		/* we want to pad the sucker */
-		unsigned spaces = spec.field_width - n;
-		if (!(spec.flags & LEFT)) {
-			widen(buf - n, end, n, spaces);
-			return buf + spaces;
-		}
-		while (spaces--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	return buf;
+	return widen_string(buf, n, end, spec);
 }
 
 static noinline_for_stack
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243176 — [RFC 3/3] lib/vsprintf.c: eliminate potential race in string()

If the string corresponding to a %s specifier can change under us, we
might end up copying a \0 byte to the output buffer. There might be
callers who expect the output buffer to contain a genuine C string
whose length is exactly the snprintf return value (assuming truncation
hasn't happened or has been checked for).

We can avoid this by only passing over the source string once,
stopping the first time we meet a nul byte (or when we reach the given
precision), and then letting widen_string() handle left/right space
padding.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 28 +++++++++-------------------
 1 file changed, 9 insertions(+), 19 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index acead77594b5..d518849b6b75 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -557,32 +557,22 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 static noinline_for_stack
 char *string(char *buf, char *end, const char *s, struct printf_spec spec)
 {
-	int len, i;
+	int len = 0;
+	size_t lim = spec.precision;
 
 	if ((unsigned long)s < PAGE_SIZE)
 		s = "(null)";
 
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
+	while (lim--) {
+		char c = *s++;
+		if (!c)
+			break;
 		if (buf < end)
-			*buf = ' ';
+			*buf = c;
 		++buf;
+		++len;
 	}
-
-	return buf;
+	return widen_string(buf, len, end, spec);
 }
 
 static noinline_for_stack
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243177 — [RFC 2/3] lib/vsprintf.c: move string() below widen_string()

This is pure code movement, making sure the widen_string() helper is
defined before the string() function.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 lib/vsprintf.c | 62 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 83b77796ac7e..acead77594b5 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -507,37 +507,6 @@ char *number(char *buf, char *end, unsigned long long num,
 	return buf;
 }
 
-static noinline_for_stack
-char *string(char *buf, char *end, const char *s, struct printf_spec spec)
-{
-	int len, i;
-
-	if ((unsigned long)s < PAGE_SIZE)
-		s = "(null)";
-
-	len = strnlen(s, spec.precision);
-
-	if (!(spec.flags & LEFT)) {
-		while (len < spec.field_width--) {
-			if (buf < end)
-				*buf = ' ';
-			++buf;
-		}
-	}
-	for (i = 0; i < len; ++i) {
-		if (buf < end)
-			*buf = *s;
-		++buf; ++s;
-	}
-	while (len < spec.field_width--) {
-		if (buf < end)
-			*buf = ' ';
-		++buf;
-	}
-
-	return buf;
-}
-
 static void move_right(char *buf, char *end, unsigned len, unsigned spaces)
 {
 	size_t size;
@@ -586,6 +555,37 @@ char *widen_string(char *buf, int n, char *end, struct printf_spec spec)
 }
 
 static noinline_for_stack
+char *string(char *buf, char *end, const char *s, struct printf_spec spec)
+{
+	int len, i;
+
+	if ((unsigned long)s < PAGE_SIZE)
+		s = "(null)";
+
+	len = strnlen(s, spec.precision);
+
+	if (!(spec.flags & LEFT)) {
+		while (len < spec.field_width--) {
+			if (buf < end)
+				*buf = ' ';
+			++buf;
+		}
+	}
+	for (i = 0; i < len; ++i) {
+		if (buf < end)
+			*buf = *s;
+		++buf; ++s;
+	}
+	while (len < spec.field_width--) {
+		if (buf < end)
+			*buf = ' ';
+		++buf;
+	}
+
+	return buf;
+}
+
+static noinline_for_stack
 char *dentry_name(char *buf, char *end, const struct dentry *d, struct printf_spec spec,
 		  const char *fmt)
 {
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1243845 — Re: [RFC 0/3] eliminate potential race in string() (was: [PATCH] string: Improve the generic strlcpy() implementation)

* Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Fri, Oct 09 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> 
> > It's hard not to agree with the overall "let's make it more robust if it
> > can be done sanely+cheaply+cleanly". I was a bit skeptical about whether
> > those three requirements could be met, since we'd have to do
> > byte-by-byte traversal of the string, maybe-copying it to the output as
> > we go along, but then right-alignment would require us to do a memmove,
> > but not before we've done some complicated bookkeeping
> > exercise. However, now that I read the source again, it seems that Al
> > Viro already did that exercise when he added dentry(). So maybe it's
> > doable without a net increase in LOC.
> 
> Something like this. The net increase is because I added a
> comment. Passes the new printf test suite, but I'm not sure that's
> thorough enough yet - still, it's better than nothing. There's also this
> small bonus:
> 
> $ scripts/bloat-o-meter /tmp/vsprintf.o.{old,new}
> add/remove: 1/0 grow/shrink: 0/2 up/down: 178/-245 (-67)
> function                                     old     new   delta
> widen_string.isra                              -     178    +178
> string.isra                                  186     109     -77
> dentry_name.isra                             358     190    -168
> 
> 
> Rasmus Villemoes (3):
>   lib/vsprintf.c: pull out padding code from dentry_name()
>   lib/vsprintf.c: move string() below widen_string()
>   lib/vsprintf.c: eliminate potential race in string()
> 
>  lib/vsprintf.c | 98 +++++++++++++++++++++++++++++++---------------------------
>  1 file changed, 52 insertions(+), 46 deletions(-)

Looks good to me!

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web