| Message ID | 50e4e7fd0b978aaa4721f022a3d5737c377c8375.1718087437.git.antony.antony@secunet.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec,1/2] xfrm: Fix input error path memory access | expand |
On Tue, Jun 11, 2024 at 08:32:15AM +0200, Antony Antony wrote: > Previously, the offload data path decrypted the packet before checking > the direction, leading to error logging and packet dropping. However, > dropped packets wouldn't be visible in tcpdump or audit log. > > With this fix, the offload path, upon noticing SA direction mismatch, > will pass the packet to the stack without decrypting it. The L3 layer > will then log the error, audit, and drop ESP without decrypting or > decapsulating it. > > This also ensures that the slow path records the error and audit log, > making dropped packets visible in tcpdump. > > Fixes: 304b44f0d5a4 ("xfrm: Add dir validation to "in" data path lookup") > Signed-off-by: Antony Antony <antony.antony@secunet.com> Thanks Antony, The comment below notwithstanding, this looks good to me. Reviewed-by: Simon Horman <horms@kernel.org> > --- > net/ipv4/esp4_offload.c | 7 +++++++ > net/ipv6/esp6_offload.c | 7 +++++++ > net/xfrm/xfrm_input.c | 5 ----- > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c > index b3271957ad9a..3f28ecbdcaef 100644 > --- a/net/ipv4/esp4_offload.c > +++ b/net/ipv4/esp4_offload.c > @@ -56,6 +56,13 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head, > x = xfrm_state_lookup(dev_net(skb->dev), skb->mark, > (xfrm_address_t *)&ip_hdr(skb)->daddr, > spi, IPPROTO_ESP, AF_INET); > + > + if (unlikely(x && x->dir && x->dir != XFRM_SA_DIR_IN)) { > + /* non-offload path will record the error and audit log */ > + xfrm_state_put(x); > + x = NULL; > + } > + > if (!x) > goto out_reset; > > diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c > index 527b7caddbc6..919ebfabbe4e 100644 > --- a/net/ipv6/esp6_offload.c > +++ b/net/ipv6/esp6_offload.c > @@ -83,6 +83,13 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head, > x = xfrm_state_lookup(dev_net(skb->dev), skb->mark, > (xfrm_address_t *)&ipv6_hdr(skb)->daddr, > spi, IPPROTO_ESP, AF_INET6); > + > + if (unlikely(x && x->dir && x->dir != XFRM_SA_DIR_IN)) { > + /* non-offload path will record the error and audit log */ > + xfrm_state_put(x); > + x = NULL; > + } > + > if (!x) > goto out_reset; > The logic in the two hunks above seems to be duplicated. FWIIW, I think it would be nice to consolidate it. ...
On Tue, Jun 11, 2024 at 08:32:15AM +0200, Antony Antony wrote: > Previously, the offload data path decrypted the packet before checking > the direction, leading to error logging and packet dropping. However, > dropped packets wouldn't be visible in tcpdump or audit log. > > With this fix, the offload path, upon noticing SA direction mismatch, > will pass the packet to the stack without decrypting it. The L3 layer > will then log the error, audit, and drop ESP without decrypting or > decapsulating it. > > This also ensures that the slow path records the error and audit log, > making dropped packets visible in tcpdump. > > Fixes: 304b44f0d5a4 ("xfrm: Add dir validation to "in" data path lookup") > Signed-off-by: Antony Antony <antony.antony@secunet.com> Also applied, thanks a lot!
diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c index b3271957ad9a..3f28ecbdcaef 100644 --- a/net/ipv4/esp4_offload.c +++ b/net/ipv4/esp4_offload.c @@ -56,6 +56,13 @@ static struct sk_buff *esp4_gro_receive(struct list_head *head, x = xfrm_state_lookup(dev_net(skb->dev), skb->mark, (xfrm_address_t *)&ip_hdr(skb)->daddr, spi, IPPROTO_ESP, AF_INET); + + if (unlikely(x && x->dir && x->dir != XFRM_SA_DIR_IN)) { + /* non-offload path will record the error and audit log */ + xfrm_state_put(x); + x = NULL; + } + if (!x) goto out_reset; diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c index 527b7caddbc6..919ebfabbe4e 100644 --- a/net/ipv6/esp6_offload.c +++ b/net/ipv6/esp6_offload.c @@ -83,6 +83,13 @@ static struct sk_buff *esp6_gro_receive(struct list_head *head, x = xfrm_state_lookup(dev_net(skb->dev), skb->mark, (xfrm_address_t *)&ipv6_hdr(skb)->daddr, spi, IPPROTO_ESP, AF_INET6); + + if (unlikely(x && x->dir && x->dir != XFRM_SA_DIR_IN)) { + /* non-offload path will record the error and audit log */ + xfrm_state_put(x); + x = NULL; + } + if (!x) goto out_reset; diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index 63c004103912..e95462b982b0 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -474,11 +474,6 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) if (encap_type < 0 || (xo && xo->flags & XFRM_GRO)) { x = xfrm_input_state(skb); - if (unlikely(x->dir && x->dir != XFRM_SA_DIR_IN)) { - XFRM_INC_STATS(net, LINUX_MIB_XFRMINSTATEDIRERROR); - goto drop; - } - if (unlikely(x->km.state != XFRM_STATE_VALID)) { if (x->km.state == XFRM_STATE_ACQ) XFRM_INC_STATS(net, LINUX_MIB_XFRMACQUIREERROR);
Previously, the offload data path decrypted the packet before checking the direction, leading to error logging and packet dropping. However, dropped packets wouldn't be visible in tcpdump or audit log. With this fix, the offload path, upon noticing SA direction mismatch, will pass the packet to the stack without decrypting it. The L3 layer will then log the error, audit, and drop ESP without decrypting or decapsulating it. This also ensures that the slow path records the error and audit log, making dropped packets visible in tcpdump. Fixes: 304b44f0d5a4 ("xfrm: Add dir validation to "in" data path lookup") Signed-off-by: Antony Antony <antony.antony@secunet.com> --- net/ipv4/esp4_offload.c | 7 +++++++ net/ipv6/esp6_offload.c | 7 +++++++ net/xfrm/xfrm_input.c | 5 ----- 3 files changed, 14 insertions(+), 5 deletions(-)