[alsa-devel] [patch] ASoC: soc: snprintf() doesn't return negative
In user space snprintf() returns negative on errors but the kernel version only returns positives. It could potentially return sizes larger than the size of the buffer so we should check for that.
Signed-off-by: Dan Carpenter error27@gmail.com
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 8751efd..8da307b 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -284,8 +284,10 @@ static ssize_t codec_list_read_file(struct file *file, char __user *user_buf, ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", codec->name);
- 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);
kfree(buf);
@@ -310,8 +312,10 @@ static ssize_t dai_list_read_file(struct file *file, char __user *user_buf, list_for_each_entry(dai, &dai_list, list) ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
- 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);
kfree(buf);
@@ -338,8 +342,10 @@ static ssize_t platform_list_read_file(struct file *file, ret += snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", platform->name);
- 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);
kfree(buf);
On Mon, 2010-10-11 at 05:54 +0200, Dan Carpenter wrote:
In user space snprintf() returns negative on errors but the kernel version only returns positives. It could potentially return sizes larger than the size of the buffer so we should check for that.
Signed-off-by: Dan Carpenter error27@gmail.com
Acked-by: Liam Girdwood lrg@slimlogic.co.uk
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.
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.
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.
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.
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?
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
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
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.
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
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().
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
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
participants (4)
-
Dan Carpenter -
Liam Girdwood -
Mark Brown -
Takashi Iwai