[PATCH v2 ipsec] Clear secpath on loopback_xmit

[PATCH v2 ipsec] Clear secpath on loopback_xmit

[Benedict Wong]

This patch clears the skb->sp when transmitted over loopback. This
ensures that the loopback-ed packet does not have any secpath
information from the outbound transforms.

At present, this causes XFRM tunnel mode packets to be dropped with
XFRMINNOPOLS, due to the outbound state being in the secpath, without
a matching inbound policy. Clearing the secpath ensures that all states
added to the secpath are exclusively from the inbound processing.

Tests: xfrm tunnel mode tests added for loopback:
    https://android-review.googlesource.com/c/kernel/tests/+/777328
Fixes: 8fe7ee2ba983 ("[IPsec]: Strengthen policy checks")
Signed-off-by: Benedict Wong <benedictwong@google.com>
---
 drivers/net/loopback.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 30612497643c..a6bf54df94bd 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -50,6 +50,7 @@
 #include <linux/ethtool.h>
 #include <net/sock.h>
 #include <net/checksum.h>
+#include <net/xfrm.h>
 #include <linux/if_ether.h>	/* For the statistics structure. */
 #include <linux/if_arp.h>	/* For ARPHRD_ETHER */
 #include <linux/ip.h>
@@ -82,6 +83,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
 	 */
 	skb_dst_force(skb);
 
+	// Clear secpath to ensure xfrm policy check not tainted by outbound SAs.
+	secpath_reset(skb);
+
 	skb->protocol = eth_type_trans(skb, dev);
 
 	/* it's OK to use per_cpu_ptr() because BHs are off */
-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v2 ipsec] Clear secpath on loopback_xmit

[Steffen Klassert]

On Mon, Oct 08, 2018 at 11:13:36AM -0700, Benedict Wong wrote:
> This patch clears the skb->sp when transmitted over loopback. This
> ensures that the loopback-ed packet does not have any secpath
> information from the outbound transforms.
> 
> At present, this causes XFRM tunnel mode packets to be dropped with
> XFRMINNOPOLS, due to the outbound state being in the secpath, without
> a matching inbound policy. Clearing the secpath ensures that all states
> added to the secpath are exclusively from the inbound processing.
> 
> Tests: xfrm tunnel mode tests added for loopback:
>     https://android-review.googlesource.com/c/kernel/tests/+/777328
> Fixes: 8fe7ee2ba983 ("[IPsec]: Strengthen policy checks")

This patch is from 2003, it predates our git history.
The fixes tag is mainly to ensure proper backporting.
The commit you mentioned can't be found in the mainline
git repo, so it does not help much.

If this bug was really introduced in pre git times,
use the very first git commit in our history.

It should look like:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

But I doubt that this commit introduced the bug. We started to
use outbound secpaths when we added offloading support, so I
think one of the offloading patches introduced it.

Do you have CONFIG_INET_ESP_OFFLOAD or CONFIG_INET6_ESP_OFFLOAD
enabled? Do you see the bug without these two config options
enabled?

> Signed-off-by: Benedict Wong <benedictwong@google.com>
> ---
>  drivers/net/loopback.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
> index 30612497643c..a6bf54df94bd 100644
> --- a/drivers/net/loopback.c
> +++ b/drivers/net/loopback.c
> @@ -50,6 +50,7 @@
>  #include <linux/ethtool.h>
>  #include <net/sock.h>
>  #include <net/checksum.h>
> +#include <net/xfrm.h>
>  #include <linux/if_ether.h>	/* For the statistics structure. */
>  #include <linux/if_arp.h>	/* For ARPHRD_ETHER */
>  #include <linux/ip.h>
> @@ -82,6 +83,9 @@ static netdev_tx_t loopback_xmit(struct sk_buff *skb,
>  	 */
>  	skb_dst_force(skb);
>  
> +	// Clear secpath to ensure xfrm policy check not tainted by outbound SAs.

Please align your comment to the format of the other comments in this file.
It should look like this:

	/* Clear secpath to ensure xfrm policy check not tainted by
	 * outbound SAs.
	 */

> +	secpath_reset(skb);
> +
>  	skb->protocol = eth_type_trans(skb, dev);
>  
>  	/* it's OK to use per_cpu_ptr() because BHs are off */
> -- 
> 2.19.0.605.g01d371f741-goog