Re: [PATCH 4.14] net/sched: cls_u32: Fix reference counter leak leading to overflow

[SeongJae Park <sj@kernel.org>]

On Thu, 27 Jul 2023 19:15:54 +0000 Rishabh Bhatnagar <risbhat@amazon.com> wrote:

> From: Lee Jones <lee@kernel.org>
> 
> Upstream commit 04c55383fa5689357bcdd2c8036725a55ed632bc.
> 
> In the event of a failure in tcf_change_indev(), u32_set_parms() will
> immediately return without decrementing the recently incremented
> reference counter.  If this happens enough times, the counter will
> rollover and the reference freed, leading to a double free which can be
> used to do 'bad things'.
> 
> In order to prevent this, move the point of possible failure above the
> point where the reference counter is incremented.  Also save any
> meaningful return values to be applied to the return data at the
> appropriate point in time.
> 
> This issue was caught with KASAN.
> 
> Fixes: 705c7091262d ("net: sched: cls_u32: no need to call tcf_exts_change for newly allocated struct")
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Lee Jones <lee@kernel.org>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Rishabh Bhatnagar <risbhat@amazon.com>

Acked-by: SeongJae Park <sj@kernel.org>

> ---
>  net/sched/cls_u32.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index fdbdcba44917..a4e01220a53a 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -774,11 +774,22 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
>  			 struct nlattr *est, bool ovr)
>  {
>  	int err;
> +#ifdef CONFIG_NET_CLS_IND
> +	int ifindex = -1;
> +#endif
>  
>  	err = tcf_exts_validate(net, tp, tb, est, &n->exts, ovr);
>  	if (err < 0)
>  		return err;
>  
> +#ifdef CONFIG_NET_CLS_IND
> +	if (tb[TCA_U32_INDEV]) {
> +		ifindex = tcf_change_indev(net, tb[TCA_U32_INDEV]);
> +		if (ifindex < 0)
> +			return -EINVAL;
> +	}
> +#endif
> +
>  	if (tb[TCA_U32_LINK]) {
>  		u32 handle = nla_get_u32(tb[TCA_U32_LINK]);
>  		struct tc_u_hnode *ht_down = NULL, *ht_old;
> @@ -806,14 +817,10 @@ static int u32_set_parms(struct net *net, struct tcf_proto *tp,
>  	}
>  
>  #ifdef CONFIG_NET_CLS_IND
> -	if (tb[TCA_U32_INDEV]) {
> -		int ret;
> -		ret = tcf_change_indev(net, tb[TCA_U32_INDEV]);
> -		if (ret < 0)
> -			return -EINVAL;
> -		n->ifindex = ret;
> -	}
> +	if (ifindex >= 0)
> +		n->ifindex = ifindex;
>  #endif
> +
>  	return 0;

Very trivial nit: Someone might think the above new line is better not to be
added?  I don't really care, though.

>  }
>  
> -- 
> 2.40.1
> 

Thanks,
SJ