Skip to contentSkip to conversation

net/sched: cls_fw: No longer copy tcf_result on update to avoid use-after-free

PATH 4.14.y Luiz Capitulino 4 participants 5 messages Sep 18, 2023 – Sep 20, 2023

Subsystems (3)
Networking Stable branch Traffic control (tc)
lore
Share & tools
lore

Luiz Capitulino

author
#0 lore raw

From: valis <[email protected]>

Commit 76e42ae831991c828cffa8c37736ebfb831ad5ec upstream.

[ Fixed small conflict as 'fnew->ifindex' assignment is not protected by
CONFIG_NET_CLS_IND on upstream since a51486266c3 ]

When fw_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 e35a8ee5993b ("net: sched: fw use RCU")
Reported-by valis <[email protected]>
Reported-by Bing-Jhong Billy Jheng <[email protected]>
Signed-off-by valis <[email protected]>
Signed-off-by Jamal Hadi Salim <[email protected]>
Reviewed-by Victor Nogueira <[email protected]>
Reviewed-by Pedro Tammela <[email protected]>
Reviewed-by M A Ramdhan <[email protected]>
Link https://lore.kernel.org/r/[email protected] open here
Signed-off-by Jakub Kicinski <[email protected]>
Signed-off-by Luiz Capitulino <[email protected]>
---
 net/sched/cls_fw.c | 1 -
 1 file changed, 1 deletion(-)

Valis, Greg,

I noticed that 4.14 is missing this fix while we backported all three fixes from this series to all stable kernels:

https://lore.kernel.org/all/[email protected]

Is there a reason to have skipped 4.14 for this fix? It seems we need it.

This is only compiled-tested though, would be good to have a confirmation from Valis that the issue is present on 4.14 before applying.

- Luiz

net/sched/cls_fw.c +0 -1
index e63f9c2e37e5..7b04b315b2bd 100644
@@ -281,7 +281,6 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
 			return -ENOBUFS;
 
 		fnew->id = f->id;
-		fnew->res = f->res;
 #ifdef CONFIG_NET_CLS_IND
 		fnew->ifindex = f->ifindex;
 #endif /* CONFIG_NET_CLS_IND */
Signature · 1 line
2.40.1
#1 lore raw
Quoted 8 lines · Luiz Capitulino

Valis, Greg,

I noticed that 4.14 is missing this fix while we backported all three fixes from this series to all stable kernels:

https://lore.kernel.org/all/[email protected]

Is there a reason to have skipped 4.14 for this fix? It seems we need it.

Hi Luiz!

I see no reason why it should be skipped for 4.14
I've just checked 4.14.325 - it is vulnerable and needs this fix.

Best regards,

valis

Quoted 21 lines

This is only compiled-tested though, would be good to have a confirmation from Valis that the issue is present on 4.14 before applying.

- Luiz

net/sched/cls_fw.c +0 -2
index e63f9c2e37e5..7b04b315b2bd 100644
@@ -281,7 +281,6 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
                        return -ENOBUFS;
 
                fnew->id = f->id;
-               fnew->res = f->res;
 #ifdef CONFIG_NET_CLS_IND
                fnew->ifindex = f->ifindex;
 #endif /* CONFIG_NET_CLS_IND */
--

2.40.1

author
#2 lore raw
Quoted 7 lines · valis
Quoted 5 lines · Luiz Capitulino

Valis, Greg,

I noticed that 4.14 is missing this fix while we backported all three fixes from this series to all stable kernels:

Quoted 8 lines
Quoted 3 lines

https://lore.kernel.org/all/[email protected]

Is there a reason to have skipped 4.14 for this fix? It seems we need it.

Hi Luiz!

I see no reason why it should be skipped for 4.14
I've just checked 4.14.325 - it is vulnerable and needs this fix.

Thank you for the quick reply!

- Luiz

Quoted 27 lines

Best regards,

valis

Quoted 21 lines

This is only compiled-tested though, would be good to have a confirmation from Valis that the issue is present on 4.14 before applying.

- Luiz

net/sched/cls_fw.c +0 -2
index e63f9c2e37e5..7b04b315b2bd 100644
@@ -281,7 +281,6 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
                         return -ENOBUFS;
 
                 fnew->id = f->id;
-               fnew->res = f->res;
  #ifdef CONFIG_NET_CLS_IND
                 fnew->ifindex = f->ifindex;
  #endif /* CONFIG_NET_CLS_IND */
--

2.40.1

#3 lore raw
In reply to valis

Hi Luiz,

Quoted 11 lines · valis
Quoted 8 lines · Luiz Capitulino

Valis, Greg,

I noticed that 4.14 is missing this fix while we backported all three fixes from this series to all stable kernels:

https://lore.kernel.org/all/[email protected]

Is there a reason to have skipped 4.14 for this fix? It seems we need it.

I think we need to apply this to 4.19.y as well.

Your backport patch applies there as well. I compiled the kernel after applying your patch to linux-4.19.y's tip.

Thanks,
Harshit

Quoted 31 lines

Hi Luiz!

I see no reason why it should be skipped for 4.14
I've just checked 4.14.325 - it is vulnerable and needs this fix.

Best regards,

valis

Quoted 21 lines

This is only compiled-tested though, would be good to have a confirmation from Valis that the issue is present on 4.14 before applying.

- Luiz

net/sched/cls_fw.c +0 -2
index e63f9c2e37e5..7b04b315b2bd 100644
@@ -281,7 +281,6 @@ static int fw_change(struct net *net, struct sk_buff *in_skb,
                         return -ENOBUFS;
 
                 fnew->id = f->id;
-               fnew->res = f->res;
  #ifdef CONFIG_NET_CLS_IND
                 fnew->ifindex = f->ifindex;
  #endif /* CONFIG_NET_CLS_IND */
--

2.40.1

maintainer
#4 lore raw
In reply to Luiz Capitulino
Quoted 6 lines · Luiz Capitulino

From: valis <[email protected]>

Commit 76e42ae831991c828cffa8c37736ebfb831ad5ec upstream.

[ Fixed small conflict as 'fnew->ifindex' assignment is not protected by
CONFIG_NET_CLS_IND on upstream since a51486266c3 ]

Now queued up, thanks.

greg k-h