[Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Dong Chenchen]

BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430
Read of size 1 at addr ffff8881051f3bf8 by task ip/668

CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014
Call Trace:
 <TASK>
 dump_stack_lvl+0x72/0xa0
 print_report+0xd0/0x620
 kasan_report+0xb6/0xf0
 xfrm_policy_inexact_list_reinsert+0xb6/0x430
 xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800
 xfrm_policy_inexact_alloc_chain+0x23f/0x320
 xfrm_policy_inexact_insert+0x6b/0x590
 xfrm_policy_insert+0x3b1/0x480
 xfrm_add_policy+0x23c/0x3c0
 xfrm_user_rcv_msg+0x2d0/0x510
 netlink_rcv_skb+0x10d/0x2d0
 xfrm_netlink_rcv+0x49/0x60
 netlink_unicast+0x3fe/0x540
 netlink_sendmsg+0x528/0x970
 sock_sendmsg+0x14a/0x160
 ____sys_sendmsg+0x4fc/0x580
 ___sys_sendmsg+0xef/0x160
 __sys_sendmsg+0xf7/0x1b0
 do_syscall_64+0x3f/0x90
 entry_SYSCALL_64_after_hwframe+0x73/0xdd

The root cause is:

cpu 0			cpu1
xfrm_dump_policy
xfrm_policy_walk
list_move_tail
			xfrm_add_policy
			... ...
			xfrm_policy_inexact_list_reinsert
			list_for_each_entry_reverse
				if (!policy->bydst_reinsert)
				//read non-existent policy
xfrm_dump_policy_done
xfrm_policy_walk_done
list_del(&walk->walk.all);

If dump_one_policy() returns err (triggered by netlink socket),
xfrm_policy_walk() will move walk initialized by socket to list
net->xfrm.policy_all. so this socket becomes visible in the global
policy list. The head *walk can be traversed when users add policies
with different prefixlen and trigger xfrm_policy node merge.

The issue can also be triggered by policy list traversal while rehashing
and flushing policies.

It can be fixed by skip such "policies" with walk.dead set to 1.

Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list")
Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
---
v2: fix similiar similar while rehashing and flushing policies
---
 net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index d6b405782b63..33efd46fb291 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -848,6 +848,9 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
 	matched_d = 0;
 
 	list_for_each_entry_reverse(policy, &net->xfrm.policy_all, walk.all) {
+		if (policy->walk.dead)
+			continue;
+
 		struct hlist_node *newpos = NULL;
 		bool matches_s, matches_d;
 
@@ -1253,11 +1256,14 @@ static void xfrm_hash_rebuild(struct work_struct *work)
 	 * we start with destructive action.
 	 */
 	list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
+		if (policy->walk.dead)
+			continue;
+
 		struct xfrm_pol_inexact_bin *bin;
 		u8 dbits, sbits;
 
 		dir = xfrm_policy_id2dir(policy->index);
-		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
+		if (dir >= XFRM_POLICY_MAX)
 			continue;
 
 		if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) {
@@ -1823,9 +1829,11 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
 
 again:
 	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+		if (pol->walk.dead)
+			continue;
+
 		dir = xfrm_policy_id2dir(pol->index);
-		if (pol->walk.dead ||
-		    dir >= XFRM_POLICY_MAX ||
+		if (dir >= XFRM_POLICY_MAX ||
 		    pol->type != type)
 			continue;
 
@@ -1862,9 +1870,11 @@ int xfrm_dev_policy_flush(struct net *net, struct net_device *dev,
 
 again:
 	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
+		if (pol->walk.dead)
+			continue;
+
 		dir = xfrm_policy_id2dir(pol->index);
-		if (pol->walk.dead ||
-		    dir >= XFRM_POLICY_MAX ||
+		if (dir >= XFRM_POLICY_MAX ||
 		    pol->xdo.dev != dev)
 			continue;
 
-- 
2.25.1

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Florian Westphal]

Dong Chenchen <dongchenchen2@huawei.com> wrote:
> BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430
> Read of size 1 at addr ffff8881051f3bf8 by task ip/668
> 
> CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x72/0xa0
>  print_report+0xd0/0x620
>  kasan_report+0xb6/0xf0
>  xfrm_policy_inexact_list_reinsert+0xb6/0x430
>  xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800
>  xfrm_policy_inexact_alloc_chain+0x23f/0x320
>  xfrm_policy_inexact_insert+0x6b/0x590
>  xfrm_policy_insert+0x3b1/0x480
>  xfrm_add_policy+0x23c/0x3c0
>  xfrm_user_rcv_msg+0x2d0/0x510
>  netlink_rcv_skb+0x10d/0x2d0
>  xfrm_netlink_rcv+0x49/0x60
>  netlink_unicast+0x3fe/0x540
>  netlink_sendmsg+0x528/0x970
>  sock_sendmsg+0x14a/0x160
>  ____sys_sendmsg+0x4fc/0x580
>  ___sys_sendmsg+0xef/0x160
>  __sys_sendmsg+0xf7/0x1b0
>  do_syscall_64+0x3f/0x90
>  entry_SYSCALL_64_after_hwframe+0x73/0xdd

Thanks for following up.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Mon, Aug 14, 2023 at 10:00:13PM +0800, Dong Chenchen wrote:
> BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430
> Read of size 1 at addr ffff8881051f3bf8 by task ip/668
> 
> CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x72/0xa0
>  print_report+0xd0/0x620
>  kasan_report+0xb6/0xf0
>  xfrm_policy_inexact_list_reinsert+0xb6/0x430
>  xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800
>  xfrm_policy_inexact_alloc_chain+0x23f/0x320
>  xfrm_policy_inexact_insert+0x6b/0x590
>  xfrm_policy_insert+0x3b1/0x480
>  xfrm_add_policy+0x23c/0x3c0
>  xfrm_user_rcv_msg+0x2d0/0x510
>  netlink_rcv_skb+0x10d/0x2d0
>  xfrm_netlink_rcv+0x49/0x60
>  netlink_unicast+0x3fe/0x540
>  netlink_sendmsg+0x528/0x970
>  sock_sendmsg+0x14a/0x160
>  ____sys_sendmsg+0x4fc/0x580
>  ___sys_sendmsg+0xef/0x160
>  __sys_sendmsg+0xf7/0x1b0
>  do_syscall_64+0x3f/0x90
>  entry_SYSCALL_64_after_hwframe+0x73/0xdd
> 
> The root cause is:
> 
> cpu 0			cpu1
> xfrm_dump_policy
> xfrm_policy_walk
> list_move_tail
> 			xfrm_add_policy
> 			... ...
> 			xfrm_policy_inexact_list_reinsert
> 			list_for_each_entry_reverse
> 				if (!policy->bydst_reinsert)
> 				//read non-existent policy
> xfrm_dump_policy_done
> xfrm_policy_walk_done
> list_del(&walk->walk.all);
> 
> If dump_one_policy() returns err (triggered by netlink socket),
> xfrm_policy_walk() will move walk initialized by socket to list
> net->xfrm.policy_all. so this socket becomes visible in the global
> policy list. The head *walk can be traversed when users add policies
> with different prefixlen and trigger xfrm_policy node merge.
> 
> The issue can also be triggered by policy list traversal while rehashing
> and flushing policies.
> 
> It can be fixed by skip such "policies" with walk.dead set to 1.
> 
> Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
> Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list")
> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
> ---
> v2: fix similiar similar while rehashing and flushing policies
> ---
>  net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index d6b405782b63..33efd46fb291 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -848,6 +848,9 @@ static void xfrm_policy_inexact_list_reinsert(struct net *net,
>  	matched_d = 0;
>  
>  	list_for_each_entry_reverse(policy, &net->xfrm.policy_all, walk.all) {
> +		if (policy->walk.dead)
> +			continue;
> +
>  		struct hlist_node *newpos = NULL;
>  		bool matches_s, matches_d;

You can't declare new variables in the middle of execution scope in C.

>  
> @@ -1253,11 +1256,14 @@ static void xfrm_hash_rebuild(struct work_struct *work)
>  	 * we start with destructive action.
>  	 */
>  	list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
> +		if (policy->walk.dead)
> +			continue;
> +
>  		struct xfrm_pol_inexact_bin *bin;
>  		u8 dbits, sbits;

Same comment as above.

>  
>  		dir = xfrm_policy_id2dir(policy->index);
> -		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
> +		if (dir >= XFRM_POLICY_MAX)

This change is unnecessary, previous code was perfectly fine.

>  			continue;
>  
>  		if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) {
> @@ -1823,9 +1829,11 @@ int xfrm_policy_flush(struct net *net, u8 type, bool task_valid)
>  
>  again:
>  	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
> +		if (pol->walk.dead)
> +			continue;
> +
>  		dir = xfrm_policy_id2dir(pol->index);
> -		if (pol->walk.dead ||
> -		    dir >= XFRM_POLICY_MAX ||
> +		if (dir >= XFRM_POLICY_MAX ||

This change is unnecessary, previous code was perfectly fine.

>  		    pol->type != type)
>  			continue;
>  
> @@ -1862,9 +1870,11 @@ int xfrm_dev_policy_flush(struct net *net, struct net_device *dev,
>  
>  again:
>  	list_for_each_entry(pol, &net->xfrm.policy_all, walk.all) {
> +		if (pol->walk.dead)
> +			continue;
> +
>  		dir = xfrm_policy_id2dir(pol->index);
> -		if (pol->walk.dead ||
> -		    dir >= XFRM_POLICY_MAX ||
> +		if (dir >= XFRM_POLICY_MAX ||
>  		    pol->xdo.dev != dev)
>  			continue;

Ditto.

>  
> -- 
> 2.25.1
> 

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Florian Westphal]

Leon Romanovsky <leon@kernel.org> wrote:
> >  		dir = xfrm_policy_id2dir(policy->index);
> > -		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
> > +		if (dir >= XFRM_POLICY_MAX)
> 
> This change is unnecessary, previous code was perfectly fine.

Are you sure? AFAICS walker struct has no 'index' member.

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Tue, Aug 15, 2023 at 08:04:54AM +0200, Florian Westphal wrote:
> Leon Romanovsky <leon@kernel.org> wrote:
> > >  		dir = xfrm_policy_id2dir(policy->index);
> > > -		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
> > > +		if (dir >= XFRM_POLICY_MAX)
> > 
> > This change is unnecessary, previous code was perfectly fine.
> 
> Are you sure? AFAICS walker struct has no 'index' member.

But policy has, and we are not interested in validity of it as first
check in if (...) will be true for policy->walk.dead.

So it is safe to call to dir = xfrm_policy_id2dir(policy->index) even
for dead policy.

Thanks

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Herbert Xu]

On Tue, Aug 15, 2023 at 10:30:33AM +0300, Leon Romanovsky wrote:
>
> But policy has, and we are not interested in validity of it as first
> check in if (...) will be true for policy->walk.dead.
> 
> So it is safe to call to dir = xfrm_policy_id2dir(policy->index) even
> for dead policy.

If you dereference policy->index on a walker object it will read memory
before the start of the walker object.  That could do anything, perhaps
even triggering a page fault.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Tue, Aug 15, 2023 at 03:51:44PM +0800, Herbert Xu wrote:
> On Tue, Aug 15, 2023 at 10:30:33AM +0300, Leon Romanovsky wrote:
> >
> > But policy has, and we are not interested in validity of it as first
> > check in if (...) will be true for policy->walk.dead.
> > 
> > So it is safe to call to dir = xfrm_policy_id2dir(policy->index) even
> > for dead policy.
> 
> If you dereference policy->index on a walker object it will read memory
> before the start of the walker object.  That could do anything, perhaps
> even triggering a page fault.

Where do you see walker object? xfrm_policy_id2dir() is called on policy
object, which is defined as "struct xfrm_policy".

Thanks

> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Tue, Aug 15, 2023 at 04:47:58PM +0800, Dong Chenchen wrote:
> On Mon, Aug 14, 2023 at 10:00:13PM +0800, Dong Chenchen wrote:
> >> BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430
> >> Read of size 1 at addr ffff8881051f3bf8 by task ip/668
> >> 
> >> CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014
> >> Call Trace:
> >>  <TASK>
> >>  dump_stack_lvl+0x72/0xa0
> >>  print_report+0xd0/0x620
> >>  kasan_report+0xb6/0xf0
> >>  xfrm_policy_inexact_list_reinsert+0xb6/0x430
> >>  xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800
> >>  xfrm_policy_inexact_alloc_chain+0x23f/0x320
> >>  xfrm_policy_inexact_insert+0x6b/0x590
> >>  xfrm_policy_insert+0x3b1/0x480
> >>  xfrm_add_policy+0x23c/0x3c0
> >>  xfrm_user_rcv_msg+0x2d0/0x510
> >>  netlink_rcv_skb+0x10d/0x2d0
> >>  xfrm_netlink_rcv+0x49/0x60
> >>  netlink_unicast+0x3fe/0x540
> >>  netlink_sendmsg+0x528/0x970
> >>  sock_sendmsg+0x14a/0x160
> >>  ____sys_sendmsg+0x4fc/0x580
> >>  ___sys_sendmsg+0xef/0x160
> >>  __sys_sendmsg+0xf7/0x1b0
> >>  do_syscall_64+0x3f/0x90
> >>  entry_SYSCALL_64_after_hwframe+0x73/0xdd
> >> 
> >> The root cause is:
> >> 
> >> cpu 0			cpu1
> >> xfrm_dump_policy
> >> xfrm_policy_walk
> >> list_move_tail
> >> 			xfrm_add_policy
> >> 			... ...
> >> 			xfrm_policy_inexact_list_reinsert
> >> 			list_for_each_entry_reverse
> >> 				if (!policy->bydst_reinsert)
> >> 				//read non-existent policy
> >> xfrm_dump_policy_done
> >> xfrm_policy_walk_done
> >> list_del(&walk->walk.all);
> >> 
> >> If dump_one_policy() returns err (triggered by netlink socket),
> >> xfrm_policy_walk() will move walk initialized by socket to list
> >> net->xfrm.policy_all. so this socket becomes visible in the global
> >> policy list. The head *walk can be traversed when users add policies
> >> with different prefixlen and trigger xfrm_policy node merge.
> >> 
> >> The issue can also be triggered by policy list traversal while rehashing
> >> and flushing policies.
> >> 
> >> It can be fixed by skip such "policies" with walk.dead set to 1.
> >> 
> >> Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
> >> Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list")
> >> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
> >> ---
> >> v2: fix similiar similar while rehashing and flushing policies
> >> ---
> >>  net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----
> >>  1 file changed, 15 insertions(+), 5 deletions(-)

<...>

> >> @@ -1253,11 +1256,14 @@ static void xfrm_hash_rebuild(struct work_struct *work)
> >>  	 * we start with destructive action.
> >>  	 */
> >>  	list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
> >> +		if (policy->walk.dead)
> >> +			continue;
> >> +
> >>  		struct xfrm_pol_inexact_bin *bin;
> >>  		u8 dbits, sbits;
> >
> >Same comment as above.
> >
> >>  
> >>  		dir = xfrm_policy_id2dir(policy->index);
> >> -		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
> >> +		if (dir >= XFRM_POLICY_MAX)
> >
> >This change is unnecessary, previous code was perfectly fine.
> >
> The walker object initialized by xfrm_policy_walk_init() doesnt have policy. 
> list_for_each_entry() will use the walker offset to calculate policy address.
> It's nonexistent and different from invalid dead policy. It will read memory 
> that doesnt belong to walker if dereference policy->index.
> I think we should protect the memory.

But all operations here are an outcome of "list_for_each_entry(policy,
&net->xfrm.policy_all, walk.all)" which stores in policy iterator
the pointer to struct xfrm_policy.

How at the same time access to policy->walk.dead is valid while
policy->index is not?

Thanks

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Tue, Aug 15, 2023 at 07:35:13PM +0800, Dong Chenchen wrote:
> On Tue, Aug 15, 2023 at 04:47:58PM +0800, Dong Chenchen wrote:
> >> On Mon, Aug 14, 2023 at 10:00:13PM +0800, Dong Chenchen wrote:
> >> >> BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430
> >> >> Read of size 1 at addr ffff8881051f3bf8 by task ip/668
> >> >> 
> >> >> CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64
> >> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014
> >> >> Call Trace:
> >> >>  <TASK>
> >> >>  dump_stack_lvl+0x72/0xa0
> >> >>  print_report+0xd0/0x620
> >> >>  kasan_report+0xb6/0xf0
> >> >>  xfrm_policy_inexact_list_reinsert+0xb6/0x430
> >> >>  xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800
> >> >>  xfrm_policy_inexact_alloc_chain+0x23f/0x320
> >> >>  xfrm_policy_inexact_insert+0x6b/0x590
> >> >>  xfrm_policy_insert+0x3b1/0x480
> >> >>  xfrm_add_policy+0x23c/0x3c0
> >> >>  xfrm_user_rcv_msg+0x2d0/0x510
> >> >>  netlink_rcv_skb+0x10d/0x2d0
> >> >>  xfrm_netlink_rcv+0x49/0x60
> >> >>  netlink_unicast+0x3fe/0x540
> >> >>  netlink_sendmsg+0x528/0x970
> >> >>  sock_sendmsg+0x14a/0x160
> >> >>  ____sys_sendmsg+0x4fc/0x580
> >> >>  ___sys_sendmsg+0xef/0x160
> >> >>  __sys_sendmsg+0xf7/0x1b0
> >> >>  do_syscall_64+0x3f/0x90
> >> >>  entry_SYSCALL_64_after_hwframe+0x73/0xdd
> >> >> 
> >> >> The root cause is:
> >> >> 
> >> >> cpu 0			cpu1
> >> >> xfrm_dump_policy
> >> >> xfrm_policy_walk
> >> >> list_move_tail
> >> >> 			xfrm_add_policy
> >> >> 			... ...
> >> >> 			xfrm_policy_inexact_list_reinsert
> >> >> 			list_for_each_entry_reverse
> >> >> 				if (!policy->bydst_reinsert)
> >> >> 				//read non-existent policy
> >> >> xfrm_dump_policy_done
> >> >> xfrm_policy_walk_done
> >> >> list_del(&walk->walk.all);
> >> >> 
> >> >> If dump_one_policy() returns err (triggered by netlink socket),
> >> >> xfrm_policy_walk() will move walk initialized by socket to list
> >> >> net->xfrm.policy_all. so this socket becomes visible in the global
> >> >> policy list. The head *walk can be traversed when users add policies
> >> >> with different prefixlen and trigger xfrm_policy node merge.
> >> >> 
> >> >> The issue can also be triggered by policy list traversal while rehashing
> >> >> and flushing policies.
> >> >> 
> >> >> It can be fixed by skip such "policies" with walk.dead set to 1.
> >> >> 
> >> >> Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
> >> >> Fixes: 12a169e7d8f4 ("ipsec: Put dumpers on the dump list")
> >> >> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
> >> >> ---
> >> >> v2: fix similiar similar while rehashing and flushing policies
> >> >> ---
> >> >>  net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----
> >> >>  1 file changed, 15 insertions(+), 5 deletions(-)
> >
> ><...>
> >
> >> >> @@ -1253,11 +1256,14 @@ static void xfrm_hash_rebuild(struct work_struct *work)
> >> >>  	 * we start with destructive action.
> >> >>  	 */
> >> >>  	list_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {
> >> >> +		if (policy->walk.dead)
> >> >> +			continue;
> >> >> +
> >> >>  		struct xfrm_pol_inexact_bin *bin;
> >> >>  		u8 dbits, sbits;
> >> >
> >> >Same comment as above.
> >> >
> >> >>  
> >> >>  		dir = xfrm_policy_id2dir(policy->index);
> >> >> -		if (policy->walk.dead || dir >= XFRM_POLICY_MAX)
> >> >> +		if (dir >= XFRM_POLICY_MAX)
> >> >
> >> >This change is unnecessary, previous code was perfectly fine.
> >> >
> >> The walker object initialized by xfrm_policy_walk_init() doesnt have policy. 
> >> list_for_each_entry() will use the walker offset to calculate policy address.
> >> It's nonexistent and different from invalid dead policy. It will read memory 
> >> that doesnt belong to walker if dereference policy->index.
> >> I think we should protect the memory.
> >
> >But all operations here are an outcome of "list_for_each_entry(policy,
> >&net->xfrm.policy_all, walk.all)" which stores in policy iterator
> >the pointer to struct xfrm_policy.
> >
> >How at the same time access to policy->walk.dead is valid while
> >policy->index is not?
> >
> >Thanks
> 1.walker init: its only a list head, no policy
> xfrm_dump_policy_start
> 	xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);
> 		INIT_LIST_HEAD(&walk->walk.all);
> 		walk->walk.dead = 1;
> 
> 2.add the walk head to net->xfrm.policy_all
> xfrm_policy_walk
>     list_for_each_entry_from(x, &net->xfrm.policy_all, all)
> 	if (error) {
> 		list_move_tail(&walk->walk.all, &x->all);
> 		//add the walk to list tail
> 
> 3.traverse the walk list
> xfrm_policy_flush
> list_for_each_entry(pol, &net->xfrm.policy_all, walk.all)
> 	 dir = xfrm_policy_id2dir(pol->index);
> 
> it gets policy by &net->xfrm.policy_all-0x130(offset of walk in policy)
> but when walk is head, we will read others memory by the calculated policy.
> such as:
>   walk addr  		policy addr
> 0xffff0000d7f3b530    0xffff0000d7f3b400 (non-existent) 
> 
> head walker of net->xfrm.policy_all can be skipped by  list_for_each_entry().
> but the walker created by socket is located list tail. so we should skip it. 

list_for_each_entry_from(x, &net->xfrm.policy_all, all) gives you
pointer to "x", you can't access some of its fields and say they
exist and other doesn't. Once you can call to "x->...", you can 
call to "x->index" too.

Thanks

Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

[Leon Romanovsky]

On Tue, Aug 15, 2023 at 09:43:28PM +0800, Dong Chenchen wrote:
> On Tue, Aug 15, 2023 at 07:35:13PM +0800, Dong Chenchen wrote:
> >> >> The walker object initialized by xfrm_policy_walk_init() doesnt have policy. 
> >> >> list_for_each_entry() will use the walker offset to calculate policy address.
> >> >> It's nonexistent and different from invalid dead policy. It will read memory 
> >> >> that doesnt belong to walker if dereference policy->index.
> >> >> I think we should protect the memory.
> >> >
> >> >But all operations here are an outcome of "list_for_each_entry(policy,
> >> >&net->xfrm.policy_all, walk.all)" which stores in policy iterator
> >> >the pointer to struct xfrm_policy.
> >> >
> >> >How at the same time access to policy->walk.dead is valid while
> >> >policy->index is not?
> >> >
> >> >Thanks
> >> 1.walker init: its only a list head, no policy
> >> xfrm_dump_policy_start
> >> 	xfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);
> >> 		INIT_LIST_HEAD(&walk->walk.all);
> >> 		walk->walk.dead = 1;
> >> 
> >> 2.add the walk head to net->xfrm.policy_all
> >> xfrm_policy_walk
> >>     list_for_each_entry_from(x, &net->xfrm.policy_all, all)
> >> 	if (error) {
> >> 		list_move_tail(&walk->walk.all, &x->all);
> >> 		//add the walk to list tail
> >> 
> >> 3.traverse the walk list
> >> xfrm_policy_flush
> >> list_for_each_entry(pol, &net->xfrm.policy_all, walk.all)
> >> 	 dir = xfrm_policy_id2dir(pol->index);
> >> 
> >> it gets policy by &net->xfrm.policy_all-0x130(offset of walk in policy)
> >> but when walk is head, we will read others memory by the calculated policy.
> >> such as:
> >>   walk addr  		policy addr
> >> 0xffff0000d7f3b530    0xffff0000d7f3b400 (non-existent) 
> >> 
> >> head walker of net->xfrm.policy_all can be skipped by  list_for_each_entry().
> >> but the walker created by socket is located list tail. so we should skip it. 
> >
> >list_for_each_entry_from(x, &net->xfrm.policy_all, all) gives you
> >pointer to "x", you can't access some of its fields and say they
> >exist and other doesn't. Once you can call to "x->...", you can 
> >call to "x->index" too.
> >
> >Thanks
> We get a pointer addr not actual variable from list_for_each_entry_from(),
> that calculated by walk address dec offset from struct xfrm_policy(0x130).

The thing is that you must get valid addr pointer and not some random
memory address.

> 
> walk addr: 0xffff0000d7f3b530 //allocated by socket, valid
> -> dec 0x130 (use macro container_of)
> policy_addr:0xffff0000d7f3b400 //only a pointer addr
> -> add 0x130 
> policy->walk:0xffff0000d7f3b530 //its still walker head
> 
> I think its invalid to read policy->index from memory that maybe allocated
> by other user.

This is not how pointers are expected to be used. Once you have pointer
to the struct, the expectation is that all fields in that struct are
accessible.

Anyway, we discussed this topic a lot.

Thanks

> 
> Thanks!
> Dong Chenchen
>