* [PATCH 0/2] iio: Use scnprintf() for avoiding potential buffer overflow @ 2020-03-11 7:43 Takashi Iwai 2020-03-11 7:43 ` [PATCH 1/2] iio: core: " Takashi Iwai 2020-03-11 7:43 ` [PATCH 2/2] iio: tsl2772: " Takashi Iwai 0 siblings, 2 replies; 11+ messages in thread From: Takashi Iwai @ 2020-03-11 7:43 UTC (permalink / raw) To: Jonathan Cameron, linux-iio Hi, a trivial patchset to replace snprintf() calls with the safer scnprintf() calls for avoiding potential buffer overflows. Takashi === Takashi Iwai (2): iio: core: Use scnprintf() for avoiding potential buffer overflow iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow drivers/iio/industrialio-core.c | 34 +++++++++++++++++----------------- drivers/iio/light/tsl2772.c | 4 ++-- 2 files changed, 19 insertions(+), 19 deletions(-) -- 2.16.4 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] iio: core: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 7:43 [PATCH 0/2] iio: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai @ 2020-03-11 7:43 ` Takashi Iwai 2020-03-15 9:54 ` Jonathan Cameron 2020-03-11 7:43 ` [PATCH 2/2] iio: tsl2772: " Takashi Iwai 1 sibling, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2020-03-11 7:43 UTC (permalink / raw) To: Jonathan Cameron, linux-iio 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/iio/industrialio-core.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 65ff0d067018..197006b5d5c2 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -559,46 +559,46 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, switch (type) { case IIO_VAL_INT: - return snprintf(buf, len, "%d", vals[0]); + return scnprintf(buf, len, "%d", vals[0]); case IIO_VAL_INT_PLUS_MICRO_DB: scale_db = true; /* fall through */ case IIO_VAL_INT_PLUS_MICRO: if (vals[1] < 0) - return snprintf(buf, len, "-%d.%06u%s", abs(vals[0]), + return scnprintf(buf, len, "-%d.%06u%s", abs(vals[0]), -vals[1], scale_db ? " dB" : ""); else - return snprintf(buf, len, "%d.%06u%s", vals[0], vals[1], + return scnprintf(buf, len, "%d.%06u%s", vals[0], vals[1], scale_db ? " dB" : ""); case IIO_VAL_INT_PLUS_NANO: if (vals[1] < 0) - return snprintf(buf, len, "-%d.%09u", abs(vals[0]), + return scnprintf(buf, len, "-%d.%09u", abs(vals[0]), -vals[1]); else - return snprintf(buf, len, "%d.%09u", vals[0], vals[1]); + return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]); case IIO_VAL_FRACTIONAL: tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); tmp1 = vals[1]; tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1); - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); + return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); case IIO_VAL_FRACTIONAL_LOG2: tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]); tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1); - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); + return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); case IIO_VAL_INT_MULTIPLE: { int i; int l = 0; for (i = 0; i < size; ++i) { - l += snprintf(&buf[l], len - l, "%d ", vals[i]); + l += scnprintf(&buf[l], len - l, "%d ", vals[i]); if (l >= len) break; } return l; } case IIO_VAL_CHAR: - return snprintf(buf, len, "%c", (char)vals[0]); + return scnprintf(buf, len, "%c", (char)vals[0]); default: return 0; } @@ -669,10 +669,10 @@ static ssize_t iio_format_avail_list(char *buf, const int *vals, if (len >= PAGE_SIZE) return -EFBIG; if (i < length - 1) - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, " "); else - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "\n"); if (len >= PAGE_SIZE) return -EFBIG; @@ -685,10 +685,10 @@ static ssize_t iio_format_avail_list(char *buf, const int *vals, if (len >= PAGE_SIZE) return -EFBIG; if (i < length / 2 - 1) - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, " "); else - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "\n"); if (len >= PAGE_SIZE) return -EFBIG; @@ -712,10 +712,10 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type) if (len >= PAGE_SIZE) return -EFBIG; if (i < 2) - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, " "); else - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "]\n"); if (len >= PAGE_SIZE) return -EFBIG; @@ -728,10 +728,10 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type) if (len >= PAGE_SIZE) return -EFBIG; if (i < 2) - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, " "); else - len += snprintf(buf + len, PAGE_SIZE - len, + len += scnprintf(buf + len, PAGE_SIZE - len, "]\n"); if (len >= PAGE_SIZE) return -EFBIG; -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iio: core: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 7:43 ` [PATCH 1/2] iio: core: " Takashi Iwai @ 2020-03-15 9:54 ` Jonathan Cameron 2020-03-15 10:17 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2020-03-15 9:54 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-iio On Wed, 11 Mar 2020 08:43:24 +0100 Takashi Iwai <tiwai@suse.de> 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(). Hmm. This is definitely in the somewhat theoretical bug category, but given it can be called to print a list of values that is defined in a driver, fair enough - it's good protection. I'm not going to rush this in given we don't have any drivers that are known to run into this. Hence I've queued it up for the togreg branch of iio.git targeting the merge window rather than as a fix. Thanks, Jonathan > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > drivers/iio/industrialio-core.c | 34 +++++++++++++++++----------------- > 1 file changed, 17 insertions(+), 17 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 65ff0d067018..197006b5d5c2 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -559,46 +559,46 @@ static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type, > > switch (type) { > case IIO_VAL_INT: > - return snprintf(buf, len, "%d", vals[0]); > + return scnprintf(buf, len, "%d", vals[0]); > case IIO_VAL_INT_PLUS_MICRO_DB: > scale_db = true; > /* fall through */ > case IIO_VAL_INT_PLUS_MICRO: > if (vals[1] < 0) > - return snprintf(buf, len, "-%d.%06u%s", abs(vals[0]), > + return scnprintf(buf, len, "-%d.%06u%s", abs(vals[0]), > -vals[1], scale_db ? " dB" : ""); > else > - return snprintf(buf, len, "%d.%06u%s", vals[0], vals[1], > + return scnprintf(buf, len, "%d.%06u%s", vals[0], vals[1], > scale_db ? " dB" : ""); > case IIO_VAL_INT_PLUS_NANO: > if (vals[1] < 0) > - return snprintf(buf, len, "-%d.%09u", abs(vals[0]), > + return scnprintf(buf, len, "-%d.%09u", abs(vals[0]), > -vals[1]); > else > - return snprintf(buf, len, "%d.%09u", vals[0], vals[1]); > + return scnprintf(buf, len, "%d.%09u", vals[0], vals[1]); > case IIO_VAL_FRACTIONAL: > tmp = div_s64((s64)vals[0] * 1000000000LL, vals[1]); > tmp1 = vals[1]; > tmp0 = (int)div_s64_rem(tmp, 1000000000, &tmp1); > - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > + return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > case IIO_VAL_FRACTIONAL_LOG2: > tmp = shift_right((s64)vals[0] * 1000000000LL, vals[1]); > tmp0 = (int)div_s64_rem(tmp, 1000000000LL, &tmp1); > - return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > + return scnprintf(buf, len, "%d.%09u", tmp0, abs(tmp1)); > case IIO_VAL_INT_MULTIPLE: > { > int i; > int l = 0; > > for (i = 0; i < size; ++i) { > - l += snprintf(&buf[l], len - l, "%d ", vals[i]); > + l += scnprintf(&buf[l], len - l, "%d ", vals[i]); > if (l >= len) > break; > } > return l; > } > case IIO_VAL_CHAR: > - return snprintf(buf, len, "%c", (char)vals[0]); > + return scnprintf(buf, len, "%c", (char)vals[0]); > default: > return 0; > } > @@ -669,10 +669,10 @@ static ssize_t iio_format_avail_list(char *buf, const int *vals, > if (len >= PAGE_SIZE) > return -EFBIG; > if (i < length - 1) > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > " "); > else > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > "\n"); > if (len >= PAGE_SIZE) > return -EFBIG; > @@ -685,10 +685,10 @@ static ssize_t iio_format_avail_list(char *buf, const int *vals, > if (len >= PAGE_SIZE) > return -EFBIG; > if (i < length / 2 - 1) > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > " "); > else > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > "\n"); > if (len >= PAGE_SIZE) > return -EFBIG; > @@ -712,10 +712,10 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type) > if (len >= PAGE_SIZE) > return -EFBIG; > if (i < 2) > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > " "); > else > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > "]\n"); > if (len >= PAGE_SIZE) > return -EFBIG; > @@ -728,10 +728,10 @@ static ssize_t iio_format_avail_range(char *buf, const int *vals, int type) > if (len >= PAGE_SIZE) > return -EFBIG; > if (i < 2) > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > " "); > else > - len += snprintf(buf + len, PAGE_SIZE - len, > + len += scnprintf(buf + len, PAGE_SIZE - len, > "]\n"); > if (len >= PAGE_SIZE) > return -EFBIG; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] iio: core: Use scnprintf() for avoiding potential buffer overflow 2020-03-15 9:54 ` Jonathan Cameron @ 2020-03-15 10:17 ` Takashi Iwai 0 siblings, 0 replies; 11+ messages in thread From: Takashi Iwai @ 2020-03-15 10:17 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio On Sun, 15 Mar 2020 10:54:46 +0100, Jonathan Cameron wrote: > > On Wed, 11 Mar 2020 08:43:24 +0100 > Takashi Iwai <tiwai@suse.de> 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(). > > Hmm. This is definitely in the somewhat theoretical bug category, but > given it can be called to print a list of values that is defined > in a driver, fair enough - it's good protection. > > I'm not going to rush this in given we don't have any drivers > that are known to run into this. Hence I've queued it up for > the togreg branch of iio.git targeting the merge window rather than > as a fix. Thanks, that's fine, the patches are just precautions. Takashi ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 7:43 [PATCH 0/2] iio: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai 2020-03-11 7:43 ` [PATCH 1/2] iio: core: " Takashi Iwai @ 2020-03-11 7:43 ` Takashi Iwai 2020-03-15 9:58 ` Jonathan Cameron 1 sibling, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2020-03-11 7:43 UTC (permalink / raw) To: Jonathan Cameron, linux-iio 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/iio/light/tsl2772.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c index be37fcbd4654..44a0b56a558c 100644 --- a/drivers/iio/light/tsl2772.c +++ b/drivers/iio/light/tsl2772.c @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, int offset = 0; while (i < TSL2772_MAX_LUX_TABLE_SIZE) { - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", chip->tsl2772_device_lux[i].ch0, chip->tsl2772_device_lux[i].ch1); if (chip->tsl2772_device_lux[i].ch0 == 0) { @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, i++; } - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); return offset; } -- 2.16.4 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-11 7:43 ` [PATCH 2/2] iio: tsl2772: " Takashi Iwai @ 2020-03-15 9:58 ` Jonathan Cameron 2020-03-15 10:25 ` Takashi Iwai 2020-03-15 10:33 ` Brian Masney 0 siblings, 2 replies; 11+ messages in thread From: Jonathan Cameron @ 2020-03-15 9:58 UTC (permalink / raw) To: Takashi Iwai; +Cc: linux-iio, Brian Masney On Wed, 11 Mar 2020 08:43:25 +0100 Takashi Iwai <tiwai@suse.de> 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> This one is printing a short well defined list of values. No way they go anywhere near the smallest possible PAGE_SIZE buffer that it's printing into. Which is handy given the remaining space isn't adjusted as we add items to the string. Hence even with scnprintf it would overflow. Brian, can you take a look at this when you get a moment? Thanks, Jonathan > --- > drivers/iio/light/tsl2772.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > index be37fcbd4654..44a0b56a558c 100644 > --- a/drivers/iio/light/tsl2772.c > +++ b/drivers/iio/light/tsl2772.c > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > int offset = 0; > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > chip->tsl2772_device_lux[i].ch0, > chip->tsl2772_device_lux[i].ch1); > if (chip->tsl2772_device_lux[i].ch0 == 0) { > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > i++; > } > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > return offset; > } > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-15 9:58 ` Jonathan Cameron @ 2020-03-15 10:25 ` Takashi Iwai 2020-03-15 10:33 ` Brian Masney 1 sibling, 0 replies; 11+ messages in thread From: Takashi Iwai @ 2020-03-15 10:25 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio, Brian Masney On Sun, 15 Mar 2020 10:58:34 +0100, Jonathan Cameron wrote: > > On Wed, 11 Mar 2020 08:43:25 +0100 > Takashi Iwai <tiwai@suse.de> 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> > > This one is printing a short well defined list of values. No way they go > anywhere near the smallest possible PAGE_SIZE buffer that it's printing > into. > > Which is handy given the remaining space isn't adjusted as we add items > to the string. Hence even with scnprintf it would overflow. Well, the issue handled here isn't whether the string is truncated. Rather whether the pointer passed to the function go beyond the boundary. In short, the call pattern like pos += snprintf(buf, limit - pos, ...); is wrong per design of snprintf(), unless you check the return value at each time. snprintf() doesn't return the actually truncated length but the length that would be. So, concatenating with snprintf() can go beyond the limit. Meanwhile, simply replacing it with scnprintf() avoids the wrong pointer increment effectively, as it returns the actual output size, so it'd return zero if it's all truncated. Practically seen, the current code should "work", but it's a wrong usage of snprintf(), as found in many other places. I've already submitted the patches to each subsystem to cover at least the concatenating cases like the above, and this is one of them. thanks, Takashi > > Brian, can you take a look at this when you get a moment? > > Thanks, > > Jonathan > > > --- > > drivers/iio/light/tsl2772.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > index be37fcbd4654..44a0b56a558c 100644 > > --- a/drivers/iio/light/tsl2772.c > > +++ b/drivers/iio/light/tsl2772.c > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > int offset = 0; > > > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > chip->tsl2772_device_lux[i].ch0, > > chip->tsl2772_device_lux[i].ch1); > > if (chip->tsl2772_device_lux[i].ch0 == 0) { > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > i++; > > } > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > > return offset; > > } > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-15 9:58 ` Jonathan Cameron 2020-03-15 10:25 ` Takashi Iwai @ 2020-03-15 10:33 ` Brian Masney 2020-03-15 13:10 ` Jonathan Cameron 1 sibling, 1 reply; 11+ messages in thread From: Brian Masney @ 2020-03-15 10:33 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Takashi Iwai, linux-iio On Sun, Mar 15, 2020 at 09:58:34AM +0000, Jonathan Cameron wrote: > On Wed, 11 Mar 2020 08:43:25 +0100 > Takashi Iwai <tiwai@suse.de> 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> > > This one is printing a short well defined list of values. No way they go > anywhere near the smallest possible PAGE_SIZE buffer that it's printing > into. > > Which is handy given the remaining space isn't adjusted as we add items > to the string. Hence even with scnprintf it would overflow. > > Brian, can you take a look at this when you get a moment? I also agree that this won't overflow in practice, however we should fix this up. Maybe the scnprintf() calls should be this: offset += scnprintf(buf + offset, PAGE_SIZE - offset, ...); Brian > > Thanks, > > Jonathan > > > --- > > drivers/iio/light/tsl2772.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > index be37fcbd4654..44a0b56a558c 100644 > > --- a/drivers/iio/light/tsl2772.c > > +++ b/drivers/iio/light/tsl2772.c > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > int offset = 0; > > > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > chip->tsl2772_device_lux[i].ch0, > > chip->tsl2772_device_lux[i].ch1); > > if (chip->tsl2772_device_lux[i].ch0 == 0) { > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > i++; > > } > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > > return offset; > > } > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-15 10:33 ` Brian Masney @ 2020-03-15 13:10 ` Jonathan Cameron 2020-03-16 8:04 ` Takashi Iwai 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Cameron @ 2020-03-15 13:10 UTC (permalink / raw) To: Brian Masney; +Cc: Takashi Iwai, linux-iio On Sun, 15 Mar 2020 06:33:58 -0400 Brian Masney <masneyb@onstation.org> wrote: > On Sun, Mar 15, 2020 at 09:58:34AM +0000, Jonathan Cameron wrote: > > On Wed, 11 Mar 2020 08:43:25 +0100 > > Takashi Iwai <tiwai@suse.de> 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> > > > > This one is printing a short well defined list of values. No way they go > > anywhere near the smallest possible PAGE_SIZE buffer that it's printing > > into. > > > > Which is handy given the remaining space isn't adjusted as we add items > > to the string. Hence even with scnprintf it would overflow. > > > > Brian, can you take a look at this when you get a moment? > > I also agree that this won't overflow in practice, however we should fix > this up. Maybe the scnprintf() calls should be this: > > offset += scnprintf(buf + offset, PAGE_SIZE - offset, ...); Agreed. Jonathan > > Brian > > > > > > Thanks, > > > > Jonathan > > > > > --- > > > drivers/iio/light/tsl2772.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > > index be37fcbd4654..44a0b56a558c 100644 > > > --- a/drivers/iio/light/tsl2772.c > > > +++ b/drivers/iio/light/tsl2772.c > > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > int offset = 0; > > > > > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > > > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > chip->tsl2772_device_lux[i].ch0, > > > chip->tsl2772_device_lux[i].ch1); > > > if (chip->tsl2772_device_lux[i].ch0 == 0) { > > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > i++; > > > } > > > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > > > return offset; > > > } > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-15 13:10 ` Jonathan Cameron @ 2020-03-16 8:04 ` Takashi Iwai 2020-03-16 11:50 ` Jonathan Cameron 0 siblings, 1 reply; 11+ messages in thread From: Takashi Iwai @ 2020-03-16 8:04 UTC (permalink / raw) To: Jonathan Cameron; +Cc: Brian Masney, Takashi Iwai, linux-iio On Sun, 15 Mar 2020 14:10:08 +0100, Jonathan Cameron wrote: > > On Sun, 15 Mar 2020 06:33:58 -0400 > Brian Masney <masneyb@onstation.org> wrote: > > > On Sun, Mar 15, 2020 at 09:58:34AM +0000, Jonathan Cameron wrote: > > > On Wed, 11 Mar 2020 08:43:25 +0100 > > > Takashi Iwai <tiwai@suse.de> 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> > > > > > > This one is printing a short well defined list of values. No way they go > > > anywhere near the smallest possible PAGE_SIZE buffer that it's printing > > > into. > > > > > > Which is handy given the remaining space isn't adjusted as we add items > > > to the string. Hence even with scnprintf it would overflow. > > > > > > Brian, can you take a look at this when you get a moment? > > > > I also agree that this won't overflow in practice, however we should fix > > this up. Maybe the scnprintf() calls should be this: > > > > offset += scnprintf(buf + offset, PAGE_SIZE - offset, ...); > > Agreed. Oh indeed, that must be. This was the totally overlooked point. So the code has to be corrected altogether. Shall I respin the patch with that change? thanks, Takashi > > Jonathan > > > > > Brian > > > > > > > > > > Thanks, > > > > > > Jonathan > > > > > > > --- > > > > drivers/iio/light/tsl2772.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > > > index be37fcbd4654..44a0b56a558c 100644 > > > > --- a/drivers/iio/light/tsl2772.c > > > > +++ b/drivers/iio/light/tsl2772.c > > > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > > int offset = 0; > > > > > > > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > > chip->tsl2772_device_lux[i].ch0, > > > > chip->tsl2772_device_lux[i].ch1); > > > > if (chip->tsl2772_device_lux[i].ch0 == 0) { > > > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > > i++; > > > > } > > > > > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > > > > return offset; > > > > } > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow 2020-03-16 8:04 ` Takashi Iwai @ 2020-03-16 11:50 ` Jonathan Cameron 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron @ 2020-03-16 11:50 UTC (permalink / raw) To: Takashi Iwai; +Cc: Jonathan Cameron, Brian Masney, linux-iio On Mon, 16 Mar 2020 09:04:46 +0100 Takashi Iwai <tiwai@suse.de> wrote: > On Sun, 15 Mar 2020 14:10:08 +0100, > Jonathan Cameron wrote: > > > > On Sun, 15 Mar 2020 06:33:58 -0400 > > Brian Masney <masneyb@onstation.org> wrote: > > > > > On Sun, Mar 15, 2020 at 09:58:34AM +0000, Jonathan Cameron wrote: > > > > On Wed, 11 Mar 2020 08:43:25 +0100 > > > > Takashi Iwai <tiwai@suse.de> 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> > > > > > > > > This one is printing a short well defined list of values. No way they go > > > > anywhere near the smallest possible PAGE_SIZE buffer that it's printing > > > > into. > > > > > > > > Which is handy given the remaining space isn't adjusted as we add items > > > > to the string. Hence even with scnprintf it would overflow. > > > > > > > > Brian, can you take a look at this when you get a moment? > > > > > > I also agree that this won't overflow in practice, however we should fix > > > this up. Maybe the scnprintf() calls should be this: > > > > > > offset += scnprintf(buf + offset, PAGE_SIZE - offset, ...); > > > > Agreed. > > Oh indeed, that must be. This was the totally overlooked point. > So the code has to be corrected altogether. > > Shall I respin the patch with that change? That would be great! It's always amazing what turns up once you start looking closely at a few lines of code :) Jonathan > > > thanks, > > Takashi > > > > > Jonathan > > > > > > > > Brian > > > > > > > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > > > --- > > > > > drivers/iio/light/tsl2772.c | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > > > > > index be37fcbd4654..44a0b56a558c 100644 > > > > > --- a/drivers/iio/light/tsl2772.c > > > > > +++ b/drivers/iio/light/tsl2772.c > > > > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > > > int offset = 0; > > > > > > > > > > while (i < TSL2772_MAX_LUX_TABLE_SIZE) { > > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,", > > > > > chip->tsl2772_device_lux[i].ch0, > > > > > chip->tsl2772_device_lux[i].ch1); > > > > > if (chip->tsl2772_device_lux[i].ch0 == 0) { > > > > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev, > > > > > i++; > > > > > } > > > > > > > > > > - offset += snprintf(buf + offset, PAGE_SIZE, "\n"); > > > > > + offset += scnprintf(buf + offset, PAGE_SIZE, "\n"); > > > > > return offset; > > > > > } > > > > > > > ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-03-16 11:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-03-11 7:43 [PATCH 0/2] iio: Use scnprintf() for avoiding potential buffer overflow Takashi Iwai 2020-03-11 7:43 ` [PATCH 1/2] iio: core: " Takashi Iwai 2020-03-15 9:54 ` Jonathan Cameron 2020-03-15 10:17 ` Takashi Iwai 2020-03-11 7:43 ` [PATCH 2/2] iio: tsl2772: " Takashi Iwai 2020-03-15 9:58 ` Jonathan Cameron 2020-03-15 10:25 ` Takashi Iwai 2020-03-15 10:33 ` Brian Masney 2020-03-15 13:10 ` Jonathan Cameron 2020-03-16 8:04 ` Takashi Iwai 2020-03-16 11:50 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).