netdev - Re: [PATCH/RFC net-next 0/4] net/sched: cls_flower: avoid false matching of truncated packets

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20170428141414.GA11256@vergenet.net>
Date:   Fri, 28 Apr 2017 16:14:15 +0200
From:   Simon Horman <simon.horman@...ronome.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>
Cc:     Jiri Pirko <jiri@...lanox.com>,
        Cong Wang <xiyou.wangcong@...il.com>,
        Dinan Gunawardena <dinan.gunawardena@...ronome.com>,
        netdev@...r.kernel.org, oss-drivers@...ronome.com
Subject: Re: [PATCH/RFC net-next 0/4] net/sched: cls_flower: avoid false
 matching of truncated packets

On Fri, Apr 28, 2017 at 09:41:00AM -0400, Jamal Hadi Salim wrote:
> On 17-04-28 09:11 AM, Simon Horman wrote:
> >On Fri, Apr 28, 2017 at 08:52:56AM -0400, Jamal Hadi Salim wrote:
> >>On 17-04-28 08:00 AM, Simon Horman wrote:
> >>>Hi,
> >>>
> >>>this series is intended to avoid false-positives which match
> >>>truncated packets against flower classifiers which match on:
> >>>* zero L4 ports or;
> >
> >How would you describe such a rule? The case that is being dealt with is
> >one where there is a parse error and thus nothing to match on from a flower
> >pov.
> >
> 
> A default lower prio match all on udp or icmp?

I'm certainly not opposed to exploring ideas here.

The way that flower currently works is that a match on ip_proto ==
UDP/TCP/SCTP/ICMP but not fields in the L4 header itself would not result in
the dissector only dissecting the packet's L4 header and thus would not
discover (or as in currently the case, silently ignore) the absence of the
ports/ICMP type and code in the L4 header.

What my patch attempts to do is to describe a policy of what to do if
a given classifier invokes the dissector (to pull out the headers needed for
the match in question) and that dissection fails. Its basically describing
the error-path.

> >>Example what would offloading of
> >>header_parse_err_action mean?
> >
> >Why would it need to differ semantically to the implementation in this
> >patch? I feel that I am missing something.
> >
> 
> Unless I misunderstood:
> Isnt the issue the dissector that confused something missing L4 ports
> and said "port is zero"?
> 
> Unless the hardware has the same "bug" as the dissector seems like would
> be a different semantic in the h/ware.

There are two issues:

1. As things stand, without this patch-set, flower does not differentiate
   between a packet truncated at the end of the IP header and a packet with
   zero ports. Likewise for icmp type and code of zero.

   The first three patches of this series address that so that a match for
   port == zero only matches if ports are present in the packet. Again,
   likewise for ICMP.

   This is a bug-fix to my way of thinking.

2. The behaviour described above, prior to this patchset, might have been
   utilised to f.e. drop packets that are either truncated or have port == 0
   (because flower didn't differentiate between these cases).

   So the question becomes if/how to provide such a feature.
   The last patch is my attempt to answer that question.