Skip to contentSkip to conversation

net/xfrm: Fix lookups for states with spi == 0

PATCH In review Dmitry Safonov 2 participants 4 messages May 2, 2018 – May 3, 2018

Subsystems (3)
Networking Network drivers Networking IPsec
lore
Share & tools
lore

Dmitry Safonov

author
#0 lore raw

PATCH 1/1 net/xfrm: Fix lookups for states with spi == 0

It seems to be a valid use case to add xfrm state without
Security Parameter Indexes (SPI) value associated:
ip xfrm state add src $src dst $dst proto $proto mode $mode sel src $src dst $dst $algo

The bad thing is that it's currently impossible to get/delete the state without SPI: __xfrm_state_insert() obviously doesn't add hash for zero SPI in xfrm.state_byspi, and xfrm_user_state_lookup() will fail as xfrm_state_lookup() does lookups by hash.

It also isn't possible to workaround from userspace as xfrm_id_proto_match() will be always true for ah/esp/comp protos.

So, don't try looking up by hash if SPI == 0.

Cc Steffen Klassert <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Dmitry Safonov <[email protected]>
---
 net/xfrm/xfrm_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/xfrm/xfrm_user.c +1 -1
index 080035f056d9..6b38503255c8 100644
@@ -681,7 +681,7 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
 	int err;
 	u32 mark = xfrm_mark_get(attrs, &m);
 
-	if (xfrm_id_proto_match(p->proto, IPSEC_PROTO_ANY)) {
+	if (xfrm_id_proto_match(p->proto, IPSEC_PROTO_ANY) && p->spi) {
 		err = -ESRCH;
 		x = xfrm_state_lookup(net, mark, &p->daddr, p->spi, p->proto, p->family);
 	} else {
Signature · 1 line
2.13.6
maintainer
#1 lore raw
Quoted 12 lines · Dmitry Safonov

It seems to be a valid use case to add xfrm state without
Security Parameter Indexes (SPI) value associated:
ip xfrm state add src $src dst $dst proto $proto mode $mode sel src $src dst $dst $algo

The bad thing is that it's currently impossible to get/delete the state without SPI: __xfrm_state_insert() obviously doesn't add hash for zero SPI in xfrm.state_byspi, and xfrm_user_state_lookup() will fail as xfrm_state_lookup() does lookups by hash.

It also isn't possible to workaround from userspace as xfrm_id_proto_match() will be always true for ah/esp/comp protos.

Quoted 7 lines

So, don't try looking up by hash if SPI == 0.

Cc Steffen Klassert <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Dmitry Safonov <[email protected]>

A zero SPI is illegal for many IPsec protocols because that value is used for other purposes, e.g., IKE encapsulation.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 18 lines · Herbert Xu
Quoted 17 lines · Dmitry Safonov

It seems to be a valid use case to add xfrm state without Security Parameter Indexes (SPI) value associated:
ip xfrm state add src $src dst $dst proto $proto mode $mode sel src $src dst $dst $algo

The bad thing is that it's currently impossible to get/delete the state
without SPI: __xfrm_state_insert() obviously doesn't add hash for zero
SPI in xfrm.state_byspi, and xfrm_user_state_lookup() will fail as xfrm_state_lookup() does lookups by hash.

It also isn't possible to workaround from userspace as xfrm_id_proto_match() will be always true for ah/esp/comp protos.

So, don't try looking up by hash if SPI == 0.

Quoted 8 lines
Quoted 5 lines
Cc Steffen Klassert <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Dmitry Safonov <[email protected]>

A zero SPI is illegal for many IPsec protocols because that value is used for other purposes, e.g., IKE encapsulation.

But still it's possible to create ipsec with zero SPI. And it seems not making sense to search for a state with SPI hash if request has zero SPI.

Signature · 1 line
            Dima
maintainer
#3 lore raw
Quoted 4 lines · Dmitry Safonov

But still it's possible to create ipsec with zero SPI. And it seems not making sense to search for a state with SPI hash if request has zero SPI.

Fair enough. In fact a zero SPI is legal and defined for IPcomp.

The bug arose from this patch:

commit 7b4dc3600e4877178ba94c7fbf7e520421378aa6

Author: Masahide NAKAMURA <[email protected]>
Date:   Wed Sep 27 22:21:52 2006 -0700

[XFRM]: Do not add a state whose SPI is zero to the SPI hash.

SPI=0 is used for acquired IPsec SA and MIPv6 RO state. Such state should not be added to the SPI hash because we do not care about it on deleting path.

Signed-off-by Masahide NAKAMURA <[email protected]>
Signed-off-by YOSHIFUJI Hideaki <[email protected]>

I think it would be better to revert this.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt