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 for Android: free password hash cracker in your pocket

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

Message-ID: <20180129080723.5f2b5311@xeon-e3>
Date:   Mon, 29 Jan 2018 08:07:23 -0800
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Wolfgang Bumiller <w.bumiller@...xmox.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH iproute2] police: don't skip parameters after actions

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.