Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Nov 2021 16:14:37 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
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" <linux-wireless@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "kernel-janitors@vger.kernel.org" <kernel-janitors@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH][next] rtw89: Fix potential dereference of the null
 pointer sta
Message-ID: <20211102131437.GF2794@kadam>
References: <20211015154530.34356-1-colin.king@canonical.com>
 <9cc681c217a449519aee524b35e6b6bc@realtek.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <9cc681c217a449519aee524b35e6b6bc@realtek.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?pGFt3MucKYLE1Xw1yEK+Tt9KThbU1sVUKyhBUNnfq1pAKxisHJhBlC+PFf2W?=
 =?us-ascii?Q?U4lQBVBmEMF307LGU9jkaBSwxxh2y+KdKYXswZBObZsiSfNEerinqi7K5Yy3?=
 =?us-ascii?Q?30u9JS818pp0SOTN2zi7LQ+0+dVmbxPJQqK8LDRRcvglkIZ9hkBmaRIONpbA?=
 =?us-ascii?Q?DMHn4mMVzRdBSnWt6v3tUC3VAPAAwFSFO0fcdIHbjsrSiDbeVYXx67ECPbGn?=
 =?us-ascii?Q?y7dSHk9iJp4QozDOZWGix3s1VKePgCPk/Nxiwb2dYGY8rLSh4fOCnuJ7fRj6?=
 =?us-ascii?Q?Z6XSCRBS2qfk7U7CgMloYva7XxOqYpGYVtZ3xfIEgikTIECD3lDuYHfxp0oY?=
 =?us-ascii?Q?9r3q7eMectyq8cTufLuvTurXN5umEpd23AKgT68EQKbg6dOcDLXupj3F7Yts?=
 =?us-ascii?Q?68hVNpchEAS8DmjGc8QEfuUwFqqFG8CNFOXkC9GWQkxo+9i9QJ9tMbbKoyr4?=
 =?us-ascii?Q?eJ0/CBpgiRDqay0JI0ugAEeDY/uEf1Wja3TssaVoa0eEiNNxlbA2vDpKEVep?=
 =?us-ascii?Q?PAMcFlNhcZbF+71TkphvfRwelbz0J7g8kxbohxb+Sd9u3NzlIWqC1i7aG49E?=
 =?us-ascii?Q?PyseoYx1c8QX1iVE+XkTOqxm+UQiYlBVokX0WKL6gHfuv1aXVxZmAXBR/wzs?=
 =?us-ascii?Q?HLsI9Chtd0iiz70nIp2Bd3HwNkz5xt4cad2gLt6rjJi8AWUUoznzgx1Pdcwb?=
 =?us-ascii?Q?JoIeIX2RWzgySPOtLPO2RKo2yWmrpQKVxoVZE92pPpbf69sAUzf25dWzcgvM?=
 =?us-ascii?Q?BWvFNIR6fYPc3f3bHM6vJYHJyrrtZ/dirH04l/aLBUt3FoVo7ScZkESqUVj6?=
 =?us-ascii?Q?FxIyShv4Fq2SK+N2YpAaCpWwIQvJJkvvUFREo+Nxxd4dEO765J8hMcP8rm/3?=
 =?us-ascii?Q?Plb+uQYSCrExAuajyT3MAL/AENZwU+bTBA1K1w3h1n5VogFB0y+V7rfjxCeN?=
 =?us-ascii?Q?ejtV4YgJ7dbeMqPucCUIZ9UBmW16itNUIRbVd5Fve3ZMjtPewzpOvgIepRZR?=
 =?us-ascii?Q?MWPtNLuoudb2H4ziKVClFWvBsMPlA5mS0nHuMJwU/1daNcsuxmgZkY3gL/Eu?=
 =?us-ascii?Q?mKI7jrqomt8kfr8m1EEGAt1V6cr4qjNSOILi9G/y5RnAA0ItHwQdwIJ5xBME?=
 =?us-ascii?Q?+T1jmYkFeDKu0dPSTVq62sZxCX6xQ9dwXQlsqFXX91Cp1uYOj78OkJYlUgWn?=
 =?us-ascii?Q?RuQ64BB3OKxf/94gVnI3crDvAuFvTeUJrRy8iD+/bUC4QXAzB6ME9jDBfJkv?=
 =?us-ascii?Q?826lsaYvZ76HDKaGJw+9/cyo0oDN3lO/0MDNx0AfyYI/hy+qFhjl9QmtWrwP?=
 =?us-ascii?Q?Aey39Anv+heyI2fs81Ellz/9ZPodGayWh4iQSerhnP0O0aJAkroTl/ThJ2vK?=
 =?us-ascii?Q?+2Rn56aVzSoj93ORc14CSyM2WU8LeJkxBNeq8ak49arnDHUZ+UFaRw1xYpyo?=
 =?us-ascii?Q?dQ/kHW7xVXPmxE0q+M8TgTHOSsgm9cyEJZqL1Gsx/9mZgZcrgFSlCaLSVNpF?=
 =?us-ascii?Q?0NXlmJbuNcWbyTRF0K0RZ/gINjNLGZ186+Tu8nP7iCVVtUEJds9vOjoo1THC?=
 =?us-ascii?Q?Pakahw16NHCCiBUtLVkp1szs4ST/qICSQfLMw4ocqzq/3AQfiLWouXYsmP+Q?=
 =?us-ascii?Q?gOExL9eyJCepGl/7lWH78OioRMBXBA5NKCFBWI4PgSDzX4vXibZy1bMlpFlI?=
 =?us-ascii?Q?lkjOQA=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7e84c056-8847-4eb5-bf5f-08d99e02c4a9
X-MS-Exchange-CrossTenant-AuthSource: MWHPR1001MB2365.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2021 13:14:58.1624
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 6cdua+6liv1Aa7K1Z9zEOOBAn/skmVL2/bZtkPEUN8KMwi1o6zQTS255e/36uBZfqaqoh+R6H2i9LHgegYEdA7qnD33xczDSPOkMVIJAcxs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR10MB1293
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10155 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 phishscore=0 bulkscore=0
 spamscore=0 adultscore=0 suspectscore=0 mlxlogscore=999 mlxscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111020078
X-Proofpoint-GUID: qimgQJgpo6_vgn3B9A9vzJnTn5i56Eim
X-Proofpoint-ORIG-GUID: qimgQJgpo6_vgn3B9A9vzJnTn5i56Eim
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

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