On Fri, Jun 22, 2018 at 11:46:44PM +0800, air icy wrote:
> Hi,
>
> static inline bool addr4_match(__be32 a1, __be32 a2, u8 prefixlen)
> {
> /* C99 6.5.7 (3): u32 << 32 is undefined behaviour */
> if (sizeof(long) == 4 && prefixlen == 0)
> return true;
> return !((a1 ^ a2) & htonl(~0UL << (32 - prefixlen)));
> }
>
>
> $ cat report0
> ================================================================================
> UBSAN: Undefined behaviour in ./include/net/xfrm.h:894:23
> shift exponent -128 is negative
Looks like we don't validate the prefixlen of the address family
in the xfrm_selector.
> This bug can be repro, if you need the repro file, please tell me.
Can you send me your reproducer?
On Thu, Jul 19, 2018 at 09:51:24AM +0200, Steffen Klassert wrote:
> On Fri, Jun 22, 2018 at 11:46:44PM +0800, air icy wrote:
> > Hi,
> >
> > static inline bool addr4_match(__be32 a1, __be32 a2, u8 prefixlen)
> > {
> > /* C99 6.5.7 (3): u32 << 32 is undefined behaviour */
> > if (sizeof(long) == 4 && prefixlen == 0)
> > return true;
> > return !((a1 ^ a2) & htonl(~0UL << (32 - prefixlen)));
> > }
> >
> >
> > $ cat report0
> > ================================================================================
> > UBSAN: Undefined behaviour in ./include/net/xfrm.h:894:23
> > shift exponent -128 is negative
>
> Looks like we don't validate the prefixlen of the address family
> in the xfrm_selector.
I plan to fix this with the patch below.
Thanks for the report!
Subject: [PATCH RFC ipsec] xfrm: Validate address prefix lengths in the xfrm selector.
We don't validate the address prefix lengths in the xfrm
selector we got from userspace. This can lead to undefined
behaviour in the address matching functions if the prefix
is too big for the given address family. Fix this by checking
the prefixes and refuse SA/policy insertation when a prefix
is invalid.
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Air Icy <[email protected]>
Signed-off-by: Steffen Klassert <[email protected]>
---
net/xfrm/xfrm_user.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 33878e6e0d0a..5151b3ebf068 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -151,10 +151,16 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
err = -EINVAL;
switch (p->family) {
case AF_INET:
+ if (p->sel.prefixlen_d > 32 || p->sel.prefixlen_s > 32)
+ goto out;
+
break;
case AF_INET6:
#if IS_ENABLED(CONFIG_IPV6)
+ if (p->sel.prefixlen_d > 128 || p->sel.prefixlen_s > 128)
+ goto out;
+
break;
#else
err = -EAFNOSUPPORT;
@@ -1359,10 +1365,16 @@ static int verify_newpolicy_info(struct xfrm_userpolicy_info *p)
switch (p->sel.family) {
case AF_INET:
+ if (p->sel.prefixlen_d > 32 || p->sel.prefixlen_s > 32)
+ return -EINVAL;
+
break;
case AF_INET6:
#if IS_ENABLED(CONFIG_IPV6)
+ if (p->sel.prefixlen_d > 128 || p->sel.prefixlen_s > 128)
+ return -EINVAL;
+
break;
#else
return -EAFNOSUPPORT;
--
2.14.1