Kevin 'ldir' Darbyshire-Bryant <l...@darbyshire-bryant.me.uk> writes:
>> On 13 Jun 2019, at 10:33, Simon Horman <simon.hor...@netronome.com> wrote:
>>
>> On Wed, Jun 12, 2019 at 11:46:27AM -0700, Jakub Kicinski wrote:
>>> 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?
>>
>> FWIW, that is my thinking too.
>
>
> Hi everyone,
>
> Apologies that somehow I seem to have caused a bit of trouble. If need be
> and because act_ctinfo hasn’t yet actually been released anything could happen
> to it, reverted if need be. I’d like it to be done right, not that I know
> what right is, the perils of inexperience and copy/pasting existing
> boilerplate
> code.
>
> Looking at other code I think I should have done something like:
>
> diff --git a/net/sched/act_ctinfo.c b/net/sched/act_ctinfo.c
> index e78b60e47c0f..4695aa76c0dc 100644
> --- a/net/sched/act_ctinfo.c
> +++ b/net/sched/act_ctinfo.c
> @@ -168,7 +168,7 @@ static int tcf_ctinfo_init(struct net *net, struct nlattr
> *nla,
> if (!nla)
> return -EINVAL;
>
> - err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy, NULL);
> + err = nla_parse_nested(tb, TCA_CTINFO_MAX, nla, ctinfo_policy,
> extack);
> if (err < 0)
> return err;
>
> @@ -182,13 +182,19 @@ static int tcf_ctinfo_init(struct net *net, struct
> nlattr *nla,
> dscpmask = nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_MASK]);
> /* need contiguous 6 bit mask */
> dscpmaskshift = dscpmask ? __ffs(dscpmask) : 0;
> - if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f)
> + if ((~0 & (dscpmask >> dscpmaskshift)) != 0x3f) {
> + NL_SET_ERR_MSG_ATTR(extack,
> tb[TCA_CTINFO_PARMS_DSCP_MASK],
> + "dscp mask must be 6 contiguous
> bits");
> return -EINVAL;
> + }
> dscpstatemask = tb[TCA_CTINFO_PARMS_DSCP_STATEMASK] ?
> nla_get_u32(tb[TCA_CTINFO_PARMS_DSCP_STATEMASK]) : 0;
> /* mask & statemask must not overlap */
> - if (dscpmask & dscpstatemask)
> + if (dscpmask & dscpstatemask) {
> + NL_SET_ERR_MSG_ATTR(extack,
> tb[TCA_CTINFO_PARMS_STATEMASK],
> + "dscp statemask must not overlap dscp
> mask");
> return -EINVAL;
> + }
> }
>
> /* done the validation:now to the actual action allocation */
>
> Warning: Not even compile tested! Am I heading in the right
> direction?
Yup! Sending a follow-up patch with an update like this would be an
excellent way to fix the issue :)
-Toke
