[<prev] [next>] [<thread-prev] [thread-next>] [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
Powered by blists - more mailing lists