| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdDfOx-foW3ju_51R0qcHzrB9Oj2G3kpiW80qBx+AwQ1g@mail.gmail.com>
Date: Mon, 1 Feb 2016 15:17:44 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Amitkumar Karwar <akarwar@...vell.com>,
Nishant Sarmukadam <nishants@...vell.com>,
Kalle Valo <kvalo@...eaurora.org>,
Steve French <sfrench@...ba.org>, linux-cifs@...r.kernel.org,
Joe Perches <joe@...ches.com>,
Rasmus Villemoes <linux@...musvillemoes.dk>,
Daniel Borkmann <daniel@...earbox.net>,
Michael Ellerman <mpe@...erman.id.au>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Martin Schwidefsky <schwidefsky@...ibm.com>,
"x86@...nel.org" <x86@...nel.org>, linuxppc-dev@...ts.ozlabs.org,
linux-s390@...r.kernel.org,
"open list:TI WILINK WIRELES..." <linux-wireless@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] lib: fix callers of strtobool to use char array
On Thu, Jan 28, 2016 at 4:17 PM, Kees Cook <keescook@...omium.org> 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(&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?
>
> 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.
> - 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
Powered by blists - more mailing lists