wifi: mac80211: fix mbss changed flags corruption on 32 bit systems

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

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.

On Mon, 2024-11-18 at 13:26 +0000, James Dutton wrote:
> 
> > @@ -1157,14 +1157,14 @@ void ieee80211_mbss_info_change_notify(struct ieee80211_sub_if_data *sdata,
> >                                        u64 changed)
> >  {
> >         struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
> > -       unsigned long bits = changed;
> > +       unsigned long bits[] = { BITMAP_FROM_U64(changed) };
> 
> Wouldn't it be easier to use this instead:
> -       unsigned long bits = changed;
> +       u64 bits = changed;

No, that's incorrect for set_bit() etc. at least on 32-bit big-endian
systems. Then you can't use for_each_set_bit() etc.

johannes