From patchwork Wed Nov 3 22:54:53 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: andrew hendry X-Patchwork-Id: 70085 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.180.67]) by ozlabs.org (Postfix) with ESMTP id CC8F5B6EF0 for ; Thu, 4 Nov 2010 09:55:07 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753477Ab0KCWzC (ORCPT ); Wed, 3 Nov 2010 18:55:02 -0400 Received: from mail-qw0-f46.google.com ([209.85.216.46]:38914 "EHLO mail-qw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752825Ab0KCWzB (ORCPT ); Wed, 3 Nov 2010 18:55:01 -0400 Received: by qwf7 with SMTP id 7so630785qwf.19 for ; Wed, 03 Nov 2010 15:55:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:subject:from:to:cc :in-reply-to:references:content-type:date:message-id:mime-version :x-mailer:content-transfer-encoding; bh=ybQve5gt0EU8c3e9841K6VWqhXlv4xqgLqJjH8QyCos=; b=mvLL38mGFYFY0b7kUIwdlZIi9+2XwrnEGjhQgLAJyg5N0AltHsZmsS7LpKMl1Ly4Us ZtOxSvodh7uSQpXKhwJRClNJQb+2wuZvjYRyt69mpy9oqFIY56cso196PMkHexUI/7nj LTkHg5x7Fb+hTTohIbkX1NTHSSAz2jStxn/CE= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=QA28ZciYl++TN3plpVApMI8eNg0unfy+WNihT2/eyPOmno90ewC2Rhga98nxCLpvGQ ZyxTyXGfp+au9zmzB4I32AZOwqJ/fgtPrqs9lDnAEFy74k+8nvE7Y6+Z9o3KfHnGYlfJ pLAsCB15WNTE3zNzWadGaKssoq8BDML7/mwMs= Received: by 10.229.189.131 with SMTP id de3mr17543479qcb.183.1288824900269; Wed, 03 Nov 2010 15:55:00 -0700 (PDT) Received: from [192.168.0.3] (124-168-77-222.dyn.iinet.net.au [124.168.77.222]) by mx.google.com with ESMTPS id p32sm4784666vbl.5.2010.11.03.15.54.56 (version=SSLv3 cipher=RC4-MD5); Wed, 03 Nov 2010 15:54:58 -0700 (PDT) Subject: Re: [SECURITY] memory corruption in X.25 facilities parsing From: Andrew Hendry To: Dan Rosenberg Cc: netdev@vger.kernel.org, security@kernel.org, stable@kernel.org In-Reply-To: References: <1288710168.2504.6.camel@dan> Date: Thu, 04 Nov 2010 09:54:53 +1100 Message-ID: <1288824893.1858.5.camel@jaunty> Mime-Version: 1.0 X-Mailer: Evolution 2.30.3 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Wed, 2010-11-03 at 12:12 +1100, Andrew Hendry wrote: > There is an issue here, under select scenarios I can crash systems. > However the patch doesn't resolve it fully, I think after breaking at > that point the len and p pointers are messed up before it tries to > parse the next facility. > > Maybe it should return not break? It should reject/clear such calls. > I'll start checking if the callers properly handle errors. > Also should it be if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1), > because it does the memcpy with p[1] -1 > > > On Wed, Nov 3, 2010 at 2:02 AM, Dan Rosenberg wrote: > > I put this together after a quick glance, so if someone knows this code > > better than I do (i.e. at all), feel free to comment or drop this patch > > if it's unnecessary. > > > > A value of 0 will cause a memcpy() of ULONG_MAX size, destroying the > > kernel heap. > > > > Signed-off-by: Dan Rosenberg > > > > --- linux-2.6.36-rc6.orig/net/x25/x25_facilities.c 2010-09-28 21:01:22.000000000 -0400 > > +++ linux-2.6.36-rc6/net/x25/x25_facilities.c 2010-11-02 10:36:02.827291324 -0400 > > @@ -134,14 +134,14 @@ int x25_parse_facilities(struct sk_buff > > case X25_FAC_CLASS_D: > > switch (*p) { > > case X25_FAC_CALLING_AE: > > - if (p[1] > X25_MAX_DTE_FACIL_LEN) > > + if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] == 0) > > break; > > dte_facs->calling_len = p[2]; > > memcpy(dte_facs->calling_ae, &p[3], p[1] - 1); > > *vc_fac_mask |= X25_MASK_CALLING_AE; > > break; > > case X25_FAC_CALLED_AE: > > - if (p[1] > X25_MAX_DTE_FACIL_LEN) > > + if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] == 0) > > break; > > dte_facs->called_len = p[2]; > > memcpy(dte_facs->called_ae, &p[3], p[1] - 1); > > > > > > How does this look? It appears to fix it for the cases I could test. Signed-of-by: Andrew Hendry --- 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/x25/x25_facilities.c b/net/x25/x25_facilities.c index 771bab0..3a8c4c4 100644 --- a/net/x25/x25_facilities.c +++ b/net/x25/x25_facilities.c @@ -134,15 +134,15 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, case X25_FAC_CLASS_D: switch (*p) { case X25_FAC_CALLING_AE: - if (p[1] > X25_MAX_DTE_FACIL_LEN) - break; + if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) + return 0; dte_facs->calling_len = p[2]; memcpy(dte_facs->calling_ae, &p[3], p[1] - 1); *vc_fac_mask |= X25_MASK_CALLING_AE; break; case X25_FAC_CALLED_AE: - if (p[1] > X25_MAX_DTE_FACIL_LEN) - break; + if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) + return 0; dte_facs->called_len = p[2]; memcpy(dte_facs->called_ae, &p[3], p[1] - 1); *vc_fac_mask |= X25_MASK_CALLED_AE; diff --git a/net/x25/x25_in.c b/net/x25/x25_in.c index 6317896..1d80e10 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -119,6 +119,8 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp &x25->vc_facil_mask); if (len > 0) skb_pull(skb, len); + else + return -1; /* * Copy any Call User Data. */