* [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
@ 2025-01-10 13:45 Marcel Hamer
2025-01-13 19:28 ` Arend van Spriel
2025-01-15 15:16 ` Kalle Valo
0 siblings, 2 replies; 6+ messages in thread
From: Marcel Hamer @ 2025-01-10 13:45 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Kalle Valo, linux-wireless, Marcel Hamer, stable
On removal of the device or unloading of the kernel module a potential
NULL pointer dereference occurs.
The following sequence deletes the interface:
brcmf_detach()
brcmf_remove_interface()
brcmf_del_if()
Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
After brcmf_remove_interface() call the brcmf_proto_detach() function is
called providing the following sequence:
brcmf_detach()
brcmf_proto_detach()
brcmf_proto_msgbuf_detach()
brcmf_flowring_detach()
brcmf_msgbuf_delete_flowring()
brcmf_msgbuf_remove_flowring()
brcmf_flowring_delete()
brcmf_get_ifp()
brcmf_txfinalize()
Since brcmf_get_ip() can and actually will return NULL in this case the
call to brcmf_txfinalize() will result in a NULL pointer dereference
inside brcmf_txfinalize() when trying to update
ifp->ndev->stats.tx_errors.
This will only happen if a flowring still has an skb.
Although the NULL pointer dereference has only been seen when trying to update
the tx statistic, all other uses of the ifp pointer have been guarded as well.
Cc: stable@vger.kernel.org
Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
---
v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index c3a57e30c855..791757a3ec13 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -543,13 +543,13 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
eh = (struct ethhdr *)(txp->data);
type = ntohs(eh->h_proto);
- if (type == ETH_P_PAE) {
+ if (type == ETH_P_PAE && ifp) {
atomic_dec(&ifp->pend_8021x_cnt);
if (waitqueue_active(&ifp->pend_8021x_wait))
wake_up(&ifp->pend_8021x_wait);
}
- if (!success)
+ if (!success && ifp)
ifp->ndev->stats.tx_errors++;
brcmu_pkt_buf_free_skb(txp);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
2025-01-10 13:45 [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update Marcel Hamer
@ 2025-01-13 19:28 ` Arend van Spriel
2025-01-14 8:18 ` Marcel Hamer
2025-01-15 15:16 ` Kalle Valo
1 sibling, 1 reply; 6+ messages in thread
From: Arend van Spriel @ 2025-01-13 19:28 UTC (permalink / raw)
To: Marcel Hamer; +Cc: Kalle Valo, linux-wireless, stable
On 1/10/2025 2:45 PM, Marcel Hamer wrote:
> On removal of the device or unloading of the kernel module a potential
> NULL pointer dereference occurs.
>
> The following sequence deletes the interface:
>
> brcmf_detach()
> brcmf_remove_interface()
> brcmf_del_if()
>
> Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
> BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
>
> After brcmf_remove_interface() call the brcmf_proto_detach() function is
> called providing the following sequence:
>
> brcmf_detach()
> brcmf_proto_detach()
> brcmf_proto_msgbuf_detach()
> brcmf_flowring_detach()
> brcmf_msgbuf_delete_flowring()
> brcmf_msgbuf_remove_flowring()
> brcmf_flowring_delete()
> brcmf_get_ifp()
> brcmf_txfinalize()
>
> Since brcmf_get_ip() can and actually will return NULL in this case the
> call to brcmf_txfinalize() will result in a NULL pointer dereference
> inside brcmf_txfinalize() when trying to update
> ifp->ndev->stats.tx_errors.
>
> This will only happen if a flowring still has an skb.
>
> Although the NULL pointer dereference has only been seen when trying to update
> the tx statistic, all other uses of the ifp pointer have been guarded as well.
Here my suggestion to make it a bit more simple...
> Cc: stable@vger.kernel.org
> Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
> Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
> ---
> v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index c3a57e30c855..791757a3ec13 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -543,13 +543,13 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
Instead of checking ifp below you can simply do following here and be
done with it:
if (!ifp) {
brcmu_pkt_buf_free_skb(txp);
return;
}
> eh = (struct ethhdr *)(txp->data);
> type = ntohs(eh->h_proto);
>
> - if (type == ETH_P_PAE) {
> + if (type == ETH_P_PAE && ifp) {
> atomic_dec(&ifp->pend_8021x_cnt);
> if (waitqueue_active(&ifp->pend_8021x_wait))
> wake_up(&ifp->pend_8021x_wait);
> }
>
> - if (!success)
> + if (!success && ifp)
> ifp->ndev->stats.tx_errors++;
>
> brcmu_pkt_buf_free_skb(txp);
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
2025-01-13 19:28 ` Arend van Spriel
@ 2025-01-14 8:18 ` Marcel Hamer
2025-01-15 15:57 ` Arend Van Spriel
0 siblings, 1 reply; 6+ messages in thread
From: Marcel Hamer @ 2025-01-14 8:18 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Kalle Valo, linux-wireless, stable
Hello,
On Mon, Jan 13, 2025 at 08:28:33PM +0100, Arend van Spriel wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On 1/10/2025 2:45 PM, Marcel Hamer wrote:
> > On removal of the device or unloading of the kernel module a potential
> > NULL pointer dereference occurs.
> >
> > The following sequence deletes the interface:
> >
> > brcmf_detach()
> > brcmf_remove_interface()
> > brcmf_del_if()
> >
> > Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
> > BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
> >
> > After brcmf_remove_interface() call the brcmf_proto_detach() function is
> > called providing the following sequence:
> >
> > brcmf_detach()
> > brcmf_proto_detach()
> > brcmf_proto_msgbuf_detach()
> > brcmf_flowring_detach()
> > brcmf_msgbuf_delete_flowring()
> > brcmf_msgbuf_remove_flowring()
> > brcmf_flowring_delete()
> > brcmf_get_ifp()
> > brcmf_txfinalize()
> >
> > Since brcmf_get_ip() can and actually will return NULL in this case the
> > call to brcmf_txfinalize() will result in a NULL pointer dereference
> > inside brcmf_txfinalize() when trying to update
> > ifp->ndev->stats.tx_errors.
> >
> > This will only happen if a flowring still has an skb.
> >
> > Although the NULL pointer dereference has only been seen when trying to update
> > the tx statistic, all other uses of the ifp pointer have been guarded as well.
>
> Here my suggestion to make it a bit more simple...
>
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
> > Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
> > ---
> > v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
> > ---
> > drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> > index c3a57e30c855..791757a3ec13 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> > @@ -543,13 +543,13 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>
> Instead of checking ifp below you can simply do following here and be
> done with it:
>
> if (!ifp) {
> brcmu_pkt_buf_free_skb(txp);
> return;
> }
>
Thank you for the suggestion and review. I considered that when doing my changes
and then thought it would be more efficient to only check the validity of ifp
when ifp is actually used. To skip checking it on each call to this function,
since it seems unlikely ifp will be used on each and every call.
Before I create a patch with your suggested changes, I just wanted to check if
you think it would be better to just wrap the two existing if statements in a
new if statement in that case, something like this:
if (ifp) {
...
if (type == ETH_P_PAE) {
...
}
if (!success)
ifp->ndev->stats.tx_errors++;
}
And then keep a single call to brcmu_pkt_buf_free_skb(txp) at the end of the
function.
> > eh = (struct ethhdr *)(txp->data);
> > type = ntohs(eh->h_proto);
> >
> > - if (type == ETH_P_PAE) {
> > + if (type == ETH_P_PAE && ifp) {
> > atomic_dec(&ifp->pend_8021x_cnt);
> > if (waitqueue_active(&ifp->pend_8021x_wait))
> > wake_up(&ifp->pend_8021x_wait);
> > }
> >
> > - if (!success)
> > + if (!success && ifp)
> > ifp->ndev->stats.tx_errors++;
> >
> > brcmu_pkt_buf_free_skb(txp);
>
Kind regards,
Marcel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
2025-01-14 8:18 ` Marcel Hamer
@ 2025-01-15 15:57 ` Arend Van Spriel
0 siblings, 0 replies; 6+ messages in thread
From: Arend Van Spriel @ 2025-01-15 15:57 UTC (permalink / raw)
To: Marcel Hamer; +Cc: Kalle Valo, linux-wireless, stable
On January 14, 2025 9:20:33 AM Marcel Hamer <marcel.hamer@windriver.com> wrote:
> Hello,
>
> On Mon, Jan 13, 2025 at 08:28:33PM +0100, Arend van Spriel wrote:
>> CAUTION: This email comes from a non Wind River email account!
>> Do not click links or open attachments unless you recognize the sender and
>> know the content is safe.
>>
>> On 1/10/2025 2:45 PM, Marcel Hamer wrote:
>>> On removal of the device or unloading of the kernel module a potential
>>> NULL pointer dereference occurs.
>>>
>>> The following sequence deletes the interface:
>>>
>>> brcmf_detach()
>>> brcmf_remove_interface()
>>> brcmf_del_if()
>>>
>>> Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
>>> BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
>>>
>>> After brcmf_remove_interface() call the brcmf_proto_detach() function is
>>> called providing the following sequence:
>>>
>>> brcmf_detach()
>>> brcmf_proto_detach()
>>> brcmf_proto_msgbuf_detach()
>>> brcmf_flowring_detach()
>>> brcmf_msgbuf_delete_flowring()
>>> brcmf_msgbuf_remove_flowring()
>>> brcmf_flowring_delete()
>>> brcmf_get_ifp()
>>> brcmf_txfinalize()
>>>
>>> Since brcmf_get_ip() can and actually will return NULL in this case the
>>> call to brcmf_txfinalize() will result in a NULL pointer dereference
>>> inside brcmf_txfinalize() when trying to update
>>> ifp->ndev->stats.tx_errors.
>>>
>>> This will only happen if a flowring still has an skb.
>>>
>>> Although the NULL pointer dereference has only been seen when trying to update
>>> the tx statistic, all other uses of the ifp pointer have been guarded as well.
>>
>> Here my suggestion to make it a bit more simple...
>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
>>> Link:
>>> https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
>>> ---
>>> v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
>>> ---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> index c3a57e30c855..791757a3ec13 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> @@ -543,13 +543,13 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct
>>> sk_buff *txp, bool success)
>>
>> Instead of checking ifp below you can simply do following here and be
>> done with it:
>>
>> if (!ifp) {
>> brcmu_pkt_buf_free_skb(txp);
>> return;
>> }
>
> Thank you for the suggestion and review. I considered that when doing my
> changes
> and then thought it would be more efficient to only check the validity of ifp
> when ifp is actually used. To skip checking it on each call to this function,
> since it seems unlikely ifp will be used on each and every call.
Actually, the function will mostly be called with a valid ifp instance.
> Before I create a patch with your suggested changes, I just wanted to check if
> you think it would be better to just wrap the two existing if statements in a
> new if statement in that case, something like this:
>
> if (ifp) {
> ...
> if (type == ETH_P_PAE) {
> ...
> }
>
> if (!success)
> ifp->ndev->stats.tx_errors++;
> }
>
> And then keep a single call to brcmu_pkt_buf_free_skb(txp) at the end of the
> function.
I understand your motivation. My generic approach is to bail out early and
thereby avoid extra indentation levels. Although this function is in the
data path I think the null-check for ifp is negligible. Could consider
using unlikely(), but that requires profiling data to justify using it here.
Regards,
Arend
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
2025-01-10 13:45 [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update Marcel Hamer
2025-01-13 19:28 ` Arend van Spriel
@ 2025-01-15 15:16 ` Kalle Valo
2025-01-15 16:02 ` Arend Van Spriel
1 sibling, 1 reply; 6+ messages in thread
From: Kalle Valo @ 2025-01-15 15:16 UTC (permalink / raw)
To: Marcel Hamer; +Cc: Arend van Spriel, linux-wireless, Marcel Hamer, stable
Marcel Hamer <marcel.hamer@windriver.com> wrote:
> On removal of the device or unloading of the kernel module a potential
> NULL pointer dereference occurs.
>
> The following sequence deletes the interface:
>
> brcmf_detach()
> brcmf_remove_interface()
> brcmf_del_if()
>
> Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
> BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
>
> After brcmf_remove_interface() call the brcmf_proto_detach() function is
> called providing the following sequence:
>
> brcmf_detach()
> brcmf_proto_detach()
> brcmf_proto_msgbuf_detach()
> brcmf_flowring_detach()
> brcmf_msgbuf_delete_flowring()
> brcmf_msgbuf_remove_flowring()
> brcmf_flowring_delete()
> brcmf_get_ifp()
> brcmf_txfinalize()
>
> Since brcmf_get_ip() can and actually will return NULL in this case the
> call to brcmf_txfinalize() will result in a NULL pointer dereference
> inside brcmf_txfinalize() when trying to update
> ifp->ndev->stats.tx_errors.
>
> This will only happen if a flowring still has an skb.
>
> Although the NULL pointer dereference has only been seen when trying to update
> the tx statistic, all other uses of the ifp pointer have been guarded as well.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
> Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
If you submit v3, please add 'wifi:'.
ERROR: 'wifi:' prefix missing: '[PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update'
Patch set to Changes Requested.
--
https://patchwork.kernel.org/project/linux-wireless/patch/20250110134502.824722-1-marcel.hamer@windriver.com/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update
2025-01-15 15:16 ` Kalle Valo
@ 2025-01-15 16:02 ` Arend Van Spriel
0 siblings, 0 replies; 6+ messages in thread
From: Arend Van Spriel @ 2025-01-15 16:02 UTC (permalink / raw)
To: Kalle Valo, Marcel Hamer; +Cc: linux-wireless, Marcel Hamer, stable
On January 15, 2025 4:16:58 PM Kalle Valo <kvalo@kernel.org> wrote:
> Marcel Hamer <marcel.hamer@windriver.com> wrote:
>
>> On removal of the device or unloading of the kernel module a potential
>> NULL pointer dereference occurs.
>>
>> The following sequence deletes the interface:
>>
>> brcmf_detach()
>> brcmf_remove_interface()
>> brcmf_del_if()
>>
>> Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to
>> BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches.
>>
>> After brcmf_remove_interface() call the brcmf_proto_detach() function is
>> called providing the following sequence:
>>
>> brcmf_detach()
>> brcmf_proto_detach()
>> brcmf_proto_msgbuf_detach()
>> brcmf_flowring_detach()
>> brcmf_msgbuf_delete_flowring()
>> brcmf_msgbuf_remove_flowring()
>> brcmf_flowring_delete()
>> brcmf_get_ifp()
>> brcmf_txfinalize()
>>
>> Since brcmf_get_ip() can and actually will return NULL in this case the
>> call to brcmf_txfinalize() will result in a NULL pointer dereference
>> inside brcmf_txfinalize() when trying to update
>> ifp->ndev->stats.tx_errors.
>>
>> This will only happen if a flowring still has an skb.
>>
>> Although the NULL pointer dereference has only been seen when trying to update
>> the tx statistic, all other uses of the ifp pointer have been guarded as well.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com>
>> Link:
>> https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
>
> If you submit v3, please add 'wifi:'.
>
> ERROR: 'wifi:' prefix missing: '[PATCH v2] brcmfmac: NULL pointer
> dereference on tx statistic update'
While at it maybe rephrase the subject to:
wifi: brcmfmac: fix NULL pointer dereference in brcmf_txfinalize()
Regards,
Arend
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-15 16:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-10 13:45 [PATCH v2] brcmfmac: NULL pointer dereference on tx statistic update Marcel Hamer
2025-01-13 19:28 ` Arend van Spriel
2025-01-14 8:18 ` Marcel Hamer
2025-01-15 15:57 ` Arend Van Spriel
2025-01-15 15:16 ` Kalle Valo
2025-01-15 16:02 ` Arend Van Spriel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox