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)

[Kees Cook <keescook@chromium.org>]

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