linux-kernel - Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZvPvQMDvWRygp4IC@hog>
Date: Wed, 25 Sep 2024 13:08:48 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: steffen.klassert@...unet.com, pabeni@...hat.com,
	syzbot <syzbot+cc39f136925517aed571@...kaller.appspotmail.com>
Cc: davem@...emloft.net, edumazet@...gle.com, herbert@...dor.apana.org.au,
	kuba@...nel.org, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in
 xfrm_selector_match (2)

2024-09-24, 14:51:20 -0700, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
> 
> HEAD commit:    151ac45348af net: sparx5: Fix invalid timestamps
> git tree:       net-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=15808a80580000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=37c006d80708398d
> dashboard link: https://syzkaller.appspot.com/bug?extid=cc39f136925517aed571
> compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=122ad2a9980000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1387b107980000

syzbot managed to create an SA with:

usersa.sel.family = 0
usersa.sel.prefixlen_s = 128
usersa.family = AF_INET

Because of the AF_UNSPEC selector, verify_newsa_info doesn't put
limits on prefixlen_{s,d}. But then copy_from_user_state sets
x->sel.family to usersa.family (AF_INET).

So I think verify_newsa_info should do the same conversion before
checking prefixlen:

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 55f039ec3d59..8d06a37adbd9 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -201,6 +201,7 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 {
 	int err;
 	u8 sa_dir = attrs[XFRMA_SA_DIR] ? nla_get_u8(attrs[XFRMA_SA_DIR]) : 0;
+	u16 family = p->sel.family;

 	err = -EINVAL;
 	switch (p->family) {
@@ -221,7 +222,10 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 		goto out;
 	}

-	switch (p->sel.family) {
+	if (!family && !(p->flags & XFRM_STATE_AF_UNSPEC))
+		family = p->family;
+
+	switch (family) {
 	case AF_UNSPEC:
 		break;

Steffen, does that make sense?

Without this, we have prefixlen=128 when we get to addr4_match, which
does a shift of (32 - prefixlen), so we get

UBSAN: shift-out-of-bounds in ./include/net/xfrm.h:900:23
shift exponent -96 is negative

Maybe a check for prefixlen < 128 would also be useful in the
XFRM_STATE_AF_UNSPEC case, to avoid the same problems with syzbot
passing prefixlen=200 for an ipv6 SA. I don't know how
XFRM_STATE_AF_UNSPEC is used, so I'm not sure what restrictions we can
put. If we end up with prefixlen = 100 used from ipv4 we'll still have
the same issues.

>  __ip4_datagram_connect+0x96c/0x1260 net/ipv4/datagram.c:49
>  __ip6_datagram_connect+0x194/0x1230
>  ip6_datagram_connect net/ipv6/datagram.c:279 [inline]
>  ip6_datagram_connect_v6_only+0x63/0xa0 net/ipv6/datagram.c:291

This path also looks a bit dubious. From the reproducer, we have a
rawv6 socket trying to connect to a v4mapped address, despite having
ip6_datagram_connect_v6_only as its ->connect.

pingv6 sockets also use ip6_datagram_connect_v6_only and set
sk->sk_ipv6only=1 (in net/ipv4/ping.c ping_init_sock), but rawv6 don't
have this, so __ip6_datagram_connect can end up in
__ip4_datagram_connect. I guess it would make sense to set it in rawv6
too. rawv6_bind already rejected v4mapped addresses.

And then we could add a DEBUG_NET_WARN_ON_ONCE(!ipv6_only_sock(sk)) in
ip6_datagram_connect_v6_only, or maybe even call ipv6_addr_type to
reject v4mapped addresses and reject them like the non-AF_INET6 case.

-- 
Sabrina