Re: [PATCH 1/3] lib: fix callers of strtobool to use char array
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(&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_
Re: [PATCH 1/3] lib: fix callers of strtobool to use char array
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(&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); > >
[PATCH 1/3] lib: fix callers of strtobool to use char array
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(&cmd, ubuf, sizeof(cmd))) + if (copy_from_user(cmd, ubuf, sizeof(char))) return -EFAULT; - if (strtobool(&cmd, &result)) + if (strtobool(cmd, &result)) 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, &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 - 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 __use