Re: warn in ncsi netlink code

Kees Cook
2022-11-22T04:23:46Z

On Sun, Nov 20, 2022 at 11:27:14AM +1100, Sam Mendoza-Jonas wrote:
> On November 17, 2022 3:35:17 PM GMT+11:00, Joel Stanley <joel@jms.id.au> wrote:
> >next-20221114 running on an ast2600 system produced this:
> >
> >[ 44.627332] ------------[ cut here ]------------
> >[ 44.632657] WARNING: CPU: 0 PID: 508 at net/ncsi/ncsi-cmd.c:231 ncsi_cmd_handler_oem+0xbc/0xd0
> >[ 44.642387] memcpy: detected field-spanning write (size 7) of single field "&cmd->mfr_id" at net/ncsi/ncsi-cmd.c:231 (size 4)
> [...]
> >
> >The relevant code:
> >
> >static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > struct ncsi_cmd_arg *nca)
> >{
> > struct ncsi_cmd_oem_pkt *cmd;
> > unsigned int len;
> > int payload;
> > /* NC-SI spec DSP_0222_1.2.0, section 8.2.2.2
> > * requires payload to be padded with 0 to
> > * 32-bit boundary before the checksum field.
> > * Ensure the padding bytes are accounted for in
> > * skb allocation
> > */
> >
> > payload = ALIGN(nca->payload, 4);
> > len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > len += max(payload, padding_bytes);
> >
> > cmd = skb_put_zero(skb, len);
> > memcpy(&cmd->mfr_id, nca->data, nca->payload);
> > ncsi_cmd_build_header(&cmd->cmd.common, nca);
> >
> > return 0;
> >}
> >
> >I think it's copying the command payload to the command packet,
> >starting at the offset of mfr_id:
> >
> >struct ncsi_cmd_oem_pkt {
> > struct ncsi_cmd_pkt_hdr cmd; /* Command header */
> > __be32 mfr_id; /* Manufacture ID */
> > unsigned char data[]; /* OEM Payload Data */
> >};
> >
> >But I'm not too sure.
> >
> >Cheers,
> >
> >Joel
>
>
> While it looks a little gross I'm pretty sure this is the intended behavior for the OEM commands. We'll need to massage that into something nicer.

Hi!

Ah, yes, another case of deserializing into a flexible array structure.
It looks like payload length is kept in cmd.common.length, and has
been effectively bounds-checked.

Regardless, we've been handling this in a couple ways:
1) split the struct member write from the dynamically sized
portion of the write.
2) use unsafe_memcpy() with a good comment.

I'd like to be using a third option, a common deserializer[1], but it
doesn't exist yet. (This case would use the proposed mem_to_flex().)

In this case, since "payload" may be less than sizeof(mfr_id), I suspect
an unsafe_memcpy() is best.

-Kees

[1] https://lore.kernel.org/all/20220504014440.3697851-3-keescook@chromium.org/

--
Kees Cook