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

From: Kees Cook
Date: Thu Feb 04 2016 - 13:56:14 EST

Next message: Stefan Wahren: "[PATCH V2 2/3] DT: add binding for SN3218 LED driver"
Previous message: Andy Shevchenko: "Data corruption on serial interface under load"
In reply to: Andy Shevchenko: "Re: [PATCH 1/3] lib: fix callers of strtobool to use char array"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Feb 1, 2016 at 5:17 AM, Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Thu, Jan 28, 2016 at 4:17 PM, Kees Cook <keescook@xxxxxxxxxxxx> 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(&cmd, ubuf, sizeof(cmd)))
>> + if (copy_from_user(cmd, ubuf, sizeof(char)))
>> return -EFAULT;
>>
>> - if (strtobool(&cmd, &result))
>> + if (strtobool(cmd, &result))
>> 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, &bv);
>> + 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, &bv);
>> if (rc)
>> return rc;
>>
>> - if (strtobool(&c, &bv) == 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(&totBufAllocCount, 0);
>> - atomic_set(&totSmBufAllocCount, 0);
>> + atomic_set(&totBufAllocCount, 0);
>> + atomic_set(&totSmBufAllocCount, 0);
>> #endif /* CONFIG_CIFS_STATS2 */
>> - spin_lock(&cifs_tcp_ses_lock);
>> - list_for_each(tmp1, &cifs_tcp_ses_list) {
>> - server = list_entry(tmp1, struct TCP_Server_Info,
>> - tcp_ses_list);
>> - list_for_each(tmp2, &server->smb_ses_list) {
>> - ses = list_entry(tmp2, struct cifs_ses,
>> - smb_ses_list);
>> - list_for_each(tmp3, &ses->tcon_list) {
>> - tcon = list_entry(tmp3,
>> - struct cifs_tcon,
>> - tcon_list);
>> - atomic_set(&tcon->num_smbs_sent, 0);
>> - if (server->ops->clear_stats)
>> - server->ops->clear_stats(tcon);
>> - }
>> + spin_lock(&cifs_tcp_ses_lock);
>> + list_for_each(tmp1, &cifs_tcp_ses_list) {
>> + server = list_entry(tmp1, struct TCP_Server_Info,
>> + tcp_ses_list);
>> + list_for_each(tmp2, &server->smb_ses_list) {
>> + ses = list_entry(tmp2, struct cifs_ses,
>> + smb_ses_list);
>> + list_for_each(tmp3, &ses->tcon_list) {
>> + tcon = list_entry(tmp3,
>> + struct cifs_tcon,
>> + tcon_list);
>> + atomic_set(&tcon->num_smbs_sent, 0);
>> + if (server->ops->clear_stats)
>> + server->ops->clear_stats(tcon);
>> }
>> }
>> - spin_unlock(&cifs_tcp_ses_lock);
>> }
>> + spin_unlock(&cifs_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)
>> {
>> - 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 +487,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 = get_user_bool(buffer, &linuxExtEnabled);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - linuxExtEnabled = bv;
>> -
>> return count;
>> }
>>
>> @@ -511,20 +519,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 = get_user_bool(buffer, &lookupCacheEnabled);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - lookupCacheEnabled = bv;
>> -
>> return count;
>> }
>>
>> @@ -551,20 +551,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 = get_user_bool(buffer, &traceSMB);
>> if (rc)
>> return rc;
>>
>> - rc = strtobool(&c, &bv);
>> - if (rc)
>> - return rc;
>> -
>> - traceSMB = bv;
>> -
>> return count;
>> }
>>
>> @@ -622,7 +614,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 +627,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

Thanks for the review!

-Kees

--
Kees Cook
Chrome OS & Brillo Security

Next message: Stefan Wahren: "[PATCH V2 2/3] DT: add binding for SN3218 LED driver"
Previous message: Andy Shevchenko: "Data corruption on serial interface under load"
In reply to: Andy Shevchenko: "Re: [PATCH 1/3] lib: fix callers of strtobool to use char array"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]