Re: ieee80211.h virtual_map splat

Re: ieee80211.h virtual_map splat

[Jeff Johnson]

On 6/21/2024 1:04 AM, Koen Vandeputte wrote:
> Hi all,
> 
> Within OpenWRT, we switched to kernel 6.6 some time ago.
> 
> During testing on a WiFi WDS setup (ath10k), I noticed an old standing
> bug which now prints a lot more data due to the kernel upgrade:
> 
> - All WDS stations are connected
> - The splat occurs
> - All WDS station seem to go in timeout and disconnect
> - The behavior is fixed after a reboot
> 
> Yes, we use ath10k-ct over here, but this part of the code is
> identical to upstream ath10k.
> 
> The main issue:
> 
> memcpy: detected field-spanning write (size 64) of single field
> "tim->virtual_map" at
> ../ath10k-ct-smallbuffers/ath10k-ct-2024.03.02~eb3f488a/ath10k-6.7/wmi.c:4043
> (size 1)
> 
> 
> looks like virtual_map is defined as  "u8 virtual_map[1]", triggering
> that error within "include/linux/ieee80211.h"
> 
> /**
>  * struct ieee80211_tim_ie - Traffic Indication Map information element
>  * @dtim_count: DTIM Count
>  * @dtim_period: DTIM Period
>  * @bitmap_ctrl: Bitmap Control
>  * @virtual_map: Partial Virtual Bitmap
>  *
>  * This structure represents the payload of the "TIM element" as
>  * described in IEEE Std 802.11-2020 section 9.4.2.5.
>  */
> struct ieee80211_tim_ie {
>         u8 dtim_count;
>         u8 dtim_period;
>         u8 bitmap_ctrl;
>         /* variable size: 1 - 251 bytes */
>         u8 virtual_map[1];
> } __packed;
> 
> 
> According to this page, defining it this way is actually deprecated:
> https://www.kernel.org/doc/html/latest/process/deprecated.html
> 
> What is the correct way to fix this?
> Converting it to "u8 virtual_map[];"  ?

Adding netdev to the initial message in the thread.
https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/

There was some discussion in the thread, with the observation that the splat 
is fixed by:
2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")

Followed by discussion if this should be backported.

Kees said that "netdev [...] maintainers have asked that contributors not 
include "Cc: stable" tags, as they want to evaluate for themselves whether 
patches should go to stable or not"

So the purpose of this message is to notify the netdev maintainers that this 
issue has been observed on a LTS kernel with the popular OpenWrt package so 
that the maintainers can make a backporting decision.

/jeff


> [29009.581820] ------------[ cut here ]------------
> [29009.581898] WARNING: CPU: 0 PID: 0 at
> ../ath10k-ct-smallbuffers/ath10k-ct-2024.03.02~eb3f488a/ath10k-6.7/wmi.c:4043
> ath10k_wmi_event_host_swba+0x7c4/0x824 [ath10k_core]
> [29009.585574] memcpy: detected field-spanning write (size 64) of
> single field "tim->virtual_map" at
> ../ath10k-ct-smallbuffers/ath10k-ct-2024.03.02~eb3f488a/ath10k-6.7/wmi.c:4043
> (size 1)
> [29009.712626]  unwind_backtrace from show_stack+0x10/0x14
> [29009.717217]  show_stack from dump_stack_lvl+0x40/0x4c
> [29009.722337]  dump_stack_lvl from __warn+0x94/0xbc
> [29009.727546]  __warn from warn_slowpath_fmt+0xf8/0x15c
> [29009.732233]  warn_slowpath_fmt from
> ath10k_wmi_event_host_swba+0x7c4/0x824 [ath10k_core]
> [29009.737309]  ath10k_wmi_event_host_swba [ath10k_core] from
> ath10k_wmi_10_4_op_rx+0x444/0x6a4 [ath10k_core]
> [29009.745437]  ath10k_wmi_10_4_op_rx [ath10k_core] from
> ath10k_htc_rx_completion_handler+0xa8/0x210 [ath10k_core]
> [29009.754899]  ath10k_htc_rx_completion_handler [ath10k_core] from
> ath10k_pci_fw_dump_work+0xf28/0xf94 [ath10k_pci]
> [29009.764894]  ath10k_pci_fw_dump_work [ath10k_pci] from
> ath10k_ce_per_engine_service+0x64/0x84 [ath10k_core]
> [29009.775299]  ath10k_ce_per_engine_service [ath10k_core] from
> ath10k_ce_per_engine_service_any+0x74/0x194 [ath10k_core]
> [29009.784848]  ath10k_ce_per_engine_service_any [ath10k_core] from
> ath10k_pci_napi_poll+0x44/0x138 [ath10k_pci]
> [29009.795611]  ath10k_pci_napi_poll [ath10k_pci] from
> __napi_poll.constprop.0+0x2c/0x180
> [29009.805589]  __napi_poll.constprop.0 from net_rx_action+0x140/0x2e8
> [29009.813400]  net_rx_action from __do_softirq+0x100/0x270
> [29009.819561]  __do_softirq from irq_exit+0x88/0xb4
> [29009.825117]  irq_exit from call_with_stack+0x18/0x20
> [29009.829715]  call_with_stack from __irq_svc+0x80/0x98
> [29009.834751] Exception stack(0xc0d01f28 to 0xc0d01f70)
> [29009.839706] 1f20:                   00000003 00000001 1d2e2e44
> 40000000 00000000 c0d04f68
> [29009.844745] 1f40: c0d084c0 c0d04fa0 00000000 00000000 c0d04f08
> 00000000 0000001f c0d01f78
> [29009.852898] 1f60: c09deaf8 c09df260 60000013 ffffffff
> [29009.861055]  __irq_svc from default_idle_call+0x2c/0x30
> [29009.866089]  default_idle_call from do_idle+0x1d8/0x228
> [29009.871124]  do_idle from cpu_startup_entry+0x28/0x2c
> [29009.876328]  cpu_startup_entry from kernel_init+0x0/0x12c
> [29009.881537]  kernel_init from arch_post_acpi_subsys_init+0x0/0x8
> [29009.886973] ---[ end trace 0000000000000000 ]---

Re: ieee80211.h virtual_map splat

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 6/21/2024 1:04 AM, Koen Vandeputte wrote:
>
>> Hi all,
>> 
>> Within OpenWRT, we switched to kernel 6.6 some time ago.
>> 
>> During testing on a WiFi WDS setup (ath10k), I noticed an old standing
>> bug which now prints a lot more data due to the kernel upgrade:
>> 
>> - All WDS stations are connected
>> - The splat occurs
>> - All WDS station seem to go in timeout and disconnect
>> - The behavior is fixed after a reboot
>> 
>> Yes, we use ath10k-ct over here, but this part of the code is
>> identical to upstream ath10k.
>> 
>> The main issue:
>> 
>> memcpy: detected field-spanning write (size 64) of single field
>> "tim->virtual_map" at
>> ../ath10k-ct-smallbuffers/ath10k-ct-2024.03.02~eb3f488a/ath10k-6.7/wmi.c:4043
>> (size 1)
>> 
>> 
>> looks like virtual_map is defined as  "u8 virtual_map[1]", triggering
>> that error within "include/linux/ieee80211.h"
>> 
>> /**
>>  * struct ieee80211_tim_ie - Traffic Indication Map information element
>>  * @dtim_count: DTIM Count
>>  * @dtim_period: DTIM Period
>>  * @bitmap_ctrl: Bitmap Control
>>  * @virtual_map: Partial Virtual Bitmap
>>  *
>>  * This structure represents the payload of the "TIM element" as
>>  * described in IEEE Std 802.11-2020 section 9.4.2.5.
>>  */
>> struct ieee80211_tim_ie {
>>         u8 dtim_count;
>>         u8 dtim_period;
>>         u8 bitmap_ctrl;
>>         /* variable size: 1 - 251 bytes */
>>         u8 virtual_map[1];
>> } __packed;
>> 
>> 
>> According to this page, defining it this way is actually deprecated:
>> https://www.kernel.org/doc/html/latest/process/deprecated.html
>> 
>> What is the correct way to fix this?
>> Converting it to "u8 virtual_map[];"  ?
>
> Adding netdev to the initial message in the thread.
> https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/
>
> There was some discussion in the thread, with the observation that the splat 
> is fixed by:
> 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")
>
> Followed by discussion if this should be backported.
>
> Kees said that "netdev [...] maintainers have asked that contributors not 
> include "Cc: stable" tags, as they want to evaluate for themselves whether 
> patches should go to stable or not"

BTW this rule doesn't apply to wireless subsystem. For wireless patches
it's ok to "Cc: stable" in patches or anyone can send a request to
stable maintainers to pick a patch.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: ieee80211.h virtual_map splat

[Jakub Kicinski]

On Tue, 25 Jun 2024 09:56:35 +0300 Kalle Valo wrote:
> > Adding netdev to the initial message in the thread.
> > https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/
> >
> > There was some discussion in the thread, with the observation that the splat 
> > is fixed by:
> > 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")
> >
> > Followed by discussion if this should be backported.
> >
> > Kees said that "netdev [...] maintainers have asked that contributors not 
> > include "Cc: stable" tags, as they want to evaluate for themselves whether 
> > patches should go to stable or not"  
> 
> BTW this rule doesn't apply to wireless subsystem. For wireless patches
> it's ok to "Cc: stable" in patches or anyone can send a request to
> stable maintainers to pick a patch.

It's an old rule. Quoting documentation:

  Stable tree
  ~~~~~~~~~~~
  
  While it used to be the case that netdev submissions were not supposed
  to carry explicit ``CC: stable@vger.kernel.org`` tags that is no longer
  the case today. Please follow the standard stable rules in
  :ref:`Documentation/process/stable-kernel-rules.rst <stable_kernel_rules>`,
  and make sure you include appropriate Fixes tags!
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#stable-tree

Re: ieee80211.h virtual_map splat

[Kalle Valo]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 25 Jun 2024 09:56:35 +0300 Kalle Valo wrote:
>> > Adding netdev to the initial message in the thread.
>> > https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/
>> >
>> > There was some discussion in the thread, with the observation that the splat 
>> > is fixed by:
>> > 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")
>> >
>> > Followed by discussion if this should be backported.
>> >
>> > Kees said that "netdev [...] maintainers have asked that contributors not 
>> > include "Cc: stable" tags, as they want to evaluate for themselves whether 
>> > patches should go to stable or not"  
>> 
>> BTW this rule doesn't apply to wireless subsystem. For wireless patches
>> it's ok to "Cc: stable" in patches or anyone can send a request to
>> stable maintainers to pick a patch.
>
> It's an old rule. Quoting documentation:
>
>   Stable tree
>   ~~~~~~~~~~~
>   
>   While it used to be the case that netdev submissions were not supposed
>   to carry explicit ``CC: stable@vger.kernel.org`` tags that is no longer
>   the case today. Please follow the standard stable rules in
>   :ref:`Documentation/process/stable-kernel-rules.rst <stable_kernel_rules>`,
>   and make sure you include appropriate Fixes tags!
>   
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#stable-tree

Oh, I haven't noticed the change. Thanks for correcting.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: ieee80211.h virtual_map splat

[Kees Cook]

On Tue, Jun 25, 2024 at 08:02:48AM -0700, Jakub Kicinski wrote:
> On Tue, 25 Jun 2024 09:56:35 +0300 Kalle Valo wrote:
> > > Adding netdev to the initial message in the thread.
> > > https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/
> > >
> > > There was some discussion in the thread, with the observation that the splat 
> > > is fixed by:
> > > 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")
> > >
> > > Followed by discussion if this should be backported.
> > >
> > > Kees said that "netdev [...] maintainers have asked that contributors not 
> > > include "Cc: stable" tags, as they want to evaluate for themselves whether 
> > > patches should go to stable or not"  
> > 
> > BTW this rule doesn't apply to wireless subsystem. For wireless patches
> > it's ok to "Cc: stable" in patches or anyone can send a request to
> > stable maintainers to pick a patch.
> 
> It's an old rule. Quoting documentation:
> 
>   Stable tree
>   ~~~~~~~~~~~
>   
>   While it used to be the case that netdev submissions were not supposed
>   to carry explicit ``CC: stable@vger.kernel.org`` tags that is no longer
>   the case today. Please follow the standard stable rules in
>   :ref:`Documentation/process/stable-kernel-rules.rst <stable_kernel_rules>`,
>   and make sure you include appropriate Fixes tags!
>   
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#stable-tree

Ah-ha! Thanks. I will fix my brain. :)

-- 
Kees Cook

Re: ieee80211.h virtual_map splat

[Jeff Johnson]

On 6/25/2024 8:02 AM, Jakub Kicinski wrote:
> On Tue, 25 Jun 2024 09:56:35 +0300 Kalle Valo wrote:
>>> Adding netdev to the initial message in the thread.
>>> https://lore.kernel.org/all/CAPh3n83zb1PwFBFijJKChBqY95zzpYh=2iPf8tmh=YTS6e3xPw@mail.gmail.com/
>>>
>>> There was some discussion in the thread, with the observation that the splat 
>>> is fixed by:
>>> 2ae5c9248e06 ("wifi: mac80211: Use flexible array in struct ieee80211_tim_ie")
>>>
>>> Followed by discussion if this should be backported.
>>>
>>> Kees said that "netdev [...] maintainers have asked that contributors not 
>>> include "Cc: stable" tags, as they want to evaluate for themselves whether 
>>> patches should go to stable or not"  
>>
>> BTW this rule doesn't apply to wireless subsystem. For wireless patches
>> it's ok to "Cc: stable" in patches or anyone can send a request to
>> stable maintainers to pick a patch.
> 
> It's an old rule. Quoting documentation:
> 
>   Stable tree
>   ~~~~~~~~~~~
>   
>   While it used to be the case that netdev submissions were not supposed
>   to carry explicit ``CC: stable@vger.kernel.org`` tags that is no longer
>   the case today. Please follow the standard stable rules in
>   :ref:`Documentation/process/stable-kernel-rules.rst <stable_kernel_rules>`,
>   and make sure you include appropriate Fixes tags!
>   
> See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#stable-tree

Thanks for the pointer.

I've now followed option 2 to notify the stable team to backport this patch