Messages in this thread | | | Date | Fri, 1 Sep 2023 19:21:31 +0800 | Subject | Re: [PATCH] ath9k: fix null-ptr-deref in ath_chanctx_event | From | 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
| |