[PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The pointer rtwsta is dereferencing pointer sta before sta is
being null checked, so there is a potential null pointer deference
issue that may occur. Fix this by only assigning rtwsta after sta
has been null checked. Add in a null pointer check on rtwsta before
dereferencing it too.

Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
Addresses-Coverity: ("Dereference before null check")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index 06fb6e5b1b37..26f52a25f545 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
 {
 	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
 	struct ieee80211_sta *sta = txq->sta;
-	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+	struct rtw89_sta *rtwsta;
 
-	if (!sta || rtwsta->max_agg_wait <= 0)
+	if (!sta)
+		return false;
+	rtwsta = (struct rtw89_sta *)sta->drv_priv;
+	if (!rtwsta)
+		return false;
+	if (rtwsta->max_agg_wait <= 0)
 		return false;
 
 	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
-- 
2.32.0

RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Pkshih]

> -----Original Message-----
> From: Colin King <colin.king@canonical.com>
> Sent: Friday, October 15, 2021 11:46 PM
> To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
> netdev@vger.kernel.org
> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> 
> From: Colin Ian King <colin.king@canonical.com>
> 
> The pointer rtwsta is dereferencing pointer sta before sta is
> being null checked, so there is a potential null pointer deference
> issue that may occur. Fix this by only assigning rtwsta after sta
> has been null checked. Add in a null pointer check on rtwsta before
> dereferencing it too.
> 
> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> Addresses-Coverity: ("Dereference before null check")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> b/drivers/net/wireless/realtek/rtw89/core.c
> index 06fb6e5b1b37..26f52a25f545 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>  {
>  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>  	struct ieee80211_sta *sta = txq->sta;
> -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;

'sta->drv_priv' is only a pointer, we don't really dereference the
data right here, so I think this is safe. More, compiler can optimize
this instruction that reorder it to the place just right before using.
So, it seems like a false alarm.

> +	struct rtw89_sta *rtwsta;
> 
> -	if (!sta || rtwsta->max_agg_wait <= 0)
> +	if (!sta)
> +		return false;
> +	rtwsta = (struct rtw89_sta *)sta->drv_priv;
> +	if (!rtwsta)
> +		return false;
> +	if (rtwsta->max_agg_wait <= 0)
>  		return false;
> 
>  	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)

I check the size of object files before/after this patch, and
the original one is smaller.

   text    data     bss     dec     hex filename
  16781    3392       1   20174    4ece core-0.o  // original
  16819    3392       1   20212    4ef4 core-1.o  // after this patch

Do you think it is worth to apply this patch?

--
Ping-Ke

Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Kalle Valo]

Pkshih <pkshih@realtek.com> writes:

>> -----Original Message-----
>> From: Colin King <colin.king@canonical.com>
>> Sent: Friday, October 15, 2021 11:46 PM
>> To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
>> <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
>> netdev@vger.kernel.org
>> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>> 
>> From: Colin Ian King <colin.king@canonical.com>
>> 
>> The pointer rtwsta is dereferencing pointer sta before sta is
>> being null checked, so there is a potential null pointer deference
>> issue that may occur. Fix this by only assigning rtwsta after sta
>> has been null checked. Add in a null pointer check on rtwsta before
>> dereferencing it too.
>> 
>> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
>> Addresses-Coverity: ("Dereference before null check")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
>> b/drivers/net/wireless/realtek/rtw89/core.c
>> index 06fb6e5b1b37..26f52a25f545 100644
>> --- a/drivers/net/wireless/realtek/rtw89/core.c
>> +++ b/drivers/net/wireless/realtek/rtw89/core.c
>> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>>  {
>>  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>>  	struct ieee80211_sta *sta = txq->sta;
>> -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>
> 'sta->drv_priv' is only a pointer, we don't really dereference the
> data right here, so I think this is safe. More, compiler can optimize
> this instruction that reorder it to the place just right before using.
> So, it seems like a false alarm.
>
>> +	struct rtw89_sta *rtwsta;
>> 
>> -	if (!sta || rtwsta->max_agg_wait <= 0)
>> +	if (!sta)
>> +		return false;
>> +	rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> +	if (!rtwsta)
>> +		return false;
>> +	if (rtwsta->max_agg_wait <= 0)
>>  		return false;
>> 
>>  	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
>
> I check the size of object files before/after this patch, and
> the original one is smaller.
>
>    text    data     bss     dec     hex filename
>   16781    3392       1   20174    4ece core-0.o  // original
>   16819    3392       1   20212    4ef4 core-1.o  // after this patch
>
> Do you think it is worth to apply this patch?

I think that we should apply the patch. Even though the compiler _may_
reorder the code, it might choose not to do that.

Another question is that can txq->sta really be null? I didn't check the
code, but if it should be always set when the null check is not needed.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Pkshih]

> -----Original Message-----
> From: kvalo=codeaurora.org@mg.codeaurora.org <kvalo=codeaurora.org@mg.codeaurora.org> On
> Behalf Of Kalle Valo
> Sent: Monday, October 18, 2021 8:12 PM
> To: Pkshih <pkshih@realtek.com>
> Cc: Colin King <colin.king@canonical.com>; David S . Miller <davem@davemloft.net>; Jakub
> Kicinski <kuba@kernel.org>; linux-wireless@vger.kernel.org; netdev@vger.kernel.org;
> kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> 
> Pkshih <pkshih@realtek.com> writes:
> 
> >> -----Original Message-----
> >> From: Colin King <colin.king@canonical.com>
> >> Sent: Friday, October 15, 2021 11:46 PM
> >> To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> >> <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
> >> netdev@vger.kernel.org
> >> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >>
> >> From: Colin Ian King <colin.king@canonical.com>
> >>
> >> The pointer rtwsta is dereferencing pointer sta before sta is
> >> being null checked, so there is a potential null pointer deference
> >> issue that may occur. Fix this by only assigning rtwsta after sta
> >> has been null checked. Add in a null pointer check on rtwsta before
> >> dereferencing it too.
> >>
> >> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> >> Addresses-Coverity: ("Dereference before null check")
> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> ---
> >>  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> >> b/drivers/net/wireless/realtek/rtw89/core.c
> >> index 06fb6e5b1b37..26f52a25f545 100644
> >> --- a/drivers/net/wireless/realtek/rtw89/core.c
> >> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> >> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >>  {
> >>  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >>  	struct ieee80211_sta *sta = txq->sta;
> >> -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >
> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > data right here, so I think this is safe. More, compiler can optimize
> > this instruction that reorder it to the place just right before using.
> > So, it seems like a false alarm.
> >
> >> +	struct rtw89_sta *rtwsta;
> >>
> >> -	if (!sta || rtwsta->max_agg_wait <= 0)
> >> +	if (!sta)
> >> +		return false;
> >> +	rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> +	if (!rtwsta)
> >> +		return false;
> >> +	if (rtwsta->max_agg_wait <= 0)
> >>  		return false;
> >>
> >>  	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
> >
> > I check the size of object files before/after this patch, and
> > the original one is smaller.
> >
> >    text    data     bss     dec     hex filename
> >   16781    3392       1   20174    4ece core-0.o  // original
> >   16819    3392       1   20212    4ef4 core-1.o  // after this patch
> >
> > Do you think it is worth to apply this patch?
> 
> I think that we should apply the patch. Even though the compiler _may_
> reorder the code, it might choose not to do that.

Understand.

I have another way to fix this coverity warning, like:

@@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
 {
        struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
        struct ieee80211_sta *sta = txq->sta;
-       struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
+       struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;

        if (!sta || rtwsta->max_agg_wait <= 0)
                return false;

Is this acceptable?
It has a little redundant checking of 'sta', but the code looks clean.

> 
> Another question is that can txq->sta really be null? I didn't check the
> code, but if it should be always set when the null check is not needed.
> 

It says

* struct ieee80211_txq - Software intermediate tx queue
* @sta: station table entry, %NULL for per-vif queue

So, we need to check if 'sta' is NULL.

--
Ping-Ke

Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Kalle Valo]

Pkshih <pkshih@realtek.com> writes:

>> -----Original Message-----
>> From: kvalo=codeaurora.org@mg.codeaurora.org
>> <kvalo=codeaurora.org@mg.codeaurora.org> On
>> Behalf Of Kalle Valo
>> Sent: Monday, October 18, 2021 8:12 PM
>> To: Pkshih <pkshih@realtek.com>
>> Cc: Colin King <colin.king@canonical.com>; David S . Miller
>> <davem@davemloft.net>; Jakub
>> Kicinski <kuba@kernel.org>; linux-wireless@vger.kernel.org; netdev@vger.kernel.org;
>> kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>> 
>> Pkshih <pkshih@realtek.com> writes:
>> 
>> >> -----Original Message-----
>> >> From: Colin King <colin.king@canonical.com>
>> >> Sent: Friday, October 15, 2021 11:46 PM
>> >> To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
>> >> <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
>> >> netdev@vger.kernel.org
>> >> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
>> >> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
>> >>
>> >> From: Colin Ian King <colin.king@canonical.com>
>> >>
>> >> The pointer rtwsta is dereferencing pointer sta before sta is
>> >> being null checked, so there is a potential null pointer deference
>> >> issue that may occur. Fix this by only assigning rtwsta after sta
>> >> has been null checked. Add in a null pointer check on rtwsta before
>> >> dereferencing it too.
>> >>
>> >> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
>> >> Addresses-Coverity: ("Dereference before null check")
>> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> >> ---
>> >>  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
>> >>  1 file changed, 7 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
>> >> b/drivers/net/wireless/realtek/rtw89/core.c
>> >> index 06fb6e5b1b37..26f52a25f545 100644
>> >> --- a/drivers/net/wireless/realtek/rtw89/core.c
>> >> +++ b/drivers/net/wireless/realtek/rtw89/core.c
>> >> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>> >>  {
>> >>  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>> >>  	struct ieee80211_sta *sta = txq->sta;
>> >> -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> >
>> > 'sta->drv_priv' is only a pointer, we don't really dereference the
>> > data right here, so I think this is safe. More, compiler can optimize
>> > this instruction that reorder it to the place just right before using.
>> > So, it seems like a false alarm.
>> >
>> >> +	struct rtw89_sta *rtwsta;
>> >>
>> >> -	if (!sta || rtwsta->max_agg_wait <= 0)
>> >> +	if (!sta)
>> >> +		return false;
>> >> +	rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> >> +	if (!rtwsta)
>> >> +		return false;
>> >> +	if (rtwsta->max_agg_wait <= 0)
>> >>  		return false;
>> >>
>> >>  	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
>> >
>> > I check the size of object files before/after this patch, and
>> > the original one is smaller.
>> >
>> >    text    data     bss     dec     hex filename
>> >   16781    3392       1   20174    4ece core-0.o  // original
>> >   16819    3392       1   20212    4ef4 core-1.o  // after this patch
>> >
>> > Do you think it is worth to apply this patch?
>> 
>> I think that we should apply the patch. Even though the compiler _may_
>> reorder the code, it might choose not to do that.
>
> Understand.
>
> I have another way to fix this coverity warning, like:
>
> @@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>  {
>         struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>         struct ieee80211_sta *sta = txq->sta;
> -       struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> +       struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
>
>         if (!sta || rtwsta->max_agg_wait <= 0)
>                 return false;
>
> Is this acceptable?
> It has a little redundant checking of 'sta', but the code looks clean.

I feel that Colin's fix is more readable, but this is just matter of
taste. You can choose.

>> Another question is that can txq->sta really be null? I didn't check the
>> code, but if it should be always set when the null check is not needed.
>> 
>
> It says
>
> * struct ieee80211_txq - Software intermediate tx queue
> * @sta: station table entry, %NULL for per-vif queue
>
> So, we need to check if 'sta' is NULL.

Ok, thanks for checking (no pun intended) :)

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Pkshih]

> -----Original Message-----
> From: kvalo=codeaurora.org@mg.codeaurora.org <kvalo=codeaurora.org@mg.codeaurora.org> On Behalf Of Kalle
> Valo
> Sent: Wednesday, October 20, 2021 4:36 PM
> To: Pkshih <pkshih@realtek.com>
> Cc: Colin King <colin.king@canonical.com>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> <kuba@kernel.org>; linux-wireless@vger.kernel.org; netdev@vger.kernel.org;
> kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> 
> Pkshih <pkshih@realtek.com> writes:
> 
> >> -----Original Message-----
> >> From: kvalo=codeaurora.org@mg.codeaurora.org
> >> <kvalo=codeaurora.org@mg.codeaurora.org> On
> >> Behalf Of Kalle Valo
> >> Sent: Monday, October 18, 2021 8:12 PM
> >> To: Pkshih <pkshih@realtek.com>
> >> Cc: Colin King <colin.king@canonical.com>; David S . Miller
> >> <davem@davemloft.net>; Jakub
> >> Kicinski <kuba@kernel.org>; linux-wireless@vger.kernel.org; netdev@vger.kernel.org;
> >> kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >>
> >> Pkshih <pkshih@realtek.com> writes:
> >>
> >> >> -----Original Message-----
> >> >> From: Colin King <colin.king@canonical.com>
> >> >> Sent: Friday, October 15, 2021 11:46 PM
> >> >> To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> >> >> <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
> >> >> netdev@vger.kernel.org
> >> >> Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> >> >> Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> >> >>
> >> >> From: Colin Ian King <colin.king@canonical.com>
> >> >>
> >> >> The pointer rtwsta is dereferencing pointer sta before sta is
> >> >> being null checked, so there is a potential null pointer deference
> >> >> issue that may occur. Fix this by only assigning rtwsta after sta
> >> >> has been null checked. Add in a null pointer check on rtwsta before
> >> >> dereferencing it too.
> >> >>
> >> >> Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> >> >> Addresses-Coverity: ("Dereference before null check")
> >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> >> >> ---
> >> >>  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> >> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >> >>
> >> >> diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> >> >> b/drivers/net/wireless/realtek/rtw89/core.c
> >> >> index 06fb6e5b1b37..26f52a25f545 100644
> >> >> --- a/drivers/net/wireless/realtek/rtw89/core.c
> >> >> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> >> >> @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >> >>  {
> >> >>  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >> >>  	struct ieee80211_sta *sta = txq->sta;
> >> >> -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> >
> >> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> >> > data right here, so I think this is safe. More, compiler can optimize
> >> > this instruction that reorder it to the place just right before using.
> >> > So, it seems like a false alarm.
> >> >
> >> >> +	struct rtw89_sta *rtwsta;
> >> >>
> >> >> -	if (!sta || rtwsta->max_agg_wait <= 0)
> >> >> +	if (!sta)
> >> >> +		return false;
> >> >> +	rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >> >> +	if (!rtwsta)
> >> >> +		return false;
> >> >> +	if (rtwsta->max_agg_wait <= 0)
> >> >>  		return false;
> >> >>
> >> >>  	if (rtwdev->stats.tx_tfc_lv <= RTW89_TFC_MID)
> >> >
> >> > I check the size of object files before/after this patch, and
> >> > the original one is smaller.
> >> >
> >> >    text    data     bss     dec     hex filename
> >> >   16781    3392       1   20174    4ece core-0.o  // original
> >> >   16819    3392       1   20212    4ef4 core-1.o  // after this patch
> >> >
> >> > Do you think it is worth to apply this patch?
> >>
> >> I think that we should apply the patch. Even though the compiler _may_
> >> reorder the code, it might choose not to do that.
> >
> > Understand.
> >
> > I have another way to fix this coverity warning, like:
> >
> > @@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >  {
> >         struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >         struct ieee80211_sta *sta = txq->sta;
> > -       struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > +       struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
> >
> >         if (!sta || rtwsta->max_agg_wait <= 0)
> >                 return false;
> >
> > Is this acceptable?
> > It has a little redundant checking of 'sta', but the code looks clean.
> 
> I feel that Colin's fix is more readable, but this is just matter of
> taste. You can choose.

I would like my version. 

There are three similar warnings reported by smatch, so I will fix them by
myself. Please drop this patch. 
But, still thank Colin to point out this issue.

--
Ping-Ke

Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Kalle Valo]

Pkshih <pkshih@realtek.com> writes:

>> >> > I check the size of object files before/after this patch, and
>> >> > the original one is smaller.
>> >> >
>> >> >    text    data     bss     dec     hex filename
>> >> >   16781    3392       1   20174    4ece core-0.o  // original
>> >> >   16819    3392       1   20212    4ef4 core-1.o  // after this patch
>> >> >
>> >> > Do you think it is worth to apply this patch?
>> >>
>> >> I think that we should apply the patch. Even though the compiler _may_
>> >> reorder the code, it might choose not to do that.
>> >
>> > Understand.
>> >
>> > I have another way to fix this coverity warning, like:
>> >
>> > @@ -1617,7 +1617,7 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
>> >  {
>> >         struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
>> >         struct ieee80211_sta *sta = txq->sta;
>> > -       struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
>> > +       struct rtw89_sta *rtwsta = sta ? (struct rtw89_sta *)sta->drv_priv : NULL;
>> >
>> >         if (!sta || rtwsta->max_agg_wait <= 0)
>> >                 return false;
>> >
>> > Is this acceptable?
>> > It has a little redundant checking of 'sta', but the code looks clean.
>> 
>> I feel that Colin's fix is more readable, but this is just matter of
>> taste. You can choose.
>
> I would like my version. 
>
> There are three similar warnings reported by smatch, so I will fix them by
> myself. Please drop this patch.

Ok, dropped.

> But, still thank Colin to point out this issue.

Indeed, thanks Colin. A good way to thank is to add Reported-by to the
commit log.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Dan Carpenter]

On Mon, Oct 18, 2021 at 03:35:28AM +0000, Pkshih wrote:
> 
> > -----Original Message-----
> > From: Colin King <colin.king@canonical.com>
> > Sent: Friday, October 15, 2021 11:46 PM
> > To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> > <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
> > netdev@vger.kernel.org
> > Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> > 
> > From: Colin Ian King <colin.king@canonical.com>
> > 
> > The pointer rtwsta is dereferencing pointer sta before sta is
> > being null checked, so there is a potential null pointer deference
> > issue that may occur. Fix this by only assigning rtwsta after sta
> > has been null checked. Add in a null pointer check on rtwsta before
> > dereferencing it too.
> > 
> > Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> > Addresses-Coverity: ("Dereference before null check")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > b/drivers/net/wireless/realtek/rtw89/core.c
> > index 06fb6e5b1b37..26f52a25f545 100644
> > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> >  {
> >  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> >  	struct ieee80211_sta *sta = txq->sta;
> > -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> 
> 'sta->drv_priv' is only a pointer, we don't really dereference the
> data right here, so I think this is safe. More, compiler can optimize
> this instruction that reorder it to the place just right before using.
> So, it seems like a false alarm.

The warning is about "sta" not "sta->priv".  It's not a false positive.

I have heard discussions about compilers trying to work around these
bugs by re-ordering the code.  Is that an option in GCC?  It's not
something we should rely on, but I'm just curious if it exists in
released versions.

regards,
dan carpenter

RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Pkshih]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Tuesday, November 2, 2021 9:15 PM
> To: Pkshih <pkshih@realtek.com>
> Cc: Colin King <colin.king@canonical.com>; Kalle Valo <kvalo@codeaurora.org>; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; linux-wireless@vger.kernel.org;
> netdev@vger.kernel.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> 
> On Mon, Oct 18, 2021 at 03:35:28AM +0000, Pkshih wrote:
> >
> > > -----Original Message-----
> > > From: Colin King <colin.king@canonical.com>
> > > Sent: Friday, October 15, 2021 11:46 PM
> > > To: Kalle Valo <kvalo@codeaurora.org>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
> > > <kuba@kernel.org>; Pkshih <pkshih@realtek.com>; linux-wireless@vger.kernel.org;
> > > netdev@vger.kernel.org
> > > Cc: kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> > >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > The pointer rtwsta is dereferencing pointer sta before sta is
> > > being null checked, so there is a potential null pointer deference
> > > issue that may occur. Fix this by only assigning rtwsta after sta
> > > has been null checked. Add in a null pointer check on rtwsta before
> > > dereferencing it too.
> > >
> > > Fixes: e3ec7017f6a2 ("rtw89: add Realtek 802.11ax driver")
> > > Addresses-Coverity: ("Dereference before null check")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > index 06fb6e5b1b37..26f52a25f545 100644
> > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > >  {
> > >  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > >  	struct ieee80211_sta *sta = txq->sta;
> > > -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> >
> > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > data right here, so I think this is safe. More, compiler can optimize
> > this instruction that reorder it to the place just right before using.
> > So, it seems like a false alarm.
> 
> The warning is about "sta" not "sta->priv".  It's not a false positive.
> 
> I have heard discussions about compilers trying to work around these
> bugs by re-ordering the code.  Is that an option in GCC?  It's not
> something we should rely on, but I'm just curious if it exists in
> released versions.
> 

I say GCC does "reorder" the code, because the object codes of following
two codes are identical with default or -Os ccflags.
If I misuse the term, please correct me.

Code-1:
	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;

	if (!sta)
		return false;

	if (rtwsta->max_agg_wait <= 0)
		return false;

Code-2:
	struct rtw89_sta *rtwsta;

	if (!sta)
		return false;

	rtwsta = (struct rtw89_sta *)sta->drv_priv;
	if (rtwsta->max_agg_wait <= 0)
		return false;


The code-1 is the original code Coverity and smatch warn use-before-check.
The code-2 can avoid this warning without doubt.

To be clear, I have sent a patch to fix this.

--
Ping-Ke

Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Dan Carpenter]

On Wed, Nov 03, 2021 at 12:36:17AM +0000, Pkshih wrote:

> > > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > > index 06fb6e5b1b37..26f52a25f545 100644
> > > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > > >  {
> > > >  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > > >  	struct ieee80211_sta *sta = txq->sta;
> > > > -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > >
> > > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > > data right here, so I think this is safe. More, compiler can optimize
> > > this instruction that reorder it to the place just right before using.
> > > So, it seems like a false alarm.
> > 
> > The warning is about "sta" not "sta->priv".  It's not a false positive.
> > 
> > I have heard discussions about compilers trying to work around these
> > bugs by re-ordering the code.  Is that an option in GCC?  It's not
> > something we should rely on, but I'm just curious if it exists in
> > released versions.
> > 
> 
> I say GCC does "reorder" the code, because the object codes of following
> two codes are identical with default or -Os ccflags.

Huh...  That's cool.  GCC doesn't re-order it for me, but I'm on GCC 8
so maybe it will work when I get to a more modern version.

regards,
dan carpenter

RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta

[Pkshih]

> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: Wednesday, November 3, 2021 6:21 PM
> To: Pkshih <pkshih@realtek.com>
> Cc: Colin King <colin.king@canonical.com>; Kalle Valo <kvalo@codeaurora.org>; David S . Miller
> <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; linux-wireless@vger.kernel.org;
> netdev@vger.kernel.org; kernel-janitors@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
> 
> On Wed, Nov 03, 2021 at 12:36:17AM +0000, Pkshih wrote:
> 
> > > > > diff --git a/drivers/net/wireless/realtek/rtw89/core.c
> > > > > b/drivers/net/wireless/realtek/rtw89/core.c
> > > > > index 06fb6e5b1b37..26f52a25f545 100644
> > > > > --- a/drivers/net/wireless/realtek/rtw89/core.c
> > > > > +++ b/drivers/net/wireless/realtek/rtw89/core.c
> > > > > @@ -1534,9 +1534,14 @@ static bool rtw89_core_txq_agg_wait(struct rtw89_dev *rtwdev,
> > > > >  {
> > > > >  	struct rtw89_txq *rtwtxq = (struct rtw89_txq *)txq->drv_priv;
> > > > >  	struct ieee80211_sta *sta = txq->sta;
> > > > > -	struct rtw89_sta *rtwsta = (struct rtw89_sta *)sta->drv_priv;
> > > >
> > > > 'sta->drv_priv' is only a pointer, we don't really dereference the
> > > > data right here, so I think this is safe. More, compiler can optimize
> > > > this instruction that reorder it to the place just right before using.
> > > > So, it seems like a false alarm.
> > >
> > > The warning is about "sta" not "sta->priv".  It's not a false positive.
> > >
> > > I have heard discussions about compilers trying to work around these
> > > bugs by re-ordering the code.  Is that an option in GCC?  It's not
> > > something we should rely on, but I'm just curious if it exists in
> > > released versions.
> > >
> >
> > I say GCC does "reorder" the code, because the object codes of following
> > two codes are identical with default or -Os ccflags.
> 
> Huh...  That's cool.  GCC doesn't re-order it for me, but I'm on GCC 8
> so maybe it will work when I get to a more modern version.
> 

My GCC is 9.3.0. 
But, I don't try other versions.

--
Ping-Ke