netdev - Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's len

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180516222400.GB25268@eros>
Date:   Thu, 17 May 2018 08:24:00 +1000
From:   "Tobin C. Harding" <tobin@...orbit.com>
To:     William Tu <u9012063@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] erspan: set bso bit based on mirrored packet's
 len

On Wed, May 16, 2018 at 07:05:34AM -0700, William Tu wrote:
> On Mon, May 14, 2018 at 10:33 PM, Tobin C. Harding <tobin@...orbit.com> wrote:
> > On Mon, May 14, 2018 at 04:54:36PM -0700, William Tu wrote:
> >> Before the patch, the erspan BSO bit (Bad/Short/Oversized) is not
> >> handled.  BSO has 4 possible values:
> >>   00 --> Good frame with no error, or unknown integrity
> >>   11 --> Payload is a Bad Frame with CRC or Alignment Error
> >>   01 --> Payload is a Short Frame
> >>   10 --> Payload is an Oversized Frame
> >>
> >> Based the short/oversized definitions in RFC1757, the patch sets
> >> the bso bit based on the mirrored packet's size.
> >>
> >> Reported-by: Xiaoyan Jin <xiaoyanj@...are.com>
> >> Signed-off-by: William Tu <u9012063@...il.com>
> >> ---
> >>  include/net/erspan.h | 25 +++++++++++++++++++++++++
> >>  1 file changed, 25 insertions(+)
> >>
> >> diff --git a/include/net/erspan.h b/include/net/erspan.h
> >> index d044aa60cc76..5eb95f78ad45 100644
> >> --- a/include/net/erspan.h
> >> +++ b/include/net/erspan.h
> >> @@ -219,6 +219,30 @@ static inline __be32 erspan_get_timestamp(void)
> >>       return htonl((u32)h_usecs);
> >>  }
> >>
> >> +/* ERSPAN BSO (Bad/Short/Oversized)
> >> + *   00b --> Good frame with no error, or unknown integrity
> >> + *   01b --> Payload is a Short Frame
> >> + *   10b --> Payload is an Oversized Frame
> >> + *   11b --> Payload is a Bad Frame with CRC or Alignment Error
> >> + */
> >> +enum erspan_bso {
> >> +     BSO_NOERROR,
> >> +     BSO_SHORT,
> >> +     BSO_OVERSIZED,
> >> +     BSO_BAD,
> >> +};
> >
> > If we are relying on the values perhaps this would be clearer
> >
> >         BSO_NOERROR     = 0x00,
> >         BSO_SHORT       = 0x01,
> >         BSO_OVERSIZED   = 0x02,
> >         BSO_BAD         = 0x03,
> >
> 
> Yes, thanks. I will change in v2.
> 
> >> +
> >> +static inline u8 erspan_detect_bso(struct sk_buff *skb)
> >> +{
> >> +     if (skb->len < ETH_ZLEN)
> >> +             return BSO_SHORT;
> >> +
> >> +     if (skb->len > ETH_FRAME_LEN)
> >> +             return BSO_OVERSIZED;
> >> +
> >> +     return BSO_NOERROR;
> >> +}
> >
> > Without having much contextual knowledge around this patch; should we be
> > doing some check on CRC or alignment (at some stage)?  Having BSO_BAD
> > seems to imply so?
> >
> 
> The definition of BSO_BAD:
> etherStatsCRCAlignErrors OBJECT-TYPE
>               SYNTAX Counter
>               ACCESS read-only
>               STATUS mandatory
>               DESCRIPTION
>                   "The total number of packets received that
>                   had a length (excluding framing bits, but
>                   including FCS octets) of between 64 and 1518
>                   octets, inclusive, but but had either a bad
>                   Frame Check Sequence (FCS) with an integral
>                   number of octets (FCS Error) or a bad FCS with
>                   a non-integral number of octets (Alignment Error)."
>
> But I don't know how to check CRC error at this code point.
> Isn't it done by the NIC hardware?

I'll just start with; I don't know anything about ERSPAN

	"ERSPAN is a Cisco proprietary feature and is available only to
	Catalyst 6500, 7600, Nexus, and ASR 1000 platforms to date. The
	ASR 1000 supports ERSPAN source (monitoring) only on Fast
	Ethernet, Gigabit Ethernet, and port-channel interfaces."

https://supportforums.cisco.com/t5/network-infrastructure-documents/understanding-span-rspan-and-erspan/ta-p/3144951

I dug around a bit and none of the files that currently import erspan.h
actually use the 'bso' field

$ grep bso $(git grep -l 'erspan\.h')
include/net/erspan.h:	u8 bso = 0; /* Bad/Short/Oversized */
include/net/erspan.h:	ershdr->en = bso;
net/ipv4/ip_gre.c:	   ICMP in the real Internet is absolutely infeasible.
net/ipv4/ip_gre.c:	 * ICMP in the real Internet is absolutely infeasible.


Normally, AFAICT, the FCS does not get passed to the operating system
since its a link layer mechanism.  If ERSPAN is passing the FCS when it
mirrors frames (does it mirror frames or packets, I don't know?) then
surely ERSPAN should provide a function to return the BSO value.

So IMHO this patch seems like a just pretense and not really doing
anything.

Hope this helps,
Tobin.