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);
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
>
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.
-Toke