From patchwork Wed Sep 30 15:18:33 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Arjan van de Ven X-Patchwork-Id: 34607 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id D52F1B7BC5 for ; Thu, 1 Oct 2009 01:18:57 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754638AbZI3PSN (ORCPT ); Wed, 30 Sep 2009 11:18:13 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754521AbZI3PSM (ORCPT ); Wed, 30 Sep 2009 11:18:12 -0400 Received: from casper.infradead.org ([85.118.1.10]:35343 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754480AbZI3PSL convert rfc822-to-8bit (ORCPT ); Wed, 30 Sep 2009 11:18:11 -0400 Received: from [83.119.188.87] (helo=localhost.localdomain) by casper.infradead.org with esmtpsa (Exim 4.69 #1 (Red Hat Linux)) id 1Mt0wQ-0003Hs-0A; Wed, 30 Sep 2009 15:18:06 +0000 Date: Wed, 30 Sep 2009 17:18:33 +0200 From: Arjan van de Ven To: Hannes Eder Cc: Wensong Zhang , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Simon Horman Subject: Re: [PATCH] ipvs: Add boundary check on ioctl arguments Message-ID: <20090930171833.5ce0011d@infradead.org> In-Reply-To: <4AC35F44.60707@google.com> References: <20090930131109.2b3f71b8@infradead.org> <4AC35F44.60707@google.com> Organization: Intel X-Mailer: Claws Mail 3.7.2 (GTK+ 2.16.6; i586-redhat-linux-gnu) Mime-Version: 1.0 X-SRS-Rewrite: SMTP reverse-path rewritten from by casper.infradead.org See http://www.infradead.org/rpr.html Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 30 Sep 2009 15:38:12 +0200 Hannes Eder 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 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 Acked-by: Julian Anastasov --- 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))