warn in ncsi netlink code

warn in ncsi netlink code

[Joel Stanley]

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)
[   44.655131] CPU: 0 PID: 508 Comm: ncsi-netlink Not tainted
6.1.0-rc5-14066-gefbdad8553d8 #17
[   44.664577] Hardware name: Generic DT based system
[   44.664599]  unwind_backtrace from show_stack+0x18/0x1c
[   44.675801]  show_stack from dump_stack_lvl+0x40/0x4c
[   44.681458]  dump_stack_lvl from __warn+0xb8/0x12c
[   44.686814]  __warn from warn_slowpath_fmt+0x9c/0xd8
[   44.692370]  warn_slowpath_fmt from ncsi_cmd_handler_oem+0xbc/0xd0
[   44.699285]  ncsi_cmd_handler_oem from ncsi_xmit_cmd+0x160/0x29c
[   44.706002]  ncsi_xmit_cmd from ncsi_send_cmd_nl+0x13c/0x1dc
[   44.712337]  ncsi_send_cmd_nl from genl_rcv_msg+0x1d0/0x440
[   44.718579]  genl_rcv_msg from netlink_rcv_skb+0xc0/0x120
[   44.724623]  netlink_rcv_skb from genl_rcv+0x28/0x3c
[   44.730182]  genl_rcv from netlink_unicast+0x208/0x370
[   44.735934]  netlink_unicast from netlink_sendmsg+0x1e4/0x450
[   44.742365]  netlink_sendmsg from ____sys_sendmsg+0x23c/0x2b8
[   44.748799]  ____sys_sendmsg from ___sys_sendmsg+0x9c/0xd0
[   44.754941]  ___sys_sendmsg from sys_sendmsg+0x78/0xbc
[   44.760695]  sys_sendmsg from ret_fast_syscall+0x0/0x54
[   44.766544] Exception stack(0xb57b1fa8 to 0xb57b1ff0)
[   44.772191] 1fa0:                   0244f330 0244f1e0 00000003
7ee36a60 00000000 00000000
[   44.781328] 1fc0: 0244f330 0244f1e0 76f35c60 00000128 76f91550
0244f387 0244f387 00498e7c
[   44.790462] 1fe0: 76f35d34 7ee36a10 76f1b510 76bba140
[   44.796186] ---[ end trace 0000000000000000 ]---

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

Re: warn in ncsi netlink code

[Sam Mendoza-Jonas]

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)
>[   44.655131] CPU: 0 PID: 508 Comm: ncsi-netlink Not tainted
>6.1.0-rc5-14066-gefbdad8553d8 #17
>[   44.664577] Hardware name: Generic DT based system
>[   44.664599]  unwind_backtrace from show_stack+0x18/0x1c
>[   44.675801]  show_stack from dump_stack_lvl+0x40/0x4c
>[   44.681458]  dump_stack_lvl from __warn+0xb8/0x12c
>[   44.686814]  __warn from warn_slowpath_fmt+0x9c/0xd8
>[   44.692370]  warn_slowpath_fmt from ncsi_cmd_handler_oem+0xbc/0xd0
>[   44.699285]  ncsi_cmd_handler_oem from ncsi_xmit_cmd+0x160/0x29c
>[   44.706002]  ncsi_xmit_cmd from ncsi_send_cmd_nl+0x13c/0x1dc
>[   44.712337]  ncsi_send_cmd_nl from genl_rcv_msg+0x1d0/0x440
>[   44.718579]  genl_rcv_msg from netlink_rcv_skb+0xc0/0x120
>[   44.724623]  netlink_rcv_skb from genl_rcv+0x28/0x3c
>[   44.730182]  genl_rcv from netlink_unicast+0x208/0x370
>[   44.735934]  netlink_unicast from netlink_sendmsg+0x1e4/0x450
>[   44.742365]  netlink_sendmsg from ____sys_sendmsg+0x23c/0x2b8
>[   44.748799]  ____sys_sendmsg from ___sys_sendmsg+0x9c/0xd0
>[   44.754941]  ___sys_sendmsg from sys_sendmsg+0x78/0xbc
>[   44.760695]  sys_sendmsg from ret_fast_syscall+0x0/0x54
>[   44.766544] Exception stack(0xb57b1fa8 to 0xb57b1ff0)
>[   44.772191] 1fa0:                   0244f330 0244f1e0 00000003
>7ee36a60 00000000 00000000
>[   44.781328] 1fc0: 0244f330 0244f1e0 76f35c60 00000128 76f91550
>0244f387 0244f387 00498e7c
>[   44.790462] 1fe0: 76f35d34 7ee36a10 76f1b510 76bba140
>[   44.796186] ---[ end trace 0000000000000000 ]---
>
>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.

Thanks!
Sam

Re: warn in ncsi netlink code

[Kees Cook]

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