netdev - Re: [PATCH net 1/3] net/sched: cls_u32: No longer copy tcf

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sat, 22 Jul 2023 01:04:29 +0700
From: M A Ramdhan <ramdhan@...rlabs.sg>
To: valis <sec@...is.email>
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, pctammela@...atatu.com, victor@...atatu.com, 
	billy@...rlabs.sg
Subject: Re: [PATCH net 1/3] net/sched: cls_u32: No longer copy tcf_result on
 update to avoid use-after-free

On Sat, Jul 22, 2023 at 12:51 AM valis <sec@...is.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@...is.email>
> Reported-by: M A Ramdhan <ramdhan@...rlabs.sg>
> Signed-off-by: valis <sec@...is.email>
> Cc: stable@...r.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