Re: [patch] ASoC: soc: snprintf() doesn't return negative

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Dan Carpenter]

On Mon, Oct 11, 2010 at 11:40:09AM +0100, Mark Brown wrote:
> On Mon, Oct 11, 2010 at 05:54:17AM +0200, Dan Carpenter wrote:
> > In user space snprintf() returns negative on errors but the kernel
> > version only returns positives.  It could potentially return sizes
> 
> I'm not going to apply this. snprintf() returns a signed type, checking
> that the return value is a reasonable thing to do here - at worst we're
> wasting a few cycles in code that's nowhere near a hot path, at best
> we're robust in the face of a decision to add error reporting to
> snprintf() so it's hard to see this change as an improvement.
> 

It's not a matter of cycles.  My check probably takes the exact same
number of cycles as your check.  The point is that checking for negative
doesn't make any sort of sense.  If we did return an error code then we
should check it immediately after return instead of adding it to previous
return code.

The snprintf() function is documented.  It can't be changed because that
would break a ton of code as explained below.

> > larger than the size of the buffer so we should check for that.
> 
> > -	if (ret >= 0)
> > -		ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> > +	if (ret > PAGE_SIZE)
> > +		ret = PAGE_SIZE;
> > +
> > +	ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> 
> The PAGE_SIZE part of the change has an issue too, the code immediately
> preceeding this is:
> 
> 	list_for_each_entry(codec, &codec_list, list)
> 		ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s\n",
> 				codec->name);
> 
> so it's rather late to be worrying about PAGE_SIZE after the loop.
> 

There is no buffer overflow in these lines.  If "ret" *were* a negative
then there would be.  In that scenario we could end up copying data
before the start of the buffer because "buf - ret" could be before the
start of the buffer and we could end up copying after the end of the
buffer because "PAGE_SIZE - ret" would be larger than PAGE_SIZE.

If "PAGE_SIZE - ret" is a negative number then it triggers a
WARN_ON_ONCE() in vsnprintf().  It's not possible in this case however.
Really the original code works fine in practice because the information
we are printing out is less than PAGE_SIZE.

So my patch is effectively just a code cleanup.  If you don't apply it,
I'll probably sob into my pillow until it's wet but in the end it
doesn't matter much either way.  :P

regards,
dan carpenter

> Please also try to be a bit more thoughtful in your use of
> get_maintainers; try to have a look at why people have come up and
> consider if it's really sensible to include them.

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Mark Brown]

On Mon, Oct 11, 2010 at 06:40:38PM +0200, Dan Carpenter wrote:
> On Mon, Oct 11, 2010 at 11:40:09AM +0100, Mark Brown wrote:

> > I'm not going to apply this. snprintf() returns a signed type, checking
> > that the return value is a reasonable thing to do here - at worst we're
> > wasting a few cycles in code that's nowhere near a hot path, at best
> > we're robust in the face of a decision to add error reporting to
> > snprintf() so it's hard to see this change as an improvement.

> It's not a matter of cycles.  My check probably takes the exact same
> number of cycles as your check.  The point is that checking for negative
> doesn't make any sort of sense.  If we did return an error code then we

Remember that we also have to be able to read the code; it's probably
safe to assume that not everyone has the quirks of every snprintf()
implementation committed to memory.  Except for the microoptimisation
all you're doing here is making the code harder to read.

> If "PAGE_SIZE - ret" is a negative number then it triggers a
> WARN_ON_ONCE() in vsnprintf().  It's not possible in this case however.
> Really the original code works fine in practice because the information
> we are printing out is less than PAGE_SIZE.

In actual fact quite a few devices have enough registers to be
truncated, meaning that it's not only possible but likely we'll exercise
the cases that deal with the end of buffer.  If snprintf() is returning
values larger than buffer size it was given we're likely to have an
issue but it seems that there's something missing in your analysis since
we're never seeing WARN_ON()s and are instead seeing the behaviour the
code is intended to give, which is to truncate the output when we run
out of space.

Could you re-check your analysis, please?

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Dan Carpenter]

On Mon, Oct 11, 2010 at 07:51:48PM +0100, Mark Brown wrote:
> In actual fact quite a few devices have enough registers to be
> truncated, meaning that it's not only possible but likely we'll exercise
> the cases that deal with the end of buffer.  If snprintf() is returning
> values larger than buffer size it was given we're likely to have an
> issue but it seems that there's something missing in your analysis since
> we're never seeing WARN_ON()s and are instead seeing the behaviour the
> code is intended to give, which is to truncate the output when we run
> out of space.
> 
> Could you re-check your analysis, please?

That's odd.  I'm sorry, I can't explain why you wouldn't see a stack
trace...  The code is straight forward:

        /* Reject out-of-range values early.  Large positive sizes are
           used for unknown buffer sizes. */
        if (WARN_ON_ONCE((int) size < 0))
                return 0;

It would still give you truncated output but after the NULL terminator
there would be information leaked from the kernel.  If the reader
program had allocated a large enough buffer to handle the extra
information it wouldn't cause a problem.

regards,
dan carpenter

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Takashi Iwai]

At Mon, 11 Oct 2010 21:45:02 +0200,
Dan Carpenter wrote:
> 
> On Mon, Oct 11, 2010 at 07:51:48PM +0100, Mark Brown wrote:
> > In actual fact quite a few devices have enough registers to be
> > truncated, meaning that it's not only possible but likely we'll exercise
> > the cases that deal with the end of buffer.  If snprintf() is returning
> > values larger than buffer size it was given we're likely to have an
> > issue but it seems that there's something missing in your analysis since
> > we're never seeing WARN_ON()s and are instead seeing the behaviour the
> > code is intended to give, which is to truncate the output when we run
> > out of space.
> > 
> > Could you re-check your analysis, please?
> 
> That's odd.  I'm sorry, I can't explain why you wouldn't see a stack
> trace...  The code is straight forward:
> 
>         /* Reject out-of-range values early.  Large positive sizes are
>            used for unknown buffer sizes. */
>         if (WARN_ON_ONCE((int) size < 0))
>                 return 0;
> 
> It would still give you truncated output but after the NULL terminator
> there would be information leaked from the kernel.  If the reader
> program had allocated a large enough buffer to handle the extra
> information it wouldn't cause a problem.

Well, actually we should fix either:

- check the return of snprintf() at each time properly,

	list_for_each_entry(dai, &dai_list, list) {
		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
		if (len < 0)
			return len;
		ret += len;
	}

- or just assume it's never negative (as is on Linux kernel code)

In either case, a negative check after for loop is superfluous.

And, when no negative return value is assured (or filtered out like
above), there can't be overflow, too.  snprintf() fills the string
at most the size including NUL-char.  OTOH, it returns the size that
doesn't include NUL-char.


Takashi

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Mark Brown]

On Mon, Oct 11, 2010 at 10:57:40PM +0200, Takashi Iwai wrote:
> Dan Carpenter wrote:

> Well, actually we should fix either:

> - check the return of snprintf() at each time properly,
> 
> 	list_for_each_entry(dai, &dai_list, list) {
> 		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
> 		if (len < 0)
> 			return len;
> 		ret += len;
> 	}

In this case we're deliberately eating the error since all these files
are about getting diagnostics out - the code is intentionally soldiering
on and trying to get as much data out as possible rather than giving up
on error.

> In either case, a negative check after for loop is superfluous.

In those ones, yes - it's pretty much there for paranoia since the copy
to userspace is more likely to explode than random memory corruption.

> And, when no negative return value is assured (or filtered out like
> above), there can't be overflow, too.  snprintf() fills the string
> at most the size including NUL-char.  OTOH, it returns the size that
> doesn't include NUL-char.

Dan was saying that it would return sizes larger than the string it
wrote (which is a behaviour of some implementations) which would be
an issue since it would cause us to pass bad buffer pointers into
subsequent snprintf() calls.

I've not had time to look at this properly but Dan's analysis seems off.

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Takashi Iwai]

At Tue, 12 Oct 2010 10:35:07 +0100,
Mark Brown wrote:
> 
> On Mon, Oct 11, 2010 at 10:57:40PM +0200, Takashi Iwai wrote:
> > Dan Carpenter wrote:
> 
> > Well, actually we should fix either:
> 
> > - check the return of snprintf() at each time properly,
> > 
> > 	list_for_each_entry(dai, &dai_list, list) {
> > 		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
> > 		if (len < 0)
> > 			return len;
> > 		ret += len;
> > 	}
> 
> In this case we're deliberately eating the error since all these files
> are about getting diagnostics out - the code is intentionally soldiering
> on and trying to get as much data out as possible rather than giving up
> on error.

Then replace return with continue.  But, the error doesn't occur in
the case of kernel snprintf(), so it should be no issue.

> > In either case, a negative check after for loop is superfluous.
> 
> In those ones, yes - it's pretty much there for paranoia since the copy
> to userspace is more likely to explode than random memory corruption.

Well, the point is that checking the error at that point is logically
wrong and pointless.  It's not about optimization or such.  It's about
the code logic.

> > And, when no negative return value is assured (or filtered out like
> > above), there can't be overflow, too.  snprintf() fills the string
> > at most the size including NUL-char.  OTOH, it returns the size that
> > doesn't include NUL-char.
> 
> Dan was saying that it would return sizes larger than the string it
> wrote (which is a behaviour of some implementations) which would be
> an issue since it would cause us to pass bad buffer pointers into
> subsequent snprintf() calls.
> 
> I've not had time to look at this properly but Dan's analysis seems off.

Argh, yes, I'm (again) confused by that behavior.
The problem is the potential buffer overflow, indeed.  snprintf()
returns the size that would be printed.  Thus a safe code would be
like:

	list_for_each_entry(dai, &dai_list, list) {
 		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
 		if (len < 0)
 			continue;
 		ret += len;
		if (ret >= PAGE_SIZE) {
			ret = PAGE_SIZE;
			break;
		}
	}

Or, by assumption of non-negative return,

	list_for_each_entry(dai, &dai_list, list) {
 		ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
		if (ret >= PAGE_SIZE) {
			ret = PAGE_SIZE;
			break;
		}
	}


Takashi

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Mark Brown]

On Tue, Oct 12, 2010 at 11:49:27AM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> Argh, yes, I'm (again) confused by that behavior.
> The problem is the potential buffer overflow, indeed.  snprintf()
> returns the size that would be printed.  Thus a safe code would be
> like:
> 
> 	list_for_each_entry(dai, &dai_list, list) {
>  		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
>  		if (len < 0)
>  			continue;
>  		ret += len;
> 		if (ret >= PAGE_SIZE) {
> 			ret = PAGE_SIZE;
> 			break;
> 		}
> 	}

Yes, this form is better for that variant of the loop - that is safe and
legible without relying on current implementation details of snprintf().

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Dan Carpenter]

On Tue, Oct 12, 2010 at 10:56:05AM +0100, Mark Brown wrote:
> On Tue, Oct 12, 2010 at 11:49:27AM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > Argh, yes, I'm (again) confused by that behavior.
> > The problem is the potential buffer overflow, indeed.  snprintf()
> > returns the size that would be printed.  Thus a safe code would be
> > like:
> > 
> > 	list_for_each_entry(dai, &dai_list, list) {
> >  		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
> >  		if (len < 0)
> >  			continue;
> >  		ret += len;
> > 		if (ret >= PAGE_SIZE) {
> > 			ret = PAGE_SIZE;
> > 			break;
> > 		}
> > 	}
> 
> Yes, this form is better for that variant of the loop - that is safe and
> legible without relying on current implementation details of snprintf().

This is fine with me as well.  My original patch had a problem with the
WARN_ON() so your version is better.

regards,
dan carpenter

Re: [patch] ASoC: soc: snprintf() doesn't return negative

[Dan Carpenter]

On Mon, Oct 11, 2010 at 09:45:02PM +0200, Dan Carpenter wrote:
> On Mon, Oct 11, 2010 at 07:51:48PM +0100, Mark Brown wrote:
> > In actual fact quite a few devices have enough registers to be
> > truncated, meaning that it's not only possible but likely we'll exercise
> > the cases that deal with the end of buffer.  If snprintf() is returning
> > values larger than buffer size it was given we're likely to have an
> > issue but it seems that there's something missing in your analysis since
> > we're never seeing WARN_ON()s and are instead seeing the behaviour the
> > code is intended to give, which is to truncate the output when we run
> > out of space.
> > 
> > Could you re-check your analysis, please?
> 
> That's odd.  I'm sorry, I can't explain why you wouldn't see a stack
> trace...  The code is straight forward:
> 
>         /* Reject out-of-range values early.  Large positive sizes are
>            used for unknown buffer sizes. */
>         if (WARN_ON_ONCE((int) size < 0))
>                 return 0;
> 
> It would still give you truncated output but after the NULL terminator
> there would be information leaked from the kernel.  If the reader
> program had allocated a large enough buffer to handle the extra
> information it wouldn't cause a problem.
> 

Actually it will never cause a problem with userspace because we pass
the size of the userspace buffer to the kernel.  The only issues are the
information leak if the user passes in a 8k buffer and the also the
WARN_ON_ONCE()


regards,
dan carpenter