Re: [syzbot] [net?] UBSAN: shift-out-of-bounds in xfrm_selector_match (2)

[Sabrina Dubroca <sd@queasysnail.net>]

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