diff mbox

ipv6: avoid copy_from_user() via ipv6_renew_options_kern()

Message ID 152970230022.7734.15824980755229329454.stgit@chester (mailing list archive)
State New, archived
Headers show

Commit Message

Paul Moore June 22, 2018, 9:18 p.m. UTC 
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(-)


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

Comments

David Miller June 23, 2018, 1:57 a.m. UTC | #1 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller June 23, 2018, 12:16 p.m. UTC | #2 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Paul Moore June 23, 2018, 4:15 p.m. UTC | #3 
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).
Al Viro June 23, 2018, 9:26 p.m. UTC | #4 
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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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,