Re: [PATCH v2 1/2] lib: parser: reject out-of-range values in match_number()

[shashank Jain]

Hi Alex,

> So your goal is simply to return an int as before, but make
> the function robustly handle all possible inputs.  It should
> return a valid value in *result when the input substring
> contains a value INT_MIN..INT_MAX inclusive, and return an
> error otherwise.

Yes, exactly.

> Would this work instead?

Yes, and it is simpler. I checked it against the callers and the
test:

- For the callers, the substring comes from match_one(), which ends a
  %d, %o or %x argument where simple_strtol()/simple_strtoul() stops
  parsing. So kstrtol() sees the same characters, and the only
  difference is that it rejects values that don't fit. I went through
  the in-tree match_int(), match_octal() and match_hex() callers I
  could find, and they all use %d, %o or %x tokens, none %s.

- It also covers 32-bit: there long is the same width as int, and
  kstrtol() itself returns -ERANGE for anything outside the long range,
  so the INT_MIN/INT_MAX check is only needed on 64-bit.

- The cases in the KUnit test in patch 2/2 give the same results with
  kstrtol(), so the test doesn't need to change.

kstrtol() also accepts a leading '+' and a trailing newline, but a
%d/%o/%x argument from match_token() never contains those.

I'll send a v3 of patch 1/2 with your version (with "int ret; long res;"
for the two declarations) and add a Suggested-by: for you, if that's OK.

Thanks,
Shashank

On Sat, Oct 3, 2026 at 2:51 AM Alex Elder <elder@xxxxxxxx> wrote:
>
> On 10/1/26 8:34 PM, shashank Jain wrote:
> > Hi Alex,
> >
> > Thanks for looking at this.
> >
> >> The result is still placed in memory referred to by an
> >> integer pointer, right?  Not a long long?
> >
> > Yes. The interface doesn't change: match_int(), match_octal() and
> > match_hex() still return an int through an int *, and the accepted
> > range is still INT_MIN..INT_MAX. The unsigned long long is only used
> > while parsing, so that a value outside the int range can be detected
> > and rejected with -ERANGE instead of being stored as a different int.
> > By the time *result is written, the value is known to fit.
>
> So your goal is simply to return an int as before, but make
> the function robustly handle all possible inputs.  It should
> return a valid value in *result when the input substring
> contains a value INT_MIN..INT_MAX inclusive, and return an
> error otherwise.
>
> Note that the comment above simple_strtol() says:
>
>    This function has caveats. Please use kstrtol instead.
>
> I started reviewing what exactly kstrtol() would do, to
> see if using that might be simpler.  It looks to me like
> it does some of the same things you do.
>
> I'm haven't run the unit test you provided, and I tried
> working through what would happen with kstrtol(), but
> gave up because it was taking too long.
>
> Would this work instead?
>
> static int match_number(substring_t *s, int *result, int base)
> {
>      char buf[NUMBER_BUF_LEN];
>      long res;
>      int res;
>
>      if (match_strlcpy(buf, s, NUMBER_BUF_LEN) >= NUMBER_BUF_LEN)
>          return -ERANGE;
>
>      ret = kstrtol(buf, base, &res);
>      if (ret)
>          return ret;
>
>      if (res < (long)INT_MIN || res > (long)INT_MAX)
>          return -ERANGE;
>
>      *result = (int)res;
>
>      return 0;
> }
>
>                                         -Alex
>
> >
> >> It might be better to define a new function that expects to
> >> produce (up to) 64 bit result values (signed, or maybe not),
> >> rather than changing this function.
> >
> > For callers that need 64-bit values there is already match_u64()
> > (match_u64int() with kstrtoull()), so I don't think a new function is
> > needed. This patch is only about the int helpers accepting
> > out-of-range input.
> >
> >> Or... maybe there's a simpler way to catch a too-long
> >> input string.
> >
> > I looked at that, but the length of the string doesn't decide whether
> > the value fits: "4294967295" is only 10 characters and doesn't fit in
> > an int, while leading zeros make a long string that does
> > ("00000000000000000001"). What is needed is the overflow indication,
> > which simple_strtoull() drops (there is a FIXME at that spot in
> > simple_strntoull()). _parse_integer() returns it, so the patch parses
> > the same way simple_strtol() does, keeps that flag, and compares the
> > magnitude with the int range.
> >
> >> This matters because match_number() calls simple_strtol(),
> >> which is implemented using simple_strtoul(), and that is
> >> implemented using simple_strtoull().  So if a very long
> >> numeric string is passed to match_number(), it will be
> >> misinterpreted as something that overflows 32 bits,
> >> which will end up having a misleading (but essentially
> >> valid) int value.
> >>
> >> Is that correct?
> >
> > Yes, that is the problem, in two forms:
> >
> > - On 64-bit, a value of 2^64 or more saturates to ULLONG_MAX in
> >    simple_strtoull(), and simple_strtol() turns that into -1L, which
> >    passes the INT_MIN/INT_MAX check. Values from 2^64 - 2^31 to
> >    2^64 - 1 wrap to small negative longs the same way. So match_int()
> >    returns 0 with *result = -1 for "18446744073709551615" or
> >    "99999999999999999999".
> >
> > - On 32-bit, long is as wide as int, so the range check can never
> >    fail: "4294967295" also gives -1, and "2147483648" gives INT_MIN.
> >
> > In both cases the caller gets success and a different number from the
> > one that was written, for example in a mount option. With the patch
> > these return -ERANGE. Values that fit in an int are accepted exactly
> > as before, with the same result and the same syntax.
> >
> > Sorry the changelog made it sound as if the result were becoming
> > 64-bit; only the intermediate magnitude is. I can reword that in a v3
> > if it helps.
> >
> > Thanks,
> > Shashank
> >
> > On Fri, Oct 2, 2026 at 3:07 AM Alex Elder <elder@xxxxxxxx> wrote:
> >>
> >> On 9/25/26 8:27 PM, Shashank Mohan Jain wrote:
> >>> match_int(), match_octal() and match_hex() store the result in an int
> >>> and return -EINVAL or -ERANGE on failure.  match_number() checks the
> >>> range by parsing into a long with simple_strtol() and comparing against
> >>> INT_MIN/INT_MAX, a check added by commit 77dd3b0bd17a ("lib/parser.c:
> >>> avoid overflow in match_number()").  That does not catch every
> >>> out-of-range input:
> >>
> >> The result is still placed in memory referred to by an
> >> integer pointer, right?  Not a long long?
> >>
> >> It might be better to define a new function that expects to
> >> produce (up to) 64 bit result values (signed, or maybe not),
> >> rather than changing this function.
> >>
> >> Or... maybe there's a simpler way to catch a too-long
> >> input string.
> >>>
> >>>    - simple_strtoull() saturates to ULLONG_MAX on overflow and
> >>
> >> This matters because match_number() calls simple_strtol(),
> >> which is implemented using simple_strtoul(), and that is
> >> implemented using simple_strtoull().  So if a very long
> >> numeric string is passed to match_number(), it will be
> >> misinterpreted as something that overflows 32 bits,
> >> which will end up having a misleading (but essentially
> >> valid) int value.
> >>
> >> Is that correct?
> >>
> >> At this point I'm just trying to make sure I understand what
> >> your intent is, because I'm unsure this is the right way to
> >> achieve what you're after.
> >>
> >> I guess I'm stuck on match_number() expecting to produce
> >> an int result, and your discussion talks a lot about 64-bit
> >> values.
> >>
> >>                                          -Alex
> >>
> >>
> >>>      simple_strtol() simply converts its result to long, so any value of
> >>>      at least 2^64 - 2^31, and anything that overflows 64 bits, ends up
> >>>      inside the int range.  On 64-bit, match_int() returns 0 and sets
> >>>      the result to -1 for "18446744073709551615" or
> >>>      "99999999999999999999", match_hex() does the same for
> >>>      "ffffffffffffffff", and "-18446744073709551615" gives 1.
> >>>
> >>>    - On 32-bit, long has the same width as int, so the range check can
> >>>      never fail: "2147483648" gives INT_MIN and "4294967295" gives -1.
> >>>
> >>> On 64-bit, values from INT_MAX + 1 up to 2^64 - 2^31 - 1 are already
> >>> rejected, so for those this only makes 32-bit kernels behave like
> >>> 64-bit ones.  Two callers store the result in a u32 and so accepted
> >>> such values on 32-bit only: the legacy NFSv4 idmapper upcall
> >>> (fs/nfs/nfs4idmap.c), which falls back to a numeric id when the lookup
> >>> fails, and rd_pages= in drivers/target/target_core_rd.c, which also
> >>> ignores match_int()'s return value.
> >>>
> >>> These helpers parse mount options and similar user-supplied strings,
> >>> so an out-of-range number is silently accepted as a different value
> >>> instead of being rejected.
> >>>
> >>> Parse the number the same way simple_strtol() does, using the same
> >>> kstrtox helpers, but keep the unsigned magnitude and the overflow
> >>> indication, and check the magnitude against the int range.  The
> >>> accepted syntax, including what counts as "no number" (-EINVAL), is
> >>> unchanged.  kstrtoint() is not used because it rejects trailing
> >>> characters ("12abc" gives 12 today) and differs in the handling of a
> >>> lone "-" or "0x".
> >>>
> >>> Fixes: 77dd3b0bd17a ("lib/parser.c: avoid overflow in match_number()")
> >>> Assisted-by: LLM
> >>> Signed-off-by: Shashank Mohan Jain <jain.sm@xxxxxxxxx>
> >>> ---
> >>> These patches were prepared with Claude Code (Anthropic), model Claude Opus 5.5
> >>> (claude-opus-5-5): the analysis, the Lean models used to find and check the bugs,
> >>> the fixes and the tests. I reviewed them and take responsibility for them. The
> >>> trailer only says "Assisted-by: LLM", as Documentation/process/coding-assistants.rst
> >>> requires since commit 816d9992d9ed ("coding-assistants: simplify attribution").
> >>>
> >>> Changes in v2:
> >>> - Resent with my full name; no code changes.
> >>>
> >>> v1: https://lore.kernel.org/r/20260925102334.49693-1-jain.sm@xxxxxxxxx
> >>>
> >>>    lib/parser.c | 39 +++++++++++++++++++++++++++------------
> >>>    1 file changed, 27 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/lib/parser.c b/lib/parser.c
> >>> index 62da0ac0d438..30fdaefe4957 100644
> >>> --- a/lib/parser.c
> >>> +++ b/lib/parser.c
> >>> @@ -11,6 +11,8 @@
> >>>    #include <linux/slab.h>
> >>>    #include <linux/string.h>
> >>>
> >>> +#include "kstrtox.h"
> >>> +
> >>>    /*
> >>>     * max size needed by different bases to express U64
> >>>     * HEX: "0xFFFFFFFFFFFFFFFF" --> 18
> >>> @@ -137,22 +139,35 @@ EXPORT_SYMBOL(match_token);
> >>>     */
> >>>    static int match_number(substring_t *s, int *result, int base)
> >>>    {
> >>> -     char *endp;
> >>>        char buf[NUMBER_BUF_LEN];
> >>> -     int ret;
> >>> -     long val;
> >>> +     unsigned long long val;
> >>> +     unsigned int radix = base;
> >>> +     const char *cp = buf;
> >>> +     bool negative;
> >>> +     unsigned int rv;
> >>>
> >>>        if (match_strlcpy(buf, s, NUMBER_BUF_LEN) >= NUMBER_BUF_LEN)
> >>>                return -ERANGE;
> >>> -     ret = 0;
> >>> -     val = simple_strtol(buf, &endp, base);
> >>> -     if (endp == buf)
> >>> -             ret = -EINVAL;
> >>> -     else if (val < (long)INT_MIN || val > (long)INT_MAX)
> >>> -             ret = -ERANGE;
> >>> -     else
> >>> -             *result = (int) val;
> >>> -     return ret;
> >>> +
> >>> +     /*
> >>> +      * Parse like simple_strtol() does, but keep the magnitude and the
> >>> +      * overflow indication instead of truncating to a long, so that
> >>> +      * values outside the range of an int can be rejected.
> >>> +      */
> >>> +     negative = *cp == '-';
> >>> +     if (negative)
> >>> +             cp++;
> >>> +     cp = _parse_integer_fixup_radix(cp, &radix);
> >>> +     rv = _parse_integer(cp, radix, &val);
> >>> +     if (cp + (rv & ~KSTRTOX_OVERFLOW) == buf)
> >>> +             return -EINVAL;
> >>> +
> >>> +     if (rv & KSTRTOX_OVERFLOW ||
> >>> +         val > (negative ? (unsigned long long)INT_MAX + 1 : INT_MAX))
> >>> +             return -ERANGE;
> >>> +
> >>> +     *result = negative ? -(long long)val : (long long)val;
> >>> +     return 0;
> >>>    }
> >>>
> >>>    /**
> >>
>