| Message ID | tencent_3D3535F93CE8BC48A50E29D1CF7A25E93D0A@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] mac802154: Fix potential RCU dereference issue in mac802154_scan_worker | expand |
Hi Jiawei, jiawei.ye@foxmail.com wrote on Fri, 20 Sep 2024 04:03:32 +0000: > In the `mac802154_scan_worker` function, the `scan_req->type` field was > accessed after the RCU read-side critical section was unlocked. According > to RCU usage rules, this is illegal and can lead to unpredictable > behavior, such as accessing memory that has been updated or causing > use-after-free issues. > > This possible bug was identified using a static analysis tool developed > by myself, specifically designed to detect RCU-related issues. > > To address this, the `scan_req->type` value is now stored in a local > variable `scan_req_type` while still within the RCU read-side critical > section. The `scan_req_type` is then used after the RCU lock is released, > ensuring that the type value is safely accessed without violating RCU > rules. > > Fixes: e2c3e6f53a7a ("mac802154: Handle active scanning") > Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com> I think net maintainers now expect the Cc: stable tag to be put here when there is a reason to backport, which I believe is the case here. So please add this line here. > Please delete this blank line as well. And with that you can add my: Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- Thanks, Miquèl
Hi Miquèl, I'm sorry for the accidental email sent while testing my email setup. Please disregard the previous message. On 9/20/24 19:21, Miquel Raynal wrote: > Hi Jiawei, > > jiawei.ye@foxmail.com wrote on Fri, 20 Sep 2024 04:03:32 +0000: > > > In the `mac802154_scan_worker` function, the `scan_req->type` field was > > accessed after the RCU read-side critical section was unlocked. According > > to RCU usage rules, this is illegal and can lead to unpredictable > > behavior, such as accessing memory that has been updated or causing > > use-after-free issues. > > > > This possible bug was identified using a static analysis tool developed > > by myself, specifically designed to detect RCU-related issues. > > > > To address this, the `scan_req->type` value is now stored in a local > > variable `scan_req_type` while still within the RCU read-side critical > > section. The `scan_req_type` is then used after the RCU lock is released, > > ensuring that the type value is safely accessed without violating RCU > > rules. > > > > Fixes: e2c3e6f53a7a ("mac802154: Handle active scanning") > > Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com> > > I think net maintainers now expect the Cc: stable tag to be put here > when there is a reason to backport, which I believe is the case here. > So please add this line here. > Does this mean I should use Cc: stable@vger.kernel.org? I am not familiar with this procedure. > > > > Please delete this blank line as well. > > And with that you can add my: > > Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > Thanks, > Miquèl Do I need to resend patch v2 with the "Resend" label? Thank you for your assistance. Best regards, Jiawei Ye
Hello. On 9/21/24 07:22, Jiawei Ye wrote: > Hi Miquèl, > > I'm sorry for the accidental email sent while testing my email setup. Please disregard the previous message. > > On 9/20/24 19:21, Miquel Raynal wrote: >> Hi Jiawei, >> >> jiawei.ye@foxmail.com wrote on Fri, 20 Sep 2024 04:03:32 +0000: >> >>> In the `mac802154_scan_worker` function, the `scan_req->type` field was >>> accessed after the RCU read-side critical section was unlocked. According >>> to RCU usage rules, this is illegal and can lead to unpredictable >>> behavior, such as accessing memory that has been updated or causing >>> use-after-free issues. >>> >>> This possible bug was identified using a static analysis tool developed >>> by myself, specifically designed to detect RCU-related issues. >>> >>> To address this, the `scan_req->type` value is now stored in a local >>> variable `scan_req_type` while still within the RCU read-side critical >>> section. The `scan_req_type` is then used after the RCU lock is released, >>> ensuring that the type value is safely accessed without violating RCU >>> rules. >>> >>> Fixes: e2c3e6f53a7a ("mac802154: Handle active scanning") >>> Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com> >> >> I think net maintainers now expect the Cc: stable tag to be put here >> when there is a reason to backport, which I believe is the case here. >> So please add this line here. >> > > Does this mean I should use Cc: stable@vger.kernel.org? I am not familiar with this procedure. Yes. >>> >> >> Please delete this blank line as well. >> >> And with that you can add my: >> >> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> >> >>> --- >> >> Thanks, >> Miquèl > > Do I need to resend patch v2 with the "Resend" label? Please make it a v3 right away, the resend with changes is to confusing for maintainers to keep up what to apply. In your case remove the blank line, add the stable cc and I am happy to apply this. regards Stefan Schmidt
diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c index 1c0eeaa76560..a6dab3cc3ad8 100644 --- a/net/mac802154/scan.c +++ b/net/mac802154/scan.c @@ -176,6 +176,7 @@ void mac802154_scan_worker(struct work_struct *work) struct ieee802154_local *local = container_of(work, struct ieee802154_local, scan_work.work); struct cfg802154_scan_request *scan_req; + enum nl802154_scan_types scan_req_type; struct ieee802154_sub_if_data *sdata; unsigned int scan_duration = 0; struct wpan_phy *wpan_phy; @@ -209,6 +210,7 @@ void mac802154_scan_worker(struct work_struct *work) } wpan_phy = scan_req->wpan_phy; + scan_req_type = scan_req->type; scan_req_duration = scan_req->duration; /* Look for the next valid chan */ @@ -246,7 +248,7 @@ void mac802154_scan_worker(struct work_struct *work) goto end_scan; } - if (scan_req->type == NL802154_SCAN_ACTIVE) { + if (scan_req_type == NL802154_SCAN_ACTIVE) { ret = mac802154_transmit_beacon_req(local, sdata); if (ret) dev_err(&sdata->dev->dev,
In the `mac802154_scan_worker` function, the `scan_req->type` field was accessed after the RCU read-side critical section was unlocked. According to RCU usage rules, this is illegal and can lead to unpredictable behavior, such as accessing memory that has been updated or causing use-after-free issues. This possible bug was identified using a static analysis tool developed by myself, specifically designed to detect RCU-related issues. To address this, the `scan_req->type` value is now stored in a local variable `scan_req_type` while still within the RCU read-side critical section. The `scan_req_type` is then used after the RCU lock is released, ensuring that the type value is safely accessed without violating RCU rules. Fixes: e2c3e6f53a7a ("mac802154: Handle active scanning") Signed-off-by: Jiawei Ye <jiawei.ye@foxmail.com> --- Changelog: v1->v2: -Repositioned the enum nl802154_scan_types scan_req_type declaration between struct cfg802154_scan_request *scan_req and struct ieee802154_sub_if_data *sdata to comply with the reverse Christmas tree rule. --- net/mac802154/scan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)