netdev - Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index' optional argument

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]

Date:   Thu, 27 Oct 2016 11:46:33 +0200
From:   Phil Sutter <phil@....cc>
To:     Shmulik Ladkani <shmulik.ladkani@...il.com>
Cc:     Stephen Hemminger <stephen@...workplumber.org>,
        Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org
Subject: Re: [PATCHv2 iproute2 net-next] tc: m_mirred: Fix parsing of 'index'
 optional argument

Hi,

On Thu, Oct 27, 2016 at 10:36:06AM +0300, Shmulik Ladkani wrote:
> Code in parse_mirred() suggests "index" argument can be placed either
> after the egress/ingress clause, or as the first argument (after
> "action mirred").
> 
> However, parse_direction() fails to correctly parse "index" if it's the
> first argument.
> 
> For example:
> 
>  # tc filter add ... action mirred index 5
>  RTNETLINK answers: Invalid argument
>  (unnecessary RTNETLINK issued, should have been parse error)
> 
>  # tc filter add ... action mirred index 5 egress redirect dev eth0
>  bad action type egress
>  (should have been parsed successfully)

According to the action's help text (and the man page which is based
upon that), this behaviour is perfectly fine:

| Usage: mirred <DIRECTION> <ACTION> [index INDEX] <dev DEVICENAME>

So first argument *must* be the direction, second one *must* be the
action, then an optional index and the last one *must* be the interface.

While I don't see a problem with changing that (apart from that I don't
think it's necessary), if you change it you should adjust help text and
man page accordingly.

But is all this really worth it? And if so, why not fix the parser for
good by delaying the whole validation (apart from double arg detection)
until after the parsing itself?

Cheers, Phil