netdev - Re: [PATCH net-next v3 2/3] net/sched: 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]

Date:   Mon, 31 May 2021 22:21:36 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Boris Sukholitko <boris.sukholitko@...adcom.com>
Cc:     netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>,
        linux-kselftest@...r.kernel.org, shuah@...nel.org,
        Ilya Lifshits <ilya.lifshits@...adcom.com>,
        Shmulik Ladkani <shmulik.ladkani@...il.com>,
        Davide Caratti <dcaratti@...hat.com>
Subject: Re: [PATCH net-next v3 2/3] net/sched: act_vlan: No dump for unset
 priority

On Sun, 30 May 2021 14:40:51 +0300 Boris Sukholitko wrote:
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index a108469c664f..ccd1acfa4c55 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -307,8 +307,8 @@ static int tcf_vlan_dump(struct sk_buff *skb, struct tc_action *a,
>  	    (nla_put_u16(skb, TCA_VLAN_PUSH_VLAN_ID, p->tcfv_push_vid) ||
>  	     nla_put_be16(skb, TCA_VLAN_PUSH_VLAN_PROTOCOL,
>  			  p->tcfv_push_proto) ||
> -	     (nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY,
> -					      p->tcfv_push_prio))))
> +	     (p->tcfv_push_prio_exists &&
> +	      nla_put_u8(skb, TCA_VLAN_PUSH_VLAN_PRIORITY, p->tcfv_push_prio))))
>  		goto nla_put_failure;
>  
>  	if (p->tcfv_action == TCA_VLAN_ACT_PUSH_ETH) {
> @@ -362,10 +362,19 @@ static int tcf_vlan_search(struct net *net, struct tc_action **a, u32 index)
>  
>  static size_t tcf_vlan_get_fill_size(const struct tc_action *act)
>  {
> -	return nla_total_size(sizeof(struct tc_vlan))
> +	struct tcf_vlan *v = to_vlan(act);
> +	struct tcf_vlan_params *p;
> +	size_t ret = nla_total_size(sizeof(struct tc_vlan))
>  		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_ID */
> -		+ nla_total_size(sizeof(u16)) /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
> -		+ nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
> +		+ nla_total_size(sizeof(u16)); /* TCA_VLAN_PUSH_VLAN_PROTOCOL */
> +
> +	spin_lock_bh(&v->tcf_lock);
> +	p = rcu_dereference_protected(v->vlan_p, lockdep_is_held(&v->tcf_lock));
> +	if (p->tcfv_push_prio_exists)
> +		ret += nla_total_size(sizeof(u8)); /* TCA_VLAN_PUSH_VLAN_PRIORITY */
> +	spin_unlock_bh(&v->tcf_lock);

This jumps out a little bit - if we need to take this lock to inspect
tcf_vlan_params, then I infer its value may change. And if it may
change what guarantees it doesn't change between calculating the skb
length and dumping?

It's common practice to calculate the max skb len required when
attributes are this small.

> +	return ret;
>  }