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

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

[Zhang Shurong]

The syzkaller reported an issue:

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/ieee802154/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_security()
which indicates hdr.fc.security_enabled should be 0. However, it is set to be cb->secen before.
Later, ieee802154_hdr_push_sechdr is invoked, causing KMSAN complains uninit-value issue.
Since mac802154_set_header_security() sets hdr.fc.security_enabled based on the variables
ieee802154_sub_if_data *sdata and ieee802154_mac_cb *cb in a collaborative manner.
Therefore, we should not set security_enabled prior to mac802154_set_header_security().

Fixed it by removing the line that sets the hdr.fc.security_enabled.

Syzkaller don't provide repro, and I provide a syz repro like:
r0 = syz_init_net_socket$802154_dgram(0x24, 0x2, 0x0)
setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f0000000000)=0x2, 0x4)
setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f0000000080), 0x4)
sendmsg$802154_dgram(r0, &(0x7f0000000100)={&(0x7f0000000040)={0x24, @short}, 0x14, &(0x7f00000000c0)={0x0}}, 0x0)

Fixes: 32edc40ae65c ("ieee802154: change _cb handling slightly")
Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
---
 net/mac802154/iface.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
index c0e2da5072be..c99b6e40a5db 100644
--- a/net/mac802154/iface.c
+++ b/net/mac802154/iface.c
@@ -368,7 +368,6 @@ static int ieee802154_header_create(struct sk_buff *skb,
 
 	memset(&hdr.fc, 0, sizeof(hdr.fc));
 	hdr.fc.type = cb->type;
-	hdr.fc.security_enabled = cb->secen;
 	hdr.fc.ack_request = cb->ackreq;
 	hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
 
-- 
2.30.2

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

[Miquel Raynal]

Hi Zhang,

zhang_shurong@foxmail.com wrote on Sat,  2 Dec 2023 22:58:52 +0800:

> The syzkaller reported an issue:

Subject should start with [PATCH wpan]

> 
> 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/ieee802154/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_security()
> which indicates hdr.fc.security_enabled should be 0. However, it is set 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).

> Since mac802154_set_header_security() sets hdr.fc.security_enabled based on the variables
> ieee802154_sub_if_data *sdata and ieee802154_mac_cb *cb in a collaborative manner.
> Therefore, we should not set security_enabled prior to mac802154_set_header_security().
> 
> Fixed it by removing the line that sets the hdr.fc.security_enabled.
> 
> Syzkaller don't provide repro, and I provide a syz repro like:
> r0 = syz_init_net_socket$802154_dgram(0x24, 0x2, 0x0)
> setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f0000000000)=0x2, 0x4)
> setsockopt$WPAN_SECURITY(r0, 0x0, 0x1, &(0x7f0000000080), 0x4)
> sendmsg$802154_dgram(r0, &(0x7f0000000100)={&(0x7f0000000040)={0x24, @short}, 0x14, &(0x7f00000000c0)={0x0}}, 0x0)
> 
> Fixes: 32edc40ae65c ("ieee802154: change _cb handling slightly")
> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
> ---

This is a resend, a message in a shortlog to express why is welcome. If
it's a ping, then no need to resend.

>  net/mac802154/iface.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index c0e2da5072be..c99b6e40a5db 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -368,7 +368,6 @@ static int ieee802154_header_create(struct sk_buff *skb,
>  
>  	memset(&hdr.fc, 0, sizeof(hdr.fc));
>  	hdr.fc.type = cb->type;
> -	hdr.fc.security_enabled = cb->secen;
>  	hdr.fc.ack_request = cb->ackreq;
>  	hdr.seq = atomic_inc_return(&dev->ieee802154_ptr->dsn) & 0xFF;
>  


Thanks,
Miquèl

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

[Alexander Aring]

Hi,

On Mon, Dec 4, 2023 at 3:57 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Zhang,
>
> zhang_shurong@foxmail.com wrote on Sat,  2 Dec 2023 22:58:52 +0800:
>
> > The syzkaller reported an issue:
>
> Subject should start with [PATCH wpan]
>
> >
> > 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/ieee802154/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_security()
> > which indicates hdr.fc.security_enabled should be 0. However, it is set 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.

side note: we should stop doing that with skb->cb and introduce some
802.15.4 specific header_ops callback structure and not depend on such
generic callback which does not fit here (and maybe somebody does a
wrapper around that and reuse skb->cb for their needs, etc.)

- Alex

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

[Alexander Aring]

Hi,

On Thu, Dec 14, 2023 at 10:01 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Mon, Dec 4, 2023 at 3:57 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> >
> > Hi Zhang,
> >
> > zhang_shurong@foxmail.com wrote on Sat,  2 Dec 2023 22:58:52 +0800:
> >
> > > The syzkaller reported an issue:
> >
> > Subject should start with [PATCH wpan]
> >
> > >
> > > 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/ieee802154/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_security()
> > > which indicates hdr.fc.security_enabled should be 0. However, it is set 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=v6.7-rc5#n474

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

[Nikita Zhandarovich]

>> > >
>> > > 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.

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

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

With regards,
Nikita

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@fintech.ru> 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

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

[Alexander Aring]

Hi,

On Sun, Jan 14, 2024 at 10:32 PM Alexander Aring <aahringo@redhat.com> wrote:
>
> Hi,
>
> On Fri, Jan 12, 2024 at 8:16 AM Nikita Zhandarovich
> <n.zhandarovich@fintech.ru> 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]
>

This would require that we init them with some invalid value defaults
but I think because we are using bit fields, we need to change the
whole struct to make some invalid number range available.
I am happy to init those values to some value at [0] to at least get
rid of the uninit value warning. We can change it so that it fails at
send() afterwards, I think it should fail in some later path of the
implementation that ends in a kernel log message.

- Alex

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

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

[Nikita Zhandarovich]

Hello,

On 1/17/24 17:42, Alexander Aring wrote:
> Hi,
> 
> On Sun, Jan 14, 2024 at 10:32 PM Alexander Aring <aahringo@redhat.com> wrote:
>>
>> Hi,
>>
>> On Fri, Jan 12, 2024 at 8:16 AM Nikita Zhandarovich
>> <n.zhandarovich@fintech.ru> 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]
>>
> 
> This would require that we init them with some invalid value defaults
> but I think because we are using bit fields, we need to change the
> whole struct to make some invalid number range available.
> I am happy to init those values to some value at [0] to at least get
> rid of the uninit value warning. We can change it so that it fails at
> send() afterwards, I think it should fail in some later path of the
> implementation that ends in a kernel log message.
> 
> - Alex
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ieee802154/socket.c#n474
> 

I was curious whether a smaller change would suffice since I might be
too green to see the full picture here.

In all honesty I am failing to see how exactly it happens that cb->secen
== 1 and cb->secen_override == 0 (which is exactly what occurs during
this error repro) at the start of mac802154_set_header_security().
Since there is a check in mac802154_set_header_security()

	if (!params.enabled && cb->secen_override && cb->secen)

maybe we take off 'cb->secen_override' part of the condition? That way
we catch the case when security is supposedly enabled without parameters
being available (not enabled) and return with error. Or is this approach
too lazy?

With regards,
Nikita

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

[Alexander Aring]

Hi,

On Thu, Jan 18, 2024 at 8:00 AM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:
...
>
> I was curious whether a smaller change would suffice since I might be
> too green to see the full picture here.
>
> In all honesty I am failing to see how exactly it happens that cb->secen
> == 1 and cb->secen_override == 0 (which is exactly what occurs during
> this error repro) at the start of mac802154_set_header_security().
> Since there is a check in mac802154_set_header_security()
>
>         if (!params.enabled && cb->secen_override && cb->secen)
>
> maybe we take off 'cb->secen_override' part of the condition? That way
> we catch the case when security is supposedly enabled without parameters
> being available (not enabled) and return with error. Or is this approach
> too lazy?

I need to see the full patch for this. In my opinion there are two patches here:

1. fix uninit values
2. return an error with some mismatched security parameters. (I think
this is where your approach comes in place)

The 1. case is what syzbot is complaining about and in my opinion easy
to fix at [0] to init some more default values of "struct dgram_sock"
[1].

Then 2. can be fixed afterwards.

- Alex

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

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

[Nikita Zhandarovich]

Hi,

On 2/5/24 11:49, Alexander Aring wrote:
> Hi,
> 
> On Thu, Jan 18, 2024 at 8:00 AM Nikita Zhandarovich
> <n.zhandarovich@fintech.ru> wrote:
> ...
>>
>> I was curious whether a smaller change would suffice since I might be
>> too green to see the full picture here.
>>
>> In all honesty I am failing to see how exactly it happens that cb->secen
>> == 1 and cb->secen_override == 0 (which is exactly what occurs during
>> this error repro) at the start of mac802154_set_header_security().
>> Since there is a check in mac802154_set_header_security()
>>
>>         if (!params.enabled && cb->secen_override && cb->secen)
>>
>> maybe we take off 'cb->secen_override' part of the condition? That way
>> we catch the case when security is supposedly enabled without parameters
>> being available (not enabled) and return with error. Or is this approach
>> too lazy?
> 
> I need to see the full patch for this. In my opinion there are two patches here:
> 

Alex, I am gonna try to test your version and send out patches before
the end of week, thank you for reminding me.

> 1. fix uninit values
> 2. return an error with some mismatched security parameters. (I think
> this is where your approach comes in place)
> 
> The 1. case is what syzbot is complaining about and in my opinion easy
> to fix at [0] to init some more default values of "struct dgram_sock"
> [1].

However, if I may, I am still worried that initing stuff in [0] won't
help much. They way I see it, there are mismatched sec. parameters that
lead to the actual uninit issue, but are not victim of it themselves.

Specifically, once we enter mac802154_set_header_security() all fields
of 'cb' have values (albeit a possibly wrong combo of them), values
copied from 'ro' seemingly w/o a hitch; the function ends early (cause
of mismatch); we end up NOT filling values in ieee802154_hdr *hdr, at
the very least these:

	hdr->sec.level = level;
	hdr->sec.key_id_mode = params.out_key.mode;

Then we are back in ieee802154_header_create():
ieee802154_header_create -> ieee802154_hdr_push ->
ieee802154_hdr_push_sechdr, where we finally access aforementioned
values despite putting nothing in them.

In other words, I am unsure that mismatch in sec. parameters (cb->secen,
params.enabled etc.), which leads to uninit issues in hdr->sec.XXX
fields, is itself caused by the uninit values in dgram_sock (since KMSAN
should have caught it earlier). But if you are certain, I don't mind
taking on the patches you suggested.

> 
> Then 2. can be fixed afterwards.
> 
> - Alex
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ieee802154/socket.c#n474
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/ieee802154/socket.c#n435
> 

Thank you for your patience,
Nikita