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@xxxxxxxxxx> wrote:
>>
>> 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]
>>
> 
> 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