netdev - Re: [PATCH iproute2] police: don't skip parameters after actions

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] [day] [month] [year] [list]

Date:   Wed, 31 Jan 2018 14:23:21 +0100
From:   Wolfgang Bumiller <w.bumiller@...xmox.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH iproute2] police: don't skip parameters after actions

On Mon, Jan 29, 2018 at 08:07:23AM -0800, Stephen Hemminger wrote:
> On Mon, 29 Jan 2018 12:13:11 +0100
> Wolfgang Bumiller <w.bumiller@...xmox.com> wrote:
> 
> > The 'parse_action_control()' helper advances the argument
> > pointers to past its parsed action already, so don't
> > advance it further in 'act_parse_polic()'.
> > 
> > Fixes: e67aba559581 ("tc: actions: add helpers to parse and print control actions")
> > Signed-off-by: Wolfgang Bumiller <w.bumiller@...xmox.com>
> > ---
> > Basically parse_action_control() silently added a NEXT_ARG() while the
> > cases before didn't have one. Not sure whether the goto is okay
> > style-wise, let me know if you prefer some other solution.
> > 
> > Example for triggering this:
> > Specifying a 'flowid X' after a `police ... drop` will skip the 'flowid'
> > and error with "What is X"
> > 
> > $ tc filter add dev eth0 parent ffff: basic police rate 13371337bps burst 1337b mtu 64kb drop flowid :1
> > What is ":1"?
> 
> Thank you for the patch. It is a real problem, and your patch addresses it.
> I just don't like jumping around in the the argument parsing with goto's.
> There was a similar problem recently, and the better fix was to fix the semantics
> of the parsing function to not  do the extra implicit NEXT_ARG in the parsing logic.
> There is less likely to be future problems if all parsing functions leave the
> with the same argument location.
> 
> Please try that and resubmit.

Actually I must apologize. Apparently I did not properly look at the
master branch but just the v4.13.0 tag. In master your requested
behavior already seems to be implemented. (There's even a commit with
the same `Fixes` tag by Michal Privoznik (3572e01a090).)
Sorry for the noise.