Re: [PATCH v2 2/4] lib: update single-char callers of strtobool

[Kees Cook]

On Fri, Feb 5, 2016 at 2:46 AM, David Laight  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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v2 2/4] lib: update single-char callers of strtobool

[David Laight]

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.

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 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.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/4] lib: update single-char callers of strtobool

[Kees Cook]

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 
Cc: Amitkumar Karwar 
Cc: Nishant Sarmukadam 
Cc: Kalle Valo 
Cc: Steve French 
Cc: linux-c...@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(, ubuf, sizeof(cmd)))
-   return -EFAULT;
-
-   if (strtobool(, ))
-   return -EINVAL;
+   rc = kstrtobool_from_user(ubuf, count, 0, );
+   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(, ) == 0) {
+   rc = kstrtobool_from_user(buffer, count, 0, );
+   if (rc == 0) {
 #ifdef CONFIG_CIFS_STATS2
atomic_set(, 0);
atomic_set(, 0);
@@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
}
}
spin_unlock(_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(, ) == 0)
+   if (strtobool(c, ) == 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, );
if (rc)
return rc;
 
-   rc = strtobool(, );
-   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, );
if (rc)
return rc;
 
-   rc = strtobool(, );
-   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, 

Re: [PATCH v2 2/4] lib: update single-char callers of strtobool

[Andy Shevchenko]

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook  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 
> Cc: Amitkumar Karwar 
> Cc: Nishant Sarmukadam 
> Cc: Kalle Valo 
> Cc: Steve French 
> Cc: linux-c...@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(, ubuf, sizeof(cmd)))
> -   return -EFAULT;
> -
> -   if (strtobool(, ))
> -   return -EINVAL;
> +   rc = kstrtobool_from_user(ubuf, count, 0, );
> +   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(, ) == 0) {
> +   rc = kstrtobool_from_user(buffer, count, 0, );
> +   if (rc == 0) {
>  #ifdef CONFIG_CIFS_STATS2
> atomic_set(, 0);
> atomic_set(, 0);
> @@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> }
> }
> spin_unlock(_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(, ) == 0)
> +   if (strtobool(c, ) == 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, );
> if (rc)
> return rc;
>
> -   rc = strtobool(, );
> -   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, );
> if (rc)
> return rc;
>
> -   rc = strtobool(, );
> -   if (rc)
> -   

Re: [PATCH v2 2/4] lib: update single-char callers of strtobool

[Kees Cook]

On Thu, Feb 4, 2016 at 2:59 PM, Andy Shevchenko
 wrote:
> On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook  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 
>> Cc: Amitkumar Karwar 
>> Cc: Nishant Sarmukadam 
>> Cc: Kalle Valo 
>> Cc: Steve French 
>> Cc: linux-c...@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(, ubuf, sizeof(cmd)))
>> -   return -EFAULT;
>> -
>> -   if (strtobool(, ))
>> -   return -EINVAL;
>> +   rc = kstrtobool_from_user(ubuf, count, 0, );
>> +   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(, ) == 0) {
>> +   rc = kstrtobool_from_user(buffer, count, 0, );
>> +   if (rc == 0) {
>>  #ifdef CONFIG_CIFS_STATS2
>> atomic_set(, 0);
>> atomic_set(, 0);
>> @@ -290,6 +286,8 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>> }
>> }
>> spin_unlock(_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(, ) == 0)
>> +   if (strtobool(c, ) == 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, );
>> if (rc)
>> return rc;
>>
>> -   rc = strtobool(, );
>> -   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;
>>