| Message ID | 20200311074325.7922-2-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: Use scnprintf() for avoiding potential buffer overflow | expand |
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;
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
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;
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(-)