Received-SPF: pass (google.com: domain of linux-wireless+bounces-7838-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 20 May 2024 09:32:10 +0200
From: Stanislaw Gruszka <stf_xl@wp.pl>
To: Kees Cook <keescook@chromium.org>
Cc: "Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Xose Vazquez Perez <xose.vazquez@gmail.com>,
	linux-wireless@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [WARNING] 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)
Message-ID: <20240520073210.GA693073@wp.pl>
References: <2c534d01-449a-43f4-9216-eacdb3b35577@gmail.com>
 <20240518092939.GA643846@wp.pl>
 <202405181033.6399B7E416@keescook>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <202405181033.6399B7E416@keescook>

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;