lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 21 Nov 2022 20:23:28 -0800 From: Kees Cook <keescook@...omium.org> To: Sam Mendoza-Jonas <sam@...dozajonas.com>, Joel Stanley <joel@....id.au> Cc: Networking <netdev@...r.kernel.org>, linux-hardening@...r.kernel.org Subject: Re: warn in ncsi netlink code 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@....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
Powered by blists - more mailing lists