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@xxxxxxxxxxx> writes:
>
> > On 2023/9/1 18:41, 'Toke Høiland-Jørgensen' via HUST OS Kernel
> > Contribution wrote:
> > > Dongliang Mu <dzm91@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > ---
> > > >    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