| Message ID | 20230607223755.1610-2-richard@nod.at (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Integer overflows while scanning for integers | expand |
On Thu, Jun 08, 2023 at 12:37:55AM +0200, Richard Weinberger wrote: > The scanf function family has no way to indicate overflows > while scanning. As consequence users of these function have to make > sure their input cannot cause an overflow. > Since this is not always the case add WARN_ON_ONCE() guards to > trigger a warning upon an overflow. ... > if (prefix_chars < max_chars) { > rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); > + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); This seems incorrect. simple_strto*() are okay to overflow. It's by design. > /* FIXME */ ...and that's why this one is here. > cp += (rv & ~KSTRTOX_OVERFLOW); > } else {
----- Ursprüngliche Mail ----- > Von: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> >> if (prefix_chars < max_chars) { >> rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); >> + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); > > This seems incorrect. simple_strto*() are okay to overflow. It's by design. Is this design decision also known to all users of scanf functions in the kernel? Thanks, //richard
On Thu, Jun 08, 2023 at 06:14:33PM +0200, Richard Weinberger wrote: > ----- Ursprüngliche Mail ----- > > Von: "Andy Shevchenko" <andriy.shevchenko@linux.intel.com> > >> if (prefix_chars < max_chars) { > >> rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); > >> + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); > > > > This seems incorrect. simple_strto*() are okay to overflow. It's by design. > > Is this design decision also known to all users of scanf functions in the kernel? We have test_scanf.c. Does it miss any test cases? Please add them!
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 40f560959b169..3d8d751306cdc 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -70,6 +70,7 @@ static noinline unsigned long long simple_strntoull(const char *startp, size_t m prefix_chars = cp - startp; if (prefix_chars < max_chars) { rv = _parse_integer_limit(cp, base, &result, max_chars - prefix_chars); + WARN_ON_ONCE(rv & KSTRTOX_OVERFLOW); /* FIXME */ cp += (rv & ~KSTRTOX_OVERFLOW); } else { @@ -3657,22 +3658,34 @@ int vsscanf(const char *buf, const char *fmt, va_list args) switch (qualifier) { case 'H': /* that's 'hh' in format */ - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > 127); + WARN_ON_ONCE(val.s < -128); *va_arg(args, signed char *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > 255); *va_arg(args, unsigned char *) = val.u; + } break; case 'h': - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > SHRT_MAX); + WARN_ON_ONCE(val.s < SHRT_MIN); *va_arg(args, short *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > USHRT_MAX); *va_arg(args, unsigned short *) = val.u; + } break; case 'l': - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > LONG_MAX); + WARN_ON_ONCE(val.s < LONG_MIN); *va_arg(args, long *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > ULONG_MAX); *va_arg(args, unsigned long *) = val.u; + } break; case 'L': if (is_sign) @@ -3684,10 +3697,14 @@ int vsscanf(const char *buf, const char *fmt, va_list args) *va_arg(args, size_t *) = val.u; break; default: - if (is_sign) + if (is_sign) { + WARN_ON_ONCE(val.s > INT_MAX); + WARN_ON_ONCE(val.s < INT_MIN); *va_arg(args, int *) = val.s; - else + } else { + WARN_ON_ONCE(val.u > UINT_MAX); *va_arg(args, unsigned int *) = val.u; + } break; } num++;
The scanf function family has no way to indicate overflows while scanning. As consequence users of these function have to make sure their input cannot cause an overflow. Since this is not always the case add WARN_ON_ONCE() guards to trigger a warning upon an overflow. Signed-off-by: Richard Weinberger <richard@nod.at> --- lib/vsprintf.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)