netdev - Re: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using UPDSA

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, 3 Jul 2018 08:14:16 +0300
From:   Eyal Birger <eyal.birger@...il.com>
To:     Nathan Harold <nharold@...gle.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ipsec-next] xfrm: Allow Set Mark to be Updated Using
 UPDSA

Hi Nathan,

On Fri, 29 Jun 2018 15:07:10 -0700
Nathan Harold <nharold@...gle.com> wrote:

> Allow UPDSA to change "set mark" to permit
> policy separation of packet routing decisions from
> SA keying in systems that use mark-based routing.
> 
> The set mark, used as a routing and firewall mark
> for outbound packets, is made update-able which
> allows routing decisions to be handled independently
> of keying/SA creation. To maintain consistency with
> other optional attributes, the set mark is only
> updated if sent with a non-zero value.
> 
> The per-SA lock and the xfrm_state_lock are taken in
> that order to avoid a deadlock with
> xfrm_timer_handler(), which also takes the locks in
> that order.
> 
> Signed-off-by: Nathan Harold <nharold@...gle.com>
> Change-Id: Ia05c6733a94c1901cd1e54eb7c7e237704678d71
> ---
>  net/xfrm/xfrm_state.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index e04a510ec992..c9ffcdfa89f6 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1562,6 +1562,15 @@ int xfrm_state_update(struct xfrm_state *x)
>  		if (x1->curlft.use_time)
>  			xfrm_state_check_expire(x1);
>  
> +		if (x->props.smark.m || x->props.smark.v) {
> +			spin_lock_bh(&net->xfrm.xfrm_state_lock);
> +
> +			x1->props.smark = x->props.smark;
> +
> +			__xfrm_state_bump_genids(x1);

So I'm trying to wrap my head around this genid thing :)

If x1 points to a state previously found using __xfrm_state_locate(x),
won't __xfrm_state_bump_genids(x1) be equivalent to x1->genid++ in
this case?

Is it possible that other states will match all of x1 parameters?

Also, any idea why this isn't needed for other changes in the state?

Thanks!
Eyal.