Re: [PATCH net 1/3] net/sched: cls_u32: No longer copy tcf_result on update to avoid use-after-free

[M A Ramdhan <ramdhan@starlabs.sg>]

On Sat, Jul 22, 2023 at 12:51 AM valis <sec@valis.email> wrote:
>
> When u32_change() is called on an existing filter, the whole
> tcf_result struct is always copied into the new instance of the filter.
>
> This causes a problem when updating a filter bound to a class,
> as tcf_unbind_filter() is always called on the old instance in the
> success path, decreasing filter_cnt of the still referenced class
> and allowing it to be deleted, leading to a use-after-free.
>
> Fix this by no longer copying the tcf_result struct from the old filter.
>
> Fixes: de5df63228fc ("net: sched: cls_u32 changes to knode must appear atomic to readers")
> Reported-by: valis <sec@valis.email>
> Reported-by: M A Ramdhan <ramdhan@starlabs.sg>
> Signed-off-by: valis <sec@valis.email>
> Cc: stable@vger.kernel.org
> ---
>  net/sched/cls_u32.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 5abf31e432ca..19aa60d1eea7 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -826,7 +826,6 @@ static struct tc_u_knode *u32_init_knode(struct net *net, struct tcf_proto *tp,
>
>         new->ifindex = n->ifindex;
>         new->fshift = n->fshift;
> -       new->res = n->res;
>         new->flags = n->flags;
>         RCU_INIT_POINTER(new->ht_down, ht);
>
> --
> 2.30.2
>
Hi,

We also thought it's also the correct fixes,
but we're not sure because it will always remove the already bound
qdisc class when we change the filter, even tho we never specify
the new TCA_U32_CLASSID in the new filter.

I also look at the implementation of cls_tcindex and cls_rsvp which still copy
the old tcf_result, but don't call the tcf_unbind_filter when changing
the filter.

If it's the intended behaviour, then I'm good with this patch.

Thanks & Regards,
M A Ramdhan