Received-SPF: pass (google.com: domain of linux-kernel+bounces-72165-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240213152414.3703-1-n.zhandarovich@fintech.ru>
In-Reply-To: <20240213152414.3703-1-n.zhandarovich@fintech.ru>
From: Alexander Aring <aahringo@redhat.com>
Date: Mon, 19 Feb 2024 20:07:14 -0500
Message-ID: <CAK-6q+j52utmO8K_h=3LqDYmXqsqFC6MKRPUM+q=1Q30c7nEMg@mail.gmail.com>
Subject: Re: [PATCH wpan] mac802154: fix uninit-value issue in ieee802154_header_create()
To: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
Cc: Alexander Aring <alex.aring@gmail.com>, Stefan Schmidt <stefan@datenfreihafen.org>, 
	Miquel Raynal <miquel.raynal@bootlin.com>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>, 
	linux-wpan@vger.kernel.org, netdev@vger.kernel.org, 
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, 
	syzbot+60a66d44892b66b56545@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Tue, Feb 13, 2024 at 10:24=E2=80=AFAM Nikita Zhandarovich
<n.zhandarovich@fintech.ru> wrote:
>
> Syzkaller with KMSAN reported [1] a problem with uninitialized value
> access in ieee802154_header_create().
>
> The issue arises from a weird combination of cb->secen =3D=3D 1 and
> cb->secen_override =3D=3D 0, while other required security parameters
> are not found enabled in mac802154_set_header_security().
>

In case of cb->secen is 1 and cb->secen_override is 0
mac802154_set_header_security() should depend on the
ieee802154_llsec_params params.

As [0] WPAN_SECURITY_DEFAULT signals this behaviour.

> Ideally such case is expected to be caught by starting check at the
> beginning of mac802154_set_header_security():
>
>         if (!params.enabled && cb->secen_override && cb->secen)
>                 return -EINVAL;
>
> However, since secen_override is zero, the function in question
> passes this check and returns with success early, without having
> set values to ieee802154_sechdr fields such as key_id_mode. This in
> turn leads to uninitialized access of such values in
> ieee802154_hdr_push_sechdr() and other places.
>
> Fix this problem by only checking for secen value and presence of
> security parameters (and ignoring secen_override). Exit early with
> error if necessary requirements are not met.
>
> [1]
> BUG: KMSAN: uninit-value in ieee802154_hdr_push_sechdr net/ieee802154/hea=
der_ops.c:54 [inline]
> BUG: KMSAN: uninit-value in ieee802154_hdr_push+0x971/0xb90 net/ieee80215=
4/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:730 [inline]
>  __sock_sendmsg net/socket.c:745 [inline]
>  ____sys_sendmsg+0x9c2/0xd60 net/socket.c:2584
>  ___sys_sendmsg+0x28d/0x3c0 net/socket.c:2638
>  __sys_sendmsg net/socket.c:2667 [inline]
>  __do_sys_sendmsg net/socket.c:2676 [inline]
>  __se_sys_sendmsg net/socket.c:2674 [inline]
>  __x64_sys_sendmsg+0x307/0x490 net/socket.c:2674
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0x44/0x110 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
>
> Local variable hdr created at:
>  ieee802154_header_create+0x4e/0xc00 net/mac802154/iface.c:360
>  wpan_dev_hard_header include/net/cfg802154.h:494 [inline]
>  dgram_sendmsg+0xd1d/0x1500 net/ieee802154/socket.c:677
>
> Fixes: f30be4d53cad ("mac802154: integrate llsec with wpan devices")
> Reported-and-tested-by: syzbot+60a66d44892b66b56545@syzkaller.appspotmail=
com
> Closes: https://syzkaller.appspot.com/bug?extid=3D60a66d44892b66b56545
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> P.S. Link to previous similar discussion:
> https://lore.kernel.org/all/tencent_1C04CA8D66ADC45608D89687B4020B2A8706@=
qq.com/
> P.P.S. This issue may affect stable versions, at least up to 6.1.
>
>  net/mac802154/iface.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mac802154/iface.c b/net/mac802154/iface.c
> index c0e2da5072be..ad799d349625 100644
> --- a/net/mac802154/iface.c
> +++ b/net/mac802154/iface.c
> @@ -328,7 +328,7 @@ static int mac802154_set_header_security(struct ieee8=
02154_sub_if_data *sdata,
>
>         mac802154_llsec_get_params(&sdata->sec, &params);
>
> -       if (!params.enabled && cb->secen_override && cb->secen)
> +       if (!params.enabled && cb->secen)
>                 return -EINVAL;
>         if (!params.enabled ||
>             (cb->secen_override && !cb->secen) ||
>

I think there is just a missing check if (!cb->secen_override) then
use whatever mac802154_llsec_get_params() says and ignore
secen_enabled.

Also I think that we don't init those socket parameters to any value
at [1] so it's completely random what values are at socket creation.

- Alex

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/net/ieee802154/socket.c?h=3Dv6.8-rc5#n911
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree=
/net/ieee802154/socket.c?h=3Dv6.8-rc5#n474