Date:   Mon, 20 Mar 2023 17:49:08 +0100
From:   Simon Horman <simon.horman@corigine.com>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>,
        Jia-Ju Bai <baijiaju@buaa.edu.cn>
Cc:     johannes@sipsolutions.net, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: mac80211: Add NULL checks for sta->sdata
Message-ID: <ZBiOhAJswYcAo8kv@corigine.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230320133809.2448047-1-baijiaju@buaa.edu.cn>
 <20230320133601.2443821-1-baijiaju1990@gmail.com>
 <20230320133644.2445321-1-baijiaju1990@gmail.com>
 <20230320133533.2442889-1-baijiaju1990@gmail.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?Ij+JXBpRgs1M/smaYRoianOK8vcMBO4GUyMH0qoHiMQISme8wrru5BnFtl8w?=
 =?us-ascii?Q?JJtK5j0uW3x2J3DAa641QAVqtQEqFJYVg6Z0s4aPr22WwzVD1dEfcvsFPsiv?=
 =?us-ascii?Q?M5gqz5jOLvlIDmPzN4oev5crjTym3AXTlsxQNW3zWScjvoCZdIGWs2EP526F?=
 =?us-ascii?Q?LREyCeavy6M/XeJdq+qU6bMnK4cXZlMkz+TjkZ4H5xYKwAXGKfhbcAYT3rW2?=
 =?us-ascii?Q?/hYlJeWeF+pLae4QstqLosl0jseR+oRKJrw7QUaq51VJ8X4bb+wMmgR1izKo?=
 =?us-ascii?Q?M1WA+odHVaKTeTy2HDyRH9qgBOg9qB/oicQjgRg/PZGBEIr1HJxiMNP8gBj5?=
 =?us-ascii?Q?4o1xONbTrmRFqrYv8O0fy/x2b01DPZ3ocFDjJBwTBcTwt6/54PzT4Okx3anC?=
 =?us-ascii?Q?yTlPdFSjI+WyQgtd3SXC06w2LrOVNOsb99JAacTcgZOjh+bHH7Zbri7VcHK9?=
 =?us-ascii?Q?6xdzaoIidjOIJl8/hXu4JMfucIxxt/uWOHNeIaiy1L1Z2ACwS/vJ64HzVpcn?=
 =?us-ascii?Q?UHCMfjw2zQFr+4+fhIiAgkfFEQUZzW9S2ozzBG5aRdlvLd7Do1yP4pEg7rCC?=
 =?us-ascii?Q?au/iOG4e3+W1ek6YJDFF0scv5dHnE2x+1txfFn8J1dJWzGHtZnfTxO8X6Phc?=
 =?us-ascii?Q?EbR3+a7DoNuMOcsZ8xeCB3ILyELADPxapkYDRiedrPaUHUIpV9km/7zRuUdS?=
 =?us-ascii?Q?IFM65gEjuRgS8C4tSeYLVrRB4Hffr2SoYc7b8Af+iQ1p5RL7Vi1zuKRZO/b6?=
 =?us-ascii?Q?6E5U2P9YprHz13ug8GB/AIaC/isLQkX7jqnJ3lc9c6rwe+ksI8gSVVnJsWeF?=
 =?us-ascii?Q?AJADQB2Jc9MlxBcLV8r5zj/lyjfqrzzv8D2KYMQJAZ1i2hDDL215h76a05f/?=
 =?us-ascii?Q?vLyW4Gx0LPK4trqsz6bQ9vnOyGG/IQttTVHAZV3sJtbd6T7wlemEIH5Pqhfw?=
 =?us-ascii?Q?hVOTV5doWclcCndZm6Bp2GKWKeRRZd9xI36hdHy5PHLHGejx51z5Sa7HzjqX?=
 =?us-ascii?Q?BLlGJQoXMg4QXbqLjJOSMWjAplF7k5HwyeDw4VAtMiXr2PwKDdHo/csppZvw?=
 =?us-ascii?Q?aIkSfvc+MtBLNS8DS8J4nBcqO3RhHV0uSdWJ0hQOQ5u/FkmXBs1Aq9TWwE/o?=
 =?us-ascii?Q?oQtJPMlHUA7c1A4dYexc0Q/+mmlmSW6tWErDbLQl39ss5osEuq0sIQgDHMNc?=
 =?us-ascii?Q?Tx+wGlUbw236XU8ymspwfAfhhFlVNcQSwKwS3TnuHqi6jW75IAkh49kp81Bf?=
 =?us-ascii?Q?xnlIrm9mL2gugIkM/oAhqAQIRGdkElS8x81UjKL0lXzVNPzlJ0PaIMRSLXgQ?=
 =?us-ascii?Q?Zgd77t81pMPuf+2sPA/yFS4oNfK1NOtcghfEKFJww2HQu64U8/WTBNChJbEl?=
 =?us-ascii?Q?IId/7k2iyrdrHVDgcT+kxUQq/+fUDbWF3va8VaXQUYHg86lV3M1sFvKHbdrj?=
 =?us-ascii?Q?ppm9wS+UubMgrnd/MXH/6Ohns5FjSpwyw9OIimQtJcwZgOho2z659gf9+rJ2?=
 =?us-ascii?Q?5rPw39gPPpDav4gtRajsei1Fci2s9X4BKZnV0rFEaIR7eCd05UxJ3BuFkC5d?=
 =?us-ascii?Q?yBWfQ+J4eccPn0FoZ9GCSG+6Yon1ljgg/IHvBDxZYe7oDRZp37l+5Jrh1p0p?=
 =?us-ascii?Q?V/KKj3iFdFIEKjupinNwgKdA6Dq5GohD5qrdlIu9doVV2tly96Xz36FIsk50?=
 =?us-ascii?Q?N0kzow=3D=3D?=
X-OriginatorOrg: corigine.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 10baa27f-eb4b-4cbf-eff8-08db296310fb
X-MS-Exchange-CrossTenant-AuthSource: PH0PR13MB4842.namprd13.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2023 16:49:27.3066
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: fe128f2c-073b-4c20-818e-7246a585940c
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /taM2GhzdJp6e6ZWebV5r9cy/XrjT+SBx63gQv+Rt3XbImI1VpklU+NKH1yR4Y1V+0P+f6Pa34Sf50kICNp3jq5FjxBZakud67AqJCumESA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR13MB5543
Precedence: bulk
List-ID: <linux-wireless.vger.kernel.org>
X-Mailing-List: linux-wireless@vger.kernel.org

On Mon, Mar 20, 2023 at 09:35:33PM +0800, Jia-Ju Bai wrote:
> In a previous commit 69403bad97aa, sta->sdata can be NULL, and thus it
> should be checked before being used.

Please run checkpatch on this patch, and correct the commit description
style.

./scripts/checkpatch.pl -g HEAD
ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 69403bad97aa ("wifi: mac80211: sdata can be NULL during AMPDU start")'
#6:
In a previous commit 69403bad97aa, sta->sdata can be NULL, and thus it

> 
> However, in the same call stack, sta->sdata is also used in the
> following functions:
> 
> ieee80211_ba_session_work()
>   ___ieee80211_stop_rx_ba_session(sta)
>     ht_dbg(sta->sdata, ...); -> No check
>     sdata_info(sta->sdata, ...); -> No check
>     ieee80211_send_delba(sta->sdata, ...) -> No check
>   ___ieee80211_start_rx_ba_session(sta)
>     ht_dbg(sta->sdata, ...); -> No check
>     ht_dbg_ratelimited(sta->sdata, ...); -> No check
>   ieee80211_tx_ba_session_handle_start(sta)
>     sdata = sta->sdata; if (!sdata) -> Add check by previous commit
>   ___ieee80211_stop_tx_ba_session(sdata)
>     ht_dbg(sta->sdata, ...); -> No check
>   ieee80211_start_tx_ba_cb(sdata)
>     sdata = sta->sdata; local = sdata->local -> No check
>   ieee80211_stop_tx_ba_cb(sdata)
>     ht_dbg(sta->sdata, ...); -> No check

I wonder if it would be better to teach ht_* do do nothing
if the first argument is NULL.

Also, are these theoretical bugs?
Or something that has been observed?
And has a reproducer?

> Thus, to avoid possible null-pointer dereferences, the related checks
> should be added.
> 
> These results are reported by a static tool designed by myself.
> 
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> Reported-by: TOTE Robot <baijiaju@buaa.edu.cn>

I see 4 copies of this patch in a few minutes.
As per the FAQ [1], please leave at least 24h between posts of a patch.

[1] https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html