Re: [PATCH RESEND] mac802154: Fix uninit-value access in ieee802154_hdr_push_sechdr

[Alexander Aring]

Hi,

On Fri, Jan 12, 2024 at 8:16 AM Nikita Zhandarovich
<n.zhandarovich@xxxxxxxxxx> wrote:
>
> >> > >
> >> > > BUG: KMSAN: uninit-value in ieee802154_hdr_push_sechdr net/ieee802154=
> > /header_ops.c:54 [inline]
> >> > > BUG: KMSAN: uninit-value in ieee802154_hdr_push+0x971/0xb90 net/ieee8=
> > 02154/header_ops.c:108
> >> > >  ieee802154_hdr_push_sechdr net/ieee802154/header_ops.c:54 [inline]
> >> > >  ieee802154_hdr_push+0x971/0xb90 net/ieee802154/header_ops.c:108
> >> > >  ieee802154_header_create+0x9c0/0xc00 net/mac802154/iface.c:396
> >> > >  wpan_dev_hard_header include/net/cfg802154.h:494 [inline]
> >> > >  dgram_sendmsg+0xd1d/0x1500 net/ieee802154/socket.c:677
> >> > >  ieee802154_sock_sendmsg+0x91/0xc0 net/ieee802154/socket.c:96
> >> > >  sock_sendmsg_nosec net/socket.c:725 [inline]
> >> > >  sock_sendmsg net/socket.c:748 [inline]
> >> > >  ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2494
> >> > >  ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2548
> >> > >  __sys_sendmsg+0x225/0x3c0 net/socket.c:2577
> >> > >  __compat_sys_sendmsg net/compat.c:346 [inline]
> >> > >  __do_compat_sys_sendmsg net/compat.c:353 [inline]
> >> > >  __se_compat_sys_sendmsg net/compat.c:350 [inline]
> >> > >
> >> > > We found hdr->key_id_mode is uninitialized in mac802154_set_header_se=
> > curity()
> >> > > which indicates hdr.fc.security_enabled should be 0. However, it is s=
> > et to be cb->secen before.
> >> > > Later, ieee802154_hdr_push_sechdr is invoked, causing KMSAN complains=
> >  uninit-value issue.
> >> >
> >> > I am not too deeply involved in the security header but for me it feels
> >> > like your patch does the opposite of what's needed. We should maybe
> >> > initialize hdr->key_id_mode based on the value in cb->secen, no? (maybe
> >> > Alexander will have a better understanding than I have).
> >>
> >> I can't help yet with a better answer why syzkaller reports it but it
> >> will break things as we using skb->cb to pass additional parameters
> >> through header_ops->create()... in this case it is some sockopts of
> >> af802154, I guess.
> >>
> >
> > Maybe we just need to init some "more" defaults in [0]
> >
> > - Alex
> >
> > [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
> > /net/ieee802154/socket.c?h=3Dv6.7-rc5#n474
>
> Hello,
>
> I was looking into the same issue (now present in syzbot [1]) and since it has a
> C-repro, the error is easy to recreate. Apparently, despite cb->secen (and
> hdr.fc.security_enabled accordingly) being equal 1, mac802154_set_header_security()
> finishes with 0 in:
>
>         if (!params.enabled ||
>             (cb->secen_override && !cb->secen) ||
>             !params.out_level)
>             return 0;
>
> Not presuming to understand the issue fully but if we do end up leaving
> mac802154_set_header_security() early, should we init hdr->key_id_mode
> with IEEE802154_SCF_KEY_IMPLICIT before returning with 0?
> I imagine that reseting hdr.fc.security_enabled to 0 ourselves in this
> case is a wrong way to go too.
>

I think here are two problems:

1.
When (in any way) secen path is hit then we should make sure some
other security parameters are set, if not return with an error. This
needs to be done somewhere in the 802.15.4 socket code. [0]

2.
The "secen path" itself in the socket code need to have defaults to be
disabled. [1]

> [1] https://syzkaller.appspot.com/bug?extid=60a66d44892b66b56545
>

Well we can test now but I think this is a check for the 1. case only.

> Hoping not to have spewed too much nonsense here...
>

I hope my words make sense here, too. :-)

- Alex

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ieee802154/socket.c#n669
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ieee802154/socket.c#n474