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

From: Andy Shevchenko
Date: Thu Feb 04 2016 - 17:59:40 EST

Next message: Andrew Morton: "Re: [PATCH] epoll: restrict EPOLLEXCLUSIVE to POLLIN and POLLOUT"
Previous message: Joshua Henderson: "Re: [PATCH 2/2] watchdog: pic32-wdt: Add PIC32 watchdog driver"
In reply to: Kees Cook: "[PATCH v2 2/4] lib: update single-char callers of strtobool"
Next in thread: Kees Cook: "Re: [PATCH v2 2/4] lib: update single-char callers of strtobool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Feb 4, 2016 at 11:00 PM, Kees Cook <keescook@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> Cc: Amitkumar Karwar <akarwar@xxxxxxxxxxx>
> Cc: Nishant Sarmukadam <nishants@xxxxxxxxxxx>
> Cc: Kalle Valo <kvalo@xxxxxxxxxxxxxx>
> Cc: Steve French <sfrench@xxxxxxxxx>
> Cc: linux-cifs@xxxxxxxxxxxxxxx
> ---
> 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

Next message: Andrew Morton: "Re: [PATCH] epoll: restrict EPOLLEXCLUSIVE to POLLIN and POLLOUT"
Previous message: Joshua Henderson: "Re: [PATCH 2/2] watchdog: pic32-wdt: Add PIC32 watchdog driver"
In reply to: Kees Cook: "[PATCH v2 2/4] lib: update single-char callers of strtobool"
Next in thread: Kees Cook: "Re: [PATCH v2 2/4] lib: update single-char callers of strtobool"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]