| Message ID | 20241104172415.3790038-1-ih@simonwunderlich.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: fix mbss changed flags corruption on 32 bit systems | expand |
On Mon, 2024-11-04 at 18:24 +0100, Issam Hamdi wrote: > On 32-bit systems, the size of an unsigned long is 4 bytes, yes > while a u64 is 8 bytes. yes > Therefore, when using > or_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE), > the code is incorrectly searching for a bit in a 32-bit > variable that is expected to be 64 bits in size, > leading to incorrect bit finding. No. > +++ b/net/mac80211/mesh.c > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, You evidently have _hundreds_ of out-of-tree lines, probably some of those cause this bug too. johannes
On Wed, 2024-11-06 at 12:09 +0100, Johannes Berg wrote: > > > +++ b/net/mac80211/mesh.c > > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, > > You evidently have _hundreds_ of out-of-tree lines, probably some of > those cause this bug too. Ahrg, sorry, no. I take it all back, I was looking at the completely wrong tree by accident. Still this seems like the wrong fix, it would be better to take care of all the 64 bits? johannes
On Wed, 2024-11-06 at 12:11 +0100, Johannes Berg wrote: > On Wed, 2024-11-06 at 12:09 +0100, Johannes Berg wrote: > > > > > +++ b/net/mac80211/mesh.c > > > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, > > > > You evidently have _hundreds_ of out-of-tree lines, probably some of > > those cause this bug too. > > Ahrg, sorry, no. I take it all back, I was looking at the completely > wrong tree by accident. > > Still this seems like the wrong fix, it would be better to take care of > all the 64 bits? > Also, a Fixes: tag would be nice. johannes
Issam Hamdi <ih@simonwunderlich.de> wrote: > diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c > index cb5f16366b9c..39cdbc11f540 100644 > --- a/net/mac80211/mesh.c > +++ b/net/mac80211/mesh.c > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, > return; > > /* if we race with running work, worst case this work becomes a noop */ > - for_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE) > + for_each_set_bit(bit, &bits, sizeof(bits) * BITS_PER_BYTE) > set_bit(bit, ifmsh->mbss_changed); > set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags); > wiphy_work_queue(sdata->local->hw.wiphy, &sdata->work); The ifmsh->mbss_changed is defined as: unsigned long mbss_changed[64 / BITS_PER_LONG]; It seems like loop of for_each_set_bit() want to copy each bit of changed (u64). When shrink traversal size of for_each_set_bit() from sizeof(changed) to sizeof(bits), upper 32 bits of changed will not be copied to ifmsh->mbss_changed. Will it be a problem?
Ping-Ke Shih <pkshih@realtek.com> wrote: > > Issam Hamdi <ih@simonwunderlich.de> wrote: > > diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index > > cb5f16366b9c..39cdbc11f540 100644 > > --- a/net/mac80211/mesh.c > > +++ b/net/mac80211/mesh.c > > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct > ieee80211_sub_if_data *sdata, > > return; > > > > /* if we race with running work, worst case this work becomes a noop */ > > - for_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE) > > + for_each_set_bit(bit, &bits, sizeof(bits) * BITS_PER_BYTE) > > set_bit(bit, ifmsh->mbss_changed); > > set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags); > > wiphy_work_queue(sdata->local->hw.wiphy, &sdata->work); > > The ifmsh->mbss_changed is defined as: > unsigned long mbss_changed[64 / BITS_PER_LONG]; > > It seems like loop of for_each_set_bit() want to copy each bit of changed (u64). > When shrink traversal size of for_each_set_bit() from sizeof(changed) to sizeof(bits), upper 32 > bits of changed will not be copied to ifmsh->mbss_changed. > Will it be a problem? > On 32-bit system, the upper 32 bits seem already lost when "unsigned long bits = changed". (no matter what the traversal size it is) IIUC, this patch is going to prevent traversal of "bits" from getting out-of-bound. But perhaps, "unsigned long bits[] = { BITMAP_FROM_U64(changed) }" would be better. Then, traversal size can keep as before.
Zong-Zhe Yang <kevin_yang@realtek.com> wrote: > Ping-Ke Shih <pkshih@realtek.com> wrote: > > > > Issam Hamdi <ih@simonwunderlich.de> wrote: > > > diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index > > > cb5f16366b9c..39cdbc11f540 100644 > > > --- a/net/mac80211/mesh.c > > > +++ b/net/mac80211/mesh.c > > > @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct > > ieee80211_sub_if_data *sdata, > > > return; > > > > > > /* if we race with running work, worst case this work becomes a noop */ > > > - for_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE) > > > + for_each_set_bit(bit, &bits, sizeof(bits) * BITS_PER_BYTE) > > > set_bit(bit, ifmsh->mbss_changed); > > > set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags); > > > wiphy_work_queue(sdata->local->hw.wiphy, &sdata->work); > > > > The ifmsh->mbss_changed is defined as: > > unsigned long mbss_changed[64 / BITS_PER_LONG]; > > > > It seems like loop of for_each_set_bit() want to copy each bit of changed (u64). > > When shrink traversal size of for_each_set_bit() from sizeof(changed) to sizeof(bits), upper 32 > > bits of changed will not be copied to ifmsh->mbss_changed. > > Will it be a problem? > > > > On 32-bit system, the upper 32 bits seem already lost when "unsigned long bits = changed". (no matter what > the traversal size it is) > IIUC, this patch is going to prevent traversal of "bits" from getting out-of-bound. > > But perhaps, "unsigned long bits[] = { BITMAP_FROM_U64(changed) }" would be better. > Then, traversal size can keep as before. BITMAP_FROM_U64() looks like a good idea.
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c index cb5f16366b9c..39cdbc11f540 100644 --- a/net/mac80211/mesh.c +++ b/net/mac80211/mesh.c @@ -1164,7 +1164,7 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata, return; /* if we race with running work, worst case this work becomes a noop */ - for_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE) + for_each_set_bit(bit, &bits, sizeof(bits) * BITS_PER_BYTE) set_bit(bit, ifmsh->mbss_changed); set_bit(MESH_WORK_MBSS_CHANGED, &ifmsh->wrkq_flags); wiphy_work_queue(sdata->local->hw.wiphy, &sdata->work);
On 32-bit systems, the size of an unsigned long is 4 bytes, while a u64 is 8 bytes. Therefore, when using or_each_set_bit(bit, &bits, sizeof(changed) * BITS_PER_BYTE), the code is incorrectly searching for a bit in a 32-bit variable that is expected to be 64 bits in size, leading to incorrect bit finding. Solution: Ensure that the size of the bits variable is correctly adjusted for each architecture. Call Trace: ? show_regs+0x54/0x58 ? __warn+0x6b/0xd4 ? ieee80211_link_info_change_notify+0xcc/0xd4 [mac80211] ? report_bug+0x113/0x150 ? exc_overflow+0x30/0x30 ? handle_bug+0x27/0x44 ? exc_invalid_op+0x18/0x50 ? handle_exception+0xf6/0xf6 ? exc_overflow+0x30/0x30 ? ieee80211_link_info_change_notify+0xcc/0xd4 [mac80211] ? exc_overflow+0x30/0x30 ? ieee80211_link_info_change_notify+0xcc/0xd4 [mac80211] ? ieee80211_mesh_work+0xff/0x260 [mac80211] ? cfg80211_wiphy_work+0x72/0x98 [cfg80211] ? process_one_work+0xf1/0x1fc ? worker_thread+0x2c0/0x3b4 ? kthread+0xc7/0xf0 ? mod_delayed_work_on+0x4c/0x4c ? kthread_complete_and_exit+0x14/0x14 ? ret_from_fork+0x24/0x38 ? kthread_complete_and_exit+0x14/0x14 ? ret_from_fork_asm+0xf/0x14 ? entry_INT80_32+0xf0/0xf0 Reported-by: Kretschmer Mathias <mathias.kretschmer@fit.fraunhofer.de> Signed-off-by: Issam Hamdi <ih@simonwunderlich.de> --- net/mac80211/mesh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) base-commit: 2b94751626a6d49bbe42a19cc1503bd391016bd5