| Message ID | 20211015154530.34356-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [next] rtw89: Fix potential dereference of the null pointer sta | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> -----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
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.
> -----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
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) :)
> -----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
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.
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
> -----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
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
> -----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
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)