* [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
@ 2021-10-15 15:45 Colin King
2021-10-18 3:35 ` Pkshih
0 siblings, 1 reply; 11+ messages in thread
From: Colin King @ 2021-10-15 15:45 UTC (permalink / raw)
To: Kalle Valo, David S . Miller, Jakub Kicinski, Ping-Ke Shih,
linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-15 15:45 [PATCH][next] rtw89: Fix potential dereference of the null pointer sta Colin King
@ 2021-10-18 3:35 ` Pkshih
2021-10-18 12:11 ` Kalle Valo
2021-11-02 13:14 ` Dan Carpenter
0 siblings, 2 replies; 11+ messages in thread
From: Pkshih @ 2021-10-18 3:35 UTC (permalink / raw)
To: Colin King, Kalle Valo, David S . Miller, Jakub Kicinski,
linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-18 3:35 ` Pkshih
@ 2021-10-18 12:11 ` Kalle Valo
2021-10-19 1:11 ` Pkshih
2021-11-02 13:14 ` Dan Carpenter
1 sibling, 1 reply; 11+ messages in thread
From: Kalle Valo @ 2021-10-18 12:11 UTC (permalink / raw)
To: Pkshih
Cc: Colin King, David S . Miller, Jakub Kicinski, linux-wireless,
netdev, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-18 12:11 ` Kalle Valo
@ 2021-10-19 1:11 ` Pkshih
2021-10-20 8:36 ` Kalle Valo
0 siblings, 1 reply; 11+ messages in thread
From: Pkshih @ 2021-10-19 1:11 UTC (permalink / raw)
To: Kalle Valo
Cc: Colin King, David S . Miller, Jakub Kicinski, linux-wireless,
netdev, kernel-janitors, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-19 1:11 ` Pkshih
@ 2021-10-20 8:36 ` Kalle Valo
2021-10-21 5:46 ` Pkshih
0 siblings, 1 reply; 11+ messages in thread
From: Kalle Valo @ 2021-10-20 8:36 UTC (permalink / raw)
To: Pkshih
Cc: Colin King, David S . Miller, Jakub Kicinski, linux-wireless,
netdev, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-20 8:36 ` Kalle Valo
@ 2021-10-21 5:46 ` Pkshih
2021-10-21 8:48 ` Kalle Valo
0 siblings, 1 reply; 11+ messages in thread
From: Pkshih @ 2021-10-21 5:46 UTC (permalink / raw)
To: Kalle Valo
Cc: Colin King, David S . Miller, Jakub Kicinski, linux-wireless,
netdev, kernel-janitors, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-21 5:46 ` Pkshih
@ 2021-10-21 8:48 ` Kalle Valo
0 siblings, 0 replies; 11+ messages in thread
From: Kalle Valo @ 2021-10-21 8:48 UTC (permalink / raw)
To: Pkshih
Cc: Colin King, David S . Miller, Jakub Kicinski, linux-wireless,
netdev, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-10-18 3:35 ` Pkshih
2021-10-18 12:11 ` Kalle Valo
@ 2021-11-02 13:14 ` Dan Carpenter
2021-11-03 0:36 ` Pkshih
1 sibling, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2021-11-02 13:14 UTC (permalink / raw)
To: Pkshih
Cc: Colin King, Kalle Valo, David S . Miller, Jakub Kicinski,
linux-wireless, netdev, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-11-02 13:14 ` Dan Carpenter
@ 2021-11-03 0:36 ` Pkshih
2021-11-03 10:21 ` Dan Carpenter
0 siblings, 1 reply; 11+ messages in thread
From: Pkshih @ 2021-11-03 0:36 UTC (permalink / raw)
To: Dan Carpenter
Cc: Colin King, Kalle Valo, David S . Miller, Jakub Kicinski,
linux-wireless, netdev, kernel-janitors, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-11-03 0:36 ` Pkshih
@ 2021-11-03 10:21 ` Dan Carpenter
2021-11-04 1:38 ` Pkshih
0 siblings, 1 reply; 11+ messages in thread
From: Dan Carpenter @ 2021-11-03 10:21 UTC (permalink / raw)
To: Pkshih
Cc: Colin King, Kalle Valo, David S . Miller, Jakub Kicinski,
linux-wireless, netdev, kernel-janitors, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH][next] rtw89: Fix potential dereference of the null pointer sta
2021-11-03 10:21 ` Dan Carpenter
@ 2021-11-04 1:38 ` Pkshih
0 siblings, 0 replies; 11+ messages in thread
From: Pkshih @ 2021-11-04 1:38 UTC (permalink / raw)
To: Dan Carpenter
Cc: Colin King, Kalle Valo, David S . Miller, Jakub Kicinski,
linux-wireless, netdev, kernel-janitors, linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-04 1:39 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15 15:45 [PATCH][next] rtw89: Fix potential dereference of the null pointer sta Colin King
2021-10-18 3:35 ` Pkshih
2021-10-18 12:11 ` Kalle Valo
2021-10-19 1:11 ` Pkshih
2021-10-20 8:36 ` Kalle Valo
2021-10-21 5:46 ` Pkshih
2021-10-21 8:48 ` Kalle Valo
2021-11-02 13:14 ` Dan Carpenter
2021-11-03 0:36 ` Pkshih
2021-11-03 10:21 ` Dan Carpenter
2021-11-04 1:38 ` Pkshih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).