netdev - Re: [PATCH net 3/3] net/sched: act_sample: fix action bind logic

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: <Y/o1VJLEwPJWsxZv@corigine.com>
Date:   Sat, 25 Feb 2023 17:20:36 +0100
From:   Simon Horman <simon.horman@...igine.com>
To:     Pedro Tammela <pctammela@...atatu.com>
Cc:     netdev@...r.kernel.org, jhs@...atatu.com, xiyou.wangcong@...il.com,
        jiri@...nulli.us, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, amir@...ai.me,
        dcaratti@...hat.com, willemb@...gle.com,
        simon.horman@...ronome.com, john.hurley@...ronome.com,
        yotamg@...lanox.com, ozsh@...dia.com, paulb@...dia.com
Subject: Re: [PATCH net 3/3] net/sched: act_sample: fix action bind logic

On Fri, Feb 24, 2023 at 12:00:58PM -0300, Pedro Tammela wrote:
> The TC architecture allows filters and actions to be created independently.
> In filters the user can reference action objects using:
> tc action add action sample ... index 1
> tc filter add ... action pedit index 1
> 
> In the current code for act_sample this is broken as it checks netlink
> attributes for create/update before actually checking if we are binding to an
> existing action.
> 
> tdc results:
> 1..29
> ok 1 9784 - Add valid sample action with mandatory arguments
> ok 2 5c91 - Add valid sample action with mandatory arguments and continue control action
> ok 3 334b - Add valid sample action with mandatory arguments and drop control action
> ok 4 da69 - Add valid sample action with mandatory arguments and reclassify control action
> ok 5 13ce - Add valid sample action with mandatory arguments and pipe control action
> ok 6 1886 - Add valid sample action with mandatory arguments and jump control action
> ok 7 7571 - Add sample action with invalid rate
> ok 8 b6d4 - Add sample action with mandatory arguments and invalid control action
> ok 9 a874 - Add invalid sample action without mandatory arguments
> ok 10 ac01 - Add invalid sample action without mandatory argument rate
> ok 11 4203 - Add invalid sample action without mandatory argument group
> ok 12 14a7 - Add invalid sample action without mandatory argument group
> ok 13 8f2e - Add valid sample action with trunc argument
> ok 14 45f8 - Add sample action with maximum rate argument
> ok 15 ad0c - Add sample action with maximum trunc argument
> ok 16 83a9 - Add sample action with maximum group argument
> ok 17 ed27 - Add sample action with invalid rate argument
> ok 18 2eae - Add sample action with invalid group argument
> ok 19 6ff3 - Add sample action with invalid trunc size
> ok 20 2b2a - Add sample action with invalid index
> ok 21 dee2 - Add sample action with maximum allowed index
> ok 22 560e - Add sample action with cookie
> ok 23 704a - Replace existing sample action with new rate argument
> ok 24 60eb - Replace existing sample action with new group argument
> ok 25 2cce - Replace existing sample action with new trunc argument
> ok 26 59d1 - Replace existing sample action with new control argument
> ok 27 0a6e - Replace sample action with invalid goto chain control
> ok 28 3872 - Delete sample action with valid index
> ok 29 a394 - Delete sample action with invalid index
> 
> Fixes: 5c5670fae430 ("net/sched: Introduce sample tc action")
> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>

I have a minor comment below.
But in the context of this series, this patch looks good to me.

Reviewed-by: Simon Horman <simon.horman@...igine.com>

> ---
>  net/sched/act_sample.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
> index f7416b5598e0..4c670e7568dc 100644
> --- a/net/sched/act_sample.c
> +++ b/net/sched/act_sample.c
> @@ -55,8 +55,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
>  					  sample_policy, NULL);
>  	if (ret < 0)
>  		return ret;
> -	if (!tb[TCA_SAMPLE_PARMS] || !tb[TCA_SAMPLE_RATE] ||
> -	    !tb[TCA_SAMPLE_PSAMPLE_GROUP])
> +
> +	if (!tb[TCA_SAMPLE_PARMS])
>  		return -EINVAL;
>  
>  	parm = nla_data(tb[TCA_SAMPLE_PARMS]);
> @@ -80,6 +80,13 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
>  		tcf_idr_release(*a, bind);
>  		return -EEXIST;

nit: not for this patch, but I think it would be more consistent
     to use goto release_idr here.

>  	}
> +
> +	if (!tb[TCA_SAMPLE_RATE] || !tb[TCA_SAMPLE_PSAMPLE_GROUP]) {
> +		NL_SET_ERR_MSG(extack, "sample rate and group are required");
> +		err = -EINVAL;
> +		goto release_idr;
> +	}
> +
>  	err = tcf_action_check_ctrlact(parm->action, tp, &goto_ch, extack);
>  	if (err < 0)
>  		goto release_idr;
> -- 
> 2.34.1
>