From patchwork Wed Jan 6 23:27:42 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jesse Brandeburg X-Patchwork-Id: 42367 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 E3841B7BE8 for ; Thu, 7 Jan 2010 10:28:08 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756535Ab0AFX1p (ORCPT ); Wed, 6 Jan 2010 18:27:45 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1756533Ab0AFX1p (ORCPT ); Wed, 6 Jan 2010 18:27:45 -0500 Received: from mga11.intel.com ([192.55.52.93]:37274 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756530Ab0AFX1n (ORCPT ); Wed, 6 Jan 2010 18:27:43 -0500 Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 06 Jan 2010 15:27:18 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.49,232,1262592000"; d="scan'208";a="529091698" Received: from jbrandeb-desk1.amr.corp.intel.com ([134.134.3.173]) by fmsmga002.fm.intel.com with ESMTP; 06 Jan 2010 15:27:04 -0800 Date: Wed, 6 Jan 2010 15:27:42 -0800 (Pacific Standard Time) From: "Brandeburg, Jesse" To: Neil Horman cc: "netdev@vger.kernel.org" , "davem@davemloft.net" , "Kirsher, Jeffrey T" , "Allan, Bruce W" , "Waskiewicz Jr, Peter P" , "Ronciak, John" , "e1000-devel@lists.sourceforge.net" , jesse.brandeburg@intel.com Subject: Re: [PATCH] e1000: enhance frame fragment detection In-Reply-To: Message-ID: References: <20091228201005.GC18422@hmsreliant.think-freely.org> User-Agent: Alpine 2.00 (WNT 1167 2008-08-23) ReplyTo: "Brandeburg, Jesse" X-X-Sender: amrjbrandeb@imapmail.glb.intel.com MIME-Version: 1.0 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org a counter patch, without atomic ops, since we are protected by napi when modifying this variable. Originally From: Neil Horman Modified by: Jesse Brandeburg Hey all- A security discussion was recently given: http://events.ccc.de/congress/2009/Fahrplan//events/3596.en.html And a patch that I submitted awhile back was brought up. Apparently some of their testing revealed that they were able to force a buffer fragment in e1000 in which the trailing fragment was greater than 4 bytes. As a result the fragment check I introduced failed to detect the fragement and a partial invalid frame was passed up into the network stack. I've written this patch to correct it. I'm in the process of testing it now, but it makes good logical sense to me. Effectively it maintains a per-adapter state variable which detects a non-EOP frame, and discards it and subsequent non-EOP frames leading up to _and_ _including_ the next positive-EOP frame (as it is by definition the last fragment). This should prevent any and all partial frames from entering the network stack from e1000. Signed-off-by: Jesse Brandeburg Acked-by: Neil Horman --- drivers/net/e1000/e1000.h | 2 ++ drivers/net/e1000/e1000_main.c | 13 +++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) -- 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/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h index 2a567df..e8932db 100644 --- a/drivers/net/e1000/e1000.h +++ b/drivers/net/e1000/e1000.h @@ -326,6 +326,8 @@ struct e1000_adapter { /* for ioport free */ int bars; int need_ioport; + + bool discarding; }; enum e1000_state_t { diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c index 7e855f9..9bc9fcd 100644 --- a/drivers/net/e1000/e1000_main.c +++ b/drivers/net/e1000/e1000_main.c @@ -3850,13 +3850,22 @@ static bool e1000_clean_rx_irq(struct e1000_adapter *adapter, length = le16_to_cpu(rx_desc->length); /* !EOP means multiple descriptors were used to store a single - * packet, also make sure the frame isn't just CRC only */ - if (unlikely(!(status & E1000_RXD_STAT_EOP) || (length <= 4))) { + * packet, if thats the case we need to toss it. In fact, we + * to toss every packet with the EOP bit clear and the next + * frame that _does_ have the EOP bit set, as it is by + * definition only a frame fragment + */ + if (unlikely(!(status & E1000_RXD_STAT_EOP))) + adapter->discarding = true; + + if (adapter->discarding) { /* All receives must fit into a single buffer */ E1000_DBG("%s: Receive packet consumed multiple" " buffers\n", netdev->name); /* recycle */ buffer_info->skb = skb; + if (status & E1000_RXD_STAT_EOP) + adapter->discarding = false; goto next_desc; }