If on iwl_dump_nic_event_log() error occurs before that function
initialize buf, we process uninitiated pointer in
iwl_dbgfs_log_event_read() and can hit "BUG at mm/slub.c:3409"
Resolves:
https://bugzilla.redhat.com/show_bug.cgi?id=951241
Reported-by: [email protected]
Cc: [email protected]
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
Patch is only compile tested, but I'm sure it fixes the problem.
drivers/net/wireless/iwlwifi/dvm/debugfs.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
index 7b8178b..cb6dd58 100644
--- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
+++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
@@ -2237,15 +2237,15 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
size_t count, loff_t *ppos)
{
struct iwl_priv *priv = file->private_data;
- char *buf;
- int pos = 0;
- ssize_t ret = -ENOMEM;
+ char *buf = NULL;
+ ssize_t ret;
- ret = pos = iwl_dump_nic_event_log(priv, true, &buf, true);
- if (buf) {
- ret = simple_read_from_buffer(user_buf, count, ppos, buf, pos);
- kfree(buf);
- }
+ ret = iwl_dump_nic_event_log(priv, true, &buf, true);
+ if (ret < 0)
+ goto err;
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+err:
+ kfree(buf);
return ret;
}
--
1.7.11.7
On Wed, 2013-04-17 at 17:10 +1000, Julian Calaby wrote:
> Hi Stanislaw,
>
> On Wed, Apr 17, 2013 at 4:23 PM, Stanislaw Gruszka <[email protected]> wrote:
> > Make code simpler a bit.
> >
> > Reported-by: Jonas Gorski <[email protected]>
> > Signed-off-by: Stanislaw Gruszka <[email protected]>
> > ---
> > drivers/net/wireless/iwlwifi/dvm/debugfs.c | 6 ++----
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> > index 17f04de..d532948 100644
> > --- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> > +++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> > @@ -2241,10 +2241,8 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
> > ssize_t ret;
> >
> > ret = iwl_dump_nic_event_log(priv, true, &buf);
> > - if (ret < 0)
> > - goto err;
> > - ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> > -err:
> > + if (ret > 0)
> > + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>
> That's not the same, what happens if ret == 0?
It's the same: nothing will be read regardless of whether you call the
function or not.
johannes
On 16 April 2013 15:38, Stanislaw Gruszka <[email protected]> wrote:
> If on iwl_dump_nic_event_log() error occurs before that function
> initialize buf, we process uninitiated pointer in
> iwl_dbgfs_log_event_read() and can hit "BUG at mm/slub.c:3409"
>
> Resolves:
> https://bugzilla.redhat.com/show_bug.cgi?id=951241
>
> Reported-by: [email protected]
> Cc: [email protected]
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> Patch is only compile tested, but I'm sure it fixes the problem.
>
> drivers/net/wireless/iwlwifi/dvm/debugfs.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> index 7b8178b..cb6dd58 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> @@ -2237,15 +2237,15 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
> size_t count, loff_t *ppos)
> {
> struct iwl_priv *priv = file->private_data;
> - char *buf;
> - int pos = 0;
> - ssize_t ret = -ENOMEM;
> + char *buf = NULL;
> + ssize_t ret;
>
> - ret = pos = iwl_dump_nic_event_log(priv, true, &buf, true);
> - if (buf) {
> - ret = simple_read_from_buffer(user_buf, count, ppos, buf, pos);
> - kfree(buf);
> - }
> + ret = iwl_dump_nic_event_log(priv, true, &buf, true);
> + if (ret < 0)
> + goto err;
> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> +err:
Not every error check needs a goto, you can avoid it by inverting the
condition: ;-)
ret = iwl_dump_nic_event_log(priv, true, &buf, true);
if (ret >= 0) /* or maybe even > 0, because AFAICT 0 => nothing to read */
ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
kfree(buf);
Jonas
We can check buf against NULL instead of having additional bool
variable.
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/iwlwifi/dvm/agn.h | 2 +-
drivers/net/wireless/iwlwifi/dvm/debugfs.c | 4 ++--
drivers/net/wireless/iwlwifi/dvm/main.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/dvm/agn.h b/drivers/net/wireless/iwlwifi/dvm/agn.h
index e575b9b..48545ab 100644
--- a/drivers/net/wireless/iwlwifi/dvm/agn.h
+++ b/drivers/net/wireless/iwlwifi/dvm/agn.h
@@ -172,7 +172,7 @@ int iwl_calib_set(struct iwl_priv *priv,
const struct iwl_calib_hdr *cmd, int len);
void iwl_calib_free_results(struct iwl_priv *priv);
int iwl_dump_nic_event_log(struct iwl_priv *priv, bool full_log,
- char **buf, bool display);
+ char **buf);
int iwlagn_hw_valid_rtc_data_addr(u32 addr);
/* lib */
diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
index cb6dd58..17f04de 100644
--- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
+++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
@@ -2240,7 +2240,7 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
char *buf = NULL;
ssize_t ret;
- ret = iwl_dump_nic_event_log(priv, true, &buf, true);
+ ret = iwl_dump_nic_event_log(priv, true, &buf);
if (ret < 0)
goto err;
ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
@@ -2269,7 +2269,7 @@ static ssize_t iwl_dbgfs_log_event_write(struct file *file,
if (sscanf(buf, "%d", &event_log_flag) != 1)
return -EFAULT;
if (event_log_flag == 1)
- iwl_dump_nic_event_log(priv, true, NULL, false);
+ iwl_dump_nic_event_log(priv, true, NULL);
return count;
}
diff --git a/drivers/net/wireless/iwlwifi/dvm/main.c b/drivers/net/wireless/iwlwifi/dvm/main.c
index b9e3517..74d7572 100644
--- a/drivers/net/wireless/iwlwifi/dvm/main.c
+++ b/drivers/net/wireless/iwlwifi/dvm/main.c
@@ -1795,7 +1795,7 @@ static int iwl_print_last_event_logs(struct iwl_priv *priv, u32 capacity,
#define DEFAULT_DUMP_EVENT_LOG_ENTRIES (20)
int iwl_dump_nic_event_log(struct iwl_priv *priv, bool full_log,
- char **buf, bool display)
+ char **buf)
{
u32 base; /* SRAM byte address of event log header */
u32 capacity; /* event log capacity in # entries */
@@ -1866,7 +1866,7 @@ int iwl_dump_nic_event_log(struct iwl_priv *priv, bool full_log,
size);
#ifdef CONFIG_IWLWIFI_DEBUG
- if (display) {
+ if (buf) {
if (full_log)
bufsz = capacity * 48;
else
@@ -1962,7 +1962,7 @@ static void iwl_nic_error(struct iwl_op_mode *op_mode)
priv->fw->fw_version);
iwl_dump_nic_error_log(priv);
- iwl_dump_nic_event_log(priv, false, NULL, false);
+ iwl_dump_nic_event_log(priv, false, NULL);
iwlagn_fw_error(priv, false);
}
--
1.7.11.7
On Tue, Apr 16, 2013 at 08:40:30PM +0200, Jonas Gorski wrote:
> > + ret = iwl_dump_nic_event_log(priv, true, &buf, true);
> > + if (ret < 0)
> > + goto err;
> > + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> > +err:
>
> Not every error check needs a goto, you can avoid it by inverting the
> condition: ;-)
>
> ret = iwl_dump_nic_event_log(priv, true, &buf, true);
> if (ret >= 0) /* or maybe even > 0, because AFAICT 0 => nothing to read */
> ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> kfree(buf);
Make sense. Since this is only code cleanup issue and
simple_read_from_buffer(..., 0) is ok, I'll post incremental
patch which fix this.
Thanks
Stanislaw
Make code simpler a bit.
Reported-by: Jonas Gorski <[email protected]>
Signed-off-by: Stanislaw Gruszka <[email protected]>
---
drivers/net/wireless/iwlwifi/dvm/debugfs.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
index 17f04de..d532948 100644
--- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
+++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
@@ -2241,10 +2241,8 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
ssize_t ret;
ret = iwl_dump_nic_event_log(priv, true, &buf);
- if (ret < 0)
- goto err;
- ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
-err:
+ if (ret > 0)
+ ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
kfree(buf);
return ret;
}
--
1.7.11.7
Hi Stanislaw,
On Wed, Apr 17, 2013 at 4:23 PM, Stanislaw Gruszka <[email protected]> wrote:
> Make code simpler a bit.
>
> Reported-by: Jonas Gorski <[email protected]>
> Signed-off-by: Stanislaw Gruszka <[email protected]>
> ---
> drivers/net/wireless/iwlwifi/dvm/debugfs.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/iwlwifi/dvm/debugfs.c b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> index 17f04de..d532948 100644
> --- a/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> +++ b/drivers/net/wireless/iwlwifi/dvm/debugfs.c
> @@ -2241,10 +2241,8 @@ static ssize_t iwl_dbgfs_log_event_read(struct file *file,
> ssize_t ret;
>
> ret = iwl_dump_nic_event_log(priv, true, &buf);
> - if (ret < 0)
> - goto err;
> - ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
> -err:
> + if (ret > 0)
> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
That's not the same, what happens if ret == 0?
Thanks,
--
Julian Calaby
Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/