On 14.11.2024 10:07, Antonio Quartulli wrote:
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 :))
Still curious to see an example of that strange architecture/compiler
combination. Anyway, as Sabrina mentioned, the header is already pretty
aligned. So it's up to you how to document the structure.
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! :))
The main reason behind the structure introduction is to improve the code
readability. To reduce a shadow, where bugs can reside. I wonder how
many people have invested their time to dig through the encryption
preparation function?
As for risk of breaking something I should say that it can be addressed
by connecting the kernel implementation to pure usespace implementation,
which can be assumed the reference. And, I believe, it worth the benefit
of merging easy to understand code.
--
Sergey
