On 18/07/2024 12:46, Sabrina Dubroca wrote:

Sorry Antonio, I'm only coming back to this now.

No worries and thanks for fishing this email.

2024-05-10, 16:41:43 +0200, Antonio Quartulli wrote:

quoted

On 10/05/2024 15:45, Sabrina Dubroca wrote:

quoted

2024-05-06, 03:16:23 +0200, Antonio Quartulli wrote:

quoted

diff --git a/drivers/net/ovpn/io.c b/drivers/net/ovpn/io.c
index 36cfb95edbf4..9935a863bffe 100644
--- a/drivers/net/ovpn/io.c
+++ b/drivers/net/ovpn/io.c
+/* Called after decrypt to write the IP packet to the device.
+ * This method is expected to manage/free the skb.
+ */
+static void ovpn_netdev_write(struct ovpn_peer *peer, struct sk_buff *skb)
+{
+	/* packet integrity was verified on the VPN layer - no need to perform
+	 * any additional check along the stack

But it could have been corrupted before it got into the VPN?

It could, but I believe a VPN should only take care of integrity along its
tunnel (and this is guaranteed by the OpenVPN protocol).
If something corrupted enters the tunnel, we will just deliver it as is to
the other end. Upper layers (where the corruption actually happened) have to
deal with that.

I agree with that, but I don't think that's what CHECKSUM_UNNECESSARY
(especially with csum_level = MAX) would do. CHECKSUM_UNNECESSARY
tells the networking stack that the checksum has been verified (up to
csum_level+1, so 0 means the first level of TCP/UDP type headers has
been validated):

// include/linux/skbuff.h

  * - %CHECKSUM_UNNECESSARY
  *
  *   The hardware you're dealing with doesn't calculate the full checksum
  *   (as in %CHECKSUM_COMPLETE), but it does parse headers and verify checksums
  *   for specific protocols. For such packets it will set %CHECKSUM_UNNECESSARY
  *   if their checksums are okay.

  *   &sk_buff.csum_level indicates the number of consecutive checksums found in
  *   the packet minus one that have been verified as %CHECKSUM_UNNECESSARY.
  *   For instance if a device receives an IPv6->UDP->GRE->IPv4->TCP packet
  *   and a device is able to verify the checksums for UDP (possibly zero),
  *   GRE (checksum flag is set) and TCP, &sk_buff.csum_level would be set to
  *   two. If the device were only able to verify the UDP checksum and not
  *   GRE, either because it doesn't support GRE checksum or because GRE
  *   checksum is bad, skb->csum_level would be set to zero (TCP checksum is
  *   not considered in this case).

I think you want CHECKSUM_NONE:

  *   Device did not checksum this packet e.g. due to lack of capabilities.

Then the stack will check if the packet was corrupted.

I went back to the wireguard code, which I used for inspiration for this 
specific part (we are dealing with the same problem here):

https://elixir.bootlin.com/linux/v6.10/source/drivers/net/wireguard/receive.c#L376

basically the idea is: with our encapsulation we can guarantee that what 
entered the tunnel is also exiting the tunnel, without corruption.
Therefore we claim that checksums are all correct.

Doesn't it make sense?

Cheers,

quoted

quoted

quoted

+	 */
+	skb->ip_summed = CHECKSUM_UNNECESSARY;
+	skb->csum_level = ~0;
+

-- 
Antonio Quartulli
OpenVPN Inc.