[PATCH 0/3] sysctl: detect overflows when setting integers

[PATCH 0/3] sysctl: detect overflows when setting integers

[Heinrich Schuchardt]

This patch series addresses undetected overflows when writing to the
sysctl file system.

E.g.
echo 0x800001234 > /proc/sys/kernel/threads-max
has the same effect as
echo 0x1234 > /proc/sys/kernel/threads-max

The first type of overflow occurs when converting from string to unsigned long.
The second type of overflow occurs when converting from unsigned long to int.

The first patch provide new functions kstrtoul_e and kstrtoull_e that can be
used to replace deprecated simple_strtoul and simple_strtoull.

The second patch replaces a call to simple_strtoul by kstrtoul_e. This is
necessary to detect overflows when converting from string to unsigned long.

The third patch adds checks when converting form unsigned long to int.

Heinrich Schuchardt (3):
  lib/kstrtox.c: functions returning end of string
  sysctl: detect overflows in proc_get_long
  sysctl: detect overflows when converting to int

 include/linux/kernel.h |  4 +++
 kernel/sysctl.c        | 13 +++++++--
 lib/kstrtox.c          | 71 +++++++++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 83 insertions(+), 5 deletions(-)

-- 
2.1.4

Re: [PATCH 0/3] sysctl: detect overflows when setting integers

[Alexey Dobriyan]

> Unfortunately functions simple_strtoul and simple_strtoull cannot
> be replaced by kstrtoul and kstrtoull in some places, because they
> expect a zero terminated string instead of returning a pointer to
> the character after the last digit.
>
> This patch introduces two new functions kstrtoul_e and kstrtoull_e
> which fill this gap.

Well, there were two ideas:
a) to convert first, see what's left and generalize it,
b) kstrtox() should be used only in one place --
   parsing integers in proc/sysfs files.

The functions can probably be replaced by sscanf() (I didn't look closely).

I hate "_e" suffix with passion.

C in 2015 doesn't have this arcane concept known as optional
parameters so I'd suggest to hack around with always returning
number of OK characters OR add one (just one) more separate
interface

   unsigned int parse_integer(const char *s, unsigned int base, T *p);

and dispatching with __builtin_choose_expr().

Of course if someone rewrites that abomination called
sysctl parsing from scratch, maybe none of this will be needed!

  Alexey

Re: [PATCH 0/3] sysctl: detect overflows when setting integers

[Heinrich Schuchardt]

Hello Alexey,

thank you for reviewing.

On 30.03.2015 14:34, Alexey Dobriyan wrote:
>> Unfortunately functions simple_strtoul and simple_strtoull cannot
>> be replaced by kstrtoul and kstrtoull in some places, because they
>> expect a zero terminated string instead of returning a pointer to
>> the character after the last digit.
>>
>> This patch introduces two new functions kstrtoul_e and kstrtoull_e
>> which fill this gap.
> 
> Well, there were two ideas:
> a) to convert first, see what's left and generalize it,
> b) kstrtox() should be used only in one place --
>    parsing integers in proc/sysfs files.

Neither a) nor b) are mentioned in your patch
https://lkml.org/lkml/2011/2/26/52.

How does a) apply to my patch series?

The patch series is about parsing integers inside the /proc mount so
what is the conflict with b)?

vsprintf.c has the following comment for simple_strtol:
* This function is obsolete. Please use kstrtol instead.

Could you, please, elaborate why kstrox.c should not be used in other
places? Should we duplicate these functions instead of reusing them?

> 
> The functions can probably be replaced by sscanf() (I didn't look closely).

No. sscanf does not return the end of the parsed string. It requires to
know beforehand if the string contains a octal, decimal or hexadecimal
number.

> 
> I hate "_e" suffix with passion.

Which names would you appreciate?

> 
> C in 2015 doesn't have this arcane concept known as optional
> parameters so I'd suggest to hack around with always returning
> number of OK characters

Which functions should return the number of OK characters?

> OR add one (just one) more separate
> interface
> 
>    unsigned int parse_integer(const char *s, unsigned int base, T *p);

Why would you call this function parse_integer and not kstrtox?

> 
> and dispatching with __builtin_choose_expr().

https://gcc.gnu.org/onlinedocs/gcc-3.4.5/gcc/Other-Builtins.html
describes how to use __builtin_types_compatible_p together with
__builtin_choose_expr().

I do not understand how to avoid exposing the interfaces of the
functions actually called by a parse_integer function multiplexer.

> 
> Of course if someone rewrites that abomination called
> sysctl parsing from scratch, maybe none of this will be needed!

Unfortunately you are very vague about what you dislike in sysctl.

When replying, please use the existing mail thread.

Best regards

Heinrich

Re: [PATCH 0/3] sysctl: detect overflows when setting integers

[Alexey Dobriyan]

On Mon, Mar 30, 2015 at 8:37 PM, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Hello Alexey,
>
> thank you for reviewing.
>
> On 30.03.2015 14:34, Alexey Dobriyan wrote:
>>> Unfortunately functions simple_strtoul and simple_strtoull cannot
>>> be replaced by kstrtoul and kstrtoull in some places, because they
>>> expect a zero terminated string instead of returning a pointer to
>>> the character after the last digit.
>>>
>>> This patch introduces two new functions kstrtoul_e and kstrtoull_e
>>> which fill this gap.
>>
>> Well, there were two ideas:
>> a) to convert first, see what's left and generalize it,
>> b) kstrtox() should be used only in one place --
>>    parsing integers in proc/sysfs files.
>
> Neither a) nor b) are mentioned in your patch
> https://lkml.org/lkml/2011/2/26/52.
>
> How does a) apply to my patch series?

Conversion is far from finished, so people do not see clearly what
interface they want.

> The patch series is about parsing integers inside the /proc mount so
> what is the conflict with b)?
>
> vsprintf.c has the following comment for simple_strtol:
> * This function is obsolete. Please use kstrtol instead.
>
> Could you, please, elaborate why kstrox.c should not be used in other
> places? Should we duplicate these functions instead of reusing them?
>
>>
>> The functions can probably be replaced by sscanf() (I didn't look closely).
>
> No. sscanf does not return the end of the parsed string. It requires to
> know beforehand if the string contains a octal, decimal or hexadecimal
> number.

Explicit base is good point.
As for knowing end of string, how often do you really want to know it?

>> I hate "_e" suffix with passion.
>
> Which names would you appreciate?

parse_integer() :-)

>> C in 2015 doesn't have this arcane concept known as optional
>> parameters so I'd suggest to hack around with always returning
>> number of OK characters
>
> Which functions should return the number of OK characters?

kstrto*() family can return positive amount or new interface.
Obvious question -- what to do with newline.

>> OR add one (just one) more separate
>> interface
>>
>>    unsigned int parse_integer(const char *s, unsigned int base, T *p);
>
> Why would you call this function parse_integer and not kstrtox?

Because C style is horrible: atoi, atol, atoll, strtol, stroll,
strtoul, strtoull, ...
Embedding type into name, meh.

>> and dispatching with __builtin_choose_expr().
>
> https://gcc.gnu.org/onlinedocs/gcc-3.4.5/gcc/Other-Builtins.html
> describes how to use __builtin_types_compatible_p together with
> __builtin_choose_expr().
>
> I do not understand how to avoid exposing the interfaces of the
> functions actually called by a parse_integer function multiplexer.

Well, there is one advertised interface -- parse_integer() and
2x(char, short, int, long, long long) = 12 technically public
but not advertised interfaces.

>> Of course if someone rewrites that abomination called
>> sysctl parsing from scratch, maybe none of this will be needed!
>
> Unfortunately you are very vague about what you dislike in sysctl.

It makes sense for proc_get_long() mentioned on one of your patches
to know length of parsed string only when sysctl is an array of integers
not just one integer. But most of the sysctls are just simple values.

Again, no one looked at what is really needed and you adding
"char **endp" because manpage says so just proves the point.

I suggest to scrap libc-style integer parsing and do it right.
Namely:

  int parse_integer(const char *s, unsigned int base, T *val);

returns -E on error/overflow
returns (positive) number of character consumed otherwise.
*val is written only if there were no errors
accepts whitespace (space, tab only), sign '+', sign '-'.

Error checking can still remain

  rv = parse_integer(...);
  if (rv < 0)
   ...

because if very first character is invalid, -EINVAL is still returned.

Parsing multiple values becomes:

  rv = parse_integer(s, base, &val1);
  if (rv < 0)
    return rv;
  s += rv;
  rv = parse_integer(s, base, &val2);
  if (rv < 0);

OpenBSD people came up with strtonum() at some point
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man3/strtonum.3?query=strtonum&arch=i386
pandora box "errstr" and missing ULL parsing are obvious.

Maybe Plan 9 got it right?
Well, no, they are still in "multiplication overflow" phase:
http://plan9.bell-labs.com/sources/plan9/sys/src/ape/lib/ap/gen/strtoul.c