[PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Dongliang Mu]

Smatch reports:

ath_chanctx_event() error: we previously assumed 'vif' could be null

The function ath_chanctx_event can be called with vif argument as NULL.
If vif is NULL, ath_dbg can trigger a null pointer dereference.

Fix this by adding a null pointer check.

Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
---
 drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
index 571062f2e82a..e343c8962d14 100644
--- a/drivers/net/wireless/ath/ath9k/channel.c
+++ b/drivers/net/wireless/ath/ath9k/channel.c
@@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
 		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
 			break;
 
-		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
+		if (vif)
+			ath_dbg(common, CHAN_CTX,
+				"Preparing beacon for vif: %pM\n", vif->addr);
 
 		sc->sched.beacon_pending = true;
 		sc->sched.next_tbtt = REG_READ(ah, AR_NEXT_TBTT_TIMER);
-- 
2.39.2

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Toke Høiland-Jørgensen]

Dongliang Mu <dzm91@hust.edu.cn> writes:

> Smatch reports:
>
> ath_chanctx_event() error: we previously assumed 'vif' could be null
>
> The function ath_chanctx_event can be called with vif argument as NULL.
> If vif is NULL, ath_dbg can trigger a null pointer dereference.
>
> Fix this by adding a null pointer check.
>
> Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
> ---
>  drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
> index 571062f2e82a..e343c8962d14 100644
> --- a/drivers/net/wireless/ath/ath9k/channel.c
> +++ b/drivers/net/wireless/ath/ath9k/channel.c
> @@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
>  		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
>  			break;
>  
> -		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
> +		if (vif)
> +			ath_dbg(common, CHAN_CTX,
> +				"Preparing beacon for vif: %pM\n", vif->addr);

Please don't send patches for static checker errors without actually
checking if there is a valid bug. Which there isn't in this case.

Specifically, that branch of the switch statement dereferences the avp
pointer, which will be NULL if 'vif' is. Meaning we will have crashed
way before reaching this statement if vif is indeed NULL.

-Toke

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Dongliang Mu]

On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel 
Contribution wrote:
> Dongliang Mu <dzm91@hust.edu.cn> writes:
>
>> Smatch reports:
>>
>> ath_chanctx_event() error: we previously assumed 'vif' could be null
>>
>> The function ath_chanctx_event can be called with vif argument as NULL.
>> If vif is NULL, ath_dbg can trigger a null pointer dereference.
>>
>> Fix this by adding a null pointer check.
>>
>> Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
>> ---
>>   drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>> index 571062f2e82a..e343c8962d14 100644
>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>> @@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
>>   		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
>>   			break;
>>   
>> -		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
>> +		if (vif)
>> +			ath_dbg(common, CHAN_CTX,
>> +				"Preparing beacon for vif: %pM\n", vif->addr);
> Please don't send patches for static checker errors without actually
> checking if there is a valid bug. Which there isn't in this case.

Before sending this patch, I searched in the code, there are many call 
sites of ath_chanctx_event with argument vif as NULL.

Functions calling this function: ath_chanctx_event

   File      Function                   Line
0 beacon.c  ath9k_beacon_tasklet        459 ath_chanctx_event(sc, vif, 
ATH_CHANCTX_EVENT_BEACON_PREPARE);
1 channel.c ath_chanctx_check_active    321 ath_chanctx_event(sc, NULL,
2 channel.c ath_chanctx_beacon_sent_ev  781 ath_chanctx_event(sc, NULL, ev);
3 channel.c ath_chanctx_beacon_recv_ev  787 ath_chanctx_event(sc, NULL, ev);
4 channel.c ath_chanctx_timer          1054 ath_chanctx_event(sc, NULL, 
ATH_CHANCTX_EVENT_TSF_TIMER);
5 channel.c ath_chanctx_set_next       1321 ath_chanctx_event(sc, NULL, 
ATH_CHANCTX_EVENT_SWITCH);
6 channel.c ath9k_p2p_ps_timer         1566 ath_chanctx_event(sc, NULL, 
ATH_CHANCTX_EVENT_TSF_TIMER);
7 main.c    ath9k_sta_state            1671 ath_chanctx_event(sc, vif,
8 main.c    ath9k_remove_chanctx       2577 ath_chanctx_event(sc, NULL, 
ATH_CHANCTX_EVENT_UNASSIGN);
9 xmit.c    ath_tx_edma_tasklet        2749 ath_chanctx_event(sc, NULL,

This NULL parameters would cause some abnormal behaviors.

> Specifically, that branch of the switch statement dereferences the avp
> pointer, which will be NULL if 'vif' is. Meaning we will have crashed
> way before reaching this statement if vif is indeed NULL.
Yeah, you are right. However, no matter where or which variable causing 
the null-ptr-def crash, the crash is there.

I think this is unexpected.

Let me know if I make any mistakes

>
> -Toke
>

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Toke Høiland-Jørgensen]

Dongliang Mu <dzm91@hust.edu.cn> writes:

> On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel 
> Contribution wrote:
>> Dongliang Mu <dzm91@hust.edu.cn> writes:
>>
>>> Smatch reports:
>>>
>>> ath_chanctx_event() error: we previously assumed 'vif' could be null
>>>
>>> The function ath_chanctx_event can be called with vif argument as NULL.
>>> If vif is NULL, ath_dbg can trigger a null pointer dereference.
>>>
>>> Fix this by adding a null pointer check.
>>>
>>> Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
>>> ---
>>>   drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>>> index 571062f2e82a..e343c8962d14 100644
>>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>>> @@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
>>>   		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
>>>   			break;
>>>   
>>> -		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
>>> +		if (vif)
>>> +			ath_dbg(common, CHAN_CTX,
>>> +				"Preparing beacon for vif: %pM\n", vif->addr);
>> Please don't send patches for static checker errors without actually
>> checking if there is a valid bug. Which there isn't in this case.
>
> Before sending this patch, I searched in the code, there are many call 
> sites of ath_chanctx_event with argument vif as NULL.
>
> Functions calling this function: ath_chanctx_event
>
>    File      Function                   Line
> 0 beacon.c  ath9k_beacon_tasklet        459 ath_chanctx_event(sc, vif, 
> ATH_CHANCTX_EVENT_BEACON_PREPARE);

But only this one has ATH_CHANCTX_EVENT_BEACON_PREPARE as an argument,
which is required to hit the code path you are changing.

> 1 channel.c ath_chanctx_check_active    321 ath_chanctx_event(sc, NULL,
> 2 channel.c ath_chanctx_beacon_sent_ev  781 ath_chanctx_event(sc, NULL, ev);
> 3 channel.c ath_chanctx_beacon_recv_ev  787 ath_chanctx_event(sc, NULL, ev);
> 4 channel.c ath_chanctx_timer          1054 ath_chanctx_event(sc, NULL, 
> ATH_CHANCTX_EVENT_TSF_TIMER);
> 5 channel.c ath_chanctx_set_next       1321 ath_chanctx_event(sc, NULL, 
> ATH_CHANCTX_EVENT_SWITCH);
> 6 channel.c ath9k_p2p_ps_timer         1566 ath_chanctx_event(sc, NULL, 
> ATH_CHANCTX_EVENT_TSF_TIMER);
> 7 main.c    ath9k_sta_state            1671 ath_chanctx_event(sc, vif,
> 8 main.c    ath9k_remove_chanctx       2577 ath_chanctx_event(sc, NULL, 
> ATH_CHANCTX_EVENT_UNASSIGN);
> 9 xmit.c    ath_tx_edma_tasklet        2749 ath_chanctx_event(sc, NULL,
>
> This NULL parameters would cause some abnormal behaviors.
>
>> Specifically, that branch of the switch statement dereferences the avp
>> pointer, which will be NULL if 'vif' is. Meaning we will have crashed
>> way before reaching this statement if vif is indeed NULL.
> Yeah, you are right. However, no matter where or which variable causing 
> the null-ptr-def crash, the crash is there.

There is no crash, see above.

-Toke

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Dongliang Mu]

On 2023/9/1 19:16, Toke Høiland-Jørgensen wrote:
> Dongliang Mu <dzm91@hust.edu.cn> writes:
>
>> On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel
>> Contribution wrote:
>>> Dongliang Mu <dzm91@hust.edu.cn> writes:
>>>
>>>> Smatch reports:
>>>>
>>>> ath_chanctx_event() error: we previously assumed 'vif' could be null
>>>>
>>>> The function ath_chanctx_event can be called with vif argument as NULL.
>>>> If vif is NULL, ath_dbg can trigger a null pointer dereference.
>>>>
>>>> Fix this by adding a null pointer check.
>>>>
>>>> Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
>>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
>>>> ---
>>>>    drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>>>> index 571062f2e82a..e343c8962d14 100644
>>>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>>>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>>>> @@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
>>>>    		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
>>>>    			break;
>>>>    
>>>> -		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
>>>> +		if (vif)
>>>> +			ath_dbg(common, CHAN_CTX,
>>>> +				"Preparing beacon for vif: %pM\n", vif->addr);
>>> Please don't send patches for static checker errors without actually
>>> checking if there is a valid bug. Which there isn't in this case.
>> Before sending this patch, I searched in the code, there are many call
>> sites of ath_chanctx_event with argument vif as NULL.
>>
>> Functions calling this function: ath_chanctx_event
>>
>>     File      Function                   Line
>> 0 beacon.c  ath9k_beacon_tasklet        459 ath_chanctx_event(sc, vif,
>> ATH_CHANCTX_EVENT_BEACON_PREPARE);
> But only this one has ATH_CHANCTX_EVENT_BEACON_PREPARE as an argument,
> which is required to hit the code path you are changing.
>
>> 1 channel.c ath_chanctx_check_active    321 ath_chanctx_event(sc, NULL,
>> 2 channel.c ath_chanctx_beacon_sent_ev  781 ath_chanctx_event(sc, NULL, ev);
>> 3 channel.c ath_chanctx_beacon_recv_ev  787 ath_chanctx_event(sc, NULL, ev);
>> 4 channel.c ath_chanctx_timer          1054 ath_chanctx_event(sc, NULL,
>> ATH_CHANCTX_EVENT_TSF_TIMER);
>> 5 channel.c ath_chanctx_set_next       1321 ath_chanctx_event(sc, NULL,
>> ATH_CHANCTX_EVENT_SWITCH);
>> 6 channel.c ath9k_p2p_ps_timer         1566 ath_chanctx_event(sc, NULL,
>> ATH_CHANCTX_EVENT_TSF_TIMER);
>> 7 main.c    ath9k_sta_state            1671 ath_chanctx_event(sc, vif,
>> 8 main.c    ath9k_remove_chanctx       2577 ath_chanctx_event(sc, NULL,
>> ATH_CHANCTX_EVENT_UNASSIGN);
>> 9 xmit.c    ath_tx_edma_tasklet        2749 ath_chanctx_event(sc, NULL,
>>
>> This NULL parameters would cause some abnormal behaviors.
>>
>>> Specifically, that branch of the switch statement dereferences the avp
>>> pointer, which will be NULL if 'vif' is. Meaning we will have crashed
>>> way before reaching this statement if vif is indeed NULL.
>> Yeah, you are right. However, no matter where or which variable causing
>> the null-ptr-def crash, the crash is there.
> There is no crash, see above.

Yeah, I see where my problem is. Please ignore this patch.

In the future I will check more and think more about the code logic when 
verifying the result of static analyzer.

Thanks for your patience.

>
> -Toke

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Toke Høiland-Jørgensen]

Dongliang Mu <dzm91@hust.edu.cn> writes:

> On 2023/9/1 19:16, Toke Høiland-Jørgensen wrote:
>> Dongliang Mu <dzm91@hust.edu.cn> writes:
>>
>>> On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel
>>> Contribution wrote:
>>>> Dongliang Mu <dzm91@hust.edu.cn> writes:
>>>>
>>>>> Smatch reports:
>>>>>
>>>>> ath_chanctx_event() error: we previously assumed 'vif' could be null
>>>>>
>>>>> The function ath_chanctx_event can be called with vif argument as NULL.
>>>>> If vif is NULL, ath_dbg can trigger a null pointer dereference.
>>>>>
>>>>> Fix this by adding a null pointer check.
>>>>>
>>>>> Fixes: 878066e745b5 ("ath9k: Add more debug statements for channel context")
>>>>> Signed-off-by: Dongliang Mu <dzm91@hust.edu.cn>
>>>>> ---
>>>>>    drivers/net/wireless/ath/ath9k/channel.c | 4 +++-
>>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/wireless/ath/ath9k/channel.c b/drivers/net/wireless/ath/ath9k/channel.c
>>>>> index 571062f2e82a..e343c8962d14 100644
>>>>> --- a/drivers/net/wireless/ath/ath9k/channel.c
>>>>> +++ b/drivers/net/wireless/ath/ath9k/channel.c
>>>>> @@ -576,7 +576,9 @@ void ath_chanctx_event(struct ath_softc *sc, struct ieee80211_vif *vif,
>>>>>    		if (sc->sched.state != ATH_CHANCTX_STATE_WAIT_FOR_BEACON)
>>>>>    			break;
>>>>>    
>>>>> -		ath_dbg(common, CHAN_CTX, "Preparing beacon for vif: %pM\n", vif->addr);
>>>>> +		if (vif)
>>>>> +			ath_dbg(common, CHAN_CTX,
>>>>> +				"Preparing beacon for vif: %pM\n", vif->addr);
>>>> Please don't send patches for static checker errors without actually
>>>> checking if there is a valid bug. Which there isn't in this case.
>>> Before sending this patch, I searched in the code, there are many call
>>> sites of ath_chanctx_event with argument vif as NULL.
>>>
>>> Functions calling this function: ath_chanctx_event
>>>
>>>     File      Function                   Line
>>> 0 beacon.c  ath9k_beacon_tasklet        459 ath_chanctx_event(sc, vif,
>>> ATH_CHANCTX_EVENT_BEACON_PREPARE);
>> But only this one has ATH_CHANCTX_EVENT_BEACON_PREPARE as an argument,
>> which is required to hit the code path you are changing.
>>
>>> 1 channel.c ath_chanctx_check_active    321 ath_chanctx_event(sc, NULL,
>>> 2 channel.c ath_chanctx_beacon_sent_ev  781 ath_chanctx_event(sc, NULL, ev);
>>> 3 channel.c ath_chanctx_beacon_recv_ev  787 ath_chanctx_event(sc, NULL, ev);
>>> 4 channel.c ath_chanctx_timer          1054 ath_chanctx_event(sc, NULL,
>>> ATH_CHANCTX_EVENT_TSF_TIMER);
>>> 5 channel.c ath_chanctx_set_next       1321 ath_chanctx_event(sc, NULL,
>>> ATH_CHANCTX_EVENT_SWITCH);
>>> 6 channel.c ath9k_p2p_ps_timer         1566 ath_chanctx_event(sc, NULL,
>>> ATH_CHANCTX_EVENT_TSF_TIMER);
>>> 7 main.c    ath9k_sta_state            1671 ath_chanctx_event(sc, vif,
>>> 8 main.c    ath9k_remove_chanctx       2577 ath_chanctx_event(sc, NULL,
>>> ATH_CHANCTX_EVENT_UNASSIGN);
>>> 9 xmit.c    ath_tx_edma_tasklet        2749 ath_chanctx_event(sc, NULL,
>>>
>>> This NULL parameters would cause some abnormal behaviors.
>>>
>>>> Specifically, that branch of the switch statement dereferences the avp
>>>> pointer, which will be NULL if 'vif' is. Meaning we will have crashed
>>>> way before reaching this statement if vif is indeed NULL.
>>> Yeah, you are right. However, no matter where or which variable causing
>>> the null-ptr-def crash, the crash is there.
>> There is no crash, see above.
>
> Yeah, I see where my problem is. Please ignore this patch.
>
> In the future I will check more and think more about the code logic when 
> verifying the result of static analyzer.

Great! As a general point, while static analysers do occasionally find
corner case bugs that have not been discovered, they are by no means
infallible, and tend to produce false positives as well. And especially
in a code path such as this, where the crash would have been really
obvious, applying some extra scrutiny to the tool's output is warranted.

> Thanks for your patience.

You're welcome :)

-Toke

Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event

[Dan Carpenter]

On Fri, Sep 01, 2023 at 01:16:00PM +0200, 'Toke Høiland-Jørgensen' via HUST OS Kernel Contribution wrote:
> > Before sending this patch, I searched in the code, there are many call 
> > sites of ath_chanctx_event with argument vif as NULL.
> >
> > Functions calling this function: ath_chanctx_event
> >
> >    File      Function                   Line
> > 0 beacon.c  ath9k_beacon_tasklet        459 ath_chanctx_event(sc, vif, 
> > ATH_CHANCTX_EVENT_BEACON_PREPARE);
> 
> But only this one has ATH_CHANCTX_EVENT_BEACON_PREPARE as an argument,
> which is required to hit the code path you are changing.
> 

Btw, if you have the cross function database enabled then Smatch will
parse this code correctly.  No false positive.

regards,
dan carpenter