2013-11-29 17:07:59

by Michal Nazarewicz

Subject: [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference

From: Michal Nazarewicz <[email protected]>

If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
a NULL pointer. There might be a better error recovery then just
printing an error, but printing an error message is better then the
current behaviour.

Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index de9eb2c..10d6ae9 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2041,13 +2041,23 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (msg_ind) {
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ }
mutex_unlock(&wcn->hal_ind_mutex);
+ if (!msg_ind) {
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error when allocation fails.
+ */
+ wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ }
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4.1

2013-12-07 17:14:03

by Michal Nazarewicz

Subject: [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section

This also simplifies flow-controll as there is now only one if
condition with a single branch.
---
drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
1 file changed, 15 insertions(+), 13 deletions(-)

On Thu, Dec 05 2013, Eugene Krasnikov <[email protected]> wrote:
> I think code will look neater if error case will be handled in "else"
> statement something like: [...]

How about the following:

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..ebab2db 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind) {
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ mutex_lock(&wcn->hal_ind_mutex);
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4

2013-12-05 08:41:30

by Eugene Krasnikov

Subject: Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference

> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",

I think code will look neater if error case will be handled in "else"
statement something like:
if (msg_ind) {
...
} else {
/*print message*/
}
/*unlock and break;*/

How about that?


On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <[email protected]> wrote:
>
> If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> a NULL pointer. There might be a better error recovery then just
> printing an error, but printing an error message is better then the
> current behaviour.
>
> Signed-off-by: Michal Nazarewicz <[email protected]>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> On Sun, Dec 01 2013, Peter Stuge wrote:
>> Michal Nazarewicz wrote:
>>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
>>> + msg_header->msg_type);
>>
>> Typo hnadling.
>
> Fixed.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index de9eb2c..3663394 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (msg_ind) {
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + }
> mutex_unlock(&wcn->hal_ind_mutex);
> + if (msg_ind)
> + break;
> + /* FIXME: Do something smarter then just printing an error. */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4.1
>
> --
> Best regards, _ _
> .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--



--
Best regards,
Eugene

2013-12-02 13:09:38

by Michal Nazarewicz

Subject: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference

If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
a NULL pointer. There might be a better error recovery then just
printing an error, but printing an error message is better then the
current behaviour.

Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)

On Sun, Dec 01 2013, Peter Stuge wrote:
> Michal Nazarewicz wrote:
>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
>> + msg_header->msg_type);
>
> Typo hnadling.

Fixed.

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index de9eb2c..3663394 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (msg_ind) {
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ memcpy(msg_ind->msg, buf, len);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ }
mutex_unlock(&wcn->hal_ind_mutex);
+ if (msg_ind)
+ break;
+ /* FIXME: Do something smarter then just printing an error. */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4.1

--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--

2013-12-09 07:27:18

by Eugene Krasnikov

Subject: Re: [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section

hal_ind_mutex suppose to protect msg_ind but with this patch
allocation will be done outside the critical section.

On Sat, Dec 7, 2013 at 5:13 PM, Michal Nazarewicz <[email protected]> wrote:
>
> This also simplifies flow-controll as there is now only one if
> condition with a single branch.
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> On Thu, Dec 05 2013, Eugene Krasnikov <[email protected]> wrote:
>> I think code will look neater if error case will be handled in "else"
>> statement something like: [...]
>
> How about the following:
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..ebab2db 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,24 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind) {
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + mutex_lock(&wcn->hal_ind_mutex);
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + memcpy(msg_ind->msg, buf, len);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>



--
Best regards,
Eugene

2013-12-11 16:43:43

by Michal Nazarewicz

Subject: [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of critical section

Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.

This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.

Signed-off-by: Michal Nazarewicz <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)

On Wed, Dec 11 2013, John W. Linville wrote:
> Missing Signed-off-by...

Sorry about that.

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 8f37562..750626b 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2060,22 +2060,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4

2013-12-06 16:15:16

by John W. Linville

Subject: Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference

FWIW, I merged the earlier version. Please send any changes as a
follow-up patch, probably against wireless-next.

John

On Thu, Dec 05, 2013 at 08:41:29AM +0000, Eugene Krasnikov wrote:
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
>
> I think code will look neater if error case will be handled in "else"
> statement something like:
> if (msg_ind) {
> ...
> } else {
> /*print message*/
> }
> /*unlock and break;*/
>
> How about that?
>
>
> On Mon, Dec 2, 2013 at 1:09 PM, Michal Nazarewicz <[email protected]> wrote:
> >
> > If kmalloc fails wcn36xx_smd_rsp_process will attempt to dereference
> > a NULL pointer. There might be a better error recovery then just
> > printing an error, but printing an error message is better then the
> > current behaviour.
> >
> > Signed-off-by: Michal Nazarewicz <[email protected]>
> > ---
> > drivers/net/wireless/ath/wcn36xx/smd.c | 19 +++++++++++++------
> > 1 file changed, 13 insertions(+), 6 deletions(-)
> >
> > On Sun, Dec 01 2013, Peter Stuge wrote:
> >> Michal Nazarewicz wrote:
> >>> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
> >>> + msg_header->msg_type);
> >>
> >> Typo hnadling.
> >
> > Fixed.
> >
> > diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> > index de9eb2c..3663394 100644
> > --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> > +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> > @@ -2041,13 +2041,20 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> > case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> > mutex_lock(&wcn->hal_ind_mutex);
> > msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> > - msg_ind->msg_len = len;
> > - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > - memcpy(msg_ind->msg, buf, len);
> > - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + if (msg_ind) {
> > + msg_ind->msg_len = len;
> > + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> > + memcpy(msg_ind->msg, buf, len);
> > + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> > + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> > + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> > + }
> > mutex_unlock(&wcn->hal_ind_mutex);
> > + if (msg_ind)
> > + break;
> > + /* FIXME: Do something smarter then just printing an error. */
> > + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> > + msg_header->msg_type);
> > break;
> > default:
> > wcn36xx_err("SMD_EVENT (%d) not supported\n",
> > --
> > 1.8.4.1
> >
> > --
> > Best regards, _ _
> > .o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
> > ..o | Computer Science, Michał “mina86” Nazarewicz (o o)
> > ooo +--<[email protected]>--<xmpp:[email protected]>--ooO--(_)--Ooo--
>
>
>
> --
> Best regards,
> Eugene
>

--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2013-12-11 16:00:18

by John W. Linville

Subject: Re: [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section

Missing Signed-off-by...

On Mon, Dec 09, 2013 at 12:34:04PM +0100, Michal Nazarewicz wrote:
> Commit [3469adb3: fix potential NULL pointer dereference] introduced
> a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
> Moreover, it introduced two if statements, which looked a bit clunky.
>
> This commit moves allocation code outside of the critical section so
> there's no need to dance around mutex_unlock, and adds the missing
> allocation check.
>
> ---
> drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
> 1 file changed, 19 insertions(+), 13 deletions(-)
>
> On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> > hal_ind_mutex suppose to protect msg_ind but with this patch
> > allocation will be done outside the critical section.
>
> msg_ind is a local variable which is not shared outside of the function
> until it's added to a list. I don't see how it matters if it's
> allocation and initialisation is done under mutex.
>
> In fact, there's another kmalloc call that should be checked and can be
> done outside of the critical section, so disregard my previous patch,
> here's a new one.
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index 823631c..713e3ce 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
> case WCN36XX_HAL_OTA_TX_COMPL_IND:
> case WCN36XX_HAL_MISSED_BEACON_IND:
> case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
> - mutex_lock(&wcn->hal_ind_mutex);
> msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
> - if (msg_ind) {
> - msg_ind->msg_len = len;
> - msg_ind->msg = kmalloc(len, GFP_KERNEL);
> - memcpy(msg_ind->msg, buf, len);
> - list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> - queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> - wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> + if (!msg_ind)
> + goto nomem;
> + msg_ind->msg_len = len;
> + msg_ind->msg = kmalloc(len, GFP_KERNEL);
> + if (!msg_ind->msg) {
> + kfree(msg_ind);
> +nomem:
> + /*
> + * FIXME: Do something smarter then just
> + * printing an error.
> + */
> + wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> + msg_header->msg_type);
> + break;
> }
> + memcpy(msg_ind->msg, buf, len);
> + mutex_lock(&wcn->hal_ind_mutex);
> + list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
> + queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
> mutex_unlock(&wcn->hal_ind_mutex);
> - if (msg_ind)
> - break;
> - /* FIXME: Do something smarter then just printing an error. */
> - wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
> - msg_header->msg_type);
> + wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
> break;
> default:
> wcn36xx_err("SMD_EVENT (%d) not supported\n",
> --
> 1.8.4
>



--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.

2013-12-09 11:34:13

by Michal Nazarewicz

Subject: [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section

Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.

This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.

---
drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)

On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> hal_ind_mutex suppose to protect msg_ind but with this patch
> allocation will be done outside the critical section.

msg_ind is a local variable which is not shared outside of the function
until it's added to a list. I don't see how it matters if it's
allocation and initialisation is done under mutex.

In fact, there's another kmalloc call that should be checked and can be
done outside of the critical section, so disregard my previous patch,
here's a new one.

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..713e3ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
case WCN36XX_HAL_OTA_TX_COMPL_IND:
case WCN36XX_HAL_MISSED_BEACON_IND:
case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
- mutex_lock(&wcn->hal_ind_mutex);
msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
- if (msg_ind) {
- msg_ind->msg_len = len;
- msg_ind->msg = kmalloc(len, GFP_KERNEL);
- memcpy(msg_ind->msg, buf, len);
- list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
- queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
- wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+ if (!msg_ind)
+ goto nomem;
+ msg_ind->msg_len = len;
+ msg_ind->msg = kmalloc(len, GFP_KERNEL);
+ if (!msg_ind->msg) {
+ kfree(msg_ind);
+nomem:
+ /*
+ * FIXME: Do something smarter then just
+ * printing an error.
+ */
+ wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+ msg_header->msg_type);
+ break;
}
+ memcpy(msg_ind->msg, buf, len);
+ mutex_lock(&wcn->hal_ind_mutex);
+ list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+ queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
mutex_unlock(&wcn->hal_ind_mutex);
- if (msg_ind)
- break;
- /* FIXME: Do something smarter then just printing an error. */
- wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
- msg_header->msg_type);
+ wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
break;
default:
wcn36xx_err("SMD_EVENT (%d) not supported\n",
--
1.8.4

Attachments:

2013-12-01 00:53:58

by Peter Stuge

Subject: Re: [ath9k-devel] [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference

Michal Nazarewicz wrote:
> + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n",
> + msg_header->msg_type);

Typo hnadling.


//Peter