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

Sabrina Dubroca
2024-09-25T11:10:07Z

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