netdev - Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown tcfa

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:   Tue, 31 Jul 2018 11:41:26 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Jamal Hadi Salim <jhs@...atatu.com>, netdev@...r.kernel.org
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Daniel Borkmann <daniel@...earbox.net>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        Eyal Birger <eyal.birger@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next v5 1/4] net/sched: user-space can't set unknown
 tcfa_action values

On Mon, 2018-07-30 at 15:31 -0400, Jamal Hadi Salim wrote:
> On 30/07/18 12:41 PM, Paolo Abeni wrote:
> > On Mon, 2018-07-30 at 10:03 -0400, Jamal Hadi Salim wrote:
> > > On 30/07/18 08:30 AM, Paolo Abeni wrote:
> > > >    	}
> > > >    
> > > > +	if (!tcf_action_valid(a->tcfa_action)) {
> > > > +		NL_SET_ERR_MSG(extack, "invalid action value, using TC_ACT_UNSPEC instead");
> > > > +		a->tcfa_action = TC_ACT_UNSPEC;
> > > > +	}
> > > > +
> > > >    	return a;
> > > >    
> > > 
> > > 
> > > I think it would make a lot more sense to just reject the entry than
> > > changing it underneath the user to a default value. Least element of
> > > suprise.
> > 
> > I fear that would break existing (bad) users ?!? This way, such users
> > are notified they are doing something uncorrect, but still continue to
> > work.
> 
> 
> By "bad users" I think you mean someone setting a policy expecting
> one behavior but getting a different one? 

Generally speaking I thought about user-space pushing to the kernel
some uninitialized/random value for 'tcfa_action'.

> If yes, that policy was
> already wrong/buggy. As an example, if i configured:
> 
> match xxx action foo action goo action bar action gah
> 
> where action goo has a bad opcode
> If  you "fix it"  with TC_ACT_UNSPEC then basically the above
> policy is now equivalent to:
> 
> match xxx action foo action goo
> 
> Infact if there was a lower prio rule in the chain
> then lookup will continue there and produce even stranger
> results.

I see. 

Before this patch, the kernel exposed the same behaviour for negative
value of 'bar', while, for positive 'bar' values, the overall behaviour
was more complex (some classifier always stops with unknown positive
action value, others go to lower prio).

Overall, the kernel behavior should be more well-defined now, but yes,
there is a change of behavior under some circumstances.

What about instead mapping undefined/unknown actions value to TC_ACT_OK
(still at initialization time)? this is what is already done by
tcf_action_exec() for faulty opcodes/graphs and by tcf_ipt() and would
handle the above example more conistently.

Cheers,

Paolo