Hi,
In Fedora kernel 6.8.5-301.fc40.x86_64, dmesg shows:
[ device: 03:00.0 Network controller [0280]: Intel Corporation PRO/Wireless 4965 AG or AGN [Kedron] Network Connection [8086:4230] (rev 61) ]
Thanks.
[ 53.407607] ------------[ cut here ]------------
[ 53.407622] memcpy: detected field-spanning write (size 1005) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
[ 53.407721] WARNING: CPU: 1 PID: 1632 at drivers/net/wireless/intel/iwlegacy/common.c:3173 il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.407804] Modules linked in: snd_hrtimer i915 snd_hda_codec_analog snd_hda_codec_generic iTCO_wdt intel_pmc_bxt iTCO_vendor_support iwl4965 btusb btrtl snd_hda_intel btintel snd_intel_dspcfg
btbcm snd_intel_sdw_acpi btmtk iwlegacy bluetooth snd_hda_codec drm_buddy mac80211 i2c_algo_bit coretemp i2c_i801 wmi_bmof ttm acpi_cpufreq i2c_smbus r592 libarc4 thinkpad_acpi memstick cfg80211
drm_display_helper snd_hda_core lpc_ich ses enclosure snd_seq_dummy snd_seq_oss scsi_transport_sas cec snd_seq_midi_event snd_pcm_oss snd_seq ledtrig_audio snd_hwdep snd_seq_device platform_profile
rfkill snd_pcm snd_timer snd_mixer_oss snd soundcore pktcdvd binfmt_misc loop nfnetlink zram sdhci_pci firewire_ohci cqhci firewire_core sdhci ata_generic sha512_ssse3 mmc_core video sha256_ssse3
e1000e sha1_ssse3 yenta_socket crc_itu_t wmi serio_raw pata_acpi uas usb_storage scsi_dh_rdac scsi_dh_emc scsi_dh_alua ip6_tables ip_tables fuse i2c_dev
[ 53.408052] CPU: 1 PID: 1632 Comm: wpa_supplicant Tainted: G U 6.8.5-301.fc40.x86_64 #1
[ 53.408063] Hardware name: LENOVO 7659AB7/7659AB7, BIOS 7LETC9WW (2.29 ) 03/18/2011
[ 53.408069] RIP: 0010:il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.408132] Code: 01 00 00 4c 89 c6 48 c7 c2 78 58 ba c0 48 c7 c7 d0 58 ba c0 48 89 44 24 20 4c 89 44 24 18 c6 05 77 48 1f 00 01 e8 e9 8b 8c c7 <0f> 0b 48 8b 44 24 20 4c 8b 44 24 18 e9 f9 fc ff ff
48 8b 85 90 3f
[ 53.408140] RSP: 0018:ffffb4d841ddb668 EFLAGS: 00010082
[ 53.408150] RAX: 0000000000000000 RBX: ffff958a1794d000 RCX: 0000000000000027
[ 53.408157] RDX: ffff958b37d218c8 RSI: 0000000000000001 RDI: ffff958b37d218c0
[ 53.408162] RBP: ffff958b18e11fc0 R08: 0000000000000000 R09: ffffb4d841ddb4d8
[ 53.408168] R10: ffffffff8a516808 R11: 0000000000000003 R12: ffffb4d841ddb770
[ 53.408173] R13: 00000000000003f1 R14: ffff958b2f6ae960 R15: ffff958b2c7d9d00
[ 53.408179] FS: 00007ff1ddeec840(0000) GS:ffff958b37d00000(0000) knlGS:0000000000000000
[ 53.408186] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 53.408193] CR2: 00007ff1de15f730 CR3: 0000000136860000 CR4: 00000000000006f0
[ 53.408199] Call Trace:
[ 53.408207] <TASK>
[ 53.408212] ? il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.408274] ? __warn+0x81/0x130
[ 53.408293] ? il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.408356] ? report_bug+0x16f/0x1a0
[ 53.408371] ? handle_bug+0x3c/0x80
[ 53.408379] ? exc_invalid_op+0x17/0x70
[ 53.408388] ? asm_exc_invalid_op+0x1a/0x20
[ 53.408405] ? il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.408466] ? il_enqueue_hcmd+0x477/0x560 [iwlegacy]
[ 53.408496] il_send_cmd_sync+0xf7/0x440 [iwlegacy]
[ 53.408551] ? il4965_request_scan+0x859/0xa00 [iwl4965]
[ 53.408551] il4965_request_scan+0x701/0xa00 [iwl4965]
[ 53.408551] il_mac_hw_scan+0x201/0x340 [iwlegacy]
[ 53.408551] drv_hw_scan+0x9f/0x150 [mac80211]
[ 53.408551] __ieee80211_start_scan+0x296/0x750 [mac80211]
[ 53.408551] rdev_scan+0x25/0xd0 [cfg80211]
[ 53.408551] nl80211_trigger_scan+0x3b4/0x7b0 [cfg80211]
[ 53.408551] genl_family_rcv_msg_doit+0xef/0x150
[ 53.408551] genl_rcv_msg+0x1b7/0x2c0
[ 53.408551] ? __pfx_nl80211_pre_doit+0x10/0x10 [cfg80211]
[ 53.408551] ? __pfx_nl80211_trigger_scan+0x10/0x10 [cfg80211]
[ 53.408551] ? __pfx_nl80211_post_doit+0x10/0x10 [cfg80211]
[ 53.408551] ? __pfx_genl_rcv_msg+0x10/0x10
[ 53.408551] netlink_rcv_skb+0x50/0x100
[ 53.408551] genl_rcv+0x28/0x40
[ 53.408551] netlink_unicast+0x249/0x370
[ 53.408551] netlink_sendmsg+0x21c/0x480
[ 53.408551] ____sys_sendmsg+0x39f/0x3d0
[ 53.408551] ___sys_sendmsg+0x9a/0xe0
[ 53.408551] __sys_sendmsg+0xcc/0x100
[ 53.408551] do_syscall_64+0x83/0x170
[ 53.408551] ? __sys_setsockopt+0xb2/0xe0
[ 53.408551] ? syscall_exit_to_user_mode+0x80/0x230
[ 53.408551] ? do_syscall_64+0x8f/0x170
[ 53.408551] ? do_user_addr_fault+0x304/0x670
[ 53.408551] ? exc_page_fault+0x7f/0x180
[ 53.408551] entry_SYSCALL_64_after_hwframe+0x78/0x80
[ 53.408551] RIP: 0033:0x7ff1dd92d6a4
[ 53.408551] Code: 15 79 87 0c 00 f7 d8 64 89 02 b8 ff ff ff ff eb bf 0f 1f 44 00 00 f3 0f 1e fa 80 3d a5 09 0d 00 00 74 13 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 4c c3 0f 1f 00 55 48 89 e5 48
83 ec 20 89 55
[ 53.408551] RSP: 002b:00007ffe8bf867e8 EFLAGS: 00000202 ORIG_RAX: 000000000000002e
[ 53.408551] RAX: ffffffffffffffda RBX: 0000555e2541efa0 RCX: 00007ff1dd92d6a4
[ 53.408551] RDX: 0000000000000000 RSI: 00007ffe8bf86820 RDI: 0000000000000006
[ 53.408551] RBP: 00007ffe8bf86810 R08: 0000000000000004 R09: 0000000000000001
[ 53.408551] R10: 00007ffe8bf86920 R11: 0000000000000202 R12: 0000555e254910d0
[ 53.408551] R13: 0000555e2541eeb0 R14: 00007ffe8bf86820 R15: 0000000000000000
[ 53.408551] </TASK>
[ 53.408551] ---[ end trace 0000000000000000 ]---
Hi
On Fri, Apr 12, 2024 at 07:48:39PM +0200, Xose Vazquez Perez wrote:
> Hi,
>
> In Fedora kernel 6.8.5-301.fc40.x86_64, dmesg shows:
>
> [ device: 03:00.0 Network controller [0280]: Intel Corporation PRO/Wireless 4965 AG or AGN [Kedron] Network Connection [8086:4230] (rev 61) ]
>
> Thanks.
>
> [ 53.407607] ------------[ cut here ]------------
> [ 53.407622] memcpy: detected field-spanning write (size 1005) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
> [ 53.407721] WARNING: CPU: 1 PID: 1632 at drivers/net/wireless/intel/iwlegacy/common.c:3173 il_enqueue_hcmd+0x477/0x560 [iwlegacy]
For CMD_SIZE_HUGE we have allocated 4k, so we do not do anything wrong.
Except maybe code is convoluted, since we use same structure for
huge and small il_device_cmd allocations.
But I'm thinking how to fix this fortify warning without refactoring and
some extra runtime cost ...
Xose, could you test below patch? I did not tested it, but I think
it should make this particular warning gone and does not break
anything. But maybe it will trigger some others fortify warnings.
Regards
Stanislaw
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index 9d33a66a49b5..c4ccc5df6419 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -3170,7 +3170,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd)
out_meta->callback = cmd->callback;
out_cmd->hdr.cmd = cmd->id;
- memcpy(&out_cmd->cmd.payload, cmd->data, cmd->len);
+ memcpy(&out_cmd->hdr.data, cmd->data, cmd->len);
/* At this point, the out_cmd now has all of the incoming cmd
* information */
On Sat, May 18, 2024 at 11:29:39AM +0200, Stanislaw Gruszka wrote:
> Hi
>
> On Fri, Apr 12, 2024 at 07:48:39PM +0200, Xose Vazquez Perez wrote:
> > Hi,
> >
> > In Fedora kernel 6.8.5-301.fc40.x86_64, dmesg shows:
> >
> > [ device: 03:00.0 Network controller [0280]: Intel Corporation PRO/Wireless 4965 AG or AGN [Kedron] Network Connection [8086:4230] (rev 61) ]
> >
> > Thanks.
> >
> > [ 53.407607] ------------[ cut here ]------------
> > [ 53.407622] memcpy: detected field-spanning write (size 1005) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
> > [ 53.407721] WARNING: CPU: 1 PID: 1632 at drivers/net/wireless/intel/iwlegacy/common.c:3173 il_enqueue_hcmd+0x477/0x560 [iwlegacy]
>
> For CMD_SIZE_HUGE we have allocated 4k, so we do not do anything wrong.
> Except maybe code is convoluted, since we use same structure for
> huge and small il_device_cmd allocations.
>
> But I'm thinking how to fix this fortify warning without refactoring and
> some extra runtime cost ...
>
> Xose, could you test below patch? I did not tested it, but I think
> it should make this particular warning gone and does not break
> anything. But maybe it will trigger some others fortify warnings.
tl;dr: the proposed patch should work. Refactoring will still be needed
in the future. :)
Long version:
struct il_device_cmd {
struct il_cmd_header hdr; /* uCode API */
union {
u32 flags;
u8 val8;
u16 val16;
u32 val32;
struct il_tx_cmd tx;
u8 payload[DEF_CMD_PAYLOAD_SIZE];
} __packed cmd;
} __packed;
struct il_cmd_header {
...
/* command or response/notification data follows immediately */
u8 data[];
} __packed;
Yes, the proposed fix will silence the warning, but this struct is
certainly on Gustavo's list to fix for "flexible arrays not-at-end"
warnings[1].
This memcpy() is the perfect example of why we need to refactor these
kinds of structs: the object size is ambiguous for the compiler. It
could be as little as sizeof(struct il_device_cmd), but it could larger,
because of the "hdr" member. Right now, it depends on how we happen to
address it (as your change is doing).
Regardless, thanks for tracking this down and fixing it!
-Kees
[1] https://lore.kernel.org/lkml/ZgsAFhl90kecIR00@neat/
>
> Regards
> Stanislaw
>
> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
> index 9d33a66a49b5..c4ccc5df6419 100644
> --- a/drivers/net/wireless/intel/iwlegacy/common.c
> +++ b/drivers/net/wireless/intel/iwlegacy/common.c
> @@ -3170,7 +3170,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd)
> out_meta->callback = cmd->callback;
>
> out_cmd->hdr.cmd = cmd->id;
> - memcpy(&out_cmd->cmd.payload, cmd->data, cmd->len);
> + memcpy(&out_cmd->hdr.data, cmd->data, cmd->len);
>
> /* At this point, the out_cmd now has all of the incoming cmd
> * information */
--
Kees Cook
On Sat, May 18, 2024 at 10:48:08AM -0700, Kees Cook wrote:
> On Sat, May 18, 2024 at 11:29:39AM +0200, Stanislaw Gruszka wrote:
> > Hi
> >
> > On Fri, Apr 12, 2024 at 07:48:39PM +0200, Xose Vazquez Perez wrote:
> > > Hi,
> > >
> > > In Fedora kernel 6.8.5-301.fc40.x86_64, dmesg shows:
> > >
> > > [ device: 03:00.0 Network controller [0280]: Intel Corporation PRO/Wireless 4965 AG or AGN [Kedron] Network Connection [8086:4230] (rev 61) ]
> > >
> > > Thanks.
> > >
> > > [ 53.407607] ------------[ cut here ]------------
> > > [ 53.407622] memcpy: detected field-spanning write (size 1005) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3173 (size 320)
> > > [ 53.407721] WARNING: CPU: 1 PID: 1632 at drivers/net/wireless/intel/iwlegacy/common.c:3173 il_enqueue_hcmd+0x477/0x560 [iwlegacy]
> >
> > For CMD_SIZE_HUGE we have allocated 4k, so we do not do anything wrong.
> > Except maybe code is convoluted, since we use same structure for
> > huge and small il_device_cmd allocations.
> >
> > But I'm thinking how to fix this fortify warning without refactoring and
> > some extra runtime cost ...
> >
> > Xose, could you test below patch? I did not tested it, but I think
> > it should make this particular warning gone and does not break
> > anything. But maybe it will trigger some others fortify warnings.
>
> tl;dr: the proposed patch should work. Refactoring will still be needed
> in the future. :)
>
> Long version:
>
> struct il_device_cmd {
> struct il_cmd_header hdr; /* uCode API */
> union {
> u32 flags;
> u8 val8;
> u16 val16;
> u32 val32;
> struct il_tx_cmd tx;
> u8 payload[DEF_CMD_PAYLOAD_SIZE];
> } __packed cmd;
> } __packed;
>
> struct il_cmd_header {
> ...
> /* command or response/notification data follows immediately */
> u8 data[];
> } __packed;
>
> Yes, the proposed fix will silence the warning, but this struct is
> certainly on Gustavo's list to fix for "flexible arrays not-at-end"
> warnings[1].
>
> This memcpy() is the perfect example of why we need to refactor these
> kinds of structs: the object size is ambiguous for the compiler. It
> could be as little as sizeof(struct il_device_cmd), but it could larger,
> because of the "hdr" member. Right now, it depends on how we happen to
> address it (as your change is doing).
I think we can just remove "data" from il_cmd_heder (I relized it's not
used anyway) and put it into command union. So below patch should fix
current warning and potential future warnings as well.
Xose, could you give it a try ?
Thanks
Stanislaw
diff --git a/drivers/net/wireless/intel/iwlegacy/commands.h b/drivers/net/wireless/intel/iwlegacy/commands.h
index 28cf4e832152..dca8ecf89d1e 100644
--- a/drivers/net/wireless/intel/iwlegacy/commands.h
+++ b/drivers/net/wireless/intel/iwlegacy/commands.h
@@ -201,9 +201,6 @@ struct il_cmd_header {
* 15 unsolicited RX or uCode-originated notification
*/
__le16 sequence;
-
- /* command or response/notification data follows immediately */
- u8 data[];
} __packed;
/**
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c
index 9d33a66a49b5..8f02f252b577 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.c
+++ b/drivers/net/wireless/intel/iwlegacy/common.c
@@ -3170,7 +3170,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd)
out_meta->callback = cmd->callback;
out_cmd->hdr.cmd = cmd->id;
- memcpy(&out_cmd->cmd.payload, cmd->data, cmd->len);
+ memcpy(&out_cmd->cmd.raw, cmd->data, cmd->len);
/* At this point, the out_cmd now has all of the incoming cmd
* information */
diff --git a/drivers/net/wireless/intel/iwlegacy/common.h b/drivers/net/wireless/intel/iwlegacy/common.h
index 69687fcf963f..5682cccfa394 100644
--- a/drivers/net/wireless/intel/iwlegacy/common.h
+++ b/drivers/net/wireless/intel/iwlegacy/common.h
@@ -555,6 +555,7 @@ struct il_device_cmd {
u32 val32;
struct il_tx_cmd tx;
u8 payload[DEF_CMD_PAYLOAD_SIZE];
+ DECLARE_FLEX_ARRAY(u8, raw);
} __packed cmd;
} __packed;
> +++ b/drivers/net/wireless/intel/iwlegacy/commands.h
> @@ -201,9 +201,6 @@ struct il_cmd_header {
> * 15 unsolicited RX or uCode-originated notification
> */
> __le16 sequence;
> -
> - /* command or response/notification data follows immediately */
> - u8 data[];
> } __packed;
[...]
> - memcpy(&out_cmd->cmd.payload, cmd->data, cmd->len);
> + memcpy(&out_cmd->cmd.raw, cmd->data, cmd->len);
[...]
> +++ b/drivers/net/wireless/intel/iwlegacy/common.h
> @@ -555,6 +555,7 @@ struct il_device_cmd {
> u32 val32;
> struct il_tx_cmd tx;
> u8 payload[DEF_CMD_PAYLOAD_SIZE];
> + DECLARE_FLEX_ARRAY(u8, raw);
>
I don't think this is right, now the raw comes after
DEF_CMD_PAYLOAD_SIZE? You want it to be a union with payload, I'd think.
johannes
On Mon, May 20, 2024 at 01:45:37PM +0200, Johannes Berg wrote:
>
> > +++ b/drivers/net/wireless/intel/iwlegacy/commands.h
> > @@ -201,9 +201,6 @@ struct il_cmd_header {
> > * 15 unsolicited RX or uCode-originated notification
> > */
> > __le16 sequence;
> > -
> > - /* command or response/notification data follows immediately */
> > - u8 data[];
> > } __packed;
>
> [...]
>
>
> > - memcpy(&out_cmd->cmd.payload, cmd->data, cmd->len);
> > + memcpy(&out_cmd->cmd.raw, cmd->data, cmd->len);
>
> [...]
>
> > +++ b/drivers/net/wireless/intel/iwlegacy/common.h
> > @@ -555,6 +555,7 @@ struct il_device_cmd {
> > u32 val32;
> > struct il_tx_cmd tx;
> > u8 payload[DEF_CMD_PAYLOAD_SIZE];
> > + DECLARE_FLEX_ARRAY(u8, raw);
> >
>
> I don't think this is right, now the raw comes after
> DEF_CMD_PAYLOAD_SIZE? You want it to be a union with payload, I'd think.
Not sure if I understand. I think we have union with payload with
the patch. The structure looks like this:
struct il_device_cmd {
struct il_cmd_header hdr; /* uCode API */
union {
u32 flags;
u8 val8;
u16 val16;
u32 val32;
struct il_tx_cmd tx;
u8 payload[DEF_CMD_PAYLOAD_SIZE];
DECLARE_FLEX_ARRAY(u8, raw);
} __packed cmd;
} __packed;
BTW, the first four cmd union fields i.e. flags, val8 ...
can be removed as well, seems nothing touch those fields in the code.
Regards
Stanislaw
On Mon, 2024-05-20 at 17:08 +0200, Stanislaw Gruszka wrote:
> >
> > I don't think this is right, now the raw comes after
> > DEF_CMD_PAYLOAD_SIZE? You want it to be a union with payload, I'd think.
>
> Not sure if I understand. I think we have union with payload with
> the patch. The structure looks like this:
>
> struct il_device_cmd {
> struct il_cmd_header hdr; /* uCode API */
> union {
> u32 flags;
> u8 val8;
> u16 val16;
> u32 val32;
> struct il_tx_cmd tx;
> u8 payload[DEF_CMD_PAYLOAD_SIZE];
> DECLARE_FLEX_ARRAY(u8, raw);
> } __packed cmd;
> } __packed;
>
Oh, sorry, my bad. I confused the tx_cmd and the cmd_header.
johannes