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] [day] [month] [year] [list]
Message-ID: <20230815181940.GO22185@unreal>
Date:   Tue, 15 Aug 2023 21:19:40 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Dong Chenchen <dongchenchen2@...wei.com>
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 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
> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ