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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Sat, 25 Jan 2014 11:32:58 -0800
From:	Joe Perches <joe@...ches.com>
To:	David Miller <davem@...emloft.net>
Cc:	horia.geanta@...escale.com, steffen.klassert@...unet.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH] xfrm: fix potential incorrect pointer dereference in
 xfrm_bundle_create

On Sat, 2014-01-25 at 11:07 -0800, David Miller wrote:
> From: Horia Geanta <horia.geanta@...escale.com>
> Date: Sat, 25 Jan 2014 20:47:53 +0200
> 
> > Return value of xfrm_alloc_dst might be an error code.
> > Need to check this before using the pointer safely.
> > 
> > Signed-off-by: Horia Geanta <horia.geanta@...escale.com>
> > ---
> >  net/xfrm/xfrm_policy.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 9a91f74..f230f2a 100644
> > --- a/net/xfrm/xfrm_policy.c
> > +++ b/net/xfrm/xfrm_policy.c
> > @@ -1541,7 +1541,7 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
> >  
> >  	for (; i < nx; i++) {
> >  		struct xfrm_dst *xdst = xfrm_alloc_dst(net, family);
> > -		struct dst_entry *dst1 = &xdst->u.dst;
> > +		struct dst_entry *dst1;
> 
> There is no harm in taking the address of a bad pointer.  The only
> trouble is if we actually dereference it, which we are not doing here.
> 
> I'm not applying this patch, sorry.

trivia:

Yes, but perhaps the failure test be done before the
assignment anyway.  There are more paths below the test
where the value is not yet used.

Perhaps something like this would be more readable.

(with a few more miscellaneous cleanups)

Use a normal for loop, add braces, remove unnecessary
out: label, move automatic declarations, move error
assignments.

---

 net/xfrm/xfrm_policy.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 4b98b25..29ce9fc 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1526,7 +1526,9 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	struct xfrm_mode *inner_mode;
 	struct dst_entry *dst_prev = NULL;
 	struct dst_entry *dst0 = NULL;
-	int i = 0;
+	struct dst_entry *dst1;
+	struct xfrm_dst *xdst;
+	int i;
 	int err;
 	int header_len = 0;
 	int nfheader_len = 0;
@@ -1538,18 +1540,17 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	xfrm_flowi_addr_get(fl, &saddr, &daddr, family);
 
 	tos = xfrm_get_tos(fl, family);
-	err = tos;
-	if (tos < 0)
+	if (tos < 0) {
+		err = tos;
 		goto put_states;
+	}
 
 	dst_hold(dst);
 
-	for (; i < nx; i++) {
-		struct xfrm_dst *xdst = xfrm_alloc_dst(net, family);
-		struct dst_entry *dst1 = &xdst->u.dst;
-
-		err = PTR_ERR(xdst);
+	for (i = 0; i < nx; i++) {
+		xdst = xfrm_alloc_dst(net, family);
 		if (IS_ERR(xdst)) {
+			err = PTR_ERR(xdst);
 			dst_release(dst);
 			goto put_states;
 		}
@@ -1562,8 +1563,11 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 				dst_release(dst);
 				goto put_states;
 			}
-		} else
+		} else {
 			inner_mode = xfrm[i]->inner_mode;
+		}
+
+		dst1 = &xdst->u.dst;
 
 		if (!dst_prev)
 			dst0 = dst1;
@@ -1579,11 +1583,13 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 			family = xfrm[i]->props.family;
 			dst = xfrm_dst_lookup(xfrm[i], tos, &saddr, &daddr,
 					      family);
-			err = PTR_ERR(dst);
-			if (IS_ERR(dst))
+			if (IS_ERR(dst)) {
+				err = PTR_ERR(dst);
 				goto put_states;
-		} else
+			}
+		} else {
 			dst_hold(dst);
+		}
 
 		dst1->xfrm = xfrm[i];
 		xdst->xfrm_genid = xfrm[i]->genid;
@@ -1607,10 +1613,11 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 	dst_prev->child = dst;
 	dst0->path = dst;
 
-	err = -ENODEV;
 	dev = dst->dev;
-	if (!dev)
+	if (!dev) {
+		err = -ENODEV;
 		goto free_dst;
+	}
 
 	xfrm_init_path((struct xfrm_dst *)dst0, dst, nfheader_len);
 	xfrm_init_pmtu(dst_prev);
@@ -1628,7 +1635,6 @@ static struct dst_entry *xfrm_bundle_create(struct xfrm_policy *policy,
 		trailer_len -= xdst->u.dst.xfrm->props.trailer_len;
 	}
 
-out:
 	return dst0;
 
 put_states:
@@ -1637,8 +1643,8 @@ put_states:
 free_dst:
 	if (dst0)
 		dst_free(dst0);
-	dst0 = ERR_PTR(err);
-	goto out;
+
+	return ERR_PTR(err);
 }
 
 #ifdef CONFIG_XFRM_SUB_POLICY



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists