[PATCH] kstrtox: make kstrtobool_from_user() very strict

[PATCH] kstrtox: make kstrtobool_from_user() very strict

[Alexey Dobriyan]

Once upon a time module parameter parsing code accepted
0, 1, y, n, Y and N for boolean values. Gratituous but contained
to module code and thus tolerable.

Commit ef951599074ba4fad2d0efa0a977129b41e6d203
("lib: move strtobool() to kstrtobool()") promoted that ugly wart
to kstrtobool() and, more importantly, kstrtobool_from_user().

Later set of accepted values was expanded to "on" and "of".
Now there are 6+8=14(!) valid strings for a boolean.

This patch reduces set of accepted values to "0" and "1"
(with optional newline) in spirit with other kstrto*() functions.

I'm starting with kstrtobool_from_user() as it is explicitly designed
to be used for interacting with userspace. Currently there are 9 users
all debug code, so there is hope.

Please send before 4.16 so no real users start to depend on verbose behaviour.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 lib/kstrtox.c |   32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -368,19 +368,41 @@ int kstrtobool(const char *s, bool *res)
 EXPORT_SYMBOL(kstrtobool);
 
 /*
- * Since "base" would be a nonsense argument, this open-codes the
- * _from_user helper instead of using the helper macro below.
+ * Convert string to boolean:
+ *	  0 => false
+ *	  1 => true
+ *	0\n => false
+ *	1\n => true
+ *
+ * Return 0 on success or -E otherwise.
  */
 int kstrtobool_from_user(const char __user *s, size_t count, bool *res)
 {
-	/* Longest string needed to differentiate, newline, terminator */
-	char buf[4];
+	/* 0|1, newline, terminator */
+	char buf[3], *p;
+	bool val;
 
 	count = min(count, sizeof(buf) - 1);
 	if (copy_from_user(buf, s, count))
 		return -EFAULT;
 	buf[count] = '\0';
-	return kstrtobool(buf, res);
+
+	p = buf;
+	if (*p == '0')
+		val = false;
+	else if (*p == '1')
+		val = true;
+	else
+		return -EINVAL;
+	p++;
+
+	if (*p == '\n')
+		p++;
+	if (*p)
+		return -EINVAL;
+
+	*res = val;
+	return 0;
 }
 EXPORT_SYMBOL(kstrtobool_from_user);
 

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Kees Cook]

On Sat, Feb 10, 2018 at 1:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Once upon a time module parameter parsing code accepted
> 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> to module code and thus tolerable.

And kernel command line.

> Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> to kstrtobool() and, more importantly, kstrtobool_from_user().
>
> Later set of accepted values was expanded to "on" and "of".
> Now there are 6+8=14(!) valid strings for a boolean.
>
> This patch reduces set of accepted values to "0" and "1"
> (with optional newline) in spirit with other kstrto*() functions.
>
> I'm starting with kstrtobool_from_user() as it is explicitly designed
> to be used for interacting with userspace. Currently there are 9 users
> all debug code, so there is hope.
>
> Please send before 4.16 so no real users start to depend on verbose behaviour.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>

Why bother? What's wrong with generous input parsing? This just
creates a divergence where none is needed.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Alexey Dobriyan]

On Sun, Feb 11, 2018 at 01:27:58PM -0800, Kees Cook wrote:
> On Sat, Feb 10, 2018 at 1:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Once upon a time module parameter parsing code accepted
> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> > to module code and thus tolerable.
> 
> And kernel command line.

True that.

> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> > to kstrtobool() and, more importantly, kstrtobool_from_user().
> >
> > Later set of accepted values was expanded to "on" and "of".
> > Now there are 6+8=14(!) valid strings for a boolean.
> >
> > This patch reduces set of accepted values to "0" and "1"
> > (with optional newline) in spirit with other kstrto*() functions.
> >
> > I'm starting with kstrtobool_from_user() as it is explicitly designed
> > to be used for interacting with userspace. Currently there are 9 users
> > all debug code, so there is hope.
> >
> > Please send before 4.16 so no real users start to depend on verbose behaviour.
> >
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> 
> Why bother? What's wrong with generous input parsing? This just
> creates a divergence where none is needed.

Generous parsing creates more problems and more code than necessary.
kstrtobool() is very instructive example.

Booleans are 2 values, so 2 strings is enough.
Add optional newline so shell users don't have to type "echo -n".
That's all.

In a generous world things are little more complicated:

First, as written code doesn't accept "on" it accepts "on.*" and
"of.*". Now imagine some parameter gets semantic change from "on/off"
to "on/off/once" (kmemleak-style detectors).

Formally you can't add "once" because user in theory may use "once"
by accident or mistake and expecting "on" behaviour which he relies
upon.

In a very strict world, I add "2" for "once" and document it.
Now I know that no users were broken, because kstrtobool() returned
errors for "2" previously.

Second, English centric view of the world. If English speaking users get
more usability with "on" and "off" why, say, 150 millions of
Russian-speaking people can't get that too. And if you accept that argument,
encoding subquestion arises immediately.

At this point you've adding Merriam Webster and libicu to the kernel.

Third, it is simply more code in ring 0. If anyone wants user-friendly
parameters, then write wrappers and leave kernel alone.

To reiterate, it was mistake to add "yYnN" to module and kernel parameter
parsing code, it was another mistake to propagate yYnN to generic code.
Those "on" and "off" exceptions should have stayed where they were as
small monuments to kernel imperfection :^)

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Kees Cook]

On Mon, Feb 12, 2018 at 3:58 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Sun, Feb 11, 2018 at 01:27:58PM -0800, Kees Cook wrote:
>> On Sat, Feb 10, 2018 at 1:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> > Once upon a time module parameter parsing code accepted
>> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
>> > to module code and thus tolerable.
>>
>> And kernel command line.
>
> True that.

The divergence in behavior is what I think is weirdest. Everything
_except_ this change will take generous inputs.

>> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
>> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
>> > to kstrtobool() and, more importantly, kstrtobool_from_user().
>> >
>> > Later set of accepted values was expanded to "on" and "of".
>> > Now there are 6+8=14(!) valid strings for a boolean.
>> >
>> > This patch reduces set of accepted values to "0" and "1"
>> > (with optional newline) in spirit with other kstrto*() functions.
>> >
>> > I'm starting with kstrtobool_from_user() as it is explicitly designed
>> > to be used for interacting with userspace. Currently there are 9 users
>> > all debug code, so there is hope.
>> >
>> > Please send before 4.16 so no real users start to depend on verbose behaviour.
>> >
>> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>>
>> Why bother? What's wrong with generous input parsing? This just
>> creates a divergence where none is needed.
>
> Generous parsing creates more problems and more code than necessary.
> kstrtobool() is very instructive example.
>
> Booleans are 2 values, so 2 strings is enough.
> Add optional newline so shell users don't have to type "echo -n".
> That's all.
>
> In a generous world things are little more complicated:
>
> First, as written code doesn't accept "on" it accepts "on.*" and
> "of.*". Now imagine some parameter gets semantic change from "on/off"

Well, this already accepts the newlines you mentioned, so that matches
the needs there.

> to "on/off/once" (kmemleak-style detectors).

At that point it's no longer a boolean, so it makes sense to have
different parsing rules.

> Second, English centric view of the world. If English speaking users get
> more usability with "on" and "off" why, say, 150 millions of
> Russian-speaking people can't get that too. And if you accept that argument,
> encoding subquestion arises immediately.

The kernel is English-centric. Just look at comments, variables, /sys,
/proc, etc. I don't find this a compelling argument.

> At this point you've adding Merriam Webster and libicu to the kernel.

See above; this won't turn into a slippery slope.

> Third, it is simply more code in ring 0. If anyone wants user-friendly
> parameters, then write wrappers and leave kernel alone.

This is the problem: it was already all kernel code. There were
separate non-compatible boolean parsers scattered around the kernel
and this unified the boolean parsing into one place to avoid code
duplication (which can have bugs) and unexpected results (which
frustrates users). This is attempting to fragment the code again, with
no obvious benefit.

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Alexey Dobriyan]

On Mon, Feb 12, 2018 at 09:03:06AM -0800, Kees Cook wrote:
> On Mon, Feb 12, 2018 at 3:58 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Sun, Feb 11, 2018 at 01:27:58PM -0800, Kees Cook wrote:
> >> On Sat, Feb 10, 2018 at 1:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >> > Once upon a time module parameter parsing code accepted
> >> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> >> > to module code and thus tolerable.
> >>
> >> And kernel command line.
> >
> > True that.
> 
> The divergence in behavior is what I think is weirdest. Everything
> _except_ this change will take generous inputs.
> 
> >> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> >> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> >> > to kstrtobool() and, more importantly, kstrtobool_from_user().
> >> >
> >> > Later set of accepted values was expanded to "on" and "of".
> >> > Now there are 6+8=14(!) valid strings for a boolean.
> >> >
> >> > This patch reduces set of accepted values to "0" and "1"
> >> > (with optional newline) in spirit with other kstrto*() functions.
> >> >
> >> > I'm starting with kstrtobool_from_user() as it is explicitly designed
> >> > to be used for interacting with userspace. Currently there are 9 users
> >> > all debug code, so there is hope.
> >> >
> >> > Please send before 4.16 so no real users start to depend on verbose behaviour.
> >> >
> >> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> >>
> >> Why bother? What's wrong with generous input parsing? This just
> >> creates a divergence where none is needed.
> >
> > Generous parsing creates more problems and more code than necessary.
> > kstrtobool() is very instructive example.
> >
> > Booleans are 2 values, so 2 strings is enough.
> > Add optional newline so shell users don't have to type "echo -n".
> > That's all.
> >
> > In a generous world things are little more complicated:
> >
> > First, as written code doesn't accept "on" it accepts "on.*" and
> > "of.*". Now imagine some parameter gets semantic change from "on/off"
> 
> Well, this already accepts the newlines you mentioned, so that matches
> the needs there.
> 
> > to "on/off/once" (kmemleak-style detectors).
> 
> At that point it's no longer a boolean, so it makes sense to have
> different parsing rules.
> 
> > Second, English centric view of the world. If English speaking users get
> > more usability with "on" and "off" why, say, 150 millions of
> > Russian-speaking people can't get that too. And if you accept that argument,
> > encoding subquestion arises immediately.
> 
> The kernel is English-centric. Just look at comments, variables, /sys,
> /proc, etc. I don't find this a compelling argument.
> 
> > At this point you've adding Merriam Webster and libicu to the kernel.
> 
> See above; this won't turn into a slippery slope.
> 
> > Third, it is simply more code in ring 0. If anyone wants user-friendly
> > parameters, then write wrappers and leave kernel alone.
> 
> This is the problem: it was already all kernel code. There were
> separate non-compatible boolean parsers scattered around the kernel
> and this unified the boolean parsing into one place to avoid code
> duplication (which can have bugs) and unexpected results (which
> frustrates users). This is attempting to fragment the code again, with
> no obvious benefit.

You're implying that all the code that can be unified should be unified.

Fancy boolean parsing was a wart, someone added yYnN because it was cool,
but I don't know the story, it was before me.

Let's look at it form the other side:
0 and 1 are accepted, you come and say "lets accept yes and no".
Natural answer will be "whay can't you use 0 and 1?".

>From the documentation point of view it may be also
beneficial: all those blogs posts will use all the same "0" and "1"
but now they have many more variants and will feel kind of random.

I have no problem with kernel parameters parsing acceping something
other than "0" and "1", some of them are ancient. But what you did is
to put it all over proc, sysfs, and debugfs which is a lot of exposure.

Benefit is that core stays well designed and minimal and the rest can
live outside. Integer kstrto*() functions don't accept "one" and "zero"
just like they don't accept Roman numerals. Kernel is not a place
for UI games.

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Andy Shevchenko]

On Sat, Feb 10, 2018 at 11:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> Once upon a time module parameter parsing code accepted
> 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> to module code and thus tolerable.
>
> Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> to kstrtobool() and, more importantly, kstrtobool_from_user().
>
> Later set of accepted values was expanded to "on" and "of".
> Now there are 6+8=14(!) valid strings for a boolean.
>
> This patch reduces set of accepted values to "0" and "1"
> (with optional newline) in spirit with other kstrto*() functions.
>
> I'm starting with kstrtobool_from_user() as it is explicitly designed
> to be used for interacting with userspace. Currently there are 9 users
> all debug code, so there is hope.
>
> Please send before 4.16 so no real users start to depend on verbose behaviour.
>

NACK.
You basically are breaking ABI here. I don't see a zillion patches
which adds a tons of duplicate code to the corresponding users.

> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
>  lib/kstrtox.c |   32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
>
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -368,19 +368,41 @@ int kstrtobool(const char *s, bool *res)
>  EXPORT_SYMBOL(kstrtobool);
>
>  /*
> - * Since "base" would be a nonsense argument, this open-codes the
> - * _from_user helper instead of using the helper macro below.
> + * Convert string to boolean:
> + *       0 => false
> + *       1 => true
> + *     0\n => false
> + *     1\n => true
> + *
> + * Return 0 on success or -E otherwise.
>   */
>  int kstrtobool_from_user(const char __user *s, size_t count, bool *res)
>  {
> -       /* Longest string needed to differentiate, newline, terminator */
> -       char buf[4];
> +       /* 0|1, newline, terminator */
> +       char buf[3], *p;
> +       bool val;
>
>         count = min(count, sizeof(buf) - 1);
>         if (copy_from_user(buf, s, count))
>                 return -EFAULT;
>         buf[count] = '\0';
> -       return kstrtobool(buf, res);
> +
> +       p = buf;
> +       if (*p == '0')
> +               val = false;
> +       else if (*p == '1')
> +               val = true;
> +       else
> +               return -EINVAL;
> +       p++;
> +
> +       if (*p == '\n')
> +               p++;
> +       if (*p)
> +               return -EINVAL;
> +
> +       *res = val;
> +       return 0;
>  }
>  EXPORT_SYMBOL(kstrtobool_from_user);
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Alexey Dobriyan]

On Tue, Feb 13, 2018 at 06:02:27PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 10, 2018 at 11:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > Once upon a time module parameter parsing code accepted
> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> > to module code and thus tolerable.
> >
> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> > to kstrtobool() and, more importantly, kstrtobool_from_user().
> >
> > Later set of accepted values was expanded to "on" and "of".
> > Now there are 6+8=14(!) valid strings for a boolean.
> >
> > This patch reduces set of accepted values to "0" and "1"
> > (with optional newline) in spirit with other kstrto*() functions.
> >
> > I'm starting with kstrtobool_from_user() as it is explicitly designed
> > to be used for interacting with userspace. Currently there are 9 users
> > all debug code, so there is hope.
> >
> > Please send before 4.16 so no real users start to depend on verbose behaviour.
> >
> 
> NACK.
> You basically are breaking ABI here. I don't see a zillion patches
> which adds a tons of duplicate code to the corresponding users.

Please do "find . -type f -name '*.[ch]' | xargs grep kstrtobool_from_user -w'
before talking about zillion of patches.

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Andy Shevchenko]

On Tue, Feb 13, 2018 at 7:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> On Tue, Feb 13, 2018 at 06:02:27PM +0200, Andy Shevchenko wrote:
>> On Sat, Feb 10, 2018 at 11:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>> > Once upon a time module parameter parsing code accepted
>> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
>> > to module code and thus tolerable.
>> >
>> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
>> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
>> > to kstrtobool() and, more importantly, kstrtobool_from_user().
>> >
>> > Later set of accepted values was expanded to "on" and "of".
>> > Now there are 6+8=14(!) valid strings for a boolean.
>> >
>> > This patch reduces set of accepted values to "0" and "1"
>> > (with optional newline) in spirit with other kstrto*() functions.
>> >
>> > I'm starting with kstrtobool_from_user() as it is explicitly designed
>> > to be used for interacting with userspace. Currently there are 9 users
>> > all debug code, so there is hope.
>> >
>> > Please send before 4.16 so no real users start to depend on verbose behaviour.

>> NACK.
>> You basically are breaking ABI here. I don't see a zillion patches
>> which adds a tons of duplicate code to the corresponding users.
>
> Please do "find . -type f -name '*.[ch]' | xargs grep kstrtobool_from_user -w'
> before talking about zillion of patches.

If you wonder, I did it of course. The stylistic device I used here is
called "hyperbole".

Even for that dozen or so users I don't see how you handled the change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] kstrtox: make kstrtobool_from_user() very strict

[Alexey Dobriyan]

On Tue, Feb 13, 2018 at 07:18:08PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 13, 2018 at 7:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> > On Tue, Feb 13, 2018 at 06:02:27PM +0200, Andy Shevchenko wrote:
> >> On Sat, Feb 10, 2018 at 11:11 PM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
> >> > Once upon a time module parameter parsing code accepted
> >> > 0, 1, y, n, Y and N for boolean values. Gratituous but contained
> >> > to module code and thus tolerable.
> >> >
> >> > Commit ef951599074ba4fad2d0efa0a977129b41e6d203
> >> > ("lib: move strtobool() to kstrtobool()") promoted that ugly wart
> >> > to kstrtobool() and, more importantly, kstrtobool_from_user().
> >> >
> >> > Later set of accepted values was expanded to "on" and "of".
> >> > Now there are 6+8=14(!) valid strings for a boolean.
> >> >
> >> > This patch reduces set of accepted values to "0" and "1"
> >> > (with optional newline) in spirit with other kstrto*() functions.
> >> >
> >> > I'm starting with kstrtobool_from_user() as it is explicitly designed
> >> > to be used for interacting with userspace. Currently there are 9 users
> >> > all debug code, so there is hope.
> >> >
> >> > Please send before 4.16 so no real users start to depend on verbose behaviour.
> 
> >> NACK.
> >> You basically are breaking ABI here. I don't see a zillion patches
> >> which adds a tons of duplicate code to the corresponding users.
> >
> > Please do "find . -type f -name '*.[ch]' | xargs grep kstrtobool_from_user -w'
> > before talking about zillion of patches.
> 
> If you wonder, I did it of course. The stylistic device I used here is
> called "hyperbole".
> 
> Even for that dozen or so users I don't see how you handled the change.

All the users of kstrtobool_from_user() are in debug code, so there are
no users.

I'd flip kstrtobool() as well, but you're guys are taking hostages now:
we sneaked in dubious change (the author of a file was not on Cc list,
but 11 other people were) and we won't let you back it out because ABI
and users.

Plan for kstrtobool() is to revert those conversions for users which
used "on" and "off" and param parsing code so that new users can use
0 and 1 consistently across the kernel.