Re: [PATCH 1/3] lib: fix callers of strtobool to use char array

[Kees Cook]

On Mon, Feb 1, 2016 at 5:17 AM, Andy Shevchenko
 wrote:
> On Thu, Jan 28, 2016 at 4:17 PM, Kees Cook  wrote:
>> Some callers of strtobool were passing a pointer to unterminated strings.
>> This fixes the issue and consolidates some logic in cifs.
>
> My comments below.
>
> First of all I don't think currently there is an issue in cifs, since
> strbool checks only first character of the input string, or are you
> talking about something else?

Right, no, this is a fix before extending strtobool to parse the
second character in the string (for handling "on" and "off").

>> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
>> b/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> index 0b9c580af988..76af60899c69 100644
>> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
>> @@ -880,13 +880,13 @@ mwifiex_reset_write(struct file *file,
>>  {
>> struct mwifiex_private *priv = file->private_data;
>> struct mwifiex_adapter *adapter = priv->adapter;
>> -   char cmd;
>> +   char cmd[2] = { '\0' };
>> bool result;
>>
>> -   if (copy_from_user(, ubuf, sizeof(cmd)))
>> +   if (copy_from_user(cmd, ubuf, sizeof(char)))
>> return -EFAULT;
>>
>> -   if (strtobool(, ))
>> +   if (strtobool(cmd, ))
>> return -EINVAL;
>
> Can we do strtobool_from_user() instead like kstrto*from_user() and
> similar helpers are done?

Yeah, that might clean this up a bit more. I will add it.

>> if (!result)
>> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
>> index 50b268483302..2f7ffcc9e364 100644
>> --- a/fs/cifs/cifs_debug.c
>> +++ b/fs/cifs/cifs_debug.c
>> @@ -251,11 +251,29 @@ static const struct file_operations 
>> cifs_debug_data_proc_fops = {
>> .release= single_release,
>>  };
>>
>> +static int get_user_bool(const char __user *buffer, bool *store)
>> +{
>> +   char c[2] = { '\0' };
>> +   bool bv;
>> +   int rc;
>> +
>> +   rc = get_user(c[0], buffer);
>> +   if (rc)
>> +   return rc;
>> +
>> +   rc = strtobool(c, );
>> +   if (rc)
>> +   return rc;
>> +
>> +   *store = bv;
>> +
>> +   return 0;
>> +}
>> +
>>  #ifdef CONFIG_CIFS_STATS
>>  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,34 +281,32 @@ static ssize_t cifs_stats_proc_write(struct file *file,
>> struct cifs_ses *ses;
>> struct cifs_tcon *tcon;
>>
>> -   rc = get_user(c, buffer);
>> +   rc = get_user_bool(buffer, );
>> if (rc)
>> return rc;
>>
>> -   if (strtobool(, ) == 0) {
>>  #ifdef CONFIG_CIFS_STATS2
>
> I would suggest to do a separate patch which just changes a pattern
> and thus indentation without changing anything in functionality.

Okay, noted.

>> -   atomic_set(, 0);
>> -   atomic_set(, 0);
>> +   atomic_set(, 0);
>> +   atomic_set(, 0);
>>  #endif /* CONFIG_CIFS_STATS2 */
>> -   spin_lock(_tcp_ses_lock);
>> -   list_for_each(tmp1, _tcp_ses_list) {
>> -   server = list_entry(tmp1, struct TCP_Server_Info,
>> -   tcp_ses_list);
>> -   list_for_each(tmp2, >smb_ses_list) {
>> -   ses = list_entry(tmp2, struct cifs_ses,
>> -smb_ses_list);
>> -   list_for_each(tmp3, >tcon_list) {
>> -   tcon = list_entry(tmp3,
>> - struct cifs_tcon,
>> - tcon_list);
>> -   atomic_set(>num_smbs_sent, 0);
>> -   if (server->ops->clear_stats)
>> -   
>> server->ops->clear_stats(tcon);
>> -   }
>> +   spin_lock(_tcp_ses_lock);
>> +   list_for_each(tmp1, _tcp_ses_list) {
>> +   server = list_entry(tmp1, struct TCP_Server_Info,
>> +   tcp_ses_list);
>> +   list_for_each(tmp2, >smb_ses_list) {
>> +   ses = list_entry(tmp2, struct cifs_ses,
>> +smb_ses_list);
>> +   list_for_each(tmp3, >tcon_list) {
>> +   tcon = list_entry(tmp3,
>> + struct cifs_tcon,
>> + tcon_list);
>> +   atomic_set(>num_smbs_sent, 0);
>> +  

Re: [PATCH 1/3] lib: fix callers of strtobool to use char array

[Andy Shevchenko]

On Thu, Jan 28, 2016 at 4:17 PM, Kees Cook  wrote:
> Some callers of strtobool were passing a pointer to unterminated strings.
> This fixes the issue and consolidates some logic in cifs.

My comments below.

First of all I don't think currently there is an issue in cifs, since
strbool checks only first character of the input string, or are you
talking about something else?

> diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
> b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> index 0b9c580af988..76af60899c69 100644
> --- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
> +++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
> @@ -880,13 +880,13 @@ mwifiex_reset_write(struct file *file,
>  {
> struct mwifiex_private *priv = file->private_data;
> struct mwifiex_adapter *adapter = priv->adapter;
> -   char cmd;
> +   char cmd[2] = { '\0' };
> bool result;
>
> -   if (copy_from_user(, ubuf, sizeof(cmd)))
> +   if (copy_from_user(cmd, ubuf, sizeof(char)))
> return -EFAULT;
>
> -   if (strtobool(, ))
> +   if (strtobool(cmd, ))
> return -EINVAL;

Can we do strtobool_from_user() instead like kstrto*from_user() and
similar helpers are done?

>
> if (!result)
> diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
> index 50b268483302..2f7ffcc9e364 100644
> --- a/fs/cifs/cifs_debug.c
> +++ b/fs/cifs/cifs_debug.c
> @@ -251,11 +251,29 @@ static const struct file_operations 
> cifs_debug_data_proc_fops = {
> .release= single_release,
>  };
>
> +static int get_user_bool(const char __user *buffer, bool *store)
> +{
> +   char c[2] = { '\0' };
> +   bool bv;
> +   int rc;
> +
> +   rc = get_user(c[0], buffer);
> +   if (rc)
> +   return rc;
> +
> +   rc = strtobool(c, );
> +   if (rc)
> +   return rc;
> +
> +   *store = bv;
> +
> +   return 0;
> +}
> +
>  #ifdef CONFIG_CIFS_STATS
>  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,34 +281,32 @@ static ssize_t cifs_stats_proc_write(struct file *file,
> struct cifs_ses *ses;
> struct cifs_tcon *tcon;
>
> -   rc = get_user(c, buffer);
> +   rc = get_user_bool(buffer, );
> if (rc)
> return rc;
>
> -   if (strtobool(, ) == 0) {
>  #ifdef CONFIG_CIFS_STATS2

I would suggest to do a separate patch which just changes a pattern
and thus indentation without changing anything in functionality.

> -   atomic_set(, 0);
> -   atomic_set(, 0);
> +   atomic_set(, 0);
> +   atomic_set(, 0);
>  #endif /* CONFIG_CIFS_STATS2 */
> -   spin_lock(_tcp_ses_lock);
> -   list_for_each(tmp1, _tcp_ses_list) {
> -   server = list_entry(tmp1, struct TCP_Server_Info,
> -   tcp_ses_list);
> -   list_for_each(tmp2, >smb_ses_list) {
> -   ses = list_entry(tmp2, struct cifs_ses,
> -smb_ses_list);
> -   list_for_each(tmp3, >tcon_list) {
> -   tcon = list_entry(tmp3,
> - struct cifs_tcon,
> - tcon_list);
> -   atomic_set(>num_smbs_sent, 0);
> -   if (server->ops->clear_stats)
> -   
> server->ops->clear_stats(tcon);
> -   }
> +   spin_lock(_tcp_ses_lock);
> +   list_for_each(tmp1, _tcp_ses_list) {
> +   server = list_entry(tmp1, struct TCP_Server_Info,
> +   tcp_ses_list);
> +   list_for_each(tmp2, >smb_ses_list) {
> +   ses = list_entry(tmp2, struct cifs_ses,
> +smb_ses_list);
> +   list_for_each(tmp3, >tcon_list) {
> +   tcon = list_entry(tmp3,
> + struct cifs_tcon,
> + tcon_list);
> +   atomic_set(>num_smbs_sent, 0);
> +   if (server->ops->clear_stats)
> +   server->ops->clear_stats(tcon);
> }
> }
> -   spin_unlock(_tcp_ses_lock);
> }
> +   spin_unlock(_tcp_ses_lock);
>
> return count;
>  }
> @@ -433,17 +449,17 @@ static int cifsFYI_proc_open(struct inode *inode, 
> struct file *file)
>  static ssize_t 

[PATCH 1/3] lib: fix callers of strtobool to use char array

[Kees Cook]

Some callers of strtobool were passing a pointer to unterminated strings.
This fixes the issue and consolidates some logic in cifs.

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 |   6 +-
 fs/cifs/cifs_debug.c   | 106 -
 fs/cifs/cifs_debug.h   |   2 +-
 fs/cifs/cifsfs.c   |   6 +-
 fs/cifs/cifsglob.h |   4 +-
 5 files changed, 58 insertions(+), 66 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/debugfs.c 
b/drivers/net/wireless/marvell/mwifiex/debugfs.c
index 0b9c580af988..76af60899c69 100644
--- a/drivers/net/wireless/marvell/mwifiex/debugfs.c
+++ b/drivers/net/wireless/marvell/mwifiex/debugfs.c
@@ -880,13 +880,13 @@ mwifiex_reset_write(struct file *file,
 {
struct mwifiex_private *priv = file->private_data;
struct mwifiex_adapter *adapter = priv->adapter;
-   char cmd;
+   char cmd[2] = { '\0' };
bool result;
 
-   if (copy_from_user(, ubuf, sizeof(cmd)))
+   if (copy_from_user(cmd, ubuf, sizeof(char)))
return -EFAULT;
 
-   if (strtobool(, ))
+   if (strtobool(cmd, ))
return -EINVAL;
 
if (!result)
diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 50b268483302..2f7ffcc9e364 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -251,11 +251,29 @@ static const struct file_operations 
cifs_debug_data_proc_fops = {
.release= single_release,
 };
 
+static int get_user_bool(const char __user *buffer, bool *store)
+{
+   char c[2] = { '\0' };
+   bool bv;
+   int rc;
+
+   rc = get_user(c[0], buffer);
+   if (rc)
+   return rc;
+
+   rc = strtobool(c, );
+   if (rc)
+   return rc;
+
+   *store = bv;
+
+   return 0;
+}
+
 #ifdef CONFIG_CIFS_STATS
 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,34 +281,32 @@ static ssize_t cifs_stats_proc_write(struct file *file,
struct cifs_ses *ses;
struct cifs_tcon *tcon;
 
-   rc = get_user(c, buffer);
+   rc = get_user_bool(buffer, );
if (rc)
return rc;
 
-   if (strtobool(, ) == 0) {
 #ifdef CONFIG_CIFS_STATS2
-   atomic_set(, 0);
-   atomic_set(, 0);
+   atomic_set(, 0);
+   atomic_set(, 0);
 #endif /* CONFIG_CIFS_STATS2 */
-   spin_lock(_tcp_ses_lock);
-   list_for_each(tmp1, _tcp_ses_list) {
-   server = list_entry(tmp1, struct TCP_Server_Info,
-   tcp_ses_list);
-   list_for_each(tmp2, >smb_ses_list) {
-   ses = list_entry(tmp2, struct cifs_ses,
-smb_ses_list);
-   list_for_each(tmp3, >tcon_list) {
-   tcon = list_entry(tmp3,
- struct cifs_tcon,
- tcon_list);
-   atomic_set(>num_smbs_sent, 0);
-   if (server->ops->clear_stats)
-   server->ops->clear_stats(tcon);
-   }
+   spin_lock(_tcp_ses_lock);
+   list_for_each(tmp1, _tcp_ses_list) {
+   server = list_entry(tmp1, struct TCP_Server_Info,
+   tcp_ses_list);
+   list_for_each(tmp2, >smb_ses_list) {
+   ses = list_entry(tmp2, struct cifs_ses,
+smb_ses_list);
+   list_for_each(tmp3, >tcon_list) {
+   tcon = list_entry(tmp3,
+ struct cifs_tcon,
+ tcon_list);
+   atomic_set(>num_smbs_sent, 0);
+   if (server->ops->clear_stats)
+   server->ops->clear_stats(tcon);
}
}
-   spin_unlock(_tcp_ses_lock);
}
+   spin_unlock(_tcp_ses_lock);
 
return count;
 }
@@ -433,17 +449,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)
 {
-