[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

From: Paul Moore <paul@paul-moore.com>

The ipv6_renew_options_kern() function eventually called into
copy_from_user(), despite it not using any userspace buffers, which
was problematic as that ended up calling access_ok() which emited
a warning on x86 (and likely other arches as well).

  ipv6_renew_options_kern()
    ipv6_renew_options()
      ipv6_renew_option()
        copy_from_user()
          _copy_from_user()
            access_ok()

The access_ok() check inside _copy_from_user() is obviously the right
thing to do which means that calling copy_from_user() via
ipv6_renew_options_kern() is obviously the wrong thing to do.  This
patch fixes this by duplicating ipv6_renew_option() in the _kern()
variant, omitting the userspace copies and attributes.  The patch
does make an attempt at limiting the duplicated code by moving the
option allocation code into a common helper function.  I'm not in
love with this solution, but everything else I could think of seemed
worse.

The ipv6_renew_options_kern() function is an required by the
CALIPSO/RFC5570 code in net/ipv6/calipso.c.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 net/ipv6/exthdrs.c |  155 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 121 insertions(+), 34 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..902748acd6fe 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1040,36 +1040,47 @@ static int ipv6_renew_option(void *ohdr,
 	return 0;
 }
 
+static int ipv6_renew_option_kern(void *ohdr,
+				  struct ipv6_opt_hdr *newopt, int newoptlen,
+				  int inherit,
+				  struct ipv6_opt_hdr **hdr,
+				  char **p)
+{
+	if (inherit) {
+		if (ohdr) {
+			memcpy(*p, ohdr,
+			       ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
+			*hdr = (struct ipv6_opt_hdr *)*p;
+			*p += CMSG_ALIGN(ipv6_optlen(*hdr));
+		}
+	} else if (newopt) {
+		memcpy(*p, newopt, newoptlen);
+		*hdr = (struct ipv6_opt_hdr *)*p;
+		if (ipv6_optlen(*hdr) > newoptlen)
+			return -EINVAL;
+		*p += CMSG_ALIGN(newoptlen);
+	}
+	return 0;
+}
+
 /**
- * ipv6_renew_options - replace a specific ext hdr with a new one.
+ * ipv6_renew_option_alloc - helper function for allocating ipv6_txoptions
  *
  * @sk: sock from which to allocate memory
  * @opt: original options
  * @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (user-mem)
- * @newoptlen: length of @newopt
- *
- * Returns a new set of options which is a copy of @opt with the
- * option type @newtype replaced with @newopt.
+ * @newoptlen: length of the new option
  *
- * @opt may be NULL, in which case a new set of options is returned
- * containing just @newopt.
- *
- * @newopt may be NULL, in which case the specified option type is
- * not copied into the new set of options.
- *
- * The new set of options is allocated from the socket option memory
- * buffer of @sk.
+ * This really should only ever be called by ipv6_renew_option() or
+ * ipv6_renew_option_kern().
  */
-struct ipv6_txoptions *
-ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
-		   int newtype,
-		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+static struct ipv6_txoptions *ipv6_renew_option_alloc(struct sock *sk,
+						      struct ipv6_txoptions *opt,
+						      int newtype,
+						      int newoptlen)
 {
 	int tot_len = 0;
-	char *p;
 	struct ipv6_txoptions *opt2;
-	int err;
 
 	if (opt) {
 		if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,7 +1093,7 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 			tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
 	}
 
-	if (newopt && newoptlen)
+	if (newoptlen)
 		tot_len += CMSG_ALIGN(newoptlen);
 
 	if (!tot_len)
@@ -1096,6 +1107,44 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 	memset(opt2, 0, tot_len);
 	refcount_set(&opt2->refcnt, 1);
 	opt2->tot_len = tot_len;
+
+	return opt2;
+}
+
+/**
+ * ipv6_renew_options - replace a specific ext hdr with a new one.
+ *
+ * @sk: sock from which to allocate memory
+ * @opt: original options
+ * @newtype: option type to replace in @opt
+ * @newopt: new option of type @newtype to replace (user-mem)
+ * @newoptlen: length of @newopt
+ *
+ * Returns a new set of options which is a copy of @opt with the
+ * option type @newtype replaced with @newopt.
+ *
+ * @opt may be NULL, in which case a new set of options is returned
+ * containing just @newopt.
+ *
+ * @newopt may be NULL, in which case the specified option type is
+ * not copied into the new set of options.
+ *
+ * The new set of options is allocated from the socket option memory
+ * buffer of @sk.
+ */
+struct ipv6_txoptions *
+ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
+		   int newtype,
+		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+{
+	char *p;
+	struct ipv6_txoptions *opt2;
+	int err;
+
+	opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+				       newopt && newoptlen ? newoptlen : 0);
+	if (!opt2 || IS_ERR(opt2))
+		return opt2;
 	p = (char *)(opt2 + 1);
 
 	err = ipv6_renew_option(opt ? opt->hopopt : NULL, newopt, newoptlen,
@@ -1142,23 +1191,61 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
  * @newopt: new option of type @newtype to replace (kernel-mem)
  * @newoptlen: length of @newopt
  *
- * See ipv6_renew_options().  The difference is that @newopt is
- * kernel memory, rather than user memory.
+ * See ipv6_renew_options().  The difference is that @newopt is kernel memory,
+ * rather than user memory.
  */
 struct ipv6_txoptions *
 ipv6_renew_options_kern(struct sock *sk, struct ipv6_txoptions *opt,
-			int newtype, struct ipv6_opt_hdr *newopt,
-			int newoptlen)
+			int newtype,
+			struct ipv6_opt_hdr *newopt, int newoptlen)
 {
-	struct ipv6_txoptions *ret_val;
-	const mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	ret_val = ipv6_renew_options(sk, opt, newtype,
-				     (struct ipv6_opt_hdr __user *)newopt,
-				     newoptlen);
-	set_fs(old_fs);
-	return ret_val;
+	char *p;
+	struct ipv6_txoptions *opt2;
+	int err;
+
+	opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+				       newopt && newoptlen ? newoptlen : 0);
+	if (!opt2 || IS_ERR(opt2))
+		return opt2;
+	p = (char *)(opt2 + 1);
+
+	err = ipv6_renew_option_kern(opt ? opt->hopopt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_HOPOPTS,
+				     &opt2->hopopt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->dst0opt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_RTHDRDSTOPTS,
+				     &opt2->dst0opt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->srcrt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_RTHDR,
+				     (struct ipv6_opt_hdr **)&opt2->srcrt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->dst1opt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_DSTOPTS,
+				     &opt2->dst1opt, &p);
+	if (err)
+		goto out;
+
+	opt2->opt_nflen = (opt2->hopopt ? ipv6_optlen(opt2->hopopt) : 0) +
+			  (opt2->dst0opt ? ipv6_optlen(opt2->dst0opt) : 0) +
+			  (opt2->srcrt ? ipv6_optlen(opt2->srcrt) : 0);
+	opt2->opt_flen = (opt2->dst1opt ? ipv6_optlen(opt2->dst1opt) : 0);
+
+	return opt2;
+out:
+	sock_kfree_s(sk, opt2, opt2->tot_len);
+	return ERR_PTR(err);
 }
 
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

From: Paul Moore <paul@paul-moore.com>

The ipv6_renew_options_kern() function eventually called into
copy_from_user(), despite it not using any userspace buffers, which
was problematic as that ended up calling access_ok() which emited
a warning on x86 (and likely other arches as well).

  ipv6_renew_options_kern()
    ipv6_renew_options()
      ipv6_renew_option()
        copy_from_user()
          _copy_from_user()
            access_ok()

The access_ok() check inside _copy_from_user() is obviously the right
thing to do which means that calling copy_from_user() via
ipv6_renew_options_kern() is obviously the wrong thing to do.  This
patch fixes this by duplicating ipv6_renew_option() in the _kern()
variant, omitting the userspace copies and attributes.  The patch
does make an attempt at limiting the duplicated code by moving the
option allocation code into a common helper function.  I'm not in
love with this solution, but everything else I could think of seemed
worse.

The ipv6_renew_options_kern() function is an required by the
CALIPSO/RFC5570 code in net/ipv6/calipso.c.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 net/ipv6/exthdrs.c |  155 +++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 121 insertions(+), 34 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..902748acd6fe 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1040,36 +1040,47 @@ static int ipv6_renew_option(void *ohdr,
 	return 0;
 }
 
+static int ipv6_renew_option_kern(void *ohdr,
+				  struct ipv6_opt_hdr *newopt, int newoptlen,
+				  int inherit,
+				  struct ipv6_opt_hdr **hdr,
+				  char **p)
+{
+	if (inherit) {
+		if (ohdr) {
+			memcpy(*p, ohdr,
+			       ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
+			*hdr = (struct ipv6_opt_hdr *)*p;
+			*p += CMSG_ALIGN(ipv6_optlen(*hdr));
+		}
+	} else if (newopt) {
+		memcpy(*p, newopt, newoptlen);
+		*hdr = (struct ipv6_opt_hdr *)*p;
+		if (ipv6_optlen(*hdr) > newoptlen)
+			return -EINVAL;
+		*p += CMSG_ALIGN(newoptlen);
+	}
+	return 0;
+}
+
 /**
- * ipv6_renew_options - replace a specific ext hdr with a new one.
+ * ipv6_renew_option_alloc - helper function for allocating ipv6_txoptions
  *
  * @sk: sock from which to allocate memory
  * @opt: original options
  * @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (user-mem)
- * @newoptlen: length of @newopt
- *
- * Returns a new set of options which is a copy of @opt with the
- * option type @newtype replaced with @newopt.
+ * @newoptlen: length of the new option
  *
- * @opt may be NULL, in which case a new set of options is returned
- * containing just @newopt.
- *
- * @newopt may be NULL, in which case the specified option type is
- * not copied into the new set of options.
- *
- * The new set of options is allocated from the socket option memory
- * buffer of @sk.
+ * This really should only ever be called by ipv6_renew_option() or
+ * ipv6_renew_option_kern().
  */
-struct ipv6_txoptions *
-ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
-		   int newtype,
-		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+static struct ipv6_txoptions *ipv6_renew_option_alloc(struct sock *sk,
+						      struct ipv6_txoptions *opt,
+						      int newtype,
+						      int newoptlen)
 {
 	int tot_len = 0;
-	char *p;
 	struct ipv6_txoptions *opt2;
-	int err;
 
 	if (opt) {
 		if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,7 +1093,7 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 			tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
 	}
 
-	if (newopt && newoptlen)
+	if (newoptlen)
 		tot_len += CMSG_ALIGN(newoptlen);
 
 	if (!tot_len)
@@ -1096,6 +1107,44 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 	memset(opt2, 0, tot_len);
 	refcount_set(&opt2->refcnt, 1);
 	opt2->tot_len = tot_len;
+
+	return opt2;
+}
+
+/**
+ * ipv6_renew_options - replace a specific ext hdr with a new one.
+ *
+ * @sk: sock from which to allocate memory
+ * @opt: original options
+ * @newtype: option type to replace in @opt
+ * @newopt: new option of type @newtype to replace (user-mem)
+ * @newoptlen: length of @newopt
+ *
+ * Returns a new set of options which is a copy of @opt with the
+ * option type @newtype replaced with @newopt.
+ *
+ * @opt may be NULL, in which case a new set of options is returned
+ * containing just @newopt.
+ *
+ * @newopt may be NULL, in which case the specified option type is
+ * not copied into the new set of options.
+ *
+ * The new set of options is allocated from the socket option memory
+ * buffer of @sk.
+ */
+struct ipv6_txoptions *
+ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
+		   int newtype,
+		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+{
+	char *p;
+	struct ipv6_txoptions *opt2;
+	int err;
+
+	opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+				       newopt && newoptlen ? newoptlen : 0);
+	if (!opt2 || IS_ERR(opt2))
+		return opt2;
 	p = (char *)(opt2 + 1);
 
 	err = ipv6_renew_option(opt ? opt->hopopt : NULL, newopt, newoptlen,
@@ -1142,23 +1191,61 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
  * @newopt: new option of type @newtype to replace (kernel-mem)
  * @newoptlen: length of @newopt
  *
- * See ipv6_renew_options().  The difference is that @newopt is
- * kernel memory, rather than user memory.
+ * See ipv6_renew_options().  The difference is that @newopt is kernel memory,
+ * rather than user memory.
  */
 struct ipv6_txoptions *
 ipv6_renew_options_kern(struct sock *sk, struct ipv6_txoptions *opt,
-			int newtype, struct ipv6_opt_hdr *newopt,
-			int newoptlen)
+			int newtype,
+			struct ipv6_opt_hdr *newopt, int newoptlen)
 {
-	struct ipv6_txoptions *ret_val;
-	const mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	ret_val = ipv6_renew_options(sk, opt, newtype,
-				     (struct ipv6_opt_hdr __user *)newopt,
-				     newoptlen);
-	set_fs(old_fs);
-	return ret_val;
+	char *p;
+	struct ipv6_txoptions *opt2;
+	int err;
+
+	opt2 = ipv6_renew_option_alloc(sk, opt, newtype,
+				       newopt && newoptlen ? newoptlen : 0);
+	if (!opt2 || IS_ERR(opt2))
+		return opt2;
+	p = (char *)(opt2 + 1);
+
+	err = ipv6_renew_option_kern(opt ? opt->hopopt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_HOPOPTS,
+				     &opt2->hopopt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->dst0opt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_RTHDRDSTOPTS,
+				     &opt2->dst0opt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->srcrt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_RTHDR,
+				     (struct ipv6_opt_hdr **)&opt2->srcrt, &p);
+	if (err)
+		goto out;
+
+	err = ipv6_renew_option_kern(opt ? opt->dst1opt : NULL,
+				     newopt, newoptlen,
+				     newtype != IPV6_DSTOPTS,
+				     &opt2->dst1opt, &p);
+	if (err)
+		goto out;
+
+	opt2->opt_nflen = (opt2->hopopt ? ipv6_optlen(opt2->hopopt) : 0) +
+			  (opt2->dst0opt ? ipv6_optlen(opt2->dst0opt) : 0) +
+			  (opt2->srcrt ? ipv6_optlen(opt2->srcrt) : 0);
+	opt2->opt_flen = (opt2->dst1opt ? ipv6_optlen(opt2->dst1opt) : 0);
+
+	return opt2;
+out:
+	sock_kfree_s(sk, opt2, opt2->tot_len);
+	return ERR_PTR(err);
 }
 
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,

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

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Fri, 22 Jun 2018 17:18:20 -0400

> -	const mm_segment_t old_fs = get_fs();
> -
> -	set_fs(KERNEL_DS);
> -	ret_val = ipv6_renew_options(sk, opt, newtype,
> -				     (struct ipv6_opt_hdr __user *)newopt,
> -				     newoptlen);
> -	set_fs(old_fs);

So is it really the case that the traditional construct:

	set_fs(KERNEL_DS);
	... copy_{from,to}_user(...);
	set_fs(old_fs);

is no longer allowed?

Setting fs to KERNEL_DS is supposed to make user copies work on kernel
memory.  Or at least it did for 20+ years :-)

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Fri, 22 Jun 2018 17:18:20 -0400

> -	const mm_segment_t old_fs = get_fs();
> -
> -	set_fs(KERNEL_DS);
> -	ret_val = ipv6_renew_options(sk, opt, newtype,
> -				     (struct ipv6_opt_hdr __user *)newopt,
> -				     newoptlen);
> -	set_fs(old_fs);

So is it really the case that the traditional construct:

	set_fs(KERNEL_DS);
	... copy_{from,to}_user(...);
	set_fs(old_fs);

is no longer allowed?

Setting fs to KERNEL_DS is supposed to make user copies work on kernel
memory.  Or at least it did for 20+ years :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Fri, 22 Jun 2018 17:18:20 -0400

> From: Paul Moore <paul@paul-moore.com>
> 
> The ipv6_renew_options_kern() function eventually called into
> copy_from_user(), despite it not using any userspace buffers, which
> was problematic as that ended up calling access_ok() which emited
> a warning on x86 (and likely other arches as well).
> 
>   ipv6_renew_options_kern()
>     ipv6_renew_options()
>       ipv6_renew_option()
>         copy_from_user()
>           _copy_from_user()
>             access_ok()
> 
> The access_ok() check inside _copy_from_user() is obviously the right
> thing to do which means that calling copy_from_user() via
> ipv6_renew_options_kern() is obviously the wrong thing to do.

Ok, I re-read the code around here.

access_ok() is not warning because we are calling copy_from_user()
with a kernel pointer.  The set_ds(KERNEL_DS) adjusts the
user_addr_max() setting, and thus that check passes.

The problem is that we are invoking this from an interrupt, and this
triggers the WARN_ON_IN_IRQ() in access_ok().

Although I think that WARN_ON_IN_IRQ() is completely unnecessary when
KERNEL_DS is set, the situation that really causes this problem is not
at all clear from your commit message.

I guess that for now your fix is fine, but I want you to please adjust
the commit message.

Provide the _full_ annotated kernel backtrace from the warning that
triggers, because this will show the reader that we are in an
interrupt.  And explain that being in the interrupt is strictly what
causes this to warn, not that we are using kernel pointers.  The
latter is %100 valid when set_fs(KERNEL_DS) is performed.

Thank you.

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Paul Moore <pmoore@redhat.com>
Date: Fri, 22 Jun 2018 17:18:20 -0400

> From: Paul Moore <paul@paul-moore.com>
> 
> The ipv6_renew_options_kern() function eventually called into
> copy_from_user(), despite it not using any userspace buffers, which
> was problematic as that ended up calling access_ok() which emited
> a warning on x86 (and likely other arches as well).
> 
>   ipv6_renew_options_kern()
>     ipv6_renew_options()
>       ipv6_renew_option()
>         copy_from_user()
>           _copy_from_user()
>             access_ok()
> 
> The access_ok() check inside _copy_from_user() is obviously the right
> thing to do which means that calling copy_from_user() via
> ipv6_renew_options_kern() is obviously the wrong thing to do.

Ok, I re-read the code around here.

access_ok() is not warning because we are calling copy_from_user()
with a kernel pointer.  The set_ds(KERNEL_DS) adjusts the
user_addr_max() setting, and thus that check passes.

The problem is that we are invoking this from an interrupt, and this
triggers the WARN_ON_IN_IRQ() in access_ok().

Although I think that WARN_ON_IN_IRQ() is completely unnecessary when
KERNEL_DS is set, the situation that really causes this problem is not
at all clear from your commit message.

I guess that for now your fix is fine, but I want you to please adjust
the commit message.

Provide the _full_ annotated kernel backtrace from the warning that
triggers, because this will show the reader that we are in an
interrupt.  And explain that being in the interrupt is strictly what
causes this to warn, not that we are using kernel pointers.  The
latter is %100 valid when set_fs(KERNEL_DS) is performed.

Thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

On Sat, Jun 23, 2018 at 8:16 AM David Miller <davem@davemloft.net> wrote:
>
> From: Paul Moore <pmoore@redhat.com>
> Date: Fri, 22 Jun 2018 17:18:20 -0400
>
> > From: Paul Moore <paul@paul-moore.com>
> >
> > The ipv6_renew_options_kern() function eventually called into
> > copy_from_user(), despite it not using any userspace buffers, which
> > was problematic as that ended up calling access_ok() which emited
> > a warning on x86 (and likely other arches as well).
> >
> >   ipv6_renew_options_kern()
> >     ipv6_renew_options()
> >       ipv6_renew_option()
> >         copy_from_user()
> >           _copy_from_user()
> >             access_ok()
> >
> > The access_ok() check inside _copy_from_user() is obviously the right
> > thing to do which means that calling copy_from_user() via
> > ipv6_renew_options_kern() is obviously the wrong thing to do.
>
> Ok, I re-read the code around here.
>
> access_ok() is not warning because we are calling copy_from_user()
> with a kernel pointer.  The set_ds(KERNEL_DS) adjusts the
> user_addr_max() setting, and thus that check passes.
>
> The problem is that we are invoking this from an interrupt, and this
> triggers the WARN_ON_IN_IRQ() in access_ok().
>
> Although I think that WARN_ON_IN_IRQ() is completely unnecessary when
> KERNEL_DS is set, the situation that really causes this problem is not
> at all clear from your commit message.
>
> I guess that for now your fix is fine, but I want you to please adjust
> the commit message.
>
> Provide the _full_ annotated kernel backtrace from the warning that
> triggers, because this will show the reader that we are in an
> interrupt.  And explain that being in the interrupt is strictly what
> causes this to warn, not that we are using kernel pointers.  The
> latter is %100 valid when set_fs(KERNEL_DS) is performed.
>
> Thank you.

Okay, so it's the right fix for all the wrong reasons :)

Thanks for the correction; I'll fixup the commit subject/description
and resend when I'm in front of the system with the patch (later this
weekend, early next week).

-- 
paul moore
www.paul-moore.com

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

On Sat, Jun 23, 2018 at 8:16 AM David Miller <davem@davemloft.net> wrote:
>
> From: Paul Moore <pmoore@redhat.com>
> Date: Fri, 22 Jun 2018 17:18:20 -0400
>
> > From: Paul Moore <paul@paul-moore.com>
> >
> > The ipv6_renew_options_kern() function eventually called into
> > copy_from_user(), despite it not using any userspace buffers, which
> > was problematic as that ended up calling access_ok() which emited
> > a warning on x86 (and likely other arches as well).
> >
> >   ipv6_renew_options_kern()
> >     ipv6_renew_options()
> >       ipv6_renew_option()
> >         copy_from_user()
> >           _copy_from_user()
> >             access_ok()
> >
> > The access_ok() check inside _copy_from_user() is obviously the right
> > thing to do which means that calling copy_from_user() via
> > ipv6_renew_options_kern() is obviously the wrong thing to do.
>
> Ok, I re-read the code around here.
>
> access_ok() is not warning because we are calling copy_from_user()
> with a kernel pointer.  The set_ds(KERNEL_DS) adjusts the
> user_addr_max() setting, and thus that check passes.
>
> The problem is that we are invoking this from an interrupt, and this
> triggers the WARN_ON_IN_IRQ() in access_ok().
>
> Although I think that WARN_ON_IN_IRQ() is completely unnecessary when
> KERNEL_DS is set, the situation that really causes this problem is not
> at all clear from your commit message.
>
> I guess that for now your fix is fine, but I want you to please adjust
> the commit message.
>
> Provide the _full_ annotated kernel backtrace from the warning that
> triggers, because this will show the reader that we are in an
> interrupt.  And explain that being in the interrupt is strictly what
> causes this to warn, not that we are using kernel pointers.  The
> latter is %100 valid when set_fs(KERNEL_DS) is performed.
>
> Thank you.

Okay, so it's the right fix for all the wrong reasons :)

Thanks for the correction; I'll fixup the commit subject/description
and resend when I'm in front of the system with the patch (later this
weekend, early next week).

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Al Viro]

On Sat, Jun 23, 2018 at 10:57:06AM +0900, David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Fri, 22 Jun 2018 17:18:20 -0400
> 
> > -	const mm_segment_t old_fs = get_fs();
> > -
> > -	set_fs(KERNEL_DS);
> > -	ret_val = ipv6_renew_options(sk, opt, newtype,
> > -				     (struct ipv6_opt_hdr __user *)newopt,
> > -				     newoptlen);
> > -	set_fs(old_fs);
> 
> So is it really the case that the traditional construct:
> 
> 	set_fs(KERNEL_DS);
> 	... copy_{from,to}_user(...);
> 	set_fs(old_fs);
> 
> is no longer allowed?

s/no longer allowed/best avoided/, but IMO in this case the replacement is
too ugly to live ;-/

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Al Viro]

On Sat, Jun 23, 2018 at 10:57:06AM +0900, David Miller wrote:
> From: Paul Moore <pmoore@redhat.com>
> Date: Fri, 22 Jun 2018 17:18:20 -0400
> 
> > -	const mm_segment_t old_fs = get_fs();
> > -
> > -	set_fs(KERNEL_DS);
> > -	ret_val = ipv6_renew_options(sk, opt, newtype,
> > -				     (struct ipv6_opt_hdr __user *)newopt,
> > -				     newoptlen);
> > -	set_fs(old_fs);
> 
> So is it really the case that the traditional construct:
> 
> 	set_fs(KERNEL_DS);
> 	... copy_{from,to}_user(...);
> 	set_fs(old_fs);
> 
> is no longer allowed?

s/no longer allowed/best avoided/, but IMO in this case the replacement is
too ugly to live ;-/
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Al Viro]

On Sat, Jun 23, 2018 at 10:26:27PM +0100, Al Viro wrote:
> On Sat, Jun 23, 2018 at 10:57:06AM +0900, David Miller wrote:
> > From: Paul Moore <pmoore@redhat.com>
> > Date: Fri, 22 Jun 2018 17:18:20 -0400
> > 
> > > -	const mm_segment_t old_fs = get_fs();
> > > -
> > > -	set_fs(KERNEL_DS);
> > > -	ret_val = ipv6_renew_options(sk, opt, newtype,
> > > -				     (struct ipv6_opt_hdr __user *)newopt,
> > > -				     newoptlen);
> > > -	set_fs(old_fs);
> > 
> > So is it really the case that the traditional construct:
> > 
> > 	set_fs(KERNEL_DS);
> > 	... copy_{from,to}_user(...);
> > 	set_fs(old_fs);
> > 
> > is no longer allowed?
> 
> s/no longer allowed/best avoided/, but IMO in this case the replacement is
> too ugly to live ;-/

BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
ipv6_renew_options_kern()...

Incidentally, is copying the entire value in case newoptlen > ipv6_optlen(...)
the right thing?  After all, the next update in any of those options will
quietly lose everything past ipv6_optlen(...), wouldn't it?

IOW, how about the following (completely untested):

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
 struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
 					  struct ipv6_txoptions *opt,
 					  int newtype,
-					  struct ipv6_opt_hdr __user *newopt,
-					  int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
-			struct ipv6_txoptions *opt,
-			int newtype,
-			struct ipv6_opt_hdr *newopt,
-			int newoptlen);
+					  struct ipv6_opt_hdr *newopt);
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
 					  struct ipv6_txoptions *opt);
 
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct ipv6_opt_hdr *hop)
 {
 	struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
 
-	txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-					 hop, hop ? ipv6_optlen(hop) : 0);
+	txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
 	txopt_put(old);
 	if (IS_ERR(txopts))
 		return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
 	if (IS_ERR(new))
 		return PTR_ERR(new);
 
-	txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-					 new, new ? ipv6_optlen(new) : 0);
+	txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
 
 	kfree(new);
 
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
 	if (calipso_opt_del(req_inet->ipv6_opt->hopopt, &new))
 		return; /* Nothing to do */
 
-	txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-					 new, new ? ipv6_optlen(new) : 0);
+	txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
 
 	if (!IS_ERR(txopts)) {
 		txopts = xchg(&req_inet->ipv6_opt, txopts);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..4b6915eedfc3 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1015,29 +1015,15 @@ ipv6_dup_options(struct sock *sk, struct ipv6_txoptions *opt)
 }
 EXPORT_SYMBOL_GPL(ipv6_dup_options);
 
-static int ipv6_renew_option(void *ohdr,
-			     struct ipv6_opt_hdr __user *newopt, int newoptlen,
-			     int inherit,
+static void ipv6_renew_option(struct ipv6_opt_hdr *opt,
 			     struct ipv6_opt_hdr **hdr,
 			     char **p)
 {
-	if (inherit) {
-		if (ohdr) {
-			memcpy(*p, ohdr, ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
-			*hdr = (struct ipv6_opt_hdr *)*p;
-			*p += CMSG_ALIGN(ipv6_optlen(*hdr));
-		}
-	} else {
-		if (newopt) {
-			if (copy_from_user(*p, newopt, newoptlen))
-				return -EFAULT;
-			*hdr = (struct ipv6_opt_hdr *)*p;
-			if (ipv6_optlen(*hdr) > newoptlen)
-				return -EINVAL;
-			*p += CMSG_ALIGN(newoptlen);
-		}
+	if (opt) {
+		memcpy(*p, opt, ipv6_optlen(opt));
+		*hdr = (struct ipv6_opt_hdr *)*p;
+		*p += CMSG_ALIGN(ipv6_optlen(*hdr));
 	}
-	return 0;
 }
 
 /**
@@ -1063,13 +1049,11 @@ static int ipv6_renew_option(void *ohdr,
  */
 struct ipv6_txoptions *
 ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
-		   int newtype,
-		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+		   int newtype, struct ipv6_opt_hdr *newopt)
 {
 	int tot_len = 0;
 	char *p;
 	struct ipv6_txoptions *opt2;
-	int err;
 
 	if (opt) {
 		if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,8 +1066,8 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 			tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
 	}
 
-	if (newopt && newoptlen)
-		tot_len += CMSG_ALIGN(newoptlen);
+	if (newopt)
+		tot_len += CMSG_ALIGN(ipv6_optlen(newopt));
 
 	if (!tot_len)
 		return NULL;
@@ -1098,67 +1082,25 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 	opt2->tot_len = tot_len;
 	p = (char *)(opt2 + 1);
 
-	err = ipv6_renew_option(opt ? opt->hopopt : NULL, newopt, newoptlen,
-				newtype != IPV6_HOPOPTS,
-				&opt2->hopopt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->dst0opt : NULL, newopt, newoptlen,
-				newtype != IPV6_RTHDRDSTOPTS,
-				&opt2->dst0opt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->srcrt : NULL, newopt, newoptlen,
-				newtype != IPV6_RTHDR,
-				(struct ipv6_opt_hdr **)&opt2->srcrt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->dst1opt : NULL, newopt, newoptlen,
-				newtype != IPV6_DSTOPTS,
-				&opt2->dst1opt, &p);
-	if (err)
-		goto out;
-
+	ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
+				opt ? opt->hopopt : NULL,
+			  &opt2->hopopt, &p);
+
+	ipv6_renew_option(newtype == IPV6_RTHDRDSTOPTS ? newopt :
+				opt ? opt->dst0opt : NULL,
+			&opt2->dst0opt, &p);
+	ipv6_renew_option(newtype == IPV6_RTHDR ? newopt :
+				opt ? (struct ipv6_opt_hdr *)opt->srcrt : NULL,
+			(struct ipv6_opt_hdr **)&opt2->srcrt, &p);
+	ipv6_renew_option(newtype == IPV6_DSTOPTS ? newopt :
+				opt ? opt->dst1opt : NULL,
+			&opt2->dst1opt, &p);
 	opt2->opt_nflen = (opt2->hopopt ? ipv6_optlen(opt2->hopopt) : 0) +
 			  (opt2->dst0opt ? ipv6_optlen(opt2->dst0opt) : 0) +
 			  (opt2->srcrt ? ipv6_optlen(opt2->srcrt) : 0);
 	opt2->opt_flen = (opt2->dst1opt ? ipv6_optlen(opt2->dst1opt) : 0);
 
 	return opt2;
-out:
-	sock_kfree_s(sk, opt2, opt2->tot_len);
-	return ERR_PTR(err);
-}
-
-/**
- * ipv6_renew_options_kern - replace a specific ext hdr with a new one.
- *
- * @sk: sock from which to allocate memory
- * @opt: original options
- * @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (kernel-mem)
- * @newoptlen: length of @newopt
- *
- * See ipv6_renew_options().  The difference is that @newopt is
- * kernel memory, rather than user memory.
- */
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk, struct ipv6_txoptions *opt,
-			int newtype, struct ipv6_opt_hdr *newopt,
-			int newoptlen)
-{
-	struct ipv6_txoptions *ret_val;
-	const mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	ret_val = ipv6_renew_options(sk, opt, newtype,
-				     (struct ipv6_opt_hdr __user *)newopt,
-				     newoptlen);
-	set_fs(old_fs);
-	return ret_val;
 }
 
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4d780c7f0130..b69ba18e0138 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -398,6 +398,12 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 	case IPV6_DSTOPTS:
 	{
 		struct ipv6_txoptions *opt;
+		struct ipv6_opt_hdr *new = NULL;
+
+		/* hop-by-hop / destination options are privileged option */
+		retv = -EPERM;
+		if (optname != IPV6_RTHDR && !ns_capable(net->user_ns, CAP_NET_RAW))
+			break;
 
 		/* remove any sticky options header with a zero option
 		 * length, per RFC3542.
@@ -409,17 +415,23 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 		else if (optlen < sizeof(struct ipv6_opt_hdr) ||
 			 optlen & 0x7 || optlen > 8 * 255)
 			goto e_inval;
-
-		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !ns_capable(net->user_ns, CAP_NET_RAW))
-			break;
+		else {
+			new = kmemdup(optval, optlen, GFP_USER);
+			if (IS_ERR(new)) {
+				retv = PTR_ERR(new);
+				break;
+			}
+			if (unlikely(ipv6_optlen(new) > optlen)) {
+				kfree(new);
+				retv = -EINVAL;
+				break;
+			}
+		}
 
 		opt = rcu_dereference_protected(np->opt,
 						lockdep_sock_is_held(sk));
-		opt = ipv6_renew_options(sk, opt, optname,
-					 (struct ipv6_opt_hdr __user *)optval,
-					 optlen);
+		opt = ipv6_renew_options(sk, opt, optname, new);
+		kfree(new);
 		if (IS_ERR(opt)) {
 			retv = PTR_ERR(opt);
 			break;

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Al Viro]

On Sat, Jun 23, 2018 at 10:26:27PM +0100, Al Viro wrote:
> On Sat, Jun 23, 2018 at 10:57:06AM +0900, David Miller wrote:
> > From: Paul Moore <pmoore@redhat.com>
> > Date: Fri, 22 Jun 2018 17:18:20 -0400
> > 
> > > -	const mm_segment_t old_fs = get_fs();
> > > -
> > > -	set_fs(KERNEL_DS);
> > > -	ret_val = ipv6_renew_options(sk, opt, newtype,
> > > -				     (struct ipv6_opt_hdr __user *)newopt,
> > > -				     newoptlen);
> > > -	set_fs(old_fs);
> > 
> > So is it really the case that the traditional construct:
> > 
> > 	set_fs(KERNEL_DS);
> > 	... copy_{from,to}_user(...);
> > 	set_fs(old_fs);
> > 
> > is no longer allowed?
> 
> s/no longer allowed/best avoided/, but IMO in this case the replacement is
> too ugly to live ;-/

BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
ipv6_renew_options_kern()...

Incidentally, is copying the entire value in case newoptlen > ipv6_optlen(...)
the right thing?  After all, the next update in any of those options will
quietly lose everything past ipv6_optlen(...), wouldn't it?

IOW, how about the following (completely untested):

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 16475c269749..d02881e4ad1f 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -355,14 +355,7 @@ struct ipv6_txoptions *ipv6_dup_options(struct sock *sk,
 struct ipv6_txoptions *ipv6_renew_options(struct sock *sk,
 					  struct ipv6_txoptions *opt,
 					  int newtype,
-					  struct ipv6_opt_hdr __user *newopt,
-					  int newoptlen);
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk,
-			struct ipv6_txoptions *opt,
-			int newtype,
-			struct ipv6_opt_hdr *newopt,
-			int newoptlen);
+					  struct ipv6_opt_hdr *newopt);
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
 					  struct ipv6_txoptions *opt);
 
diff --git a/net/ipv6/calipso.c b/net/ipv6/calipso.c
index 1323b9679cf7..1c0bb9fb76e6 100644
--- a/net/ipv6/calipso.c
+++ b/net/ipv6/calipso.c
@@ -799,8 +799,7 @@ static int calipso_opt_update(struct sock *sk, struct ipv6_opt_hdr *hop)
 {
 	struct ipv6_txoptions *old = txopt_get(inet6_sk(sk)), *txopts;
 
-	txopts = ipv6_renew_options_kern(sk, old, IPV6_HOPOPTS,
-					 hop, hop ? ipv6_optlen(hop) : 0);
+	txopts = ipv6_renew_options(sk, old, IPV6_HOPOPTS, hop);
 	txopt_put(old);
 	if (IS_ERR(txopts))
 		return PTR_ERR(txopts);
@@ -1222,8 +1221,7 @@ static int calipso_req_setattr(struct request_sock *req,
 	if (IS_ERR(new))
 		return PTR_ERR(new);
 
-	txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-					 new, new ? ipv6_optlen(new) : 0);
+	txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
 
 	kfree(new);
 
@@ -1260,8 +1258,7 @@ static void calipso_req_delattr(struct request_sock *req)
 	if (calipso_opt_del(req_inet->ipv6_opt->hopopt, &new))
 		return; /* Nothing to do */
 
-	txopts = ipv6_renew_options_kern(sk, req_inet->ipv6_opt, IPV6_HOPOPTS,
-					 new, new ? ipv6_optlen(new) : 0);
+	txopts = ipv6_renew_options(sk, req_inet->ipv6_opt, IPV6_HOPOPTS, new);
 
 	if (!IS_ERR(txopts)) {
 		txopts = xchg(&req_inet->ipv6_opt, txopts);
diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 5bc2bf3733ab..4b6915eedfc3 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -1015,29 +1015,15 @@ ipv6_dup_options(struct sock *sk, struct ipv6_txoptions *opt)
 }
 EXPORT_SYMBOL_GPL(ipv6_dup_options);
 
-static int ipv6_renew_option(void *ohdr,
-			     struct ipv6_opt_hdr __user *newopt, int newoptlen,
-			     int inherit,
+static void ipv6_renew_option(struct ipv6_opt_hdr *opt,
 			     struct ipv6_opt_hdr **hdr,
 			     char **p)
 {
-	if (inherit) {
-		if (ohdr) {
-			memcpy(*p, ohdr, ipv6_optlen((struct ipv6_opt_hdr *)ohdr));
-			*hdr = (struct ipv6_opt_hdr *)*p;
-			*p += CMSG_ALIGN(ipv6_optlen(*hdr));
-		}
-	} else {
-		if (newopt) {
-			if (copy_from_user(*p, newopt, newoptlen))
-				return -EFAULT;
-			*hdr = (struct ipv6_opt_hdr *)*p;
-			if (ipv6_optlen(*hdr) > newoptlen)
-				return -EINVAL;
-			*p += CMSG_ALIGN(newoptlen);
-		}
+	if (opt) {
+		memcpy(*p, opt, ipv6_optlen(opt));
+		*hdr = (struct ipv6_opt_hdr *)*p;
+		*p += CMSG_ALIGN(ipv6_optlen(*hdr));
 	}
-	return 0;
 }
 
 /**
@@ -1063,13 +1049,11 @@ static int ipv6_renew_option(void *ohdr,
  */
 struct ipv6_txoptions *
 ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
-		   int newtype,
-		   struct ipv6_opt_hdr __user *newopt, int newoptlen)
+		   int newtype, struct ipv6_opt_hdr *newopt)
 {
 	int tot_len = 0;
 	char *p;
 	struct ipv6_txoptions *opt2;
-	int err;
 
 	if (opt) {
 		if (newtype != IPV6_HOPOPTS && opt->hopopt)
@@ -1082,8 +1066,8 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 			tot_len += CMSG_ALIGN(ipv6_optlen(opt->dst1opt));
 	}
 
-	if (newopt && newoptlen)
-		tot_len += CMSG_ALIGN(newoptlen);
+	if (newopt)
+		tot_len += CMSG_ALIGN(ipv6_optlen(newopt));
 
 	if (!tot_len)
 		return NULL;
@@ -1098,67 +1082,25 @@ ipv6_renew_options(struct sock *sk, struct ipv6_txoptions *opt,
 	opt2->tot_len = tot_len;
 	p = (char *)(opt2 + 1);
 
-	err = ipv6_renew_option(opt ? opt->hopopt : NULL, newopt, newoptlen,
-				newtype != IPV6_HOPOPTS,
-				&opt2->hopopt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->dst0opt : NULL, newopt, newoptlen,
-				newtype != IPV6_RTHDRDSTOPTS,
-				&opt2->dst0opt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->srcrt : NULL, newopt, newoptlen,
-				newtype != IPV6_RTHDR,
-				(struct ipv6_opt_hdr **)&opt2->srcrt, &p);
-	if (err)
-		goto out;
-
-	err = ipv6_renew_option(opt ? opt->dst1opt : NULL, newopt, newoptlen,
-				newtype != IPV6_DSTOPTS,
-				&opt2->dst1opt, &p);
-	if (err)
-		goto out;
-
+	ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
+				opt ? opt->hopopt : NULL,
+			  &opt2->hopopt, &p);
+
+	ipv6_renew_option(newtype == IPV6_RTHDRDSTOPTS ? newopt :
+				opt ? opt->dst0opt : NULL,
+			&opt2->dst0opt, &p);
+	ipv6_renew_option(newtype == IPV6_RTHDR ? newopt :
+				opt ? (struct ipv6_opt_hdr *)opt->srcrt : NULL,
+			(struct ipv6_opt_hdr **)&opt2->srcrt, &p);
+	ipv6_renew_option(newtype == IPV6_DSTOPTS ? newopt :
+				opt ? opt->dst1opt : NULL,
+			&opt2->dst1opt, &p);
 	opt2->opt_nflen = (opt2->hopopt ? ipv6_optlen(opt2->hopopt) : 0) +
 			  (opt2->dst0opt ? ipv6_optlen(opt2->dst0opt) : 0) +
 			  (opt2->srcrt ? ipv6_optlen(opt2->srcrt) : 0);
 	opt2->opt_flen = (opt2->dst1opt ? ipv6_optlen(opt2->dst1opt) : 0);
 
 	return opt2;
-out:
-	sock_kfree_s(sk, opt2, opt2->tot_len);
-	return ERR_PTR(err);
-}
-
-/**
- * ipv6_renew_options_kern - replace a specific ext hdr with a new one.
- *
- * @sk: sock from which to allocate memory
- * @opt: original options
- * @newtype: option type to replace in @opt
- * @newopt: new option of type @newtype to replace (kernel-mem)
- * @newoptlen: length of @newopt
- *
- * See ipv6_renew_options().  The difference is that @newopt is
- * kernel memory, rather than user memory.
- */
-struct ipv6_txoptions *
-ipv6_renew_options_kern(struct sock *sk, struct ipv6_txoptions *opt,
-			int newtype, struct ipv6_opt_hdr *newopt,
-			int newoptlen)
-{
-	struct ipv6_txoptions *ret_val;
-	const mm_segment_t old_fs = get_fs();
-
-	set_fs(KERNEL_DS);
-	ret_val = ipv6_renew_options(sk, opt, newtype,
-				     (struct ipv6_opt_hdr __user *)newopt,
-				     newoptlen);
-	set_fs(old_fs);
-	return ret_val;
 }
 
 struct ipv6_txoptions *ipv6_fixup_options(struct ipv6_txoptions *opt_space,
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 4d780c7f0130..b69ba18e0138 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -398,6 +398,12 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 	case IPV6_DSTOPTS:
 	{
 		struct ipv6_txoptions *opt;
+		struct ipv6_opt_hdr *new = NULL;
+
+		/* hop-by-hop / destination options are privileged option */
+		retv = -EPERM;
+		if (optname != IPV6_RTHDR && !ns_capable(net->user_ns, CAP_NET_RAW))
+			break;
 
 		/* remove any sticky options header with a zero option
 		 * length, per RFC3542.
@@ -409,17 +415,23 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 		else if (optlen < sizeof(struct ipv6_opt_hdr) ||
 			 optlen & 0x7 || optlen > 8 * 255)
 			goto e_inval;
-
-		/* hop-by-hop / destination options are privileged option */
-		retv = -EPERM;
-		if (optname != IPV6_RTHDR && !ns_capable(net->user_ns, CAP_NET_RAW))
-			break;
+		else {
+			new = kmemdup(optval, optlen, GFP_USER);
+			if (IS_ERR(new)) {
+				retv = PTR_ERR(new);
+				break;
+			}
+			if (unlikely(ipv6_optlen(new) > optlen)) {
+				kfree(new);
+				retv = -EINVAL;
+				break;
+			}
+		}
 
 		opt = rcu_dereference_protected(np->opt,
 						lockdep_sock_is_held(sk));
-		opt = ipv6_renew_options(sk, opt, optname,
-					 (struct ipv6_opt_hdr __user *)optval,
-					 optlen);
+		opt = ipv6_renew_options(sk, opt, optname, new);
+		kfree(new);
 		if (IS_ERR(opt)) {
 			retv = PTR_ERR(opt);
 			break;
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 23 Jun 2018 23:21:07 +0100

> BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> ipv6_renew_options_kern()...

I agree that this makes things a lot simpler.

One thing that drives me crazy though is this inherit stuff:

> +	ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> +				opt ? opt->hopopt : NULL,

Why don't we pass the type into ipv6_renew_option() and have it
do this pointer dance instead?

That's going to definitely be easier to read.

I don't know enough about this code to give feedback about the
option length handling wrt. copies, sorry.

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Sat, 23 Jun 2018 23:21:07 +0100

> BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> ipv6_renew_options_kern()...

I agree that this makes things a lot simpler.

One thing that drives me crazy though is this inherit stuff:

> +	ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> +				opt ? opt->hopopt : NULL,

Why don't we pass the type into ipv6_renew_option() and have it
do this pointer dance instead?

That's going to definitely be easier to read.

I don't know enough about this code to give feedback about the
option length handling wrt. copies, sorry.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

On Sun, Jun 24, 2018 at 3:48 AM David Miller <davem@davemloft.net> wrote:
>
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Sat, 23 Jun 2018 23:21:07 +0100
>
> > BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> > the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> > simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> > ipv6_renew_options_kern()...
>
> I agree that this makes things a lot simpler.

I had looked at moving the userspace copy up, but feared it was a bit
too invasive.  It sounds like you are open to the idea so I'll code
something up.

> One thing that drives me crazy though is this inherit stuff:
>
> > +     ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> > +                             opt ? opt->hopopt : NULL,
>
> Why don't we pass the type into ipv6_renew_option() and have it
> do this pointer dance instead?
>
> That's going to definitely be easier to read.

I agree, that struck me as a little odd.  I'll rework that too.  I'll
send you guys something this week to take a look at.

Thanks.

> I don't know enough about this code to give feedback about the
> option length handling wrt. copies, sorry.

-- 
paul moore
www.paul-moore.com

[PATCH] ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

[Paul Moore]

On Sun, Jun 24, 2018 at 3:48 AM David Miller <davem@davemloft.net> wrote:
>
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Sat, 23 Jun 2018 23:21:07 +0100
>
> > BTW, I wonder if the life would be simpler with do_ipv6_setsockopt() doing
> > the copy-in and verifying ipv6_optlen(*hdr) <= newoptlen; that would've
> > simplified ipv6_renew_option{,s}() quite a bit and completely eliminated
> > ipv6_renew_options_kern()...
>
> I agree that this makes things a lot simpler.

I had looked at moving the userspace copy up, but feared it was a bit
too invasive.  It sounds like you are open to the idea so I'll code
something up.

> One thing that drives me crazy though is this inherit stuff:
>
> > +     ipv6_renew_option(newtype == IPV6_HOPOPTS ? newopt :
> > +                             opt ? opt->hopopt : NULL,
>
> Why don't we pass the type into ipv6_renew_option() and have it
> do this pointer dance instead?
>
> That's going to definitely be easier to read.

I agree, that struck me as a little odd.  I'll rework that too.  I'll
send you guys something this week to take a look at.

Thanks.

> I don't know enough about this code to give feedback about the
> option length handling wrt. copies, sorry.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html