[PATCH 1/2] iwlwifi: fix freeing uninitialized pointer

[PATCH 1/2] iwlwifi: fix freeing uninitialized pointer

[Stanislaw Gruszka]

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: ian.odette@eprize.com
Cc: stable@vger.kernel.org
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
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

[PATCH 2/2] iwlwifi: remove redundant argument from iwl_dump_nic_event_log

[Stanislaw Gruszka]

We can check buf against NULL instead of having additional bool
variable.

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 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

Re: [PATCH 1/2] iwlwifi: fix freeing uninitialized pointer

[Jonas Gorski]

On 16 April 2013 15:38, Stanislaw Gruszka <sgruszka@redhat.com> 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: ian.odette@eprize.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> 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

Re: [PATCH 1/2] iwlwifi: fix freeing uninitialized pointer

[Stanislaw Gruszka]

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

[PATCH 3/2] iwlwifi: remove unneeded goto from iwl_dbgfs_log_event_read

[Stanislaw Gruszka]

Make code simpler a bit.

Reported-by: Jonas Gorski <jogo@openwrt.org>
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
 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

Re: [PATCH 3/2] iwlwifi: remove unneeded goto from iwl_dbgfs_log_event_read

[Julian Calaby]

Hi Stanislaw,

On Wed, Apr 17, 2013 at 4:23 PM, Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> Make code simpler a bit.
>
> Reported-by: Jonas Gorski <jogo@openwrt.org>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
>  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: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [PATCH 3/2] iwlwifi: remove unneeded goto from iwl_dbgfs_log_event_read

[Johannes Berg]

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 <sgruszka@redhat.com> wrote:
> > Make code simpler a bit.
> >
> > Reported-by: Jonas Gorski <jogo@openwrt.org>
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> >  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