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

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 15 Aug 2023 21:43:28 +0800
From: Dong Chenchen <dongchenchen2@...wei.com>
To: <leon@...nel.org>
CC: <fw@...len.de>, <steffen.klassert@...unet.com>,
	<herbert@...dor.apana.org.au>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <timo.teras@....fi>,
	<yuehaibing@...wei.com>, <weiyongjun1@...wei.com>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [Patch net, v2] net: xfrm: skip policies marked as dead while reinserting policies

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).

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.

Thanks!
Dong Chenchen