| Message ID | tencent_24586A9E52F56C5C12E9535AD3F243C98B06@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mac802154: Fix potential RCU dereference issue in mac802154_scan_worker | expand |
On 9/19/24 09:16, Jiawei Ye wrote: > 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> > --- > net/mac802154/scan.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c > index 1c0eeaa76560..29cd84c9f69c 100644 > --- a/net/mac802154/scan.c > +++ b/net/mac802154/scan.c > @@ -180,6 +180,7 @@ void mac802154_scan_worker(struct work_struct *work) > unsigned int scan_duration = 0; > struct wpan_phy *wpan_phy; > u8 scan_req_duration; > + enum nl802154_scan_types scan_req_type; this line violates the reverse X-mass tree rule of code formatting > u8 page, channel; > int ret; > > @@ -210,6 +211,7 @@ void mac802154_scan_worker(struct work_struct *work) > > wpan_phy = scan_req->wpan_phy; this line (not yours) just saves the first level of pointer, but then accesses wpan_phy->... outside of the rcu_read_lock() section, for me it's very similar case to what you are fixing here > scan_req_duration = scan_req->duration; > + scan_req_type = scan_req->type; > > /* Look for the next valid chan */ > page = local->scan_page; > @@ -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,
On 9/19/24 17:01, Przemek Kitszel wrote: > > 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> > > --- > > net/mac802154/scan.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c > > index 1c0eeaa76560..29cd84c9f69c 100644 > > --- a/net/mac802154/scan.c > > +++ b/net/mac802154/scan.c > > @@ -180,6 +180,7 @@ void mac802154_scan_worker(struct work_struct *work) > > unsigned int scan_duration = 0; > > struct wpan_phy *wpan_phy; > > u8 scan_req_duration; > > + enum nl802154_scan_types scan_req_type; > > this line violates the reverse X-mass tree rule of code formatting Thank you for pointing out the concern regarding the violation of the reverse Christmas tree rule. I will adjust the placement of `enum nl802154_scan_types scan_req_type` to be between `struct cfg802154_scan_request *scan_req` and `struct ieee802154_sub_if_data *sdata`. If this change is suitable, should I resend the patch as a v2 patch? > > > u8 page, channel; > > int ret; > > > > @@ -210,6 +211,7 @@ void mac802154_scan_worker(struct work_struct *work) > > > > wpan_phy = scan_req->wpan_phy; > > this line (not yours) just saves the first level of pointer, but then > accesses wpan_phy->... outside of the rcu_read_lock() section, for me > it's very similar case to what you are fixing here > According to the RCU usage rules, the value returned by `rcu_dereference()` should be safely dereferenced only within the RCU read-side critical section. It is important to note that `wpan_phy` is not obtained through `rcu_dereference()`, so in this context, it may not be sufficient to infer whether it is protected by RCU. > > scan_req_duration = scan_req->duration; > > + scan_req_type = scan_req->type; > > > > /* Look for the next valid chan */ > > page = local->scan_page; > > @@ -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,
Hello Jiawei, On 9/19/24 14:26, Jiawei Ye wrote: > On 9/19/24 17:01, Przemek Kitszel wrote: >>> 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> >>> --- >>> net/mac802154/scan.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c >>> index 1c0eeaa76560..29cd84c9f69c 100644 >>> --- a/net/mac802154/scan.c >>> +++ b/net/mac802154/scan.c >>> @@ -180,6 +180,7 @@ void mac802154_scan_worker(struct work_struct *work) >>> unsigned int scan_duration = 0; >>> struct wpan_phy *wpan_phy; >>> u8 scan_req_duration; >>> + enum nl802154_scan_types scan_req_type; >> >> this line violates the reverse X-mass tree rule of code formatting > > Thank you for pointing out the concern regarding the violation of the > reverse Christmas tree rule. I will adjust the placement of > `enum nl802154_scan_types scan_req_type` to be between > `struct cfg802154_scan_request *scan_req` and > `struct ieee802154_sub_if_data *sdata`. If this change is suitable, > should I resend the patch as a v2 patch? Yes, please always increase the version whenever you change something and re-send. Also a ChangeLog of the changes makes it a lot easier for the reviewer. regards Stefan Schmidt
diff --git a/net/mac802154/scan.c b/net/mac802154/scan.c index 1c0eeaa76560..29cd84c9f69c 100644 --- a/net/mac802154/scan.c +++ b/net/mac802154/scan.c @@ -180,6 +180,7 @@ void mac802154_scan_worker(struct work_struct *work) unsigned int scan_duration = 0; struct wpan_phy *wpan_phy; u8 scan_req_duration; + enum nl802154_scan_types scan_req_type; u8 page, channel; int ret; @@ -210,6 +211,7 @@ void mac802154_scan_worker(struct work_struct *work) wpan_phy = scan_req->wpan_phy; scan_req_duration = scan_req->duration; + scan_req_type = scan_req->type; /* Look for the next valid chan */ page = local->scan_page; @@ -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> --- net/mac802154/scan.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)