* [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference @ 2013-11-29 17:07 Michal Nazarewicz 2013-12-01 0:47 ` [ath9k-devel] " Peter Stuge 0 siblings, 1 reply; 10+ messages in thread From: Michal Nazarewicz @ 2013-11-29 17:07 UTC (permalink / raw) To: John W. Linville, Eugene Krasnikov Cc: linux-wireless, ath9k-devel, linux-kernel From: Michal Nazarewicz <mina86@mina86.com> 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 <mina86@mina86.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [ath9k-devel] [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference 2013-11-29 17:07 [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference Michal Nazarewicz @ 2013-12-01 0:47 ` Peter Stuge 2013-12-02 13:09 ` [PATCHv2] " Michal Nazarewicz 0 siblings, 1 reply; 10+ messages in thread From: Peter Stuge @ 2013-12-01 0:47 UTC (permalink / raw) To: Michal Nazarewicz Cc: John W. Linville, Eugene Krasnikov, ath9k-devel, linux-wireless, linux-kernel Michal Nazarewicz wrote: > + wcn36xx_err("Run out of memory while hnadling SMD_EVENT (%d)\n", > + msg_header->msg_type); Typo hnadling. //Peter ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference 2013-12-01 0:47 ` [ath9k-devel] " Peter Stuge @ 2013-12-02 13:09 ` Michal Nazarewicz 2013-12-05 8:41 ` Eugene Krasnikov 0 siblings, 1 reply; 10+ messages in thread From: Michal Nazarewicz @ 2013-12-02 13:09 UTC (permalink / raw) To: Peter Stuge Cc: John W. Linville, Eugene Krasnikov, ath9k-devel, linux-wireless, linux-kernel 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 <mina86@mina86.com> --- 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 +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo-- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference 2013-12-02 13:09 ` [PATCHv2] " Michal Nazarewicz @ 2013-12-05 8:41 ` Eugene Krasnikov 2013-12-06 16:12 ` John W. Linville 2013-12-07 17:13 ` [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section Michal Nazarewicz 0 siblings, 2 replies; 10+ messages in thread From: Eugene Krasnikov @ 2013-12-05 8:41 UTC (permalink / raw) To: Michal Nazarewicz Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org > + 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 <mina86@mina86.com> 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 <mina86@mina86.com> > --- > 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 +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo-- -- Best regards, Eugene ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCHv2] net: wireless: wcn36xx: fix potential NULL pointer dereference 2013-12-05 8:41 ` Eugene Krasnikov @ 2013-12-06 16:12 ` John W. Linville 2013-12-07 17:13 ` [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section Michal Nazarewicz 1 sibling, 0 replies; 10+ messages in thread From: John W. Linville @ 2013-12-06 16:12 UTC (permalink / raw) To: Eugene Krasnikov Cc: Michal Nazarewicz, Peter Stuge, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org 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 <mina86@mina86.com> 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 <mina86@mina86.com> > > --- > > 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 +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo-- > > > > -- > Best regards, > Eugene > -- John W. Linville Someday the world will need a hero, and you linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section 2013-12-05 8:41 ` Eugene Krasnikov 2013-12-06 16:12 ` John W. Linville @ 2013-12-07 17:13 ` Michal Nazarewicz 2013-12-09 7:27 ` Eugene Krasnikov 1 sibling, 1 reply; 10+ messages in thread From: Michal Nazarewicz @ 2013-12-07 17:13 UTC (permalink / raw) To: Eugene Krasnikov Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org 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 <k.eugene.e@gmail.com> 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section 2013-12-07 17:13 ` [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section Michal Nazarewicz @ 2013-12-09 7:27 ` Eugene Krasnikov 2013-12-09 11:34 ` [PATCH] net: wirelesse: wcn36xx: check allocation and trim " Michal Nazarewicz 0 siblings, 1 reply; 10+ messages in thread From: Eugene Krasnikov @ 2013-12-09 7:27 UTC (permalink / raw) To: Michal Nazarewicz Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org 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 <mina86@mina86.com> 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 <k.eugene.e@gmail.com> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section 2013-12-09 7:27 ` Eugene Krasnikov @ 2013-12-09 11:34 ` Michal Nazarewicz 2013-12-11 15:52 ` John W. Linville 0 siblings, 1 reply; 10+ messages in thread From: Michal Nazarewicz @ 2013-12-09 11:34 UTC (permalink / raw) To: Eugene Krasnikov Cc: Peter Stuge, John W. Linville, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2809 bytes --] 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 835 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section 2013-12-09 11:34 ` [PATCH] net: wirelesse: wcn36xx: check allocation and trim " Michal Nazarewicz @ 2013-12-11 15:52 ` John W. Linville 2013-12-11 16:43 ` [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of " Michal Nazarewicz 0 siblings, 1 reply; 10+ messages in thread From: John W. Linville @ 2013-12-11 15:52 UTC (permalink / raw) To: Michal Nazarewicz Cc: Eugene Krasnikov, Peter Stuge, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org 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 linville@tuxdriver.com might be all we have. Be ready. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of critical section 2013-12-11 15:52 ` John W. Linville @ 2013-12-11 16:43 ` Michal Nazarewicz 0 siblings, 0 replies; 10+ messages in thread From: Michal Nazarewicz @ 2013-12-11 16:43 UTC (permalink / raw) To: John W. Linville Cc: Eugene Krasnikov, Peter Stuge, ath9k-devel, linux-wireless, linux-kernel@vger.kernel.org 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 <mina86@mina86.com> --- 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-12-11 16:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-29 17:07 [PATCH] net: wireless: wcn36xx: fix potential NULL pointer dereference Michal Nazarewicz 2013-12-01 0:47 ` [ath9k-devel] " Peter Stuge 2013-12-02 13:09 ` [PATCHv2] " Michal Nazarewicz 2013-12-05 8:41 ` Eugene Krasnikov 2013-12-06 16:12 ` John W. Linville 2013-12-07 17:13 ` [PATCH] net: wirelesse: wcn36xx: pull allocation outside of critical section Michal Nazarewicz 2013-12-09 7:27 ` Eugene Krasnikov 2013-12-09 11:34 ` [PATCH] net: wirelesse: wcn36xx: check allocation and trim " Michal Nazarewicz 2013-12-11 15:52 ` John W. Linville 2013-12-11 16:43 ` [PATCH RESEND] net: wirelesse: wcn36xx: pull allocation outside of " Michal Nazarewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).