Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/greybus/tools/loopback_test.c b/drivers/staging/greybus/tools/loopback_test.c index ba6f905f26fa..0b95a1c2b2e9 100644 --- a/drivers/staging/greybus/tools/loopback_test.c +++ b/drivers/staging/greybus/tools/loopback_test.c @@ -426,7 +426,7 @@ int format_output(struct loopback_test *t, tm->tm_hour, tm->tm_min, tm->tm_sec);
if (t->porcelain) { - len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, "\n test:\t\t\t%s\n path:\t\t\t%s\n size:\t\t\t%u\n iterations:\t\t%u\n errors:\t\t%u\n async:\t\t\t%s\n", t->test_name, dev_name, @@ -435,33 +435,33 @@ int format_output(struct loopback_test *t, r->error, t->use_async ? "Enabled" : "Disabled");
- len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, " requests per-sec:\tmin=%u, max=%u, average=%f, jitter=%u\n", r->request_min, r->request_max, r->request_avg, r->request_jitter);
- len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, " ap-throughput B/s:\tmin=%u max=%u average=%f jitter=%u\n", r->throughput_min, r->throughput_max, r->throughput_avg, r->throughput_jitter); - len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, " ap-latency usec:\tmin=%u max=%u average=%f jitter=%u\n", r->latency_min, r->latency_max, r->latency_avg, r->latency_jitter); - len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, " apbridge-latency usec:\tmin=%u max=%u average=%f jitter=%u\n", r->apbridge_unipro_latency_min, r->apbridge_unipro_latency_max, r->apbridge_unipro_latency_avg, r->apbridge_unipro_latency_jitter);
- len += snprintf(&buf[len], buf_len - len, + len += scnprintf(&buf[len], buf_len - len, " gbphy-latency usec:\tmin=%u max=%u average=%f jitter=%u\n", r->gbphy_firmware_latency_min, r->gbphy_firmware_latency_max, @@ -469,35 +469,35 @@ int format_output(struct loopback_test *t, r->gbphy_firmware_latency_jitter);
} else { - len += snprintf(&buf[len], buf_len - len, ",%s,%s,%u,%u,%u", + len += scnprintf(&buf[len], buf_len - len, ",%s,%s,%u,%u,%u", t->test_name, dev_name, t->size, t->iteration_max, r->error);
- len += snprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", + len += scnprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", r->request_min, r->request_max, r->request_avg, r->request_jitter);
- len += snprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", + len += scnprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", r->latency_min, r->latency_max, r->latency_avg, r->latency_jitter);
- len += snprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", + len += scnprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", r->throughput_min, r->throughput_max, r->throughput_avg, r->throughput_jitter);
- len += snprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", + len += scnprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", r->apbridge_unipro_latency_min, r->apbridge_unipro_latency_max, r->apbridge_unipro_latency_avg, r->apbridge_unipro_latency_jitter);
- len += snprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", + len += scnprintf(&buf[len], buf_len - len, ",%u,%u,%f,%u", r->gbphy_firmware_latency_min, r->gbphy_firmware_latency_max, r->gbphy_firmware_latency_avg,
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
Johan
On Wed, 11 Mar 2020 10:58:14 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
OK, will do that.
thanks,
Takashi
On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 10:58:14 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
OK, will do that.
Thanks.
Perhaps you should replace the snprintf() at the start of the function in question as well by the way.
Johan
On Wed, 11 Mar 2020 11:09:03 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 10:58:14 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
OK, will do that.
Thanks.
Perhaps you should replace the snprintf() at the start of the function in question as well by the way.
Yeah, it's I also wonder while working on many other codes, too. I decided to minimize the changes at this time and concentrate only on the code that has a pattern like: pos += snprintf(buf, limit - pos, ...)
thanks,
Takashi
On Wed, Mar 11, 2020 at 12:01:26PM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 11:09:03 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 10:58:14 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
OK, will do that.
Thanks.
Perhaps you should replace the snprintf() at the start of the function in question as well by the way.
Yeah, it's I also wonder while working on many other codes, too. I decided to minimize the changes at this time and concentrate only on the code that has a pattern like: pos += snprintf(buf, limit - pos, ...)
But isn't the first snprintf() in such a sequence as much a part of the problem as the following ones?
If the first pos = snprintf(buf, limit, ...) overflows buf, then the next pos += snprintf(buf, limit - pos, ...) will be called with with a negative size argument (i.e. a very large unsigned value), which effectively breaks the length check regardless of whether you replace it with scnprintf() or not. And all later calls will similarly continue writing beyond the end of buf.
But wait a minute. This is user-space code, so there's no scnprintf(). Did you not compile test this? ;P
In fact it seems no-one has for a while. This code is just broken and doesn't even compile any more. Maybe we should just drop it instead.
Johan
On Wed, 11 Mar 2020 17:40:02 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 12:01:26PM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 11:09:03 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 11:02:33AM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 10:58:14 +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
OK, will do that.
Thanks.
Perhaps you should replace the snprintf() at the start of the function in question as well by the way.
Yeah, it's I also wonder while working on many other codes, too. I decided to minimize the changes at this time and concentrate only on the code that has a pattern like: pos += snprintf(buf, limit - pos, ...)
But isn't the first snprintf() in such a sequence as much a part of the problem as the following ones?
If the first pos = snprintf(buf, limit, ...) overflows buf, then the next pos += snprintf(buf, limit - pos, ...) will be called with with a negative size argument (i.e. a very large unsigned value), which effectively breaks the length check regardless of whether you replace it with scnprintf() or not. And all later calls will similarly continue writing beyond the end of buf.
Yeah, that's the possible case although most calls are fine with it since the limit is PAGE_SIZE or so. This might need a bit more special care.
But wait a minute. This is user-space code, so there's no scnprintf(). Did you not compile test this? ;P
In fact it seems no-one has for a while. This code is just broken and doesn't even compile any more. Maybe we should just drop it instead.
Bah, I'm afraid that I overlooked this point!
I've scraped over many places via a script-like work, and did the compile testing of the kernel, but not about tools. If that's the case, sorry for the mess, feel free to drop it.
Takashi
On Wed, Mar 11, 2020 at 05:45:31PM +0100, Takashi Iwai wrote:
On Wed, 11 Mar 2020 17:40:02 +0100, Johan Hovold wrote:
But isn't the first snprintf() in such a sequence as much a part of the problem as the following ones?
If the first pos = snprintf(buf, limit, ...) overflows buf, then the next pos += snprintf(buf, limit - pos, ...) will be called with with a negative size argument (i.e. a very large unsigned value), which effectively breaks the length check regardless of whether you replace it with scnprintf() or not. And all later calls will similarly continue writing beyond the end of buf.
Yeah, that's the possible case although most calls are fine with it since the limit is PAGE_SIZE or so. This might need a bit more special care.
Yeah, sure, it should be fine here too currently, even if the buffer size is defined in the caller.
But wait a minute. This is user-space code, so there's no scnprintf(). Did you not compile test this? ;P
In fact it seems no-one has for a while. This code is just broken and doesn't even compile any more. Maybe we should just drop it instead.
Bah, I'm afraid that I overlooked this point!
I've scraped over many places via a script-like work, and did the compile testing of the kernel, but not about tools. If that's the case, sorry for the mess, feel free to drop it.
No worries. I'll try to take a look at the other breakages tomorrow and see if there's any point trying to salvage this at all.
Johan
On Wed, Mar 11, 2020 at 10:58:14AM +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
I'm surprised that user-space even has scnprintf().
regards, dan carpenter
On Thu, Mar 12, 2020 at 05:51:11PM +0300, Dan Carpenter wrote:
On Wed, Mar 11, 2020 at 10:58:14AM +0100, Johan Hovold wrote:
On Wed, Mar 11, 2020 at 10:19:06AM +0100, Takashi Iwai wrote:
Since snprintf() returns the would-be-output size instead of the actual output size, the succeeding calls may go beyond the given buffer limit. Fix it by replacing with scnprintf().
Signed-off-by: Takashi Iwai tiwai@suse.de
drivers/staging/greybus/tools/loopback_test.c | 24 ++++++++++++------------
Thanks for the fix.
Would you mind resending with a "staging: greybus: loopback_test:" prefix since this is not a subsystem wide issue, bur rather a bug in a specific user-space tool?
I'm surprised that user-space even has scnprintf().
Yeah, see the rest of the thread.
Johan
-
Dan Carpenter -
Johan Hovold -
Takashi Iwai