All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH V3 net-next] ixgbe: Avoid unaligned access in ixgbe_atr() for LLC packets
Date: Wed, 16 Mar 2016 15:58:52 -0700	[thread overview]
Message-ID: <1458169132.2878.25.camel@intel.com> (raw)
In-Reply-To: <20160314194716.GM5084@oracle.com>

On Mon, 2016-03-14 at 15:47 -0400, Sowmini Varadhan wrote:
> 
> For LLC based protocols like lldp, stp etc., the ethernet header
> is an 802.3 header with a h_proto that is not 0x800, 0x86dd, or
> even 0x806.? In this world, the skb_network_header() points at
> the DSAP/SSAP/..? and is not likely to be NET_IP_ALIGNed in
> ixgbe_atr().
> 
> With LLC, drivers are not likely to correctly find IPVERSION,
> or "6", at hdr.ipv4->version, but will instead just needlessly
> trigger an unaligned access. (IPv4/IPv6 over LLC is almost never
> implemented).
> 
> The unaligned access is thus avoidable: bail out quickly after
> examining skb->protocol. The only Ethernet II protocols handled
> are IPv4 and IPv6
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> v2: Alexander Duyck comments
> v3: filter out all ethertypes? except for Ethernet II and (IPv4 or
> IPv6)
> 
> ?drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |??? 5 +++++
> ?1 files changed, 5 insertions(+), 0 deletions(-)

This does not apply since Alex Duyck beat you to the fix. ?Here is the
patch he submitted on 3/15 which corrects the issue. ?So I am dropping
your patch from the queue.

commit 04b8b51c34837765cf6250f69d419c439dc393bf
Author: Alexander Duyck <aduyck@mirantis.com>
Date:???Tue Mar 15 15:10:22 2016 -0700

????ixgbe: Fix ATR so that it correctly handles IPv6 extension headers
????
????The ATR code was assuming that it would be able to use tcp_hdr for
????every TCP frame that came through.??However this isn't the case as
it
????is possible for a frame to arrive that is TCP but sent through
something
????like a raw socket.??As a result the driver was setting up bad
filters in
????which tcp_hdr was really pointing to the network header so the data
was
????all invalid.
????
????In order to correct this I have added a bit of parsing logic that
will
????determine the TCP header location based off of the network header
and
????either the offset in the case of the IPv4 header, or a walk through
the
????IPv6 extension headers until it encounters the header that
indicates
????IPPROTO_TCP.??In addition I have added checks to verify that the
lowest
????protocol provided is recognized as IPv4 or IPv6 to help mitigate
raw
????sockets using ETH_P_ALL from having ATR applied to them.
????
????Signed-off-by: Alexander Duyck <aduyck@mirantis.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160316/05758d47/attachment.asc>

WARNING: multiple messages have this Message-ID (diff)
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
To: Sowmini Varadhan <sowmini.varadhan@oracle.com>,
	intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	alexander.duyck@gmail.com
Cc: jesse.brandeburg@intel.com, shannon.nelson@intel.com,
	carolyn.wyborny@intel.com, donald.c.skidmore@intel.com,
	bruce.w.allan@intel.com, john.ronciak@intel.com,
	mitch.a.williams@intel.com
Subject: Re: [PATCH V3 net-next] ixgbe: Avoid unaligned access in ixgbe_atr() for LLC packets
Date: Wed, 16 Mar 2016 15:58:52 -0700	[thread overview]
Message-ID: <1458169132.2878.25.camel@intel.com> (raw)
In-Reply-To: <20160314194716.GM5084@oracle.com>

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]

On Mon, 2016-03-14 at 15:47 -0400, Sowmini Varadhan wrote:
> 
> For LLC based protocols like lldp, stp etc., the ethernet header
> is an 802.3 header with a h_proto that is not 0x800, 0x86dd, or
> even 0x806.  In this world, the skb_network_header() points at
> the DSAP/SSAP/..  and is not likely to be NET_IP_ALIGNed in
> ixgbe_atr().
> 
> With LLC, drivers are not likely to correctly find IPVERSION,
> or "6", at hdr.ipv4->version, but will instead just needlessly
> trigger an unaligned access. (IPv4/IPv6 over LLC is almost never
> implemented).
> 
> The unaligned access is thus avoidable: bail out quickly after
> examining skb->protocol. The only Ethernet II protocols handled
> are IPv4 and IPv6
> 
> Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> ---
> v2: Alexander Duyck comments
> v3: filter out all ethertypes  except for Ethernet II and (IPv4 or
> IPv6)
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)

This does not apply since Alex Duyck beat you to the fix.  Here is the
patch he submitted on 3/15 which corrects the issue.  So I am dropping
your patch from the queue.

commit 04b8b51c34837765cf6250f69d419c439dc393bf
Author: Alexander Duyck <aduyck@mirantis.com>
Date:   Tue Mar 15 15:10:22 2016 -0700

    ixgbe: Fix ATR so that it correctly handles IPv6 extension headers
    
    The ATR code was assuming that it would be able to use tcp_hdr for
    every TCP frame that came through.  However this isn't the case as
it
    is possible for a frame to arrive that is TCP but sent through
something
    like a raw socket.  As a result the driver was setting up bad
filters in
    which tcp_hdr was really pointing to the network header so the data
was
    all invalid.
    
    In order to correct this I have added a bit of parsing logic that
will
    determine the TCP header location based off of the network header
and
    either the offset in the case of the IPv4 header, or a walk through
the
    IPv6 extension headers until it encounters the header that
indicates
    IPPROTO_TCP.  In addition I have added checks to verify that the
lowest
    protocol provided is recognized as IPv4 or IPv6 to help mitigate
raw
    sockets using ETH_P_ALL from having ATR applied to them.
    
    Signed-off-by: Alexander Duyck <aduyck@mirantis.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

  reply	other threads:[~2016-03-16 22:58 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-14 19:47 [Intel-wired-lan] [PATCH V3 net-next] ixgbe: Avoid unaligned access in ixgbe_atr() for LLC packets Sowmini Varadhan
2016-03-14 19:47 ` Sowmini Varadhan
2016-03-16 22:58 ` Jeff Kirsher [this message]
2016-03-16 22:58   ` Jeff Kirsher
2016-03-16 23:01   ` [Intel-wired-lan] " Sowmini Varadhan
2016-03-16 23:01     ` Sowmini Varadhan
2016-03-17  1:31     ` [Intel-wired-lan] " Alexander Duyck
2016-03-17  1:31       ` Alexander Duyck

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1458169132.2878.25.camel@intel.com \
    --to=jeffrey.t.kirsher@intel.com \
    --cc=intel-wired-lan@osuosl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.