diff for duplicates of <20230815123233.GM22185@unreal> diff --git a/a/1.txt b/N1/1.txt index 56ee483..494cb21 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,135 +1,62 @@ 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. +>> >> 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). -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. +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 -Thanks +I think its invalid to read policy->index from memory that maybe allocated +by other user. + +Thanks! +Dong Chenchen diff --git a/a/content_digest b/N1/content_digest index 72d036c..1e7f617 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,158 +1,85 @@ "ref\020230814140013.712001-1-dongchenchen2@huawei.com\0" "ref\020230815060026.GE22185@unreal\0" "ref\020230815091324.GL22185@unreal\0" - "From\0Leon Romanovsky <leon@kernel.org>\0" + "From\0Dong Chenchen <dongchenchen2@huawei.com>\0" "Subject\0Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies\0" - "Date\0Tue, 15 Aug 2023 15:32:33 +0300\0" - "To\0Dong Chenchen <dongchenchen2@huawei.com>\0" - "Cc\0fw@strlen.de" - steffen.klassert@secunet.com - herbert@gondor.apana.org.au - davem@davemloft.net - edumazet@google.com - kuba@kernel.org - pabeni@redhat.com - timo.teras@iki.fi - yuehaibing@huawei.com - weiyongjun1@huawei.com - netdev@vger.kernel.org - " linux-kernel@vger.kernel.org\0" + "Date\0Tue, 15 Aug 2023 21:43:28 +0800\0" + "To\0<leon@kernel.org>\0" + "Cc\0<fw@strlen.de>" + <steffen.klassert@secunet.com> + <herbert@gondor.apana.org.au> + <davem@davemloft.net> + <edumazet@google.com> + <kuba@kernel.org> + <pabeni@redhat.com> + <timo.teras@iki.fi> + <yuehaibing@huawei.com> + <weiyongjun1@huawei.com> + <netdev@vger.kernel.org> + " <linux-kernel@vger.kernel.org>\0" "\00:1\0" "b\0" "On Tue, Aug 15, 2023 at 07:35:13PM +0800, Dong Chenchen wrote:\n" - "> On Tue, Aug 15, 2023 at 04:47:58PM +0800, Dong Chenchen wrote:\n" - "> >> On Mon, Aug 14, 2023 at 10:00:13PM +0800, Dong Chenchen wrote:\n" - "> >> >> BUG: KASAN: slab-use-after-free in xfrm_policy_inexact_list_reinsert+0xb6/0x430\n" - "> >> >> Read of size 1 at addr ffff8881051f3bf8 by task ip/668\n" - "> >> >> \n" - "> >> >> CPU: 2 PID: 668 Comm: ip Not tainted 6.5.0-rc5-00182-g25aa0bebba72-dirty #64\n" - "> >> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13 04/01/2014\n" - "> >> >> Call Trace:\n" - "> >> >> <TASK>\n" - "> >> >> dump_stack_lvl+0x72/0xa0\n" - "> >> >> print_report+0xd0/0x620\n" - "> >> >> kasan_report+0xb6/0xf0\n" - "> >> >> xfrm_policy_inexact_list_reinsert+0xb6/0x430\n" - "> >> >> xfrm_policy_inexact_insert_node.constprop.0+0x537/0x800\n" - "> >> >> xfrm_policy_inexact_alloc_chain+0x23f/0x320\n" - "> >> >> xfrm_policy_inexact_insert+0x6b/0x590\n" - "> >> >> xfrm_policy_insert+0x3b1/0x480\n" - "> >> >> xfrm_add_policy+0x23c/0x3c0\n" - "> >> >> xfrm_user_rcv_msg+0x2d0/0x510\n" - "> >> >> netlink_rcv_skb+0x10d/0x2d0\n" - "> >> >> xfrm_netlink_rcv+0x49/0x60\n" - "> >> >> netlink_unicast+0x3fe/0x540\n" - "> >> >> netlink_sendmsg+0x528/0x970\n" - "> >> >> sock_sendmsg+0x14a/0x160\n" - "> >> >> ____sys_sendmsg+0x4fc/0x580\n" - "> >> >> ___sys_sendmsg+0xef/0x160\n" - "> >> >> __sys_sendmsg+0xf7/0x1b0\n" - "> >> >> do_syscall_64+0x3f/0x90\n" - "> >> >> entry_SYSCALL_64_after_hwframe+0x73/0xdd\n" - "> >> >> \n" - "> >> >> The root cause is:\n" - "> >> >> \n" - "> >> >> cpu 0\t\t\tcpu1\n" - "> >> >> xfrm_dump_policy\n" - "> >> >> xfrm_policy_walk\n" - "> >> >> list_move_tail\n" - "> >> >> \t\t\txfrm_add_policy\n" - "> >> >> \t\t\t... ...\n" - "> >> >> \t\t\txfrm_policy_inexact_list_reinsert\n" - "> >> >> \t\t\tlist_for_each_entry_reverse\n" - "> >> >> \t\t\t\tif (!policy->bydst_reinsert)\n" - "> >> >> \t\t\t\t//read non-existent policy\n" - "> >> >> xfrm_dump_policy_done\n" - "> >> >> xfrm_policy_walk_done\n" - "> >> >> list_del(&walk->walk.all);\n" - "> >> >> \n" - "> >> >> If dump_one_policy() returns err (triggered by netlink socket),\n" - "> >> >> xfrm_policy_walk() will move walk initialized by socket to list\n" - "> >> >> net->xfrm.policy_all. so this socket becomes visible in the global\n" - "> >> >> policy list. The head *walk can be traversed when users add policies\n" - "> >> >> with different prefixlen and trigger xfrm_policy node merge.\n" - "> >> >> \n" - "> >> >> The issue can also be triggered by policy list traversal while rehashing\n" - "> >> >> and flushing policies.\n" - "> >> >> \n" - "> >> >> It can be fixed by skip such \"policies\" with walk.dead set to 1.\n" - "> >> >> \n" - "> >> >> Fixes: 9cf545ebd591 (\"xfrm: policy: store inexact policies in a tree ordered by destination address\")\n" - "> >> >> Fixes: 12a169e7d8f4 (\"ipsec: Put dumpers on the dump list\")\n" - "> >> >> Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>\n" - "> >> >> ---\n" - "> >> >> v2: fix similiar similar while rehashing and flushing policies\n" - "> >> >> ---\n" - "> >> >> net/xfrm/xfrm_policy.c | 20 +++++++++++++++-----\n" - "> >> >> 1 file changed, 15 insertions(+), 5 deletions(-)\n" - "> >\n" - "> ><...>\n" - "> >\n" - "> >> >> @@ -1253,11 +1256,14 @@ static void xfrm_hash_rebuild(struct work_struct *work)\n" - "> >> >> \t * we start with destructive action.\n" - "> >> >> \t */\n" - "> >> >> \tlist_for_each_entry(policy, &net->xfrm.policy_all, walk.all) {\n" - "> >> >> +\t\tif (policy->walk.dead)\n" - "> >> >> +\t\t\tcontinue;\n" - "> >> >> +\n" - "> >> >> \t\tstruct xfrm_pol_inexact_bin *bin;\n" - "> >> >> \t\tu8 dbits, sbits;\n" - "> >> >\n" - "> >> >Same comment as above.\n" - "> >> >\n" - "> >> >> \n" - "> >> >> \t\tdir = xfrm_policy_id2dir(policy->index);\n" - "> >> >> -\t\tif (policy->walk.dead || dir >= XFRM_POLICY_MAX)\n" - "> >> >> +\t\tif (dir >= XFRM_POLICY_MAX)\n" - "> >> >\n" - "> >> >This change is unnecessary, previous code was perfectly fine.\n" - "> >> >\n" - "> >> The walker object initialized by xfrm_policy_walk_init() doesnt have policy. \n" - "> >> list_for_each_entry() will use the walker offset to calculate policy address.\n" - "> >> It's nonexistent and different from invalid dead policy. It will read memory \n" - "> >> that doesnt belong to walker if dereference policy->index.\n" - "> >> I think we should protect the memory.\n" - "> >\n" - "> >But all operations here are an outcome of \"list_for_each_entry(policy,\n" - "> >&net->xfrm.policy_all, walk.all)\" which stores in policy iterator\n" - "> >the pointer to struct xfrm_policy.\n" - "> >\n" - "> >How at the same time access to policy->walk.dead is valid while\n" - "> >policy->index is not?\n" - "> >\n" - "> >Thanks\n" - "> 1.walker init: its only a list head, no policy\n" - "> xfrm_dump_policy_start\n" - "> \txfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);\n" - "> \t\tINIT_LIST_HEAD(&walk->walk.all);\n" - "> \t\twalk->walk.dead = 1;\n" - "> \n" - "> 2.add the walk head to net->xfrm.policy_all\n" - "> xfrm_policy_walk\n" - "> list_for_each_entry_from(x, &net->xfrm.policy_all, all)\n" - "> \tif (error) {\n" - "> \t\tlist_move_tail(&walk->walk.all, &x->all);\n" - "> \t\t//add the walk to list tail\n" - "> \n" - "> 3.traverse the walk list\n" - "> xfrm_policy_flush\n" - "> list_for_each_entry(pol, &net->xfrm.policy_all, walk.all)\n" - "> \t dir = xfrm_policy_id2dir(pol->index);\n" - "> \n" - "> it gets policy by &net->xfrm.policy_all-0x130(offset of walk in policy)\n" - "> but when walk is head, we will read others memory by the calculated policy.\n" - "> such as:\n" - "> walk addr \t\tpolicy addr\n" - "> 0xffff0000d7f3b530 0xffff0000d7f3b400 (non-existent) \n" - "> \n" - "> head walker of net->xfrm.policy_all can be skipped by list_for_each_entry().\n" - "> but the walker created by socket is located list tail. so we should skip it. \n" + ">> >> The walker object initialized by xfrm_policy_walk_init() doesnt have policy. \n" + ">> >> list_for_each_entry() will use the walker offset to calculate policy address.\n" + ">> >> It's nonexistent and different from invalid dead policy. It will read memory \n" + ">> >> that doesnt belong to walker if dereference policy->index.\n" + ">> >> I think we should protect the memory.\n" + ">> >\n" + ">> >But all operations here are an outcome of \"list_for_each_entry(policy,\n" + ">> >&net->xfrm.policy_all, walk.all)\" which stores in policy iterator\n" + ">> >the pointer to struct xfrm_policy.\n" + ">> >\n" + ">> >How at the same time access to policy->walk.dead is valid while\n" + ">> >policy->index is not?\n" + ">> >\n" + ">> >Thanks\n" + ">> 1.walker init: its only a list head, no policy\n" + ">> xfrm_dump_policy_start\n" + ">> \txfrm_policy_walk_init(walk, XFRM_POLICY_TYPE_ANY);\n" + ">> \t\tINIT_LIST_HEAD(&walk->walk.all);\n" + ">> \t\twalk->walk.dead = 1;\n" + ">> \n" + ">> 2.add the walk head to net->xfrm.policy_all\n" + ">> xfrm_policy_walk\n" + ">> list_for_each_entry_from(x, &net->xfrm.policy_all, all)\n" + ">> \tif (error) {\n" + ">> \t\tlist_move_tail(&walk->walk.all, &x->all);\n" + ">> \t\t//add the walk to list tail\n" + ">> \n" + ">> 3.traverse the walk list\n" + ">> xfrm_policy_flush\n" + ">> list_for_each_entry(pol, &net->xfrm.policy_all, walk.all)\n" + ">> \t dir = xfrm_policy_id2dir(pol->index);\n" + ">> \n" + ">> it gets policy by &net->xfrm.policy_all-0x130(offset of walk in policy)\n" + ">> but when walk is head, we will read others memory by the calculated policy.\n" + ">> such as:\n" + ">> walk addr \t\tpolicy addr\n" + ">> 0xffff0000d7f3b530 0xffff0000d7f3b400 (non-existent) \n" + ">> \n" + ">> head walker of net->xfrm.policy_all can be skipped by list_for_each_entry().\n" + ">> but the walker created by socket is located list tail. so we should skip it. \n" + ">\n" + ">list_for_each_entry_from(x, &net->xfrm.policy_all, all) gives you\n" + ">pointer to \"x\", you can't access some of its fields and say they\n" + ">exist and other doesn't. Once you can call to \"x->...\", you can \n" + ">call to \"x->index\" too.\n" + ">\n" + ">Thanks\n" + "We get a pointer addr not actual variable from list_for_each_entry_from(),\n" + "that calculated by walk address dec offset from struct xfrm_policy(0x130).\n" "\n" - "list_for_each_entry_from(x, &net->xfrm.policy_all, all) gives you\n" - "pointer to \"x\", you can't access some of its fields and say they\n" - "exist and other doesn't. Once you can call to \"x->...\", you can \n" - "call to \"x->index\" too.\n" + "walk addr: 0xffff0000d7f3b530 //allocated by socket, valid\n" + "-> dec 0x130 (use macro container_of)\n" + "policy_addr:0xffff0000d7f3b400 //only a pointer addr\n" + "-> add 0x130 \n" + "policy->walk:0xffff0000d7f3b530 //its still walker head\n" "\n" - Thanks + "I think its invalid to read policy->index from memory that maybe allocated\n" + "by other user.\n" + "\n" + "Thanks!\n" + Dong Chenchen -c9411b073d211617d7fa9ccfac0ed5bf2fd64918e5161b019ea99ba6802248b1 +29ad55b80d72c456d77c75fb4800678551350af5d777d527640aafc6b90bd529
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox