netdev - Re: [PATCH ipsec] xfrm: state: match with both mark and mask on user interfaces

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]

Message-ID: <d510d172-c605-725d-e6bc-e6462a3718ab@strongswan.org>
Date:   Tue, 30 Jun 2020 11:08:34 +0200
From:   Tobias Brunner <tobias@...ongswan.org>
To:     Xin Long <lucien.xin@...il.com>, netdev@...r.kernel.org
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jamal Hadi Salim <hadi@...erus.ca>,
        Sabrina Dubroca <sd@...asysnail.net>
Subject: Re: [PATCH ipsec] xfrm: state: match with both mark and mask on user
 interfaces

Hi Xin,

> Similar to commit 4f47e8ab6ab79 ("xfrm: policy: match with both mark and
> mask on user interfaces"), this patch is to match both mark and mask for
> state on these user interfaces:
> 
>   xfrm_state_lookup_byaddr_user
>   xfrm_state_lookup_user
>   xfrm_state_update
>   xfrm_state_find
>   xfrm_state_add
>       __xfrm_state_lookup_byaddr(struct xfrm_mark)
>       __xfrm_state_lookup(struct xfrm_mark)
>   xfrm_find_acq_byseq
>   xfrm_stateonly_find
> 
>           mark.v == x->mark.v && mark.m == x->mark.m

I generally agree with matching marks/masks exactly for operations from
userland, and it doesn't introduce any issues in our test suite.
However, xfrm_state_find() is used to find an outbound state based on
the templates in a policy and the marks on both, so it's not directly
userland-facing.  Before this change, the mask configured on the state
was a applied to the policy's mark/mask and then compared to the state's
mark.  Now, the mark and mask both must match exactly:

> @@ -1051,7 +1061,6 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	int acquire_in_progress = 0;
>  	int error = 0;
>  	struct xfrm_state *best = NULL;
> -	u32 mark = pol->mark.v & pol->mark.m;
>  	unsigned short encap_family = tmpl->encap_family;
>  	unsigned int sequence;
>  	struct km_event c;
> @@ -1065,7 +1074,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h, bydst) {
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
> -		    (mark & x->mark.m) == x->mark.v &&
> +		    (pol->mark.v == x->mark.v && pol->mark.m == x->mark.m) &&
>  		    x->if_id == if_id &&
>  		    !(x->props.flags & XFRM_STATE_WILDRECV) &&
>  		    xfrm_state_addr_check(x, daddr, saddr, encap_family) &&
> @@ -1082,7 +1091,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	hlist_for_each_entry_rcu(x, net->xfrm.state_bydst + h_wildcard, bydst) {
>  		if (x->props.family == encap_family &&
>  		    x->props.reqid == tmpl->reqid &&
> -		    (mark & x->mark.m) == x->mark.v &&
> +		    (pol->mark.v == x->mark.v && pol->mark.m == x->mark.m) &&
>  		    x->if_id == if_id &&
>  		    !(x->props.flags & XFRM_STATE_WILDRECV) &&
>  		    xfrm_addr_equal(&x->id.daddr, daddr, encap_family) &&

While this should usually not be a problem for strongSwan, as we set the
same mark/value on both states and corresponding policies (although the
latter can be disabled as users may want to install policies themselves
or via another daemon e.g. for MIPv6), it might be a limitation for some
use cases.  The current code allows sharing states with multiple
policies whose mark/mask doesn't match exactly (i.e. depended on the
masks of both).  I wonder if anybody uses it like this, and how others
think about it.

Regards,
Tobias