[PATCH] ipvs: Add boundary check on ioctl arguments

[PATCH] ipvs: Add boundary check on ioctl arguments

[Arjan van de Ven]

>From 761a182f96b3707e1fee44e1079ba227e48745d1 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..3c52796 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 >  MAX_ARG_LEN)
+		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 > 128)
+		return -EINVAL;
+
+	if (copy_from_user(arg, user, copylen) != 0)
 		return -EFAULT;
 
 	if (mutex_lock_interruptible(&__ip_vs_mutex))
-- 
1.6.2.5


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Hannes Eder]

[cc: +Simon Horman]

Arjan van de Ven wrote:
 > From 761a182f96b3707e1fee44e1079ba227e48745d1 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..3c52796 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 >  MAX_ARG_LEN)
 > +		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];

can MAX_ARG_LEN be used here?

 >  	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 > 128)

I think it's better to use 'copylen > sizeof(arg)' here.

 > +		return -EINVAL;
 > +
 > +	if (copy_from_user(arg, user, copylen) != 0)
 >  		return -EFAULT;
 >
 >  	if (mutex_lock_interruptible(&__ip_vs_mutex))

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Arjan van de Ven]

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...

>  > +	copylen = get_arglen[GET_CMDID(cmd)];
>  > +	if (copylen > 128)
> 
> I think it's better to use 'copylen > sizeof(arg)' here.

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.

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))
-- 
1.6.2.5



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Hannes Eder]

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

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Julian Anastasov]

	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>

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Arjan van de Ven]

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.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Julian Anastasov]

	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>

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Arjan van de Ven]

On Wed, 30 Sep 2009 13:11:09 +0200
Arjan van de Ven <arjan@infradead.org> wrote:

ping on the patch below.... the warning is now triggered in mainline


> 
> >From 761a182f96b3707e1fee44e1079ba227e48745d1 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..3c52796 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 >  MAX_ARG_LEN)
> +		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 > 128)
> +		return -EINVAL;
> +
> +	if (copy_from_user(arg, user, copylen) != 0)
>  		return -EFAULT;
>  
>  	if (mutex_lock_interruptible(&__ip_vs_mutex))


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Simon Horman]

On Mon, Dec 14, 2009 at 10:17:04PM -0800, Arjan van de Ven wrote:
> On Wed, 30 Sep 2009 13:11:09 +0200
> Arjan van de Ven <arjan@infradead.org> wrote:
> 
> ping on the patch below.... the warning is now triggered in mainline

Hi Arjan,

could you address the comments Julian made about this?
http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html

	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 761a182f96b3707e1fee44e1079ba227e48745d1 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..3c52796 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 >  MAX_ARG_LEN)
> > +		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 > 128)
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(arg, user, copylen) != 0)
> >  		return -EFAULT;
> >  
> >  	if (mutex_lock_interruptible(&__ip_vs_mutex))
> 
> 
> -- 
> Arjan van de Ven 	Intel Open Source Technology Centre
> For development, discussion and tips for power savings, 
> visit http://www.lesswatts.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Simon Horman]

On Tue, Dec 15, 2009 at 05:32:01PM +1100, Simon Horman wrote:
> On Mon, Dec 14, 2009 at 10:17:04PM -0800, Arjan van de Ven wrote:
> > On Wed, 30 Sep 2009 13:11:09 +0200
> > Arjan van de Ven <arjan@infradead.org> wrote:
> > 
> > ping on the patch below.... the warning is now triggered in mainline
> 
> Hi Arjan,
> 
> could you address the comments Julian made about this?
> http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html
> 
> 	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>

Ping.

While I agree with Julian that the patch you suggest below
ought not to be necessary, I'm also happy with it if
cmd > IP_VS_SO_SET_MAX + 1 is changed to cmd > IP_VS_SO_SET_MAX.

> > > >From 761a182f96b3707e1fee44e1079ba227e48745d1 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..3c52796 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 >  MAX_ARG_LEN)
> > > +		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 > 128)
> > > +		return -EINVAL;
> > > +
> > > +	if (copy_from_user(arg, user, copylen) != 0)
> > >  		return -EFAULT;
> > >  
> > >  	if (mutex_lock_interruptible(&__ip_vs_mutex))
> > 
> > 
> > -- 
> > Arjan van de Ven 	Intel Open Source Technology Centre
> > For development, discussion and tips for power savings, 
> > visit http://www.lesswatts.org
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH] ipvs: Add boundary check on ioctl arguments

[Simon Horman]

From: Arjan van de Ven <arjan@linux.intel.com>

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.

[ horms@verge.net.au: adjusted limit to IP_VS_SO_GET_MAX ]
Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
Acked-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>

---

 net/netfilter/ipvs/ip_vs_ctl.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

Hi Arjen,

this is the 4th response to your patch. I am guessing the previous
ones didn't reach you for some reason. And I guess this one wont
for the same reason.

I agree with Julian's assessment that your patch shouldn't be
necessary, but on the other hand I think that the checks are
reasonable. Your original patch made checks of the form of
"cmd > IP_VS_SO_GET_MAX + 1". I have updated this to
"cmd > IP_VS_SO_GET_MAX", as suggested by Julian, as the optmax
elements of struct nf_sockopt_ops set a non-inclusive range.

http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html

Index: net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c
===================================================================
--- net-next-2.6.orig/net/netfilter/ipvs/ip_vs_ctl.c	2009-12-29 12:39:40.000000000 +1100
+++ net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c	2009-12-29 12:46:47.000000000 +1100
@@ -2077,6 +2077,10 @@ do_ip_vs_set_ctl(struct sock *sk, int cm
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
+	if (cmd < IP_VS_BASE_CTL || cmd > IP_VS_SO_SET_MAX)
+		return -EINVAL;
+	if (len < 0 || len >  MAX_ARG_LEN)
+		return -EINVAL;
 	if (len != set_arglen[SET_CMDID(cmd)]) {
 		pr_err("set_ctl: len %u != %u\n",
 		       len, set_arglen[SET_CMDID(cmd)]);
@@ -2352,17 +2356,25 @@ do_ip_vs_get_ctl(struct sock *sk, int cm
 {
 	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)
+		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 > 128)
+		return -EINVAL;
+
+	if (copy_from_user(arg, user, copylen) != 0)
 		return -EFAULT;
 
 	if (mutex_lock_interruptible(&__ip_vs_mutex))

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Patrick McHardy]

Simon Horman wrote:
> From: Arjan van de Ven <arjan@linux.intel.com>
> 
> 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.
> 
> [ horms@verge.net.au: adjusted limit to IP_VS_SO_GET_MAX ]
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> Acked-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> 
>  net/netfilter/ipvs/ip_vs_ctl.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> Hi Arjen,
> 
> this is the 4th response to your patch. I am guessing the previous
> ones didn't reach you for some reason. And I guess this one wont
> for the same reason.
> 
> I agree with Julian's assessment that your patch shouldn't be
> necessary, but on the other hand I think that the checks are
> reasonable. Your original patch made checks of the form of
> "cmd > IP_VS_SO_GET_MAX + 1". I have updated this to
> "cmd > IP_VS_SO_GET_MAX", as suggested by Julian, as the optmax
> elements of struct nf_sockopt_ops set a non-inclusive range.
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html
> 
> Index: net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c

As a bugfix, this seems more appropriate for net-2.6.git. Please let
me know which tree you want me to apply this to.

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Arjan van de Ven]

On 1/4/2010 5:59, Patrick McHardy wrote:

[sorry for the late response, just got back from a good holiday, which means no work email access ;-) ]

> Simon Horman wrote:

>> I agree with Julian's assessment that your patch shouldn't be
>> necessary, but on the other hand I think that the checks are
>> reasonable. Your original patch made checks of the form of
>> "cmd>  IP_VS_SO_GET_MAX + 1". I have updated this to
>> "cmd>  IP_VS_SO_GET_MAX", as suggested by Julian, as the optmax
>> elements of struct nf_sockopt_ops set a non-inclusive range.
>>
>> http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html
>>
>> Index: net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c
>
> As a bugfix, this seems more appropriate for net-2.6.git. Please let
> me know which tree you want me to apply this to.

this really ought to go into 2.6.33.....

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Patrick McHardy]

Arjan van de Ven wrote:
> On 1/4/2010 5:59, Patrick McHardy wrote:
> 
> [sorry for the late response, just got back from a good holiday, which
> means no work email access ;-) ]
> 
>> Simon Horman wrote:
> 
>>> I agree with Julian's assessment that your patch shouldn't be
>>> necessary, but on the other hand I think that the checks are
>>> reasonable. Your original patch made checks of the form of
>>> "cmd>  IP_VS_SO_GET_MAX + 1". I have updated this to
>>> "cmd>  IP_VS_SO_GET_MAX", as suggested by Julian, as the optmax
>>> elements of struct nf_sockopt_ops set a non-inclusive range.
>>>
>>> http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html
>>>
>>> Index: net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c
>>
>> As a bugfix, this seems more appropriate for net-2.6.git. Please let
>> me know which tree you want me to apply this to.
> 
> this really ought to go into 2.6.33.....

Thanks, applied and will send it upstream soon.

Re: [PATCH] ipvs: Add boundary check on ioctl arguments

[Simon Horman]

On Mon, Jan 04, 2010 at 04:39:03PM +0100, Patrick McHardy wrote:
> Arjan van de Ven wrote:
> > On 1/4/2010 5:59, Patrick McHardy wrote:
> > 
> > [sorry for the late response, just got back from a good holiday, which
> > means no work email access ;-) ]
> > 
> >> Simon Horman wrote:
> > 
> >>> I agree with Julian's assessment that your patch shouldn't be
> >>> necessary, but on the other hand I think that the checks are
> >>> reasonable. Your original patch made checks of the form of
> >>> "cmd>  IP_VS_SO_GET_MAX + 1". I have updated this to
> >>> "cmd>  IP_VS_SO_GET_MAX", as suggested by Julian, as the optmax
> >>> elements of struct nf_sockopt_ops set a non-inclusive range.
> >>>
> >>> http://lkml.indiana.edu/hypermail/linux/kernel/0910.0/00852.html
> >>>
> >>> Index: net-next-2.6/net/netfilter/ipvs/ip_vs_ctl.c
> >>
> >> As a bugfix, this seems more appropriate for net-2.6.git. Please let
> >> me know which tree you want me to apply this to.
> > 
> > this really ought to go into 2.6.33.....
> 
> Thanks, applied and will send it upstream soon.

Thanks