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 <[email protected]>
---
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
On 2023/9/1 19:16, Toke Høiland-Jørgensen wrote:
> Dongliang Mu <[email protected]> writes:
>
>> On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel
>> Contribution wrote:
>>> Dongliang Mu <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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