| Message ID | 20090930171833.5ce0011d@infradead.org |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Sep 30, 2009 at 17:18, Arjan van de Ven <arjan@infradead.org> wrote: > On Wed, 30 Sep 2009 15:38:12 +0200 > Hannes Eder <heder@google.com> wrote: >> > @@ -2353,17 +2357,25 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, >> > void __user >> *user, int *len) >> > { >> > unsigned char arg[128]; >> >> can MAX_ARG_LEN be used here? > > I am not convinced... it is a different numerical value, > so it could be an ABI change. Rather not do that in this > type of patch... For do_ip_vs_set_ctl MAX_ARG_LEN is used: static int do_ip_vs_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) { int ret; unsigned char arg[MAX_ARG_LEN]; ... I assume that will be fine for do_ip_vs_get_ctl as well. -Hannes -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Wed, 30 Sep 2009, Arjan van de Ven wrote: > fair enough; updated patch below > > >From 28ae217858e683c0c94c02219d46a9a9c87f61c6 Mon Sep 17 00:00:00 2001 > From: Arjan van de Ven <arjan@linux.intel.com> > Date: Wed, 30 Sep 2009 13:05:51 +0200 > Subject: [PATCH] ipvs: Add boundary check on ioctl arguments > > The ipvs code has a nifty system for doing the size of ioctl command copies; > it defines an array with values into which it indexes the cmd to find the > right length. > > Unfortunately, the ipvs code forgot to check if the cmd was in the range > that the array provides, allowing for an index outside of the array, > which then gives a "garbage" result into the length, which then gets > used for copying into a stack buffer. do_ip_vs_get_ctl and do_ip_vs_set_ctl are nf_sockopt_ops handlers, so the range is checked by nf_sockopt_find() in Netfilter code. get_arglen[] and set_arglen[] are minimum values for the length and they can be 0. Later len can be checked additionally and surely can exceed 128 (include/linux/ip_vs.h has all user structures). Can you show the exact cmd and len used, may be there is error in some command or may be the provided user structure is wrong? Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 30 Sep 2009 22:41:05 +0300 (EEST) Julian Anastasov <ja@ssi.bg> wrote: > > Hello, > > On Wed, 30 Sep 2009, Arjan van de Ven wrote: > > > fair enough; updated patch below > Later len can be checked > additionally and surely can exceed 128 (include/linux/ip_vs.h has > all user structures). the on-stack structure currently is 128 bytes though... > Can you show the exact cmd and len > used, may be there is error in some command or may be the > provided user structure is wrong? this comes from code inspection using gcc features; this is one of the (few) cases in the kernel where gcc cannot prove that the copy_from_user() length for the copy-to-stack is sufficiently bounds checked. I'm trying to make sure all these cases have complete enough checks, both for the obvious security reasons but also to be able to then make gcc emit a warning to prevent future issues from popping up.
Hello, On Wed, 30 Sep 2009, Arjan van de Ven wrote: > fair enough; updated patch below OK, you can add my signed-off line after changing 'cmd > ...MAX + 1' to 'cmd > ...MAX' at both places, nf_sockopt_ops ranges are [optmin ... optmax) May be comments should be changed because: - i'm not the author but after ispection we do not see any holes, we do not want users to upgrade just for this change - the cmd checks are just to help code checking tools - the len checks should help programmers (may be BUG_ON is better, user does not deserve EINVAL for wrong set_arglen/get_arglen). Checks for *len and len are not needed. For example, for len checks this should be enough, before copy_from_user(): in do_ip_vs_get_ctl check can be BUG_ON(get_arglen[GET_CMDID(cmd)] > sizeof(arg)); in do_ip_vs_set_ctl check can be BUG_ON(set_arglen[SET_CMDID(cmd)] > sizeof(arg)); Acked-by: Julian Anastasov <ja@ssi.bg> > >From 28ae217858e683c0c94c02219d46a9a9c87f61c6 Mon Sep 17 00:00:00 2001 > From: Arjan van de Ven <arjan@linux.intel.com> > Date: Wed, 30 Sep 2009 13:05:51 +0200 > Subject: [PATCH] ipvs: Add boundary check on ioctl arguments > > The ipvs code has a nifty system for doing the size of ioctl command copies; > it defines an array with values into which it indexes the cmd to find the > right length. > > Unfortunately, the ipvs code forgot to check if the cmd was in the range > that the array provides, allowing for an index outside of the array, > which then gives a "garbage" result into the length, which then gets > used for copying into a stack buffer. > > Fix this by adding sanity checks on these as well as the copy size. > > Signed-off-by: Arjan van de Ven <arjan@linux.intel.com> > --- > net/netfilter/ipvs/ip_vs_ctl.c | 14 +++++++++++++- > 1 files changed, 13 insertions(+), 1 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c > index ac624e5..7adc876 100644 > --- a/net/netfilter/ipvs/ip_vs_ctl.c > +++ b/net/netfilter/ipvs/ip_vs_ctl.c > @@ -2077,6 +2077,10 @@ do_ip_vs_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) > if (!capable(CAP_NET_ADMIN)) > return -EPERM; > > + if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_SET_MAX + 1) > + return -EINVAL; > + if (len < 0 || len > sizeof(arg)) > + return -EINVAL; > if (len != set_arglen[SET_CMDID(cmd)]) { > pr_err("set_ctl: len %u != %u\n", > len, set_arglen[SET_CMDID(cmd)]); > @@ -2353,17 +2357,25 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) > { > unsigned char arg[128]; > int ret = 0; > + unsigned int copylen; > > if (!capable(CAP_NET_ADMIN)) > return -EPERM; > > + if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_GET_MAX + 1) > + return -EINVAL; > + > if (*len < get_arglen[GET_CMDID(cmd)]) { > pr_err("get_ctl: len %u < %u\n", > *len, get_arglen[GET_CMDID(cmd)]); > return -EINVAL; > } > > - if (copy_from_user(arg, user, get_arglen[GET_CMDID(cmd)]) != 0) > + copylen = get_arglen[GET_CMDID(cmd)]; > + if (copylen > sizeof(arg)) > + return -EINVAL; > + > + if (copy_from_user(arg, user, copylen) != 0) > return -EFAULT; > > if (mutex_lock_interruptible(&__ip_vs_mutex)) Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c index ac624e5..7adc876 100644 --- a/net/netfilter/ipvs/ip_vs_ctl.c +++ b/net/netfilter/ipvs/ip_vs_ctl.c @@ -2077,6 +2077,10 @@ do_ip_vs_set_ctl(struct sock *sk, int cmd, void __user *user, unsigned int len) if (!capable(CAP_NET_ADMIN)) return -EPERM; + if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_SET_MAX + 1) + return -EINVAL; + if (len < 0 || len > sizeof(arg)) + return -EINVAL; if (len != set_arglen[SET_CMDID(cmd)]) { pr_err("set_ctl: len %u != %u\n", len, set_arglen[SET_CMDID(cmd)]); @@ -2353,17 +2357,25 @@ do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) { unsigned char arg[128]; int ret = 0; + unsigned int copylen; if (!capable(CAP_NET_ADMIN)) return -EPERM; + if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_GET_MAX + 1) + return -EINVAL; + if (*len < get_arglen[GET_CMDID(cmd)]) { pr_err("get_ctl: len %u < %u\n", *len, get_arglen[GET_CMDID(cmd)]); return -EINVAL; } - if (copy_from_user(arg, user, get_arglen[GET_CMDID(cmd)]) != 0) + copylen = get_arglen[GET_CMDID(cmd)]; + if (copylen > sizeof(arg)) + return -EINVAL; + + if (copy_from_user(arg, user, copylen) != 0) return -EFAULT; if (mutex_lock_interruptible(&__ip_vs_mutex))