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  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

Powered by blists - more mailing lists