diff mbox series

[for-4.4,13/13] af_key: Fix sadb_x_ipsecrequest parsing

Message ID 1501013241-31961-14-git-send-email-amit.pundir@linaro.org
State Superseded
Headers show
Series Stable candidates for 4.4.y | expand

Commit Message

Amit Pundir July 25, 2017, 8:07 p.m. UTC 
From: Herbert Xu <herbert@gondor.apana.org.au>


commit 096f41d3a8fcbb8dde7f71379b1ca85fe213eded upstream.

The parsing of sadb_x_ipsecrequest is broken in a number of ways.
First of all we're not verifying sadb_x_ipsecrequest_len.  This
is needed when the structure carries addresses at the end.  Worse
we don't even look at the length when we parse those optional
addresses.

The migration code had similar parsing code that's better but
it also has some deficiencies.  The length is overcounted first
of all as it includes the header itself.  It also fails to check
the length before dereferencing the sa_family field.

This patch fixes those problems in parse_sockaddr_pair and then
uses it in parse_ipsecrequest.

Reported-by: Andrey Konovalov <andreyknvl@google.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

---
 net/key/af_key.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

-- 
2.7.4

Comments

Amit Pundir July 26, 2017, 5:02 a.m. UTC | #1 
Please ignore this one. This patch is already in your stable queue.

Regards,
Amit Pundir

On 26 July 2017 at 01:37, Amit Pundir <amit.pundir@linaro.org> wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>

>

> commit 096f41d3a8fcbb8dde7f71379b1ca85fe213eded upstream.

>

> The parsing of sadb_x_ipsecrequest is broken in a number of ways.

> First of all we're not verifying sadb_x_ipsecrequest_len.  This

> is needed when the structure carries addresses at the end.  Worse

> we don't even look at the length when we parse those optional

> addresses.

>

> The migration code had similar parsing code that's better but

> it also has some deficiencies.  The length is overcounted first

> of all as it includes the header itself.  It also fails to check

> the length before dereferencing the sa_family field.

>

> This patch fixes those problems in parse_sockaddr_pair and then

> uses it in parse_ipsecrequest.

>

> Reported-by: Andrey Konovalov <andreyknvl@google.com>

> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

> ---

>  net/key/af_key.c | 47 ++++++++++++++++++++++++++---------------------

>  1 file changed, 26 insertions(+), 21 deletions(-)

>

> diff --git a/net/key/af_key.c b/net/key/af_key.c

> index e67c28e614b9..d8d95b6415e4 100644

> --- a/net/key/af_key.c

> +++ b/net/key/af_key.c

> @@ -65,6 +65,10 @@ struct pfkey_sock {

>         } dump;

>  };

>

> +static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,

> +                              xfrm_address_t *saddr, xfrm_address_t *daddr,

> +                              u16 *family);

> +

>  static inline struct pfkey_sock *pfkey_sk(struct sock *sk)

>  {

>         return (struct pfkey_sock *)sk;

> @@ -1922,19 +1926,14 @@ parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)

>

>         /* addresses present only in tunnel mode */

>         if (t->mode == XFRM_MODE_TUNNEL) {

> -               u8 *sa = (u8 *) (rq + 1);

> -               int family, socklen;

> +               int err;

>

> -               family = pfkey_sockaddr_extract((struct sockaddr *)sa,

> -                                               &t->saddr);

> -               if (!family)

> -                       return -EINVAL;

> -

> -               socklen = pfkey_sockaddr_len(family);

> -               if (pfkey_sockaddr_extract((struct sockaddr *)(sa + socklen),

> -                                          &t->id.daddr) != family)

> -                       return -EINVAL;

> -               t->encap_family = family;

> +               err = parse_sockaddr_pair(

> +                       (struct sockaddr *)(rq + 1),

> +                       rq->sadb_x_ipsecrequest_len - sizeof(*rq),

> +                       &t->saddr, &t->id.daddr, &t->encap_family);

> +               if (err)

> +                       return err;

>         } else

>                 t->encap_family = xp->family;

>

> @@ -1954,7 +1953,11 @@ parse_ipsecrequests(struct xfrm_policy *xp, struct sadb_x_policy *pol)

>         if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))

>                 return -EINVAL;

>

> -       while (len >= sizeof(struct sadb_x_ipsecrequest)) {

> +       while (len >= sizeof(*rq)) {

> +               if (len < rq->sadb_x_ipsecrequest_len ||

> +                   rq->sadb_x_ipsecrequest_len < sizeof(*rq))

> +                       return -EINVAL;

> +

>                 if ((err = parse_ipsecrequest(xp, rq)) < 0)

>                         return err;

>                 len -= rq->sadb_x_ipsecrequest_len;

> @@ -2417,7 +2420,6 @@ out:

>         return err;

>  }

>

> -#ifdef CONFIG_NET_KEY_MIGRATE

>  static int pfkey_sockaddr_pair_size(sa_family_t family)

>  {

>         return PFKEY_ALIGN8(pfkey_sockaddr_len(family) * 2);

> @@ -2429,7 +2431,7 @@ static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,

>  {

>         int af, socklen;

>

> -       if (ext_len < pfkey_sockaddr_pair_size(sa->sa_family))

> +       if (ext_len < 2 || ext_len < pfkey_sockaddr_pair_size(sa->sa_family))

>                 return -EINVAL;

>

>         af = pfkey_sockaddr_extract(sa, saddr);

> @@ -2445,6 +2447,7 @@ static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,

>         return 0;

>  }

>

> +#ifdef CONFIG_NET_KEY_MIGRATE

>  static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,

>                                     struct xfrm_migrate *m)

>  {

> @@ -2452,13 +2455,14 @@ static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,

>         struct sadb_x_ipsecrequest *rq2;

>         int mode;

>

> -       if (len <= sizeof(struct sadb_x_ipsecrequest) ||

> -           len < rq1->sadb_x_ipsecrequest_len)

> +       if (len < sizeof(*rq1) ||

> +           len < rq1->sadb_x_ipsecrequest_len ||

> +           rq1->sadb_x_ipsecrequest_len < sizeof(*rq1))

>                 return -EINVAL;

>

>         /* old endoints */

>         err = parse_sockaddr_pair((struct sockaddr *)(rq1 + 1),

> -                                 rq1->sadb_x_ipsecrequest_len,

> +                                 rq1->sadb_x_ipsecrequest_len - sizeof(*rq1),

>                                   &m->old_saddr, &m->old_daddr,

>                                   &m->old_family);

>         if (err)

> @@ -2467,13 +2471,14 @@ static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,

>         rq2 = (struct sadb_x_ipsecrequest *)((u8 *)rq1 + rq1->sadb_x_ipsecrequest_len);

>         len -= rq1->sadb_x_ipsecrequest_len;

>

> -       if (len <= sizeof(struct sadb_x_ipsecrequest) ||

> -           len < rq2->sadb_x_ipsecrequest_len)

> +       if (len <= sizeof(*rq2) ||

> +           len < rq2->sadb_x_ipsecrequest_len ||

> +           rq2->sadb_x_ipsecrequest_len < sizeof(*rq2))

>                 return -EINVAL;

>

>         /* new endpoints */

>         err = parse_sockaddr_pair((struct sockaddr *)(rq2 + 1),

> -                                 rq2->sadb_x_ipsecrequest_len,

> +                                 rq2->sadb_x_ipsecrequest_len - sizeof(*rq2),

>                                   &m->new_saddr, &m->new_daddr,

>                                   &m->new_family);

>         if (err)

> --

> 2.7.4

>
diff mbox series

Patch

diff --git a/net/key/af_key.c b/net/key/af_key.c
index e67c28e614b9..d8d95b6415e4 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -65,6 +65,10 @@  struct pfkey_sock {
 	} dump;
 };
 
+static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
+			       xfrm_address_t *saddr, xfrm_address_t *daddr,
+			       u16 *family);
+
 static inline struct pfkey_sock *pfkey_sk(struct sock *sk)
 {
 	return (struct pfkey_sock *)sk;
@@ -1922,19 +1926,14 @@  parse_ipsecrequest(struct xfrm_policy *xp, struct sadb_x_ipsecrequest *rq)
 
 	/* addresses present only in tunnel mode */
 	if (t->mode == XFRM_MODE_TUNNEL) {
-		u8 *sa = (u8 *) (rq + 1);
-		int family, socklen;
+		int err;
 
-		family = pfkey_sockaddr_extract((struct sockaddr *)sa,
-						&t->saddr);
-		if (!family)
-			return -EINVAL;
-
-		socklen = pfkey_sockaddr_len(family);
-		if (pfkey_sockaddr_extract((struct sockaddr *)(sa + socklen),
-					   &t->id.daddr) != family)
-			return -EINVAL;
-		t->encap_family = family;
+		err = parse_sockaddr_pair(
+			(struct sockaddr *)(rq + 1),
+			rq->sadb_x_ipsecrequest_len - sizeof(*rq),
+			&t->saddr, &t->id.daddr, &t->encap_family);
+		if (err)
+			return err;
 	} else
 		t->encap_family = xp->family;
 
@@ -1954,7 +1953,11 @@  parse_ipsecrequests(struct xfrm_policy *xp, struct sadb_x_policy *pol)
 	if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
 		return -EINVAL;
 
-	while (len >= sizeof(struct sadb_x_ipsecrequest)) {
+	while (len >= sizeof(*rq)) {
+		if (len < rq->sadb_x_ipsecrequest_len ||
+		    rq->sadb_x_ipsecrequest_len < sizeof(*rq))
+			return -EINVAL;
+
 		if ((err = parse_ipsecrequest(xp, rq)) < 0)
 			return err;
 		len -= rq->sadb_x_ipsecrequest_len;
@@ -2417,7 +2420,6 @@  out:
 	return err;
 }
 
-#ifdef CONFIG_NET_KEY_MIGRATE
 static int pfkey_sockaddr_pair_size(sa_family_t family)
 {
 	return PFKEY_ALIGN8(pfkey_sockaddr_len(family) * 2);
@@ -2429,7 +2431,7 @@  static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
 {
 	int af, socklen;
 
-	if (ext_len < pfkey_sockaddr_pair_size(sa->sa_family))
+	if (ext_len < 2 || ext_len < pfkey_sockaddr_pair_size(sa->sa_family))
 		return -EINVAL;
 
 	af = pfkey_sockaddr_extract(sa, saddr);
@@ -2445,6 +2447,7 @@  static int parse_sockaddr_pair(struct sockaddr *sa, int ext_len,
 	return 0;
 }
 
+#ifdef CONFIG_NET_KEY_MIGRATE
 static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
 				    struct xfrm_migrate *m)
 {
@@ -2452,13 +2455,14 @@  static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
 	struct sadb_x_ipsecrequest *rq2;
 	int mode;
 
-	if (len <= sizeof(struct sadb_x_ipsecrequest) ||
-	    len < rq1->sadb_x_ipsecrequest_len)
+	if (len < sizeof(*rq1) ||
+	    len < rq1->sadb_x_ipsecrequest_len ||
+	    rq1->sadb_x_ipsecrequest_len < sizeof(*rq1))
 		return -EINVAL;
 
 	/* old endoints */
 	err = parse_sockaddr_pair((struct sockaddr *)(rq1 + 1),
-				  rq1->sadb_x_ipsecrequest_len,
+				  rq1->sadb_x_ipsecrequest_len - sizeof(*rq1),
 				  &m->old_saddr, &m->old_daddr,
 				  &m->old_family);
 	if (err)
@@ -2467,13 +2471,14 @@  static int ipsecrequests_to_migrate(struct sadb_x_ipsecrequest *rq1, int len,
 	rq2 = (struct sadb_x_ipsecrequest *)((u8 *)rq1 + rq1->sadb_x_ipsecrequest_len);
 	len -= rq1->sadb_x_ipsecrequest_len;
 
-	if (len <= sizeof(struct sadb_x_ipsecrequest) ||
-	    len < rq2->sadb_x_ipsecrequest_len)
+	if (len <= sizeof(*rq2) ||
+	    len < rq2->sadb_x_ipsecrequest_len ||
+	    rq2->sadb_x_ipsecrequest_len < sizeof(*rq2))
 		return -EINVAL;
 
 	/* new endpoints */
 	err = parse_sockaddr_pair((struct sockaddr *)(rq2 + 1),
-				  rq2->sadb_x_ipsecrequest_len,
+				  rq2->sadb_x_ipsecrequest_len - sizeof(*rq2),
 				  &m->new_saddr, &m->new_daddr,
 				  &m->new_family);
 	if (err)