From: Colin King <hidden> Date: 2021-10-15 15:45:35
From: Colin Ian King <redacted>
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 <redacted>
---
drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
-----Original Message-----
From: Colin King <redacted>
Sent: Friday, October 15, 2021 11:46 PM
To: Kalle Valo <redacted>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
[off-list ref]; Pkshih [off-list ref]; 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 <redacted>
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 <redacted>
---
drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
'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
From: Dan Carpenter <hidden> Date: 2021-11-02 13:15:12
On Mon, Oct 18, 2021 at 03:35:28AM +0000, Pkshih wrote:
quoted
-----Original Message-----
From: Colin King <redacted>
Sent: Friday, October 15, 2021 11:46 PM
To: Kalle Valo <redacted>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
[off-list ref]; Pkshih [off-list ref]; 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 <redacted>
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 <redacted>
---
drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
'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
-----Original Message-----
From: Dan Carpenter <redacted>
Sent: Tuesday, November 2, 2021 9:15 PM
To: Pkshih <pkshih@realtek.com>
Cc: Colin King <redacted>; Kalle Valo <redacted>; David S . Miller
[off-list ref]; Jakub Kicinski [off-list ref]; 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:
quoted
quoted
-----Original Message-----
From: Colin King <redacted>
Sent: Friday, October 15, 2021 11:46 PM
To: Kalle Valo <redacted>; David S . Miller <davem@davemloft.net>; Jakub Kicinski
[off-list ref]; Pkshih [off-list ref]; 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 <redacted>
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 <redacted>
---
drivers/net/wireless/realtek/rtw89/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
'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
'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
-----Original Message-----
From: Dan Carpenter <redacted>
Sent: Wednesday, November 3, 2021 6:21 PM
To: Pkshih <pkshih@realtek.com>
Cc: Colin King <redacted>; Kalle Valo <redacted>; David S . Miller
[off-list ref]; Jakub Kicinski [off-list ref]; 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:
'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