Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   SeongJae Park <sj@kernel.org>
To:     Rishabh Bhatnagar <risbhat@amazon.com>
Cc:     gregkh@linuxfoundation.org, lee@kernel.org, davem@davemloft.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        stable@vger.kernel.org, Eric Dumazet <edumazet@google.com>,
        "Jamal Hadi Salim" <jhs@mojatatu.com>,
        SeongJae Park <sj@kernel.org>
Subject: Re: [PATCH 4.14] net/sched: cls_u32: Fix reference counter leak leading to overflow
Date:   Thu, 27 Jul 2023 21:32:36 +0000
Message-Id: <20230727213236.49413-1-sj@kernel.org>
In-Reply-To: <20230727191554.21333-1-risbhat@amazon.com>
References: 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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