From patchwork Mon Feb 7 10:08:15 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: andrew hendry X-Patchwork-Id: 82093 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 CDA85B7088 for ; Mon, 7 Feb 2011 21:08:58 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752586Ab1BGKI3 (ORCPT ); Mon, 7 Feb 2011 05:08:29 -0500 Received: from mail-gw0-f46.google.com ([74.125.83.46]:56197 "EHLO mail-gw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752075Ab1BGKI1 (ORCPT ); Mon, 7 Feb 2011 05:08:27 -0500 Received: by gwj20 with SMTP id 20so1623270gwj.19 for ; Mon, 07 Feb 2011 02:08:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:subject:from:to:cc:in-reply-to:references :content-type:date:message-id:mime-version:x-mailer :content-transfer-encoding; bh=m6oVoyBf3j4OoQ1/OtiHrTbECEvgri99izMs2Pdkgkg=; b=bE56oz4WhXfEPvy8KvAA9RJorv6ZJ81o9gzaCI8U32R86ZAc7v4q51N2y9j7u8BW1Z G39SdA3T9yAjaSOAPW2dizMJUlcufM8A32t2/trZRq009aUF0+3K1eBGVV0s5BxcHDmv ZupKEU3WyYi8hGbuLDA0zdCaHpYNpwxdRTviA= 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=ITbBu3RUyI07wqb0J2mXMrZSIpgv+IGO5MrXvV9gwvfmtC+qmOBhvkj/CS4uStAitQ 440tc6BfIZVFIz0mSHJZ9UxSVsOAntg2sqpvEZuU3BbwXVNXWdXzd+eBVg3UKKATZ1rK qQ0HnGXgHtKIl0ZA79JXonYlA6tPFSoSRDaqw= Received: by 10.147.170.14 with SMTP id x14mr4417520yao.36.1297073306314; Mon, 07 Feb 2011 02:08:26 -0800 (PST) Received: from [192.168.0.3] (203-217-92-139.dyn.iinet.net.au [203.217.92.139]) by mx.google.com with ESMTPS id g63sm2779674yhd.15.2011.02.07.02.08.20 (version=SSLv3 cipher=RC4-MD5); Mon, 07 Feb 2011 02:08:24 -0800 (PST) Subject: Re: x25: possible skb leak on bad facilities From: Andrew Hendry To: David Miller Cc: apw@canonical.com, john@calva.com, linux-x25@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, tim.gardner@canonical.com In-Reply-To: References: <20110131130826.GC16804@shadowen.org> <20110206.202824.260090071.davem@davemloft.net> Date: Mon, 07 Feb 2011 21:08:15 +1100 Message-ID: <1297073295.9577.13.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 Originally x25_parse_facilities returned -1 for an error 0 meaning 0 length facilities >0 the length of the facilities parsed. 5ef41308f94dc introduced more error checking in x25_parse_facilities however used 0 to indicate bad parsing a6331d6f9a429 followed this further for DTE facilities, again using 0 for bad parsing. The meaning of 0 got confused in the callers. If the facilities are messed up we can't determine where the data starts. So patch makes all parsing errors return -1 and ensures callers close and don't use the skb further. Reported-by: Andy Whitcroft Signed-off-by: Andrew Hendry --- net/x25/x25_facilities.c | 28 +++++++++++++++++++--------- net/x25/x25_in.c | 14 +++++++++++--- 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/net/x25/x25_facilities.c b/net/x25/x25_facilities.c index 55187c8..4062075 100644 --- a/net/x25/x25_facilities.c +++ b/net/x25/x25_facilities.c @@ -27,9 +27,19 @@ #include #include -/* - * Parse a set of facilities into the facilities structures. Unrecognised - * facilities are written to the debug log file. +/** + * x25_parse_facilities - Parse facilities from skb into the facilities structs + * + * @skb: sk_buff to parse + * @facilities: Regular facilites, updated as facilities are found + * @dte_facs: ITU DTE facilities, updated as DTE facilities are found + * @vc_fac_mask: mask is updated with all facilities found + * + * Return codes: + * -1 - Parsing error, caller should drop call and clean up + * 0 - Parse OK, this skb has no facilities + * >0 - Parse OK, returns the length of the facilities header + * */ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, struct x25_dte_facilities *dte_facs, unsigned long *vc_fac_mask) @@ -62,7 +72,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, switch (*p & X25_FAC_CLASS_MASK) { case X25_FAC_CLASS_A: if (len < 2) - return 0; + return -1; switch (*p) { case X25_FAC_REVERSE: if((p[1] & 0x81) == 0x81) { @@ -107,7 +117,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, break; case X25_FAC_CLASS_B: if (len < 3) - return 0; + return -1; switch (*p) { case X25_FAC_PACKET_SIZE: facilities->pacsize_in = p[1]; @@ -130,7 +140,7 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, break; case X25_FAC_CLASS_C: if (len < 4) - return 0; + return -1; printk(KERN_DEBUG "X.25: unknown facility %02X, " "values %02X, %02X, %02X\n", p[0], p[1], p[2], p[3]); @@ -139,18 +149,18 @@ int x25_parse_facilities(struct sk_buff *skb, struct x25_facilities *facilities, break; case X25_FAC_CLASS_D: if (len < p[1] + 2) - return 0; + return -1; switch (*p) { case X25_FAC_CALLING_AE: if (p[1] > X25_MAX_DTE_FACIL_LEN || p[1] <= 1) - return 0; + return -1; 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 || p[1] <= 1) - return 0; + return -1; 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 f729f02..15de65f 100644 --- a/net/x25/x25_in.c +++ b/net/x25/x25_in.c @@ -91,10 +91,10 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp { struct x25_address source_addr, dest_addr; int len; + struct x25_sock *x25 = x25_sk(sk); switch (frametype) { case X25_CALL_ACCEPTED: { - struct x25_sock *x25 = x25_sk(sk); x25_stop_timer(sk); x25->condition = 0x00; @@ -113,14 +113,16 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp &dest_addr); if (len > 0) skb_pull(skb, len); + else if (len < 0) + goto out_clear; len = x25_parse_facilities(skb, &x25->facilities, &x25->dte_facilities, &x25->vc_facil_mask); if (len > 0) skb_pull(skb, len); - else - return -1; + else if (len < 0) + goto out_clear; /* * Copy any Call User Data. */ @@ -144,6 +146,12 @@ static int x25_state1_machine(struct sock *sk, struct sk_buff *skb, int frametyp } return 0; + +out_clear: + x25_write_internal(sk, X25_CLEAR_REQUEST); + x25->state = X25_STATE_2; + x25_start_t23timer(sk); + return 0; } /*