netdev - Re: [PATCH net-next v6] net: sched: Introduce act

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]

Message-ID: <20190612114627.4dd137ab@cakuba.netronome.com>
Date:   Wed, 12 Jun 2019 11:46:27 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     Kevin 'ldir' Darbyshire-Bryant <ldir@...byshire-bryant.me.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Paul Blakey <paulb@...lanox.com>,
        John Hurley <john.hurley@...ronome.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Michal Kubecek <mkubecek@...e.cz>,
        Johannes Berg <johannes.berg@...el.com>, dcaratti@...hat.com,
        David Ahern <dsahern@...il.com>
Subject: Re: [PATCH net-next v6] net: sched: Introduce act_ctinfo action

On Wed, 12 Jun 2019 15:02:39 -0300, Marcelo Ricardo Leitner wrote:
> On Tue, May 28, 2019 at 05:03:50PM +0000, Kevin 'ldir' Darbyshire-Bryant wrote:
> ...
> > +static int tcf_ctinfo_init(struct net *net, struct nlattr *nla,
> > +			   struct nlattr *est, struct tc_action **a,
> > +			   int ovr, int bind, bool rtnl_held,
> > +			   struct tcf_proto *tp,
> > +			   struct netlink_ext_ack *extack)
> > +{
> > +	struct tc_action_net *tn = net_generic(net, ctinfo_net_id);
> > +	struct nlattr *tb[TCA_CTINFO_MAX + 1];
> > +	struct tcf_ctinfo_params *cp_new;
> > +	struct tcf_chain *goto_ch = NULL;
> > +	u32 dscpmask = 0, dscpstatemask;
> > +	struct tc_ctinfo *actparm;
> > +	struct tcf_ctinfo *ci;
> > +	u8 dscpmaskshift;
> > +	int ret = 0, err;
> > +
> > +	if (!nla)
> > +		return -EINVAL;
> > +
> > +	err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);  
>                                                                        ^^^^
> Hi, two things here:
> Why not use the extack parameter here? Took me a while to notice
> that the EINVAL was actually hiding the issue below.
> And also on the other two EINVALs this function returns.
> 
> 
> Seems there was a race when this code went in and the stricter check
> added by
> b424e432e770 ("netlink: add validation of NLA_F_NESTED flag") and
> 8cb081746c03 ("netlink: make validation more configurable for future
> strictness").
> 
> I can't add these actions with current net-next and iproute-next:
> # ~/iproute2/tc/tc action add action ctinfo dscp 0xfc000000 0x01000000
> Error: NLA_F_NESTED is missing.
> We have an error talking to the kernel
> 
> This also happens with the current post of act_ct and should also
> happen with the act_mpls post (thus why Cc'ing John as well).
> 
> I'm not sure how we should fix this. In theory the kernel can't get
> stricter with userspace here, as that breaks user applications as
> above, so older actions can't use the more stricter parser. Should we
> have some actions behaving one way, and newer ones in a different way?
> That seems bad.
> 
> Or maybe all actions should just use nla_parse_nested_deprecated()?
> I'm thinking this last. Yet, then the _deprecated suffix may not make
> much sense here. WDYT?

Surely for new actions we can require strict validation, there is
no existing user space to speak of..  Perhaps act_ctinfo and act_ct
got slightly confused with the race you described, but in principle
there is nothing stopping new actions from implementing the user space
correctly, right?