netdev - [PATCH] ipvs: Add boundary check on ioctl arguments

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20091229015822.GF10172@verge.net.au>
Date:	Tue, 29 Dec 2009 12:58:22 +1100
From:	Simon Horman <horms@...ge.net.au>
To:	netdev@...r.kernel.org, lvs-devel@...r.kernel.org
Cc:	Arjan van de Ven <arjan@...ux.intel.com>,
	Wensong Zhang <wensong@...ux-vs.org>,
	Julian Anastasov <ja@....bg>,
	David Miller <davem@...emloft.net>,
	Patrick McHardy <kaber@...sh.net>
Subject: [PATCH] ipvs: Add boundary check on ioctl arguments

From: Arjan van de Ven <arjan@...ux.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@...ge.net.au: adjusted limit to IP_VS_SO_GET_MAX ]
Signed-off-by: Arjan van de Ven <arjan@...ux.intel.com>
Acked-by: Julian Anastasov <ja@....bg>
Signed-off-by: Simon Horman <horms@...ge.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))
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html