| Message ID | ZuIhQRi/791vlUhE@decadent.org.uk (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: iwlegacy: Fix "field-spanning write" warning in il_enqueue_hcmd() | expand |
On Thu, Sep 12, 2024 at 01:01:21AM +0200, Ben Hutchings wrote: > iwlegacy uses command buffers with a payload size of 320 > bytes (default) or 4092 bytes (huge). The struct il_device_cmd type > describes the default buffers and there is no separate type describing > the huge buffers. > > The il_enqueue_hcmd() function works with both default and huge > buffers, and has a memcpy() to the buffer payload. The size of > this copy may exceed 320 bytes when using a huge buffer, which > now results in a run-time warning: > > memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) > > To fix this: > > - Define a new struct type for huge buffers, with a correctly sized > payload field > - When using a huge buffer in il_enqueue_hcmd(), cast the command > buffer pointer to that type when looking up the payload field > > Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > References: https://bugs.debian.org/1062421 > References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") > Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> I proposed diffrent fix for this here: https://lore.kernel.org/linux-wireless/20240520073210.GA693073@wp.pl/ but never get feedback if it works on real HW. So I prefer this one, sice it was tested. Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> Martin-Éric and Brandon, could you plase also test patch from https://lore.kernel.org/linux-wireless/Zr2gxERA3RL3EwRe@elsanto/ if it does not break the driver? Thanks Stanislaw > --- > drivers/net/wireless/intel/iwlegacy/common.c | 13 ++++++++++++- > drivers/net/wireless/intel/iwlegacy/common.h | 12 ++++++++++++ > 2 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c > index 9d33a66a49b5..4616293ec0cf 100644 > --- a/drivers/net/wireless/intel/iwlegacy/common.c > +++ b/drivers/net/wireless/intel/iwlegacy/common.c > @@ -3122,6 +3122,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) > struct il_cmd_meta *out_meta; > dma_addr_t phys_addr; > unsigned long flags; > + u8 *out_payload; > u32 idx; > u16 fix_size; > > @@ -3157,6 +3158,16 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) > out_cmd = txq->cmd[idx]; > out_meta = &txq->meta[idx]; > > + /* The payload is in the same place in regular and huge > + * command buffers, but we need to let the compiler know when > + * we're using a larger payload buffer to avoid "field- > + * spanning write" warnings at run-time for huge commands. > + */ > + if (cmd->flags & CMD_SIZE_HUGE) > + out_payload = ((struct il_device_cmd_huge *)out_cmd)->cmd.payload; > + else > + out_payload = out_cmd->cmd.payload; > + > if (WARN_ON(out_meta->flags & CMD_MAPPED)) { > spin_unlock_irqrestore(&il->hcmd_lock, flags); > return -ENOSPC; > @@ -3170,7 +3181,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_payload, 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..027dae5619a3 100644 > --- a/drivers/net/wireless/intel/iwlegacy/common.h > +++ b/drivers/net/wireless/intel/iwlegacy/common.h > @@ -560,6 +560,18 @@ struct il_device_cmd { > > #define TFD_MAX_PAYLOAD_SIZE (sizeof(struct il_device_cmd)) > > +/** > + * struct il_device_cmd_huge > + * > + * For use when sending huge commands. > + */ > +struct il_device_cmd_huge { > + struct il_cmd_header hdr; /* uCode API */ > + union { > + u8 payload[IL_MAX_CMD_SIZE - sizeof(struct il_cmd_header)]; > + } __packed cmd; > +} __packed; > + > struct il_host_cmd { > const void *data; > unsigned long reply_page;
On 9/12/24 3:39 AM, Stanislaw Gruszka wrote: > On Thu, Sep 12, 2024 at 01:01:21AM +0200, Ben Hutchings wrote: >> iwlegacy uses command buffers with a payload size of 320 >> bytes (default) or 4092 bytes (huge). The struct il_device_cmd type >> describes the default buffers and there is no separate type describing >> the huge buffers. >> >> The il_enqueue_hcmd() function works with both default and huge >> buffers, and has a memcpy() to the buffer payload. The size of >> this copy may exceed 320 bytes when using a huge buffer, which >> now results in a run-time warning: >> >> memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) >> >> To fix this: >> >> - Define a new struct type for huge buffers, with a correctly sized >> payload field >> - When using a huge buffer in il_enqueue_hcmd(), cast the command >> buffer pointer to that type when looking up the payload field >> >> Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> >> References: https://bugs.debian.org/1062421 >> References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 >> Signed-off-by: Ben Hutchings <ben@decadent.org.uk> >> Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") >> Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> >> Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> > > I proposed diffrent fix for this here: > https://lore.kernel.org/linux-wireless/20240520073210.GA693073@wp.pl/ > but never get feedback if it works on real HW. > So I prefer this one, sice it was tested. > > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> > > Martin-Éric and Brandon, could you plase also test patch from > https://lore.kernel.org/linux-wireless/Zr2gxERA3RL3EwRe@elsanto/ > if it does not break the driver? > As far as I can tell nothing breaks with that additional patch applied. > Thanks > Stanislaw > >> --- >> drivers/net/wireless/intel/iwlegacy/common.c | 13 ++++++++++++- >> drivers/net/wireless/intel/iwlegacy/common.h | 12 ++++++++++++ >> 2 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c >> index 9d33a66a49b5..4616293ec0cf 100644 >> --- a/drivers/net/wireless/intel/iwlegacy/common.c >> +++ b/drivers/net/wireless/intel/iwlegacy/common.c >> @@ -3122,6 +3122,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) >> struct il_cmd_meta *out_meta; >> dma_addr_t phys_addr; >> unsigned long flags; >> + u8 *out_payload; >> u32 idx; >> u16 fix_size; >> >> @@ -3157,6 +3158,16 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) >> out_cmd = txq->cmd[idx]; >> out_meta = &txq->meta[idx]; >> >> + /* The payload is in the same place in regular and huge >> + * command buffers, but we need to let the compiler know when >> + * we're using a larger payload buffer to avoid "field- >> + * spanning write" warnings at run-time for huge commands. >> + */ >> + if (cmd->flags & CMD_SIZE_HUGE) >> + out_payload = ((struct il_device_cmd_huge *)out_cmd)->cmd.payload; >> + else >> + out_payload = out_cmd->cmd.payload; >> + >> if (WARN_ON(out_meta->flags & CMD_MAPPED)) { >> spin_unlock_irqrestore(&il->hcmd_lock, flags); >> return -ENOSPC; >> @@ -3170,7 +3181,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_payload, 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..027dae5619a3 100644 >> --- a/drivers/net/wireless/intel/iwlegacy/common.h >> +++ b/drivers/net/wireless/intel/iwlegacy/common.h >> @@ -560,6 +560,18 @@ struct il_device_cmd { >> >> #define TFD_MAX_PAYLOAD_SIZE (sizeof(struct il_device_cmd)) >> >> +/** >> + * struct il_device_cmd_huge >> + * >> + * For use when sending huge commands. >> + */ >> +struct il_device_cmd_huge { >> + struct il_cmd_header hdr; /* uCode API */ >> + union { >> + u8 payload[IL_MAX_CMD_SIZE - sizeof(struct il_cmd_header)]; >> + } __packed cmd; >> +} __packed; >> + >> struct il_host_cmd { >> const void *data; >> unsigned long reply_page; > >
On Thu, Sep 12, 2024 at 12:30:42PM -0500, Brandon Nielsen wrote: > On 9/12/24 3:39 AM, Stanislaw Gruszka wrote: > > On Thu, Sep 12, 2024 at 01:01:21AM +0200, Ben Hutchings wrote: > > > iwlegacy uses command buffers with a payload size of 320 > > > bytes (default) or 4092 bytes (huge). The struct il_device_cmd type > > > describes the default buffers and there is no separate type describing > > > the huge buffers. > > > > > > The il_enqueue_hcmd() function works with both default and huge > > > buffers, and has a memcpy() to the buffer payload. The size of > > > this copy may exceed 320 bytes when using a huge buffer, which > > > now results in a run-time warning: > > > > > > memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) > > > > > > To fix this: > > > > > > - Define a new struct type for huge buffers, with a correctly sized > > > payload field > > > - When using a huge buffer in il_enqueue_hcmd(), cast the command > > > buffer pointer to that type when looking up the payload field > > > > > > Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > > > References: https://bugs.debian.org/1062421 > > > References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 > > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > > Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") > > > Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > > > Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> > > > > I proposed diffrent fix for this here: > > https://lore.kernel.org/linux-wireless/20240520073210.GA693073@wp.pl/ > > but never get feedback if it works on real HW. > > So I prefer this one, sice it was tested. > > > > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> > > > > Martin-Éric and Brandon, could you plase also test patch from > > https://lore.kernel.org/linux-wireless/Zr2gxERA3RL3EwRe@elsanto/ > > if it does not break the driver? > > > > As far as I can tell nothing breaks with that additional patch applied. Great, thanks for testing the patch. Stanislaw
to 12. syysk. 2024 klo 11.40 Stanislaw Gruszka (stf_xl@wp.pl) kirjoitti: > > On Thu, Sep 12, 2024 at 01:01:21AM +0200, Ben Hutchings wrote: > > iwlegacy uses command buffers with a payload size of 320 > > bytes (default) or 4092 bytes (huge). The struct il_device_cmd type > > describes the default buffers and there is no separate type describing > > the huge buffers. > > > > The il_enqueue_hcmd() function works with both default and huge > > buffers, and has a memcpy() to the buffer payload. The size of > > this copy may exceed 320 bytes when using a huge buffer, which > > now results in a run-time warning: > > > > memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) > > > > To fix this: > > > > - Define a new struct type for huge buffers, with a correctly sized > > payload field > > - When using a huge buffer in il_enqueue_hcmd(), cast the command > > buffer pointer to that type when looking up the payload field > > > > Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > > References: https://bugs.debian.org/1062421 > > References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > > Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") > > Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > > Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> > > I proposed diffrent fix for this here: > https://lore.kernel.org/linux-wireless/20240520073210.GA693073@wp.pl/ > but never get feedback if it works on real HW. I submitted bug reports about this at Debian and on kernel.org's Bugzilla, but never saw this patch, so obviously could not provide feedback. > So I prefer this one, sice it was tested. > > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> Thanks. > Martin-Éric and Brandon, could you plase also test patch from > https://lore.kernel.org/linux-wireless/Zr2gxERA3RL3EwRe@elsanto/ > if it does not break the driver? I am not currently in a position to test this. I will trust Brandon's results on this one. Meanwhile, an entirely unrelated bug crept into the kernel. See attachments on the above bugzilla report. Martin-Éric
Ben Hutchings <ben@decadent.org.uk> wrote: > iwlegacy uses command buffers with a payload size of 320 > bytes (default) or 4092 bytes (huge). The struct il_device_cmd type > describes the default buffers and there is no separate type describing > the huge buffers. > > The il_enqueue_hcmd() function works with both default and huge > buffers, and has a memcpy() to the buffer payload. The size of > this copy may exceed 320 bytes when using a huge buffer, which > now results in a run-time warning: > > memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) > > To fix this: > > - Define a new struct type for huge buffers, with a correctly sized > payload field > - When using a huge buffer in il_enqueue_hcmd(), cast the command > buffer pointer to that type when looking up the payload field > > Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > References: https://bugs.debian.org/1062421 > References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 > Signed-off-by: Ben Hutchings <ben@decadent.org.uk> > Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") > Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> > Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> > Acked-by: Stanislaw Gruszka <stf_xl@wp.pl> Should this patch go wireless tree for v6.12? As this is a regression I think it should.
diff --git a/drivers/net/wireless/intel/iwlegacy/common.c b/drivers/net/wireless/intel/iwlegacy/common.c index 9d33a66a49b5..4616293ec0cf 100644 --- a/drivers/net/wireless/intel/iwlegacy/common.c +++ b/drivers/net/wireless/intel/iwlegacy/common.c @@ -3122,6 +3122,7 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) struct il_cmd_meta *out_meta; dma_addr_t phys_addr; unsigned long flags; + u8 *out_payload; u32 idx; u16 fix_size; @@ -3157,6 +3158,16 @@ il_enqueue_hcmd(struct il_priv *il, struct il_host_cmd *cmd) out_cmd = txq->cmd[idx]; out_meta = &txq->meta[idx]; + /* The payload is in the same place in regular and huge + * command buffers, but we need to let the compiler know when + * we're using a larger payload buffer to avoid "field- + * spanning write" warnings at run-time for huge commands. + */ + if (cmd->flags & CMD_SIZE_HUGE) + out_payload = ((struct il_device_cmd_huge *)out_cmd)->cmd.payload; + else + out_payload = out_cmd->cmd.payload; + if (WARN_ON(out_meta->flags & CMD_MAPPED)) { spin_unlock_irqrestore(&il->hcmd_lock, flags); return -ENOSPC; @@ -3170,7 +3181,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_payload, 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..027dae5619a3 100644 --- a/drivers/net/wireless/intel/iwlegacy/common.h +++ b/drivers/net/wireless/intel/iwlegacy/common.h @@ -560,6 +560,18 @@ struct il_device_cmd { #define TFD_MAX_PAYLOAD_SIZE (sizeof(struct il_device_cmd)) +/** + * struct il_device_cmd_huge + * + * For use when sending huge commands. + */ +struct il_device_cmd_huge { + struct il_cmd_header hdr; /* uCode API */ + union { + u8 payload[IL_MAX_CMD_SIZE - sizeof(struct il_cmd_header)]; + } __packed cmd; +} __packed; + struct il_host_cmd { const void *data; unsigned long reply_page;
iwlegacy uses command buffers with a payload size of 320 bytes (default) or 4092 bytes (huge). The struct il_device_cmd type describes the default buffers and there is no separate type describing the huge buffers. The il_enqueue_hcmd() function works with both default and huge buffers, and has a memcpy() to the buffer payload. The size of this copy may exceed 320 bytes when using a huge buffer, which now results in a run-time warning: memcpy: detected field-spanning write (size 1014) of single field "&out_cmd->cmd.payload" at drivers/net/wireless/intel/iwlegacy/common.c:3170 (size 320) To fix this: - Define a new struct type for huge buffers, with a correctly sized payload field - When using a huge buffer in il_enqueue_hcmd(), cast the command buffer pointer to that type when looking up the payload field Reported-by: Martin-Éric Racine <martin-eric.racine@iki.fi> References: https://bugs.debian.org/1062421 References: https://bugzilla.kernel.org/show_bug.cgi?id=219124 Signed-off-by: Ben Hutchings <ben@decadent.org.uk> Fixes: 54d9469bc515 ("fortify: Add run-time WARN for cross-field memcpy()") Tested-by: Martin-Éric Racine <martin-eric.racine@iki.fi> Tested-by: Brandon Nielsen <nielsenb@jetfuse.net> --- drivers/net/wireless/intel/iwlegacy/common.c | 13 ++++++++++++- drivers/net/wireless/intel/iwlegacy/common.h | 12 ++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-)