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 <[email protected]>

In user space snprintf() returns negative on errors but the kernel version only returns positives. It could potentially return sizes

Quoted 2 lines · Dan Carpenter

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.

sound/soc/soc-core.c static ssize_t codec_list_read_file(struct file *file, char __user *user_buf, · line 290

Quoted 8 lines

larger than the size of the buffer so we should check for that.

sound/soc/soc-core.c +4 -2

-	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.

Quoted 5 lines · Mark Brown

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

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?

Quoted 10 lines · Mark Brown

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.

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 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.

Quoted 4 lines · Takashi Iwai

Dan Carpenter wrote:

Well, actually we should fix either:

Quoted 8 lines

- 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.

Quoted 1 line

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.

Quoted 4 lines

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.

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) {

(quoted diff) +0 -0

 		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;
		}
	}

Quoted 12 lines · Takashi Iwai

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) {

(quoted diff) +0 -0

 		int len = snprintf(buf + ret, PAGE_SIZE - ret, "%s\n", dai->name);
 		if (len < 0)
 			continue;
 		ret += len;

Quoted 5 lines

		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().

Quoted 10 lines · Mark Brown

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.