* [PATCH v2 1/4] lib: move strtobool to kstrtobool
2016-02-04 21:00 [PATCH v2 0/4] lib: add "on" and "off" to strtobool Kees Cook
@ 2016-02-04 21:00 ` Kees Cook
2016-02-04 22:43 ` Andy Shevchenko
2016-02-04 23:55 ` Rasmus Villemoes
2016-02-04 21:00 ` [PATCH v2 2/4] lib: update single-char callers of strtobool Kees Cook
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-04 21:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Joe Perches, Andy Shevchenko, Rasmus Villemoes,
Daniel Borkmann, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, x86, linuxppc-dev, linux-s390, linux-wireless,
netdev, linux-cifs, linux-kernel
Create the kstrtobool_from_user helper and moves strtobool logic into
the new kstrtobool (matching all the other kstrto* functions). Provides
an inline wrapper for existing strtobool callers.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
include/linux/kernel.h | 3 +++
include/linux/string.h | 6 +++++-
lib/kstrtox.c | 35 +++++++++++++++++++++++++++++++++++
lib/string.c | 29 -----------------------------
4 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f31638c6e873..cdc25f47a23f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
+int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
@@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
+int __must_check kstrtobool_from_user(const char __user *s, size_t count,
+ unsigned int base, bool *res);
static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
{
diff --git a/include/linux/string.h b/include/linux/string.h
index 9eebc66d957a..d2fb21b1081d 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
extern void argv_free(char **argv);
extern bool sysfs_streq(const char *s1, const char *s2);
-extern int strtobool(const char *s, bool *res);
+extern int kstrtobool(const char *s, unsigned int base, bool *res);
+static inline int strtobool(const char *s, bool *res)
+{
+ return kstrtobool(s, 0, res);
+}
#ifdef CONFIG_BINARY_PRINTF
int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index 94be244e8441..e18f088704d7 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
}
EXPORT_SYMBOL(kstrtos8);
+/**
+ * kstrtobool - convert common user inputs into boolean values
+ * @s: input string
+ * @base: ignored
+ * @res: result
+ *
+ * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
+ * Otherwise it will return -EINVAL. Value pointed to by res is
+ * updated upon finding a match.
+ */
+int kstrtobool(const char *s, unsigned int base, bool *res)
+{
+ if (!s)
+ return -EINVAL;
+
+ switch (s[0]) {
+ case 'y':
+ case 'Y':
+ case '1':
+ *res = true;
+ return 0;
+ case 'n':
+ case 'N':
+ case '0':
+ *res = false;
+ return 0;
+ default:
+ break;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL(kstrtobool);
+
#define kstrto_from_user(f, g, type) \
int f(const char __user *s, size_t count, unsigned int base, type *res) \
{ \
@@ -345,3 +379,4 @@ kstrto_from_user(kstrtou16_from_user, kstrtou16, u16);
kstrto_from_user(kstrtos16_from_user, kstrtos16, s16);
kstrto_from_user(kstrtou8_from_user, kstrtou8, u8);
kstrto_from_user(kstrtos8_from_user, kstrtos8, s8);
+kstrto_from_user(kstrtobool_from_user, kstrtobool, bool);
diff --git a/lib/string.c b/lib/string.c
index 0323c0d5629a..1a90db9bc6e1 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -630,35 +630,6 @@ bool sysfs_streq(const char *s1, const char *s2)
}
EXPORT_SYMBOL(sysfs_streq);
-/**
- * strtobool - convert common user inputs into boolean values
- * @s: input string
- * @res: result
- *
- * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
- * Otherwise it will return -EINVAL. Value pointed to by res is
- * updated upon finding a match.
- */
-int strtobool(const char *s, bool *res)
-{
- switch (s[0]) {
- case 'y':
- case 'Y':
- case '1':
- *res = true;
- break;
- case 'n':
- case 'N':
- case '0':
- *res = false;
- break;
- default:
- return -EINVAL;
- }
- return 0;
-}
-EXPORT_SYMBOL(strtobool);
-
#ifndef __HAVE_ARCH_MEMSET
/**
* memset - Fill a region of memory with the given value
--
2.6.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/4] lib: move strtobool to kstrtobool
2016-02-04 21:00 ` [PATCH v2 1/4] lib: move strtobool to kstrtobool Kees Cook
@ 2016-02-04 22:43 ` Andy Shevchenko
2016-02-04 22:48 ` Kees Cook
2016-02-04 23:55 ` Rasmus Villemoes
1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2016-02-04 22:43 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Joe Perches, Rasmus Villemoes, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev, linux-s390,
open list:TI WILINK WIRELES..., netdev, linux-cifs,
linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
> Create the kstrtobool_from_user helper and moves strtobool logic into
> the new kstrtobool (matching all the other kstrto* functions). Provides
> an inline wrapper for existing strtobool callers.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
One minor below.
> ---
> include/linux/kernel.h | 3 +++
> include/linux/string.h | 6 +++++-
> lib/kstrtox.c | 35 +++++++++++++++++++++++++++++++++++
> lib/string.c | 29 -----------------------------
> 4 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index f31638c6e873..cdc25f47a23f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
> int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
> int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
> int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
> +int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
>
> int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
> int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
> @@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
> int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
> int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
> int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
> +int __must_check kstrtobool_from_user(const char __user *s, size_t count,
> + unsigned int base, bool *res);
We already are using long lines here, perhaps do the same?
>
> static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
> {
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 9eebc66d957a..d2fb21b1081d 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>
> extern bool sysfs_streq(const char *s1, const char *s2);
> -extern int strtobool(const char *s, bool *res);
> +extern int kstrtobool(const char *s, unsigned int base, bool *res);
> +static inline int strtobool(const char *s, bool *res)
> +{
> + return kstrtobool(s, 0, res);
> +}
>
> #ifdef CONFIG_BINARY_PRINTF
> int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 94be244e8441..e18f088704d7 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
> }
> EXPORT_SYMBOL(kstrtos8);
>
> +/**
> + * kstrtobool - convert common user inputs into boolean values
> + * @s: input string
> + * @base: ignored
> + * @res: result
> + *
> + * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
> + * Otherwise it will return -EINVAL. Value pointed to by res is
> + * updated upon finding a match.
> + */
> +int kstrtobool(const char *s, unsigned int base, bool *res)
> +{
> + if (!s)
> + return -EINVAL;
> +
> + switch (s[0]) {
> + case 'y':
> + case 'Y':
> + case '1':
> + *res = true;
> + return 0;
> + case 'n':
> + case 'N':
> + case '0':
> + *res = false;
> + return 0;
> + default:
> + break;
> + }
> +
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(kstrtobool);
> +
> #define kstrto_from_user(f, g, type) \
> int f(const char __user *s, size_t count, unsigned int base, type *res) \
> { \
> @@ -345,3 +379,4 @@ kstrto_from_user(kstrtou16_from_user, kstrtou16, u16);
> kstrto_from_user(kstrtos16_from_user, kstrtos16, s16);
> kstrto_from_user(kstrtou8_from_user, kstrtou8, u8);
> kstrto_from_user(kstrtos8_from_user, kstrtos8, s8);
> +kstrto_from_user(kstrtobool_from_user, kstrtobool, bool);
> diff --git a/lib/string.c b/lib/string.c
> index 0323c0d5629a..1a90db9bc6e1 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -630,35 +630,6 @@ bool sysfs_streq(const char *s1, const char *s2)
> }
> EXPORT_SYMBOL(sysfs_streq);
>
> -/**
> - * strtobool - convert common user inputs into boolean values
> - * @s: input string
> - * @res: result
> - *
> - * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
> - * Otherwise it will return -EINVAL. Value pointed to by res is
> - * updated upon finding a match.
> - */
> -int strtobool(const char *s, bool *res)
> -{
> - switch (s[0]) {
> - case 'y':
> - case 'Y':
> - case '1':
> - *res = true;
> - break;
> - case 'n':
> - case 'N':
> - case '0':
> - *res = false;
> - break;
> - default:
> - return -EINVAL;
> - }
> - return 0;
> -}
> -EXPORT_SYMBOL(strtobool);
> -
> #ifndef __HAVE_ARCH_MEMSET
> /**
> * memset - Fill a region of memory with the given value
> --
> 2.6.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/4] lib: move strtobool to kstrtobool
2016-02-04 22:43 ` Andy Shevchenko
@ 2016-02-04 22:48 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-04 22:48 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, Joe Perches, Rasmus Villemoes, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, open list:TI WILINK WIRELES...,
netdev, linux-cifs, linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 2:43 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>> Create the kstrtobool_from_user helper and moves strtobool logic into
>> the new kstrtobool (matching all the other kstrto* functions). Provides
>> an inline wrapper for existing strtobool callers.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> One minor below.
Thanks!
>
>> ---
>> include/linux/kernel.h | 3 +++
>> include/linux/string.h | 6 +++++-
>> lib/kstrtox.c | 35 +++++++++++++++++++++++++++++++++++
>> lib/string.c | 29 -----------------------------
>> 4 files changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index f31638c6e873..cdc25f47a23f 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
>> int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
>> int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
>> int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
>> +int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
>>
>> int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
>> int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
>> @@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
>> int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
>> int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
>> int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
>
>> +int __must_check kstrtobool_from_user(const char __user *s, size_t count,
>> + unsigned int base, bool *res);
>
> We already are using long lines here, perhaps do the same?
I went back and forth on that, and decided that between checkpatch
yelling at me, and trying to be an agent of less entropy, I wrapped
the definition. I am fine either way, though.
-Kees
>
>>
>> static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
>> {
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 9eebc66d957a..d2fb21b1081d 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
>> extern void argv_free(char **argv);
>>
>> extern bool sysfs_streq(const char *s1, const char *s2);
>> -extern int strtobool(const char *s, bool *res);
>> +extern int kstrtobool(const char *s, unsigned int base, bool *res);
>> +static inline int strtobool(const char *s, bool *res)
>> +{
>> + return kstrtobool(s, 0, res);
>> +}
>>
>> #ifdef CONFIG_BINARY_PRINTF
>> int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 94be244e8441..e18f088704d7 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
>> }
>> EXPORT_SYMBOL(kstrtos8);
>>
>> +/**
>> + * kstrtobool - convert common user inputs into boolean values
>> + * @s: input string
>> + * @base: ignored
>> + * @res: result
>> + *
>> + * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
>> + * Otherwise it will return -EINVAL. Value pointed to by res is
>> + * updated upon finding a match.
>> + */
>> +int kstrtobool(const char *s, unsigned int base, bool *res)
>> +{
>> + if (!s)
>> + return -EINVAL;
>> +
>> + switch (s[0]) {
>> + case 'y':
>> + case 'Y':
>> + case '1':
>> + *res = true;
>> + return 0;
>> + case 'n':
>> + case 'N':
>> + case '0':
>> + *res = false;
>> + return 0;
>> + default:
>> + break;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(kstrtobool);
>> +
>> #define kstrto_from_user(f, g, type) \
>> int f(const char __user *s, size_t count, unsigned int base, type *res) \
>> { \
>> @@ -345,3 +379,4 @@ kstrto_from_user(kstrtou16_from_user, kstrtou16, u16);
>> kstrto_from_user(kstrtos16_from_user, kstrtos16, s16);
>> kstrto_from_user(kstrtou8_from_user, kstrtou8, u8);
>> kstrto_from_user(kstrtos8_from_user, kstrtos8, s8);
>> +kstrto_from_user(kstrtobool_from_user, kstrtobool, bool);
>> diff --git a/lib/string.c b/lib/string.c
>> index 0323c0d5629a..1a90db9bc6e1 100644
>> --- a/lib/string.c
>> +++ b/lib/string.c
>> @@ -630,35 +630,6 @@ bool sysfs_streq(const char *s1, const char *s2)
>> }
>> EXPORT_SYMBOL(sysfs_streq);
>>
>> -/**
>> - * strtobool - convert common user inputs into boolean values
>> - * @s: input string
>> - * @res: result
>> - *
>> - * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
>> - * Otherwise it will return -EINVAL. Value pointed to by res is
>> - * updated upon finding a match.
>> - */
>> -int strtobool(const char *s, bool *res)
>> -{
>> - switch (s[0]) {
>> - case 'y':
>> - case 'Y':
>> - case '1':
>> - *res = true;
>> - break;
>> - case 'n':
>> - case 'N':
>> - case '0':
>> - *res = false;
>> - break;
>> - default:
>> - return -EINVAL;
>> - }
>> - return 0;
>> -}
>> -EXPORT_SYMBOL(strtobool);
>> -
>> #ifndef __HAVE_ARCH_MEMSET
>> /**
>> * memset - Fill a region of memory with the given value
>> --
>> 2.6.3
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] lib: move strtobool to kstrtobool
2016-02-04 21:00 ` [PATCH v2 1/4] lib: move strtobool to kstrtobool Kees Cook
2016-02-04 22:43 ` Andy Shevchenko
@ 2016-02-04 23:55 ` Rasmus Villemoes
2016-02-05 20:50 ` Kees Cook
1 sibling, 1 reply; 18+ messages in thread
From: Rasmus Villemoes @ 2016-02-04 23:55 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Joe Perches, Andy Shevchenko, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky, x86,
linuxppc-dev, linux-s390, linux-wireless, netdev, linux-cifs,
linux-kernel
On Thu, Feb 04 2016, Kees Cook <keescook@chromium.org> wrote:
> Create the kstrtobool_from_user helper and moves strtobool logic into
> the new kstrtobool (matching all the other kstrto* functions). Provides
> an inline wrapper for existing strtobool callers.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> include/linux/kernel.h | 3 +++
> include/linux/string.h | 6 +++++-
> lib/kstrtox.c | 35 +++++++++++++++++++++++++++++++++++
> lib/string.c | 29 -----------------------------
> 4 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index f31638c6e873..cdc25f47a23f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
> int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
> int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
> int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
> +int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
>
> int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
> int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
> @@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
> int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
> int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
> int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
> +int __must_check kstrtobool_from_user(const char __user *s, size_t count,
> + unsigned int base, bool *res);
>
> static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
> {
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 9eebc66d957a..d2fb21b1081d 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
> extern void argv_free(char **argv);
>
> extern bool sysfs_streq(const char *s1, const char *s2);
> -extern int strtobool(const char *s, bool *res);
> +extern int kstrtobool(const char *s, unsigned int base, bool *res);
> +static inline int strtobool(const char *s, bool *res)
> +{
> + return kstrtobool(s, 0, res);
> +}
>
> #ifdef CONFIG_BINARY_PRINTF
> int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index 94be244e8441..e18f088704d7 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
> }
> EXPORT_SYMBOL(kstrtos8);
>
> +/**
> + * kstrtobool - convert common user inputs into boolean values
> + * @s: input string
> + * @base: ignored
> + * @res: result
> + *
> + * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
> + * Otherwise it will return -EINVAL. Value pointed to by res is
> + * updated upon finding a match.
> + */
> +int kstrtobool(const char *s, unsigned int base, bool *res)
> +{
Being able to create the kstrtobool_from_user with a single macro
invocation is convenient, but I don't think that justifies the ugliness
of having an unused parameter. People reading this code or trying to use
the interface will wonder what it's doing there, and it will generate
slightly larger code for all the users of strtobool.
So I'd just make a separate explicit definition of kstrtobool_from_user
(the stack buffer sizing doesn't apply to the strings we want to parse
anyway, though 11 is of course plenty).
Rasmus
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 1/4] lib: move strtobool to kstrtobool
2016-02-04 23:55 ` Rasmus Villemoes
@ 2016-02-05 20:50 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-05 20:50 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: Andrew Morton, Joe Perches, Andy Shevchenko, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, linux-wireless, Network Development,
linux-cifs, LKML
On Thu, Feb 4, 2016 at 3:55 PM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Thu, Feb 04 2016, Kees Cook <keescook@chromium.org> wrote:
>
>> Create the kstrtobool_from_user helper and moves strtobool logic into
>> the new kstrtobool (matching all the other kstrto* functions). Provides
>> an inline wrapper for existing strtobool callers.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> include/linux/kernel.h | 3 +++
>> include/linux/string.h | 6 +++++-
>> lib/kstrtox.c | 35 +++++++++++++++++++++++++++++++++++
>> lib/string.c | 29 -----------------------------
>> 4 files changed, 43 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index f31638c6e873..cdc25f47a23f 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -357,6 +357,7 @@ int __must_check kstrtou16(const char *s, unsigned int base, u16 *res);
>> int __must_check kstrtos16(const char *s, unsigned int base, s16 *res);
>> int __must_check kstrtou8(const char *s, unsigned int base, u8 *res);
>> int __must_check kstrtos8(const char *s, unsigned int base, s8 *res);
>> +int __must_check kstrtobool(const char *s, unsigned int base, bool *res);
>>
>> int __must_check kstrtoull_from_user(const char __user *s, size_t count, unsigned int base, unsigned long long *res);
>> int __must_check kstrtoll_from_user(const char __user *s, size_t count, unsigned int base, long long *res);
>> @@ -368,6 +369,8 @@ int __must_check kstrtou16_from_user(const char __user *s, size_t count, unsigne
>> int __must_check kstrtos16_from_user(const char __user *s, size_t count, unsigned int base, s16 *res);
>> int __must_check kstrtou8_from_user(const char __user *s, size_t count, unsigned int base, u8 *res);
>> int __must_check kstrtos8_from_user(const char __user *s, size_t count, unsigned int base, s8 *res);
>> +int __must_check kstrtobool_from_user(const char __user *s, size_t count,
>> + unsigned int base, bool *res);
>>
>> static inline int __must_check kstrtou64_from_user(const char __user *s, size_t count, unsigned int base, u64 *res)
>> {
>> diff --git a/include/linux/string.h b/include/linux/string.h
>> index 9eebc66d957a..d2fb21b1081d 100644
>> --- a/include/linux/string.h
>> +++ b/include/linux/string.h
>> @@ -128,7 +128,11 @@ extern char **argv_split(gfp_t gfp, const char *str, int *argcp);
>> extern void argv_free(char **argv);
>>
>> extern bool sysfs_streq(const char *s1, const char *s2);
>> -extern int strtobool(const char *s, bool *res);
>> +extern int kstrtobool(const char *s, unsigned int base, bool *res);
>> +static inline int strtobool(const char *s, bool *res)
>> +{
>> + return kstrtobool(s, 0, res);
>> +}
>>
>> #ifdef CONFIG_BINARY_PRINTF
>> int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index 94be244e8441..e18f088704d7 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -321,6 +321,40 @@ int kstrtos8(const char *s, unsigned int base, s8 *res)
>> }
>> EXPORT_SYMBOL(kstrtos8);
>>
>> +/**
>> + * kstrtobool - convert common user inputs into boolean values
>> + * @s: input string
>> + * @base: ignored
>> + * @res: result
>> + *
>> + * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
>> + * Otherwise it will return -EINVAL. Value pointed to by res is
>> + * updated upon finding a match.
>> + */
>> +int kstrtobool(const char *s, unsigned int base, bool *res)
>> +{
>
> Being able to create the kstrtobool_from_user with a single macro
> invocation is convenient, but I don't think that justifies the ugliness
> of having an unused parameter. People reading this code or trying to use
> the interface will wonder what it's doing there, and it will generate
> slightly larger code for all the users of strtobool.
>
> So I'd just make a separate explicit definition of kstrtobool_from_user
> (the stack buffer sizing doesn't apply to the strings we want to parse
> anyway, though 11 is of course plenty).
Okay, thanks. So many things were bothering me, but I feared code
duplication would be seen as worse. I'm much happier to drop the
unused argument. :)
I'll send a v3 with all the changes.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 2/4] lib: update single-char callers of strtobool
2016-02-04 21:00 [PATCH v2 0/4] lib: add "on" and "off" to strtobool Kees Cook
2016-02-04 21:00 ` [PATCH v2 1/4] lib: move strtobool to kstrtobool Kees Cook
@ 2016-02-04 21:00 ` Kees Cook
2016-02-04 22:59 ` Andy Shevchenko
2016-02-05 10:46 ` David Laight
2016-02-04 21:00 ` [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool Kees Cook
2016-02-04 21:00 ` [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool Kees Cook
3 siblings, 2 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-04 21:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, linux-cifs, Joe Perches, Andy Shevchenko,
Rasmus Villemoes, Daniel Borkmann, Michael Ellerman,
Heiko Carstens, Martin Schwidefsky, x86, linuxppc-dev, linux-s390,
linux-wireless, netdev, linux-kernel
Some callers of strtobool were passing a pointer to unterminated strings.
In preparation of adding multi-character processing to kstrtobool, update
the callers to not pass single-character pointers, and switch to using the
new kstrtobool_from_user helper where possible.
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Amitkumar Karwar <akarwar@marvell.com>
Cc: Nishant Sarmukadam <nishants@marvell.com>
Cc: Kalle Valo <kvalo@codeaurora.org>
Cc: Steve French <sfrench@samba.org>
Cc: linux-cifs@vger.kernel.org
---
drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 ++---
fs/cifs/cifs_debug.c | 58 +++++++-------------------
fs/cifs/cifs_debug.h | 2 +-
fs/cifs/cifsfs.c | 6 +--
fs/cifs/cifsglob.h | 4 +-
5 files changed, 26 insertions(+), 54 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index 0b9c580af988..bd061b02bc04 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -880,14 +880,12 @@ mwifiex_reset_write(struct file *file,
{
struct mwifiex_private *priv = file->private_data;
struct mwifiex_adapter *adapter = priv->adapter;
- char cmd;
bool result;
+ int rc;
- if (copy_from_user(&cmd, ubuf, sizeof(cmd)))
- return -EFAULT;
-
- if (strtobool(&cmd, &result))
- return -EINVAL;
+ rc = kstrtobool_from_user(ubuf, count, 0, &result);
+ if (rc)
+ return rc;
if (!result)
return -EINVAL;
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 50b268483302..6ee59abcb69b 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -255,7 +255,6 @@ static const struct file_operations cifs_debug_data_proc_fops = {
static ssize_t cifs_stats_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
- char c;
bool bv;
int rc;
struct list_head *tmp1, *tmp2, *tmp3;
@@ -263,11 +262,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
struct cifs_ses *ses;
struct cifs_tcon *tcon;
- rc = get_user(c, buffer);
- if (rc)
- return rc;
-
- if (strtobool(&c, &bv) == 0) {
+ rc = kstrtobool_from_user(buffer, count, 0, &bv);
+ if (rc == 0) {
#ifdef CONFIG_CIFS_STATS2
atomic_set(&totBufAllocCount, 0);
atomic_set(&totSmBufAllocCount, 0);
@@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
}
}
spin_unlock(&cifs_tcp_ses_lock);
+ } else {
+ return rc;
}
return count;
@@ -433,17 +431,17 @@ static int cifsFYI_proc_open(struct inode *inode, struct file *file)
static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
- char c;
+ char c[2] = { '\0' };
bool bv;
int rc;
- rc = get_user(c, buffer);
+ rc = get_user(c[0], buffer);
if (rc)
return rc;
- if (strtobool(&c, &bv) == 0)
+ if (strtobool(c, &bv) == 0)
cifsFYI = bv;
- else if ((c > '1') && (c <= '9'))
- cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
+ else if ((c[0] > '1') && (c[0] <= '9'))
+ cifsFYI = (int) (c[0] - '0'); /* see cifs_debug.h for meanings */
return count;
}
@@ -471,20 +469,12 @@ static int cifs_linux_ext_proc_open(struct inode *inode, struct file *file)
static ssize_t cifs_linux_ext_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
- char c;
- bool bv;
int rc;
- rc = get_user(c, buffer);
+ rc = kstrtobool_from_user(buffer, count, 0, &linuxExtEnabled);
if (rc)
return rc;
- rc = strtobool(&c, &bv);
- if (rc)
- return rc;
-
- linuxExtEnabled = bv;
-
return count;
}
@@ -511,20 +501,12 @@ static int cifs_lookup_cache_proc_open(struct inode *inode, struct file *file)
static ssize_t cifs_lookup_cache_proc_write(struct file *file,
const char __user *buffer, size_t count, loff_t *ppos)
{
- char c;
- bool bv;
int rc;
- rc = get_user(c, buffer);
+ rc = kstrtobool_from_user(buffer, count, 0, &lookupCacheEnabled);
if (rc)
return rc;
- rc = strtobool(&c, &bv);
- if (rc)
- return rc;
-
- lookupCacheEnabled = bv;
-
return count;
}
@@ -551,20 +533,12 @@ static int traceSMB_proc_open(struct inode *inode, struct file *file)
static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
size_t count, loff_t *ppos)
{
- char c;
- bool bv;
int rc;
- rc = get_user(c, buffer);
+ rc = kstrtobool_from_user(buffer, count, 0, &traceSMB);
if (rc)
return rc;
- rc = strtobool(&c, &bv);
- if (rc)
- return rc;
-
- traceSMB = bv;
-
return count;
}
@@ -622,7 +596,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
int rc;
unsigned int flags;
char flags_string[12];
- char c;
+ char c[2] = { '\0' };
bool bv;
if ((count < 1) || (count > 11))
@@ -635,11 +609,11 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
if (count < 3) {
/* single char or single char followed by null */
- c = flags_string[0];
- if (strtobool(&c, &bv) == 0) {
+ c[0] = flags_string[0];
+ if (strtobool(c, &bv) == 0) {
global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
return count;
- } else if (!isdigit(c)) {
+ } else if (!isdigit(c[0])) {
cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
flags_string);
return -EINVAL;
diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
index 66cf0f9fff89..c611ca2339d7 100644
--- a/fs/cifs/cifs_debug.h
+++ b/fs/cifs/cifs_debug.h
@@ -25,7 +25,7 @@
void cifs_dump_mem(char *label, void *data, int length);
void cifs_dump_detail(void *);
void cifs_dump_mids(struct TCP_Server_Info *);
-extern int traceSMB; /* flag which enables the function below */
+extern bool traceSMB; /* flag which enables the function below */
void dump_smb(void *, int);
#define CIFS_INFO 0x01
#define CIFS_RC 0x02
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index c48ca13673e3..931b446f2a44 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -54,10 +54,10 @@
#endif
int cifsFYI = 0;
-int traceSMB = 0;
+bool traceSMB;
bool enable_oplocks = true;
-unsigned int linuxExtEnabled = 1;
-unsigned int lookupCacheEnabled = 1;
+bool linuxExtEnabled = true;
+bool lookupCacheEnabled = true;
unsigned int global_secflags = CIFSSEC_DEF;
/* unsigned int ntlmv2_support = 0; */
unsigned int sign_CIFS_PDUs = 1;
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a25b2513f146..d21da9f05bae 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -1596,11 +1596,11 @@ GLOBAL_EXTERN atomic_t midCount;
/* Misc globals */
GLOBAL_EXTERN bool enable_oplocks; /* enable or disable oplocks */
-GLOBAL_EXTERN unsigned int lookupCacheEnabled;
+GLOBAL_EXTERN bool lookupCacheEnabled;
GLOBAL_EXTERN unsigned int global_secflags; /* if on, session setup sent
with more secure ntlmssp2 challenge/resp */
GLOBAL_EXTERN unsigned int sign_CIFS_PDUs; /* enable smb packet signing */
-GLOBAL_EXTERN unsigned int linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
+GLOBAL_EXTERN bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
GLOBAL_EXTERN unsigned int CIFSMaxBufSize; /* max size not including hdr */
GLOBAL_EXTERN unsigned int cifs_min_rcv; /* min size of big ntwrk buf pool */
GLOBAL_EXTERN unsigned int cifs_min_small; /* min size of small buf pool */
--
2.6.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/4] lib: update single-char callers of strtobool
2016-02-04 21:00 ` [PATCH v2 2/4] lib: update single-char callers of strtobool Kees Cook
@ 2016-02-04 22:59 ` Andy Shevchenko
2016-02-04 23:01 ` Kees Cook
2016-02-05 10:46 ` David Laight
1 sibling, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2016-02-04 22:59 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, linux-cifs, Joe Perches, Rasmus Villemoes,
Daniel Borkmann, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, x86@kernel.org, linuxppc-dev, linux-s390,
open list:TI WILINK WIRELES..., netdev,
linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
> Some callers of strtobool were passing a pointer to unterminated strings.
> In preparation of adding multi-character processing to kstrtobool, update
> the callers to not pass single-character pointers, and switch to using the
> new kstrtobool_from_user helper where possible.
Looks much better now!
My comment below.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: Amitkumar Karwar <akarwar@marvell.com>
> Cc: Nishant Sarmukadam <nishants@marvell.com>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: Steve French <sfrench@samba.org>
> Cc: linux-cifs@vger.kernel.org
> ---
> drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 ++---
> fs/cifs/cifs_debug.c | 58 +++++++-------------------
> fs/cifs/cifs_debug.h | 2 +-
> fs/cifs/cifsfs.c | 6 +--
> fs/cifs/cifsglob.h | 4 +-
> 5 files changed, 26 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> index 0b9c580af988..bd061b02bc04 100644
> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> @@ -880,14 +880,12 @@ mwifiex_reset_write(struct file *file,
> {
> struct mwifiex_private *priv = file->private_data;
> struct mwifiex_adapter *adapter = priv->adapter;
> - char cmd;
> bool result;
> + int rc;
>
> - if (copy_from_user(&cmd, ubuf, sizeof(cmd)))
> - return -EFAULT;
> -
> - if (strtobool(&cmd, &result))
> - return -EINVAL;
> + rc = kstrtobool_from_user(ubuf, count, 0, &result);
> + if (rc)
> + return rc;
>
> if (!result)
> return -EINVAL;
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index 50b268483302..6ee59abcb69b 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -255,7 +255,6 @@ static const struct file_operations cifs_debug_data_proc_fops = {
> static ssize_t cifs_stats_proc_write(struct file *file,
> const char __user *buffer, size_t count, loff_t *ppos)
> {
> - char c;
> bool bv;
> int rc;
> struct list_head *tmp1, *tmp2, *tmp3;
> @@ -263,11 +262,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> struct cifs_ses *ses;
> struct cifs_tcon *tcon;
>
> - rc = get_user(c, buffer);
> - if (rc)
> - return rc;
> -
> - if (strtobool(&c, &bv) == 0) {
> + rc = kstrtobool_from_user(buffer, count, 0, &bv);
> + if (rc == 0) {
> #ifdef CONFIG_CIFS_STATS2
> atomic_set(&totBufAllocCount, 0);
> atomic_set(&totSmBufAllocCount, 0);
> @@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> }
> }
> spin_unlock(&cifs_tcp_ses_lock);
> + } else {
> + return rc;
> }
>
> return count;
> @@ -433,17 +431,17 @@ static int cifsFYI_proc_open(struct inode *inode, struct file *file)
> static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> - char c;
> + char c[2] = { '\0' };
> bool bv;
> int rc;
>
> - rc = get_user(c, buffer);
> + rc = get_user(c[0], buffer);
> if (rc)
> return rc;
> - if (strtobool(&c, &bv) == 0)
> + if (strtobool(c, &bv) == 0)
> cifsFYI = bv;
> - else if ((c > '1') && (c <= '9'))
> - cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
> + else if ((c[0] > '1') && (c[0] <= '9'))
> + cifsFYI = (int) (c[0] - '0'); /* see cifs_debug.h for meanings */
>
> return count;
> }
> @@ -471,20 +469,12 @@ static int cifs_linux_ext_proc_open(struct inode *inode, struct file *file)
> static ssize_t cifs_linux_ext_proc_write(struct file *file,
> const char __user *buffer, size_t count, loff_t *ppos)
> {
> - char c;
> - bool bv;
> int rc;
>
> - rc = get_user(c, buffer);
> + rc = kstrtobool_from_user(buffer, count, 0, &linuxExtEnabled);
> if (rc)
> return rc;
>
> - rc = strtobool(&c, &bv);
> - if (rc)
> - return rc;
> -
> - linuxExtEnabled = bv;
> -
> return count;
> }
>
> @@ -511,20 +501,12 @@ static int cifs_lookup_cache_proc_open(struct inode *inode, struct file *file)
> static ssize_t cifs_lookup_cache_proc_write(struct file *file,
> const char __user *buffer, size_t count, loff_t *ppos)
> {
> - char c;
> - bool bv;
> int rc;
>
> - rc = get_user(c, buffer);
> + rc = kstrtobool_from_user(buffer, count, 0, &lookupCacheEnabled);
> if (rc)
> return rc;
>
> - rc = strtobool(&c, &bv);
> - if (rc)
> - return rc;
> -
> - lookupCacheEnabled = bv;
> -
> return count;
> }
>
> @@ -551,20 +533,12 @@ static int traceSMB_proc_open(struct inode *inode, struct file *file)
> static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> - char c;
> - bool bv;
> int rc;
>
> - rc = get_user(c, buffer);
> + rc = kstrtobool_from_user(buffer, count, 0, &traceSMB);
> if (rc)
> return rc;
>
> - rc = strtobool(&c, &bv);
> - if (rc)
> - return rc;
> -
> - traceSMB = bv;
> -
> return count;
> }
>
> @@ -622,7 +596,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
> int rc;
> unsigned int flags;
> char flags_string[12];
> - char c;
> + char c[2] = { '\0' };
Can we use flag_string directly here?
> bool bv;
>
> if ((count < 1) || (count > 11))
> @@ -635,11 +609,11 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
>
> if (count < 3) {
> /* single char or single char followed by null */
> - c = flags_string[0];
> - if (strtobool(&c, &bv) == 0) {
> + c[0] = flags_string[0];
> + if (strtobool(c, &bv) == 0) {
> global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
> return count;
> - } else if (!isdigit(c)) {
> + } else if (!isdigit(c[0])) {
> cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
> flags_string);
> return -EINVAL;
> diff --git a/fs/cifs/cifs_debug.h b/fs/cifs/cifs_debug.h
> index 66cf0f9fff89..c611ca2339d7 100644
> --- a/fs/cifs/cifs_debug.h
> +++ b/fs/cifs/cifs_debug.h
> @@ -25,7 +25,7 @@
> void cifs_dump_mem(char *label, void *data, int length);
> void cifs_dump_detail(void *);
> void cifs_dump_mids(struct TCP_Server_Info *);
> -extern int traceSMB; /* flag which enables the function below */
> +extern bool traceSMB; /* flag which enables the function below */
> void dump_smb(void *, int);
> #define CIFS_INFO 0x01
> #define CIFS_RC 0x02
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index c48ca13673e3..931b446f2a44 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -54,10 +54,10 @@
> #endif
>
> int cifsFYI = 0;
> -int traceSMB = 0;
> +bool traceSMB;
> bool enable_oplocks = true;
> -unsigned int linuxExtEnabled = 1;
> -unsigned int lookupCacheEnabled = 1;
> +bool linuxExtEnabled = true;
> +bool lookupCacheEnabled = true;
> unsigned int global_secflags = CIFSSEC_DEF;
> /* unsigned int ntlmv2_support = 0; */
> unsigned int sign_CIFS_PDUs = 1;
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index a25b2513f146..d21da9f05bae 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -1596,11 +1596,11 @@ GLOBAL_EXTERN atomic_t midCount;
>
> /* Misc globals */
> GLOBAL_EXTERN bool enable_oplocks; /* enable or disable oplocks */
> -GLOBAL_EXTERN unsigned int lookupCacheEnabled;
> +GLOBAL_EXTERN bool lookupCacheEnabled;
> GLOBAL_EXTERN unsigned int global_secflags; /* if on, session setup sent
> with more secure ntlmssp2 challenge/resp */
> GLOBAL_EXTERN unsigned int sign_CIFS_PDUs; /* enable smb packet signing */
> -GLOBAL_EXTERN unsigned int linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> +GLOBAL_EXTERN bool linuxExtEnabled;/*enable Linux/Unix CIFS extensions*/
> GLOBAL_EXTERN unsigned int CIFSMaxBufSize; /* max size not including hdr */
> GLOBAL_EXTERN unsigned int cifs_min_rcv; /* min size of big ntwrk buf pool */
> GLOBAL_EXTERN unsigned int cifs_min_small; /* min size of small buf pool */
> --
> 2.6.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 2/4] lib: update single-char callers of strtobool
2016-02-04 22:59 ` Andy Shevchenko
@ 2016-02-04 23:01 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-04 23:01 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, linux-cifs, Joe Perches, Rasmus Villemoes,
Daniel Borkmann, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, open list:TI WILINK WIRELES...,
netdev, linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 2:59 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>> Some callers of strtobool were passing a pointer to unterminated strings.
>> In preparation of adding multi-character processing to kstrtobool, update
>> the callers to not pass single-character pointers, and switch to using the
>> new kstrtobool_from_user helper where possible.
>
> Looks much better now!
> My comment below.
>
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: Amitkumar Karwar <akarwar@marvell.com>
>> Cc: Nishant Sarmukadam <nishants@marvell.com>
>> Cc: Kalle Valo <kvalo@codeaurora.org>
>> Cc: Steve French <sfrench@samba.org>
>> Cc: linux-cifs@vger.kernel.org
>> ---
>> drivers/net/wireless/marvell/mwifiex/debugfs.c | 10 ++---
>> fs/cifs/cifs_debug.c | 58 +++++++-------------------
>> fs/cifs/cifs_debug.h | 2 +-
>> fs/cifs/cifsfs.c | 6 +--
>> fs/cifs/cifsglob.h | 4 +-
>> 5 files changed, 26 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c b/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> index 0b9c580af988..bd061b02bc04 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> @@ -880,14 +880,12 @@ mwifiex_reset_write(struct file *file,
>> {
>> struct mwifiex_private *priv = file->private_data;
>> struct mwifiex_adapter *adapter = priv->adapter;
>> - char cmd;
>> bool result;
>> + int rc;
>>
>> - if (copy_from_user(&cmd, ubuf, sizeof(cmd)))
>> - return -EFAULT;
>> -
>> - if (strtobool(&cmd, &result))
>> - return -EINVAL;
>> + rc = kstrtobool_from_user(ubuf, count, 0, &result);
>> + if (rc)
>> + return rc;
>>
>> if (!result)
>> return -EINVAL;
>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
>> index 50b268483302..6ee59abcb69b 100644
>> --- a/fs/cifs/cifs_debug.c
>> +++ b/fs/cifs/cifs_debug.c
>> @@ -255,7 +255,6 @@ static const struct file_operations cifs_debug_data_proc_fops = {
>> static ssize_t cifs_stats_proc_write(struct file *file,
>> const char __user *buffer, size_t count, loff_t *ppos)
>> {
>> - char c;
>> bool bv;
>> int rc;
>> struct list_head *tmp1, *tmp2, *tmp3;
>> @@ -263,11 +262,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>> struct cifs_ses *ses;
>> struct cifs_tcon *tcon;
>>
>> - rc = get_user(c, buffer);
>> - if (rc)
>> - return rc;
>> -
>> - if (strtobool(&c, &bv) == 0) {
>> + rc = kstrtobool_from_user(buffer, count, 0, &bv);
>> + if (rc == 0) {
>> #ifdef CONFIG_CIFS_STATS2
>> atomic_set(&totBufAllocCount, 0);
>> atomic_set(&totSmBufAllocCount, 0);
>> @@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>> }
>> }
>> spin_unlock(&cifs_tcp_ses_lock);
>> + } else {
>> + return rc;
>> }
>>
>> return count;
>> @@ -433,17 +431,17 @@ static int cifsFYI_proc_open(struct inode *inode, struct file *file)
>> static ssize_t cifsFYI_proc_write(struct file *file, const char __user *buffer,
>> size_t count, loff_t *ppos)
>> {
>> - char c;
>> + char c[2] = { '\0' };
>> bool bv;
>> int rc;
>>
>> - rc = get_user(c, buffer);
>> + rc = get_user(c[0], buffer);
>
>> if (rc)
>> return rc;
>> - if (strtobool(&c, &bv) == 0)
>> + if (strtobool(c, &bv) == 0)
>> cifsFYI = bv;
>> - else if ((c > '1') && (c <= '9'))
>> - cifsFYI = (int) (c - '0'); /* see cifs_debug.h for meanings */
>> + else if ((c[0] > '1') && (c[0] <= '9'))
>> + cifsFYI = (int) (c[0] - '0'); /* see cifs_debug.h for meanings */
>>
>> return count;
>> }
>> @@ -471,20 +469,12 @@ static int cifs_linux_ext_proc_open(struct inode *inode, struct file *file)
>> static ssize_t cifs_linux_ext_proc_write(struct file *file,
>> const char __user *buffer, size_t count, loff_t *ppos)
>> {
>> - char c;
>> - bool bv;
>> int rc;
>>
>> - rc = get_user(c, buffer);
>> + rc = kstrtobool_from_user(buffer, count, 0, &linuxExtEnabled);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - linuxExtEnabled = bv;
>> -
>> return count;
>> }
>>
>> @@ -511,20 +501,12 @@ static int cifs_lookup_cache_proc_open(struct inode *inode, struct file *file)
>> static ssize_t cifs_lookup_cache_proc_write(struct file *file,
>> const char __user *buffer, size_t count, loff_t *ppos)
>> {
>> - char c;
>> - bool bv;
>> int rc;
>>
>> - rc = get_user(c, buffer);
>> + rc = kstrtobool_from_user(buffer, count, 0, &lookupCacheEnabled);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - lookupCacheEnabled = bv;
>> -
>> return count;
>> }
>>
>> @@ -551,20 +533,12 @@ static int traceSMB_proc_open(struct inode *inode, struct file *file)
>> static ssize_t traceSMB_proc_write(struct file *file, const char __user *buffer,
>> size_t count, loff_t *ppos)
>> {
>> - char c;
>> - bool bv;
>> int rc;
>>
>> - rc = get_user(c, buffer);
>> + rc = kstrtobool_from_user(buffer, count, 0, &traceSMB);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - traceSMB = bv;
>> -
>> return count;
>> }
>>
>> @@ -622,7 +596,7 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
>> int rc;
>> unsigned int flags;
>> char flags_string[12];
>> - char c;
>> + char c[2] = { '\0' };
>
> Can we use flag_string directly here?
>
>> bool bv;
>>
>> if ((count < 1) || (count > 11))
>> @@ -635,11 +609,11 @@ static ssize_t cifs_security_flags_proc_write(struct file *file,
>>
>> if (count < 3) {
>> /* single char or single char followed by null */
>> - c = flags_string[0];
>> - if (strtobool(&c, &bv) == 0) {
>> + c[0] = flags_string[0];
>
>> + if (strtobool(c, &bv) == 0) {
>
>
>> global_secflags = bv ? CIFSSEC_MAX : CIFSSEC_DEF;
>> return count;
>> - } else if (!isdigit(c)) {
>> + } else if (!isdigit(c[0])) {
>> cifs_dbg(VFS, "Invalid SecurityFlags: %s\n",
>> flags_string);
>> return -EINVAL;
Hah, yes, durrr. I will send a fix-up for akpm. Thanks!
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread
* RE: [PATCH v2 2/4] lib: update single-char callers of strtobool
2016-02-04 21:00 ` [PATCH v2 2/4] lib: update single-char callers of strtobool Kees Cook
2016-02-04 22:59 ` Andy Shevchenko
@ 2016-02-05 10:46 ` David Laight
2016-02-05 21:18 ` Kees Cook
1 sibling, 1 reply; 18+ messages in thread
From: David Laight @ 2016-02-05 10:46 UTC (permalink / raw)
To: 'Kees Cook', Andrew Morton
Cc: Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
linux-cifs@vger.kernel.org, Joe Perches, Andy Shevchenko,
Rasmus Villemoes, Daniel Borkmann, Michael Ellerman,
Heiko Carstens, Martin Schwidefsky, x86@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-s390@vger.kernel.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
From: Kees Cook
> Sent: 04 February 2016 21:01
> Some callers of strtobool were passing a pointer to unterminated strings.
> In preparation of adding multi-character processing to kstrtobool, update
> the callers to not pass single-character pointers, and switch to using th=
e
> new kstrtobool_from_user helper where possible.
Personally I think you should change the name of the function so that the
compiler (and linker) will pick up places that have not been changed.
Relying on people to make the required changes will cause problems.
The current code (presumably) treats "no", "nyet" and "nkjkkrkjrkjterkj" as=
false.
Changing that behaviour will break things.
If you want to support "on" and "off", then maybe check for the supplied st=
ring
starting with the character sequences "on\0" and "off\0" (as well as any ot=
hers).
This doesn't need the input string be '\0' terminated - since you match y a=
nd n
without looking at the 2nd byte.
You'd have to be extremely unlucky to get a page fault in the 3 bytes
following an 'o' if the caller supplied a single byte buffer.
David
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/4] lib: update single-char callers of strtobool
2016-02-05 10:46 ` David Laight
@ 2016-02-05 21:18 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-05 21:18 UTC (permalink / raw)
To: David Laight
Cc: Andrew Morton, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, linux-cifs@vger.kernel.org, Joe Perches,
Andy Shevchenko, Rasmus Villemoes, Daniel Borkmann,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
On Fri, Feb 5, 2016 at 2:46 AM, David Laight <David.Laight@aculab.com> wrote:
> From: Kees Cook
>> Sent: 04 February 2016 21:01
>> Some callers of strtobool were passing a pointer to unterminated strings.
>> In preparation of adding multi-character processing to kstrtobool, update
>> the callers to not pass single-character pointers, and switch to using the
>> new kstrtobool_from_user helper where possible.
>
> Personally I think you should change the name of the function so that the
> compiler (and linker) will pick up places that have not been changed.
> Relying on people to make the required changes will cause problems.
After the single-character users were pointed out, I looked for others
and there aren't any.
> The current code (presumably) treats "no", "nyet" and "nkjkkrkjrkjterkj" as false.
> Changing that behaviour will break things.
There's no change there. All three of those will still be "false".
Perhaps my changelog shouldn't say "unterminated" but rather
"character array".
> If you want to support "on" and "off", then maybe check for the supplied string
> starting with the character sequences "on\0" and "off\0" (as well as any others).
> This doesn't need the input string be '\0' terminated - since you match y and n
> without looking at the 2nd byte.
> You'd have to be extremely unlucky to get a page fault in the 3 bytes
> following an 'o' if the caller supplied a single byte buffer.
I'd prefer to keep the switch statement as short as possible, and I
don't want to do full string compares. And as you say, even fixing the
single-byte callers seems like a needless exercise, but seeing as how
it's a net clean-up, I think it's good they way I've got the series.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool
2016-02-04 21:00 [PATCH v2 0/4] lib: add "on" and "off" to strtobool Kees Cook
2016-02-04 21:00 ` [PATCH v2 1/4] lib: move strtobool to kstrtobool Kees Cook
2016-02-04 21:00 ` [PATCH v2 2/4] lib: update single-char callers of strtobool Kees Cook
@ 2016-02-04 21:00 ` Kees Cook
2016-02-04 23:00 ` Andy Shevchenko
2016-02-04 21:00 ` [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool Kees Cook
3 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2016-02-04 21:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Joe Perches, Andy Shevchenko, Rasmus Villemoes,
Daniel Borkmann, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, x86, linuxppc-dev, linux-s390, linux-wireless,
netdev, linux-cifs, linux-kernel
Add support for "on" and "off" when converting to boolean.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/kstrtox.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/lib/kstrtox.c b/lib/kstrtox.c
index e18f088704d7..09e83a19a96d 100644
--- a/lib/kstrtox.c
+++ b/lib/kstrtox.c
@@ -347,6 +347,20 @@ int kstrtobool(const char *s, unsigned int base, bool *res)
case '0':
*res = false;
return 0;
+ case 'o':
+ case 'O':
+ switch (s[1]) {
+ case 'n':
+ case 'N':
+ *res = true;
+ return 0;
+ case 'f':
+ case 'F':
+ *res = false;
+ return 0;
+ default:
+ break;
+ }
default:
break;
}
--
2.6.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool
2016-02-04 21:00 ` [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool Kees Cook
@ 2016-02-04 23:00 ` Andy Shevchenko
2016-02-04 23:11 ` Kees Cook
0 siblings, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2016-02-04 23:00 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, Joe Perches, Rasmus Villemoes, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev, linux-s390,
open list:TI WILINK WIRELES..., netdev, linux-cifs,
linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
> Add support for "on" and "off" when converting to boolean.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> lib/kstrtox.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
> index e18f088704d7..09e83a19a96d 100644
> --- a/lib/kstrtox.c
> +++ b/lib/kstrtox.c
> @@ -347,6 +347,20 @@ int kstrtobool(const char *s, unsigned int base, bool *res)
Forgot update description?
> case '0':
> *res = false;
> return 0;
> + case 'o':
> + case 'O':
> + switch (s[1]) {
> + case 'n':
> + case 'N':
> + *res = true;
> + return 0;
> + case 'f':
> + case 'F':
> + *res = false;
> + return 0;
> + default:
> + break;
> + }
> default:
> break;
> }
> --
> 2.6.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool
2016-02-04 23:00 ` Andy Shevchenko
@ 2016-02-04 23:11 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-04 23:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, Joe Perches, Rasmus Villemoes, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, open list:TI WILINK WIRELES...,
netdev, linux-cifs, linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 3:00 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>> Add support for "on" and "off" when converting to boolean.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> lib/kstrtox.c | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>>
>> diff --git a/lib/kstrtox.c b/lib/kstrtox.c
>> index e18f088704d7..09e83a19a96d 100644
>> --- a/lib/kstrtox.c
>> +++ b/lib/kstrtox.c
>> @@ -347,6 +347,20 @@ int kstrtobool(const char *s, unsigned int base, bool *res)
>
> Forgot update description?
Argh, thank you. Good eye. Sent another update.
-Kees
>
>> case '0':
>> *res = false;
>> return 0;
>> + case 'o':
>> + case 'O':
>> + switch (s[1]) {
>> + case 'n':
>> + case 'N':
>> + *res = true;
>> + return 0;
>> + case 'f':
>> + case 'F':
>> + *res = false;
>> + return 0;
>> + default:
>> + break;
>> + }
>> default:
>> break;
>> }
>> --
>> 2.6.3
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
2016-02-04 21:00 [PATCH v2 0/4] lib: add "on" and "off" to strtobool Kees Cook
` (2 preceding siblings ...)
2016-02-04 21:00 ` [PATCH v2 3/4] lib: add "on"/"off" support to kstrtobool Kees Cook
@ 2016-02-04 21:00 ` Kees Cook
2016-02-04 23:04 ` Andy Shevchenko
3 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2016-02-04 21:00 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, x86, linuxppc-dev, linux-s390, Joe Perches,
Andy Shevchenko, Rasmus Villemoes, Daniel Borkmann,
Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo, Steve French,
Michael Ellerman, Heiko Carstens, Martin Schwidefsky,
linux-wireless, netdev, linux-cifs, linux-kernel
This changes several users of manual "on"/"off" parsing to use strtobool.
(Which means they will now parse y/n/1/0 meaningfully too.)
Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: x86@kernel.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-s390@vger.kernel.org
---
arch/powerpc/kernel/rtasd.c | 9 ++-------
arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
arch/s390/kernel/time.c | 8 ++------
arch/s390/kernel/topology.c | 7 ++-----
arch/x86/kernel/aperture_64.c | 12 ++----------
include/linux/tick.h | 2 +-
kernel/time/hrtimer.c | 10 ++--------
kernel/time/tick-sched.c | 10 ++--------
8 files changed, 15 insertions(+), 53 deletions(-)
diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
index 5a2c049c1c61..567ed5a2f43a 100644
--- a/arch/powerpc/kernel/rtasd.c
+++ b/arch/powerpc/kernel/rtasd.c
@@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
static unsigned int event_scan;
static unsigned int rtas_event_scan_rate;
-static int full_rtas_msgs = 0;
+static bool full_rtas_msgs;
/* Stop logging to nvram after first fatal error */
static int logging_enabled; /* Until we initialize everything,
@@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
static int __init rtasmsgs_setup(char *str)
{
- if (strcmp(str, "on") == 0)
- full_rtas_msgs = 1;
- else if (strcmp(str, "off") == 0)
- full_rtas_msgs = 0;
-
- return 1;
+ return kstrtobool(str, 0, &full_rtas_msgs);
}
__setup("rtasmsgs=", rtasmsgs_setup);
diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
index 32274f72fe3f..b9787cae4108 100644
--- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
+++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
@@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
-static int cede_offline_enabled __read_mostly = 1;
+static bool cede_offline_enabled __read_mostly = true;
/*
* Enable/disable cede_offline when available.
*/
static int __init setup_cede_offline(char *str)
{
- if (!strcmp(str, "off"))
- cede_offline_enabled = 0;
- else if (!strcmp(str, "on"))
- cede_offline_enabled = 1;
- else
- return 0;
- return 1;
+ return kstrtobool(str, 0, &cede_offline_enabled);
}
__setup("cede_offline=", setup_cede_offline);
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 99f84ac31307..dff6ce1b84b2 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
/*
* Server Time Protocol (STP) code.
*/
-static int stp_online;
+static bool stp_online;
static struct stp_sstpi stp_info;
static void *stp_page;
@@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
static int __init early_parse_stp(char *p)
{
- if (strncmp(p, "off", 3) == 0)
- stp_online = 0;
- else if (strncmp(p, "on", 2) == 0)
- stp_online = 1;
- return 0;
+ return kstrtobool(p, 0, &stp_online);
}
early_param("stp", early_parse_stp);
diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
index 40b8102fdadb..5d8a80651f61 100644
--- a/arch/s390/kernel/topology.c
+++ b/arch/s390/kernel/topology.c
@@ -37,7 +37,7 @@ static void set_topology_timer(void);
static void topology_work_fn(struct work_struct *work);
static struct sysinfo_15_1_x *tl_info;
-static int topology_enabled = 1;
+static bool topology_enabled = true;
static DECLARE_WORK(topology_work, topology_work_fn);
/*
@@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
static int __init early_parse_topology(char *p)
{
- if (strncmp(p, "off", 3))
- return 0;
- topology_enabled = 0;
- return 0;
+ return kstrtobool(p, 0, &topology_enabled);
}
early_param("topology", early_parse_topology);
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 6e85f713641d..6b423754083a 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
return 0;
}
-static int gart_fix_e820 __initdata = 1;
+static bool gart_fix_e820 __initdata = true;
static int __init parse_gart_mem(char *p)
{
- if (!p)
- return -EINVAL;
-
- if (!strncmp(p, "off", 3))
- gart_fix_e820 = 0;
- else if (!strncmp(p, "on", 2))
- gart_fix_e820 = 1;
-
- return 0;
+ return kstrtobool(p, 0, &gart_fix_e820);
}
early_param("gart_fix_e820", parse_gart_mem);
diff --git a/include/linux/tick.h b/include/linux/tick.h
index 97fd4e543846..0ecdf0e248f4 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
}
#ifdef CONFIG_NO_HZ_COMMON
-extern int tick_nohz_enabled;
+extern bool tick_nohz_enabled;
extern int tick_nohz_tick_stopped(void);
extern void tick_nohz_idle_enter(void);
extern void tick_nohz_idle_exit(void);
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 435b8850dd80..456f066148d5 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
/*
* High resolution timer enabled ?
*/
-static int hrtimer_hres_enabled __read_mostly = 1;
+static bool hrtimer_hres_enabled __read_mostly = true;
unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
EXPORT_SYMBOL_GPL(hrtimer_resolution);
@@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
*/
static int __init setup_hrtimer_hres(char *str)
{
- if (!strcmp(str, "off"))
- hrtimer_hres_enabled = 0;
- else if (!strcmp(str, "on"))
- hrtimer_hres_enabled = 1;
- else
- return 0;
- return 1;
+ return kstrtobool(str, 0, &hrtimer_hres_enabled);
}
__setup("highres=", setup_hrtimer_hres);
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9d7a053545f5..b57f822c2069 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
/*
* NO HZ enabled ?
*/
-int tick_nohz_enabled __read_mostly = 1;
+bool tick_nohz_enabled __read_mostly = true;
unsigned long tick_nohz_active __read_mostly;
/*
* Enable / Disable tickless mode
*/
static int __init setup_tick_nohz(char *str)
{
- if (!strcmp(str, "off"))
- tick_nohz_enabled = 0;
- else if (!strcmp(str, "on"))
- tick_nohz_enabled = 1;
- else
- return 0;
- return 1;
+ return kstrtobool(str, 0, &tick_nohz_enabled);
}
__setup("nohz=", setup_tick_nohz);
--
2.6.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
2016-02-04 21:00 ` [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool Kees Cook
@ 2016-02-04 23:04 ` Andy Shevchenko
2016-02-05 0:11 ` Kees Cook
0 siblings, 1 reply; 18+ messages in thread
From: Andy Shevchenko @ 2016-02-04 23:04 UTC (permalink / raw)
To: Kees Cook
Cc: Andrew Morton, x86@kernel.org, linuxppc-dev, linux-s390,
Joe Perches, Rasmus Villemoes, Daniel Borkmann, Amitkumar Karwar,
Nishant Sarmukadam, Kalle Valo, Steve French, Michael Ellerman,
Heiko Carstens, Martin Schwidefsky,
open list:TI WILINK WIRELES..., netdev, linux-cifs,
linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
> This changes several users of manual "on"/"off" parsing to use strtobool.
> (Which means they will now parse y/n/1/0 meaningfully too.)
>
I like this change, but can you carefully check the acceptance of the
returned value?
Briefly I saw 1 or 0 as okay in different places.
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
> Cc: x86@kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-s390@vger.kernel.org
> ---
> arch/powerpc/kernel/rtasd.c | 9 ++-------
> arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
> arch/s390/kernel/time.c | 8 ++------
> arch/s390/kernel/topology.c | 7 ++-----
> arch/x86/kernel/aperture_64.c | 12 ++----------
> include/linux/tick.h | 2 +-
> kernel/time/hrtimer.c | 10 ++--------
> kernel/time/tick-sched.c | 10 ++--------
> 8 files changed, 15 insertions(+), 53 deletions(-)
>
> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
> index 5a2c049c1c61..567ed5a2f43a 100644
> --- a/arch/powerpc/kernel/rtasd.c
> +++ b/arch/powerpc/kernel/rtasd.c
> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
> static unsigned int event_scan;
> static unsigned int rtas_event_scan_rate;
>
> -static int full_rtas_msgs = 0;
> +static bool full_rtas_msgs;
>
> /* Stop logging to nvram after first fatal error */
> static int logging_enabled; /* Until we initialize everything,
> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>
> static int __init rtasmsgs_setup(char *str)
> {
> - if (strcmp(str, "on") == 0)
> - full_rtas_msgs = 1;
> - else if (strcmp(str, "off") == 0)
> - full_rtas_msgs = 0;
> -
> - return 1;
> + return kstrtobool(str, 0, &full_rtas_msgs);
> }
> __setup("rtasmsgs=", rtasmsgs_setup);
> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> index 32274f72fe3f..b9787cae4108 100644
> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>
> static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>
> -static int cede_offline_enabled __read_mostly = 1;
> +static bool cede_offline_enabled __read_mostly = true;
>
> /*
> * Enable/disable cede_offline when available.
> */
> static int __init setup_cede_offline(char *str)
> {
> - if (!strcmp(str, "off"))
> - cede_offline_enabled = 0;
> - else if (!strcmp(str, "on"))
> - cede_offline_enabled = 1;
> - else
> - return 0;
> - return 1;
> + return kstrtobool(str, 0, &cede_offline_enabled);
> }
>
> __setup("cede_offline=", setup_cede_offline);
> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
> index 99f84ac31307..dff6ce1b84b2 100644
> --- a/arch/s390/kernel/time.c
> +++ b/arch/s390/kernel/time.c
> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
> /*
> * Server Time Protocol (STP) code.
> */
> -static int stp_online;
> +static bool stp_online;
> static struct stp_sstpi stp_info;
> static void *stp_page;
>
> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>
> static int __init early_parse_stp(char *p)
> {
> - if (strncmp(p, "off", 3) == 0)
> - stp_online = 0;
> - else if (strncmp(p, "on", 2) == 0)
> - stp_online = 1;
> - return 0;
> + return kstrtobool(p, 0, &stp_online);
> }
> early_param("stp", early_parse_stp);
>
> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
> index 40b8102fdadb..5d8a80651f61 100644
> --- a/arch/s390/kernel/topology.c
> +++ b/arch/s390/kernel/topology.c
> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
> static void topology_work_fn(struct work_struct *work);
> static struct sysinfo_15_1_x *tl_info;
>
> -static int topology_enabled = 1;
> +static bool topology_enabled = true;
> static DECLARE_WORK(topology_work, topology_work_fn);
>
> /*
> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>
> static int __init early_parse_topology(char *p)
> {
> - if (strncmp(p, "off", 3))
> - return 0;
> - topology_enabled = 0;
> - return 0;
> + return kstrtobool(p, 0, &topology_enabled);
> }
> early_param("topology", early_parse_topology);
>
> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
> index 6e85f713641d..6b423754083a 100644
> --- a/arch/x86/kernel/aperture_64.c
> +++ b/arch/x86/kernel/aperture_64.c
> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
> return 0;
> }
>
> -static int gart_fix_e820 __initdata = 1;
> +static bool gart_fix_e820 __initdata = true;
>
> static int __init parse_gart_mem(char *p)
> {
> - if (!p)
> - return -EINVAL;
> -
> - if (!strncmp(p, "off", 3))
> - gart_fix_e820 = 0;
> - else if (!strncmp(p, "on", 2))
> - gart_fix_e820 = 1;
> -
> - return 0;
> + return kstrtobool(p, 0, &gart_fix_e820);
> }
> early_param("gart_fix_e820", parse_gart_mem);
>
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index 97fd4e543846..0ecdf0e248f4 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
> }
>
> #ifdef CONFIG_NO_HZ_COMMON
> -extern int tick_nohz_enabled;
> +extern bool tick_nohz_enabled;
> extern int tick_nohz_tick_stopped(void);
> extern void tick_nohz_idle_enter(void);
> extern void tick_nohz_idle_exit(void);
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 435b8850dd80..456f066148d5 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
> /*
> * High resolution timer enabled ?
> */
> -static int hrtimer_hres_enabled __read_mostly = 1;
> +static bool hrtimer_hres_enabled __read_mostly = true;
> unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
> EXPORT_SYMBOL_GPL(hrtimer_resolution);
>
> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
> */
> static int __init setup_hrtimer_hres(char *str)
> {
> - if (!strcmp(str, "off"))
> - hrtimer_hres_enabled = 0;
> - else if (!strcmp(str, "on"))
> - hrtimer_hres_enabled = 1;
> - else
> - return 0;
> - return 1;
> + return kstrtobool(str, 0, &hrtimer_hres_enabled);
> }
>
> __setup("highres=", setup_hrtimer_hres);
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 9d7a053545f5..b57f822c2069 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
> /*
> * NO HZ enabled ?
> */
> -int tick_nohz_enabled __read_mostly = 1;
> +bool tick_nohz_enabled __read_mostly = true;
> unsigned long tick_nohz_active __read_mostly;
> /*
> * Enable / Disable tickless mode
> */
> static int __init setup_tick_nohz(char *str)
> {
> - if (!strcmp(str, "off"))
> - tick_nohz_enabled = 0;
> - else if (!strcmp(str, "on"))
> - tick_nohz_enabled = 1;
> - else
> - return 0;
> - return 1;
> + return kstrtobool(str, 0, &tick_nohz_enabled);
> }
>
> __setup("nohz=", setup_tick_nohz);
> --
> 2.6.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
2016-02-04 23:04 ` Andy Shevchenko
@ 2016-02-05 0:11 ` Kees Cook
2016-02-05 2:12 ` Kees Cook
0 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2016-02-05 0:11 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, Joe Perches, Rasmus Villemoes,
Daniel Borkmann, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, open list:TI WILINK WIRELES..., netdev,
linux-cifs, linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>> This changes several users of manual "on"/"off" parsing to use strtobool.
>> (Which means they will now parse y/n/1/0 meaningfully too.)
>>
>
> I like this change, but can you carefully check the acceptance of the
> returned value?
> Briefly I saw 1 or 0 as okay in different places.
Maybe I missed something, but I think this is actually a bug fix. The
two cases are early_param and __setup:
For early_param, the functions are called when walking the command
line in do_early_param via parse_args in parse_early_options. Any
non-zero return values produce a warning (in do_early_param not
parse_args). So this is a bug fix, since the function I touched would
(almost) always return 0, even with bad values (i.e. fixes unreported
bad arguments):
early_param early_parse_stp always 0
early_param early_parse_topology always 0
early_param parse_gart_mem always 0 unless !p (then -EINVAL)
For __setup, these are handled by obsolete_checksetup via
unknown_bootoption via parse_args in start_kernel, as a way to merge
__setup calls that should really be in param (i.e. non-early __setup).
Return values are bubbled up into parse_args and hit:
default:
pr_err("%s: `%s' invalid for parameter `%s'\n",
doing, val ?: "", param);
break;
So this is also a bug fix, since these __setup functions returned inverted
values or always failed:
__setup rtasmsgs_setup always 1
__setup setup_cede_offline 1 on success, otherwise 0
__setup setup_hrtimer_hres 1 on success, otherwise 0
__setup setup_tick_nohz 1 on success, otherwise 0
So if you specified any of these, they would trigger a bogus "invalid
parameter" report.
I will double-check...
-Kees
>
>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: x86@kernel.org
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-s390@vger.kernel.org
>> ---
>> arch/powerpc/kernel/rtasd.c | 9 ++-------
>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
>> arch/s390/kernel/time.c | 8 ++------
>> arch/s390/kernel/topology.c | 7 ++-----
>> arch/x86/kernel/aperture_64.c | 12 ++----------
>> include/linux/tick.h | 2 +-
>> kernel/time/hrtimer.c | 10 ++--------
>> kernel/time/tick-sched.c | 10 ++--------
>> 8 files changed, 15 insertions(+), 53 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
>> index 5a2c049c1c61..567ed5a2f43a 100644
>> --- a/arch/powerpc/kernel/rtasd.c
>> +++ b/arch/powerpc/kernel/rtasd.c
>> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
>> static unsigned int event_scan;
>> static unsigned int rtas_event_scan_rate;
>>
>> -static int full_rtas_msgs = 0;
>> +static bool full_rtas_msgs;
>>
>> /* Stop logging to nvram after first fatal error */
>> static int logging_enabled; /* Until we initialize everything,
>> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>>
>> static int __init rtasmsgs_setup(char *str)
>> {
>> - if (strcmp(str, "on") == 0)
>> - full_rtas_msgs = 1;
>> - else if (strcmp(str, "off") == 0)
>> - full_rtas_msgs = 0;
>> -
>> - return 1;
>> + return kstrtobool(str, 0, &full_rtas_msgs);
>> }
>> __setup("rtasmsgs=", rtasmsgs_setup);
>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> index 32274f72fe3f..b9787cae4108 100644
>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>>
>> static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>>
>> -static int cede_offline_enabled __read_mostly = 1;
>> +static bool cede_offline_enabled __read_mostly = true;
>>
>> /*
>> * Enable/disable cede_offline when available.
>> */
>> static int __init setup_cede_offline(char *str)
>> {
>> - if (!strcmp(str, "off"))
>> - cede_offline_enabled = 0;
>> - else if (!strcmp(str, "on"))
>> - cede_offline_enabled = 1;
>> - else
>> - return 0;
>> - return 1;
>> + return kstrtobool(str, 0, &cede_offline_enabled);
>> }
>>
>> __setup("cede_offline=", setup_cede_offline);
>> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
>> index 99f84ac31307..dff6ce1b84b2 100644
>> --- a/arch/s390/kernel/time.c
>> +++ b/arch/s390/kernel/time.c
>> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
>> /*
>> * Server Time Protocol (STP) code.
>> */
>> -static int stp_online;
>> +static bool stp_online;
>> static struct stp_sstpi stp_info;
>> static void *stp_page;
>>
>> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>>
>> static int __init early_parse_stp(char *p)
>> {
>> - if (strncmp(p, "off", 3) == 0)
>> - stp_online = 0;
>> - else if (strncmp(p, "on", 2) == 0)
>> - stp_online = 1;
>> - return 0;
>> + return kstrtobool(p, 0, &stp_online);
>> }
>> early_param("stp", early_parse_stp);
>>
>> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
>> index 40b8102fdadb..5d8a80651f61 100644
>> --- a/arch/s390/kernel/topology.c
>> +++ b/arch/s390/kernel/topology.c
>> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
>> static void topology_work_fn(struct work_struct *work);
>> static struct sysinfo_15_1_x *tl_info;
>>
>> -static int topology_enabled = 1;
>> +static bool topology_enabled = true;
>> static DECLARE_WORK(topology_work, topology_work_fn);
>>
>> /*
>> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>>
>> static int __init early_parse_topology(char *p)
>> {
>> - if (strncmp(p, "off", 3))
>> - return 0;
>> - topology_enabled = 0;
>> - return 0;
>> + return kstrtobool(p, 0, &topology_enabled);
>> }
>> early_param("topology", early_parse_topology);
>>
>> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
>> index 6e85f713641d..6b423754083a 100644
>> --- a/arch/x86/kernel/aperture_64.c
>> +++ b/arch/x86/kernel/aperture_64.c
>> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
>> return 0;
>> }
>>
>> -static int gart_fix_e820 __initdata = 1;
>> +static bool gart_fix_e820 __initdata = true;
>>
>> static int __init parse_gart_mem(char *p)
>> {
>> - if (!p)
>> - return -EINVAL;
>> -
>> - if (!strncmp(p, "off", 3))
>> - gart_fix_e820 = 0;
>> - else if (!strncmp(p, "on", 2))
>> - gart_fix_e820 = 1;
>> -
>> - return 0;
>> + return kstrtobool(p, 0, &gart_fix_e820);
>> }
>> early_param("gart_fix_e820", parse_gart_mem);
>>
>> diff --git a/include/linux/tick.h b/include/linux/tick.h
>> index 97fd4e543846..0ecdf0e248f4 100644
>> --- a/include/linux/tick.h
>> +++ b/include/linux/tick.h
>> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
>> }
>>
>> #ifdef CONFIG_NO_HZ_COMMON
>> -extern int tick_nohz_enabled;
>> +extern bool tick_nohz_enabled;
>> extern int tick_nohz_tick_stopped(void);
>> extern void tick_nohz_idle_enter(void);
>> extern void tick_nohz_idle_exit(void);
>> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
>> index 435b8850dd80..456f066148d5 100644
>> --- a/kernel/time/hrtimer.c
>> +++ b/kernel/time/hrtimer.c
>> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>> /*
>> * High resolution timer enabled ?
>> */
>> -static int hrtimer_hres_enabled __read_mostly = 1;
>> +static bool hrtimer_hres_enabled __read_mostly = true;
>> unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
>> EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>
>> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
>> */
>> static int __init setup_hrtimer_hres(char *str)
>> {
>> - if (!strcmp(str, "off"))
>> - hrtimer_hres_enabled = 0;
>> - else if (!strcmp(str, "on"))
>> - hrtimer_hres_enabled = 1;
>> - else
>> - return 0;
>> - return 1;
>> + return kstrtobool(str, 0, &hrtimer_hres_enabled);
>> }
>>
>> __setup("highres=", setup_hrtimer_hres);
>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>> index 9d7a053545f5..b57f822c2069 100644
>> --- a/kernel/time/tick-sched.c
>> +++ b/kernel/time/tick-sched.c
>> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
>> /*
>> * NO HZ enabled ?
>> */
>> -int tick_nohz_enabled __read_mostly = 1;
>> +bool tick_nohz_enabled __read_mostly = true;
>> unsigned long tick_nohz_active __read_mostly;
>> /*
>> * Enable / Disable tickless mode
>> */
>> static int __init setup_tick_nohz(char *str)
>> {
>> - if (!strcmp(str, "off"))
>> - tick_nohz_enabled = 0;
>> - else if (!strcmp(str, "on"))
>> - tick_nohz_enabled = 1;
>> - else
>> - return 0;
>> - return 1;
>> + return kstrtobool(str, 0, &tick_nohz_enabled);
>> }
>>
>> __setup("nohz=", setup_tick_nohz);
>> --
>> 2.6.3
>>
>
>
>
> --
> With Best Regards,
> Andy Shevchenko
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 4/4] param: convert some "on"/"off" users to strtobool
2016-02-05 0:11 ` Kees Cook
@ 2016-02-05 2:12 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2016-02-05 2:12 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andrew Morton, x86@kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-s390@vger.kernel.org, Joe Perches, Rasmus Villemoes,
Daniel Borkmann, Amitkumar Karwar, Nishant Sarmukadam, Kalle Valo,
Steve French, Michael Ellerman, Heiko Carstens,
Martin Schwidefsky, open list:TI WILINK WIRELES..., netdev,
linux-cifs, linux-kernel@vger.kernel.org
On Thu, Feb 4, 2016 at 4:11 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 4, 2016 at 3:04 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@chromium.org> wrote:
>>> This changes several users of manual "on"/"off" parsing to use strtobool.
>>> (Which means they will now parse y/n/1/0 meaningfully too.)
>>>
>>
>> I like this change, but can you carefully check the acceptance of the
>> returned value?
>> Briefly I saw 1 or 0 as okay in different places.
>
> Maybe I missed something, but I think this is actually a bug fix. The
> two cases are early_param and __setup:
>
> For early_param, the functions are called when walking the command
> line in do_early_param via parse_args in parse_early_options. Any
> non-zero return values produce a warning (in do_early_param not
> parse_args). So this is a bug fix, since the function I touched would
> (almost) always return 0, even with bad values (i.e. fixes unreported
> bad arguments):
>
> early_param early_parse_stp always 0
> early_param early_parse_topology always 0
> early_param parse_gart_mem always 0 unless !p (then -EINVAL)
>
> For __setup, these are handled by obsolete_checksetup via
> unknown_bootoption via parse_args in start_kernel, as a way to merge
> __setup calls that should really be in param (i.e. non-early __setup).
> Return values are bubbled up into parse_args and hit:
>
> default:
> pr_err("%s: `%s' invalid for parameter `%s'\n",
> doing, val ?: "", param);
> break;
>
> So this is also a bug fix, since these __setup functions returned inverted
> values or always failed:
>
> __setup rtasmsgs_setup always 1
> __setup setup_cede_offline 1 on success, otherwise 0
> __setup setup_hrtimer_hres 1 on success, otherwise 0
> __setup setup_tick_nohz 1 on success, otherwise 0
>
> So if you specified any of these, they would trigger a bogus "invalid
> parameter" report.
>
> I will double-check...
I am wrong! __setup functions (as handled by unknown_bootoption) need
to return 1, or they end up in the init environment. I will send a
fix...
-Kees
>
> -Kees
>
>>
>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Acked-by: Heiko Carstens <heiko.carstens@de.ibm.com>
>>> Acked-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Cc: x86@kernel.org
>>> Cc: linuxppc-dev@lists.ozlabs.org
>>> Cc: linux-s390@vger.kernel.org
>>> ---
>>> arch/powerpc/kernel/rtasd.c | 9 ++-------
>>> arch/powerpc/platforms/pseries/hotplug-cpu.c | 10 ++--------
>>> arch/s390/kernel/time.c | 8 ++------
>>> arch/s390/kernel/topology.c | 7 ++-----
>>> arch/x86/kernel/aperture_64.c | 12 ++----------
>>> include/linux/tick.h | 2 +-
>>> kernel/time/hrtimer.c | 10 ++--------
>>> kernel/time/tick-sched.c | 10 ++--------
>>> 8 files changed, 15 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/rtasd.c b/arch/powerpc/kernel/rtasd.c
>>> index 5a2c049c1c61..567ed5a2f43a 100644
>>> --- a/arch/powerpc/kernel/rtasd.c
>>> +++ b/arch/powerpc/kernel/rtasd.c
>>> @@ -49,7 +49,7 @@ static unsigned int rtas_error_log_buffer_max;
>>> static unsigned int event_scan;
>>> static unsigned int rtas_event_scan_rate;
>>>
>>> -static int full_rtas_msgs = 0;
>>> +static bool full_rtas_msgs;
>>>
>>> /* Stop logging to nvram after first fatal error */
>>> static int logging_enabled; /* Until we initialize everything,
>>> @@ -592,11 +592,6 @@ __setup("surveillance=", surveillance_setup);
>>>
>>> static int __init rtasmsgs_setup(char *str)
>>> {
>>> - if (strcmp(str, "on") == 0)
>>> - full_rtas_msgs = 1;
>>> - else if (strcmp(str, "off") == 0)
>>> - full_rtas_msgs = 0;
>>> -
>>> - return 1;
>>> + return kstrtobool(str, 0, &full_rtas_msgs);
>>> }
>>> __setup("rtasmsgs=", rtasmsgs_setup);
>>> diff --git a/arch/powerpc/platforms/pseries/hotplug-cpu.c b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> index 32274f72fe3f..b9787cae4108 100644
>>> --- a/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> +++ b/arch/powerpc/platforms/pseries/hotplug-cpu.c
>>> @@ -47,20 +47,14 @@ static DEFINE_PER_CPU(enum cpu_state_vals, current_state) = CPU_STATE_OFFLINE;
>>>
>>> static enum cpu_state_vals default_offline_state = CPU_STATE_OFFLINE;
>>>
>>> -static int cede_offline_enabled __read_mostly = 1;
>>> +static bool cede_offline_enabled __read_mostly = true;
>>>
>>> /*
>>> * Enable/disable cede_offline when available.
>>> */
>>> static int __init setup_cede_offline(char *str)
>>> {
>>> - if (!strcmp(str, "off"))
>>> - cede_offline_enabled = 0;
>>> - else if (!strcmp(str, "on"))
>>> - cede_offline_enabled = 1;
>>> - else
>>> - return 0;
>>> - return 1;
>>> + return kstrtobool(str, 0, &cede_offline_enabled);
>>> }
>>>
>>> __setup("cede_offline=", setup_cede_offline);
>>> diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
>>> index 99f84ac31307..dff6ce1b84b2 100644
>>> --- a/arch/s390/kernel/time.c
>>> +++ b/arch/s390/kernel/time.c
>>> @@ -1433,7 +1433,7 @@ device_initcall(etr_init_sysfs);
>>> /*
>>> * Server Time Protocol (STP) code.
>>> */
>>> -static int stp_online;
>>> +static bool stp_online;
>>> static struct stp_sstpi stp_info;
>>> static void *stp_page;
>>>
>>> @@ -1444,11 +1444,7 @@ static struct timer_list stp_timer;
>>>
>>> static int __init early_parse_stp(char *p)
>>> {
>>> - if (strncmp(p, "off", 3) == 0)
>>> - stp_online = 0;
>>> - else if (strncmp(p, "on", 2) == 0)
>>> - stp_online = 1;
>>> - return 0;
>>> + return kstrtobool(p, 0, &stp_online);
>>> }
>>> early_param("stp", early_parse_stp);
>>>
>>> diff --git a/arch/s390/kernel/topology.c b/arch/s390/kernel/topology.c
>>> index 40b8102fdadb..5d8a80651f61 100644
>>> --- a/arch/s390/kernel/topology.c
>>> +++ b/arch/s390/kernel/topology.c
>>> @@ -37,7 +37,7 @@ static void set_topology_timer(void);
>>> static void topology_work_fn(struct work_struct *work);
>>> static struct sysinfo_15_1_x *tl_info;
>>>
>>> -static int topology_enabled = 1;
>>> +static bool topology_enabled = true;
>>> static DECLARE_WORK(topology_work, topology_work_fn);
>>>
>>> /*
>>> @@ -444,10 +444,7 @@ static const struct cpumask *cpu_book_mask(int cpu)
>>>
>>> static int __init early_parse_topology(char *p)
>>> {
>>> - if (strncmp(p, "off", 3))
>>> - return 0;
>>> - topology_enabled = 0;
>>> - return 0;
>>> + return kstrtobool(p, 0, &topology_enabled);
>>> }
>>> early_param("topology", early_parse_topology);
>>>
>>> diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
>>> index 6e85f713641d..6b423754083a 100644
>>> --- a/arch/x86/kernel/aperture_64.c
>>> +++ b/arch/x86/kernel/aperture_64.c
>>> @@ -227,19 +227,11 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
>>> return 0;
>>> }
>>>
>>> -static int gart_fix_e820 __initdata = 1;
>>> +static bool gart_fix_e820 __initdata = true;
>>>
>>> static int __init parse_gart_mem(char *p)
>>> {
>>> - if (!p)
>>> - return -EINVAL;
>>> -
>>> - if (!strncmp(p, "off", 3))
>>> - gart_fix_e820 = 0;
>>> - else if (!strncmp(p, "on", 2))
>>> - gart_fix_e820 = 1;
>>> -
>>> - return 0;
>>> + return kstrtobool(p, 0, &gart_fix_e820);
>>> }
>>> early_param("gart_fix_e820", parse_gart_mem);
>>>
>>> diff --git a/include/linux/tick.h b/include/linux/tick.h
>>> index 97fd4e543846..0ecdf0e248f4 100644
>>> --- a/include/linux/tick.h
>>> +++ b/include/linux/tick.h
>>> @@ -98,7 +98,7 @@ static inline void tick_broadcast_exit(void)
>>> }
>>>
>>> #ifdef CONFIG_NO_HZ_COMMON
>>> -extern int tick_nohz_enabled;
>>> +extern bool tick_nohz_enabled;
>>> extern int tick_nohz_tick_stopped(void);
>>> extern void tick_nohz_idle_enter(void);
>>> extern void tick_nohz_idle_exit(void);
>>> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
>>> index 435b8850dd80..456f066148d5 100644
>>> --- a/kernel/time/hrtimer.c
>>> +++ b/kernel/time/hrtimer.c
>>> @@ -515,7 +515,7 @@ static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
>>> /*
>>> * High resolution timer enabled ?
>>> */
>>> -static int hrtimer_hres_enabled __read_mostly = 1;
>>> +static bool hrtimer_hres_enabled __read_mostly = true;
>>> unsigned int hrtimer_resolution __read_mostly = LOW_RES_NSEC;
>>> EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>>
>>> @@ -524,13 +524,7 @@ EXPORT_SYMBOL_GPL(hrtimer_resolution);
>>> */
>>> static int __init setup_hrtimer_hres(char *str)
>>> {
>>> - if (!strcmp(str, "off"))
>>> - hrtimer_hres_enabled = 0;
>>> - else if (!strcmp(str, "on"))
>>> - hrtimer_hres_enabled = 1;
>>> - else
>>> - return 0;
>>> - return 1;
>>> + return kstrtobool(str, 0, &hrtimer_hres_enabled);
>>> }
>>>
>>> __setup("highres=", setup_hrtimer_hres);
>>> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
>>> index 9d7a053545f5..b57f822c2069 100644
>>> --- a/kernel/time/tick-sched.c
>>> +++ b/kernel/time/tick-sched.c
>>> @@ -387,20 +387,14 @@ void __init tick_nohz_init(void)
>>> /*
>>> * NO HZ enabled ?
>>> */
>>> -int tick_nohz_enabled __read_mostly = 1;
>>> +bool tick_nohz_enabled __read_mostly = true;
>>> unsigned long tick_nohz_active __read_mostly;
>>> /*
>>> * Enable / Disable tickless mode
>>> */
>>> static int __init setup_tick_nohz(char *str)
>>> {
>>> - if (!strcmp(str, "off"))
>>> - tick_nohz_enabled = 0;
>>> - else if (!strcmp(str, "on"))
>>> - tick_nohz_enabled = 1;
>>> - else
>>> - return 0;
>>> - return 1;
>>> + return kstrtobool(str, 0, &tick_nohz_enabled);
>>> }
>>>
>>> __setup("nohz=", setup_tick_nohz);
>>> --
>>> 2.6.3
>>>
>>
>>
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 18+ messages in thread