| Message ID | tencent_1C04CA8D66ADC45608D89687B4020B2A8706@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] mac802154: Fix uninit-value access in ieee802154_hdr_push_sechdr | expand |
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
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
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
>> > > >> > > 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
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
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
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
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
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
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;
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(-)