On 12/11/2024 17:47, Sabrina Dubroca wrote:
2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
+/* When the OpenVPN protocol is ran in AEAD mode, use
+ * the OpenVPN packet ID as the AEAD nonce:
+ *
+ * 00000005 521c3b01 4308c041
+ * [seq # ] [ nonce_tail ]
+ * [ 12-byte full IV ] -> NONCE_SIZE
+ * [4-bytes -> NONCE_WIRE_SIZE
+ * on wire]
+ */
Nice diagram! Can we go futher and define the OpenVPN packet header as a
stucture? Referencing the structure instead of using magic sizes and offsets
can greatly improve the code readability. Especially when it comes to header
construction/parsing in the encryption/decryption code.
E.g. define a structures like this:
struct ovpn_pkt_hdr {
__be32 op;
__be32 pktid;
u8 auth[];
} __attribute__((packed));
struct ovpn_aead_iv {
__be32 pktid;
u8 nonce[OVPN_NONCE_TAIL_SIZE];
} __attribute__((packed));
__attribute__((packed)) should not be needed here as the fields in
both structs look properly aligned, and IIRC using packed can cause
the compiler to generate worse code.
Agreed. Using packed will make certain architecture read every field
byte by byte (I remember David M. biting us on this in batman-adv :))
This said, I like the idea of using a struct, but I don't feel confident
enough to change the code now that we are hitting v12.
This kind of change will be better implemented later and tested
carefully. (and patches are always welcome! :))
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 8516c1ccd57a7c7634a538fe3ac16c858f647420..84d294aab20b79b8e9cb9b736a074105c99338f3 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -1975,4 +1975,19 @@ enum {
#define IFLA_DSA_MAX (__IFLA_DSA_MAX - 1)
+/* OVPN section */
+
+enum ovpn_mode {
+ OVPN_MODE_P2P,
+ OVPN_MODE_MP,
+};
Mode min/max values can be defined here and the netlink policy can reference
these values:
enum ovpn_mode {
OVPN_MODE_P2P,
OVPN_MODE_MP,
__OVPN_MODE_MAX
};
#define OVPN_MODE_MIN OVPN_MODE_P2P
#define OVPN_MODE_MAX (__OVPN_MODE_MAX - 1)
... = NLA_POLICY_RANGE(NLA_U8, OVPN_MODE_MIN, OVPN_MODE_MAX)
I don't think there's much benefit to that, other than making the diff
smaller on a (very unlikely) patch that would add a new mode in the
future. It even looks more inconvenient to me when reading the code
("ok what are _MIN and _MAX? the code is using _P2P and _MP, do they
match?").
I agree with Sabrina here.
I also initially thought about having MIN/MAX, but it wouldn't make
things simpler for the ovpn_mode.
Regards,
--
Antonio Quartulli
OpenVPN Inc.
