This change introduces the netlink commands needed to add, get, delete
and swap keys for a specific peer.
Userspace is expected to use these commands to create, inspect (non
sensitive data only), destroy and rotate session keys for a specific
peer.
Signed-off-by: Antonio Quartulli <antonio@openvpn.net>
---
drivers/net/ovpn/crypto.c | 40 ++++++
drivers/net/ovpn/crypto.h | 4 +
drivers/net/ovpn/crypto_aead.c | 17 +++
drivers/net/ovpn/crypto_aead.h | 2 +
drivers/net/ovpn/netlink.c | 301 ++++++++++++++++++++++++++++++++++++++++-
5 files changed, 360 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ovpn/crypto.c b/drivers/net/ovpn/crypto.c
index ac15aaa9844083328020fcc5ea6866e08ecbc184..f30c59c1193a167d7f420d89b665e2e61c57d81c 100644
--- a/drivers/net/ovpn/crypto.c
+++ b/drivers/net/ovpn/crypto.c
@@ -150,3 +150,43 @@ void ovpn_crypto_key_slots_swap(struct ovpn_crypto_state *cs)
spin_unlock_bh(&cs->lock);
}
+
+/**
+ * ovpn_crypto_config_get - populate keyconf object with non-sensible key data
+ * @cs: the crypto state to extract the key data from
+ * @slot: the specific slot to inspect
+ * @keyconf: the output object to populate
+ *
+ * Return: 0 on success or a negative error code otherwise
+ */
+int ovpn_crypto_config_get(struct ovpn_crypto_state *cs,
+ enum ovpn_key_slot slot,
+ struct ovpn_key_config *keyconf)
+{
+ struct ovpn_crypto_key_slot *ks;
+ int idx;
+
+ switch (slot) {
+ case OVPN_KEY_SLOT_PRIMARY:
+ idx = cs->primary_idx;
+ break;
+ case OVPN_KEY_SLOT_SECONDARY:
+ idx = !cs->primary_idx;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ rcu_read_lock();
+ ks = rcu_dereference(cs->slots[idx]);
+ if (!ks) {
+ rcu_read_unlock();
+ return -ENOENT;
+ }
+
+ keyconf->cipher_alg = ovpn_aead_crypto_alg(ks);
+ keyconf->key_id = ks->key_id;
+ rcu_read_unlock();
+
+ return 0;
+}
diff --git a/drivers/net/ovpn/crypto.h b/drivers/net/ovpn/crypto.h
index f3b3be9866df910e3d68762377505f65c767a4fe..d6e888381c82d208c7ff619381858302ea6fbbc7 100644
--- a/drivers/net/ovpn/crypto.h
+++ b/drivers/net/ovpn/crypto.h
@@ -136,4 +136,8 @@ void ovpn_crypto_state_release(struct ovpn_crypto_state *cs);
void ovpn_crypto_key_slots_swap(struct ovpn_crypto_state *cs);
+int ovpn_crypto_config_get(struct ovpn_crypto_state *cs,
+ enum ovpn_key_slot slot,
+ struct ovpn_key_config *keyconf);
+
#endif /* _NET_OVPN_OVPNCRYPTO_H_ */
diff --git a/drivers/net/ovpn/crypto_aead.c b/drivers/net/ovpn/crypto_aead.c
index ac678f087451da66bc39148902db21045a94eb19..fe410b615d8442fb7e17364e2fa6c95b50162577 100644
--- a/drivers/net/ovpn/crypto_aead.c
+++ b/drivers/net/ovpn/crypto_aead.c
@@ -389,3 +389,20 @@ ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc)
ovpn_aead_crypto_key_slot_destroy(ks);
return ERR_PTR(ret);
}
+
+enum ovpn_cipher_alg ovpn_aead_crypto_alg(struct ovpn_crypto_key_slot *ks)
+{
+ const char *alg_name;
+
+ if (!ks->encrypt)
+ return OVPN_CIPHER_ALG_NONE;
+
+ alg_name = crypto_tfm_alg_name(crypto_aead_tfm(ks->encrypt));
+
+ if (!strcmp(alg_name, ALG_NAME_AES))
+ return OVPN_CIPHER_ALG_AES_GCM;
+ else if (!strcmp(alg_name, ALG_NAME_CHACHAPOLY))
+ return OVPN_CIPHER_ALG_CHACHA20_POLY1305;
+ else
+ return OVPN_CIPHER_ALG_NONE;
+}
diff --git a/drivers/net/ovpn/crypto_aead.h b/drivers/net/ovpn/crypto_aead.h
index 8a9d01f5eed700d2f7dfbbecbddbb370966682b9..d43aafa622315e3fd705e5da0f175356618a456d 100644
--- a/drivers/net/ovpn/crypto_aead.h
+++ b/drivers/net/ovpn/crypto_aead.h
@@ -28,4 +28,6 @@ struct ovpn_crypto_key_slot *
ovpn_aead_crypto_key_slot_new(const struct ovpn_key_config *kc);
void ovpn_aead_crypto_key_slot_destroy(struct ovpn_crypto_key_slot *ks);
+enum ovpn_cipher_alg ovpn_aead_crypto_alg(struct ovpn_crypto_key_slot *ks);
+
#endif /* _NET_OVPN_OVPNAEAD_H_ */
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c
index 9824ce4437e63e602d37d31105fb9ac06f99d421..3d8b5e02e103e4177f3abacecfc4b52eb7226b87 100644
--- a/drivers/net/ovpn/netlink.c
+++ b/drivers/net/ovpn/netlink.c
@@ -17,6 +17,7 @@
#include "netlink.h"
#include "netlink-gen.h"
#include "bind.h"
+#include "crypto.h"
#include "peer.h"
#include "socket.h"
@@ -781,24 +782,316 @@ int ovpn_nl_peer_del_doit(struct sk_buff *skb, struct genl_info *info)
return ret;
}
+static int ovpn_nl_get_key_dir(struct genl_info *info, struct nlattr *key,
+ enum ovpn_cipher_alg cipher,
+ struct ovpn_key_direction *dir)
+{
+ struct nlattr *attrs[OVPN_A_KEYDIR_MAX + 1];
+ int ret;
+
+ ret = nla_parse_nested(attrs, OVPN_A_KEYDIR_MAX, key,
+ ovpn_keydir_nl_policy, info->extack);
+ if (ret)
+ return ret;
+
+ switch (cipher) {
+ case OVPN_CIPHER_ALG_AES_GCM:
+ case OVPN_CIPHER_ALG_CHACHA20_POLY1305:
+ if (NL_REQ_ATTR_CHECK(info->extack, key, attrs,
+ OVPN_A_KEYDIR_CIPHER_KEY) ||
+ NL_REQ_ATTR_CHECK(info->extack, key, attrs,
+ OVPN_A_KEYDIR_NONCE_TAIL))
+ return -EINVAL;
+
+ dir->cipher_key = nla_data(attrs[OVPN_A_KEYDIR_CIPHER_KEY]);
+ dir->cipher_key_size = nla_len(attrs[OVPN_A_KEYDIR_CIPHER_KEY]);
+
+ /* These algorithms require a 96bit nonce,
+ * Construct it by combining 4-bytes packet id and
+ * 8-bytes nonce-tail from userspace
+ */
+ dir->nonce_tail = nla_data(attrs[OVPN_A_KEYDIR_NONCE_TAIL]);
+ dir->nonce_tail_size = nla_len(attrs[OVPN_A_KEYDIR_NONCE_TAIL]);
+ break;
+ default:
+ NL_SET_ERR_MSG_MOD(info->extack, "unsupported cipher");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * ovpn_nl_key_new_doit - configure a new key for the specified peer
+ * @skb: incoming netlink message
+ * @info: genetlink metadata
+ *
+ * This function allows the user to install a new key in the peer crypto
+ * state.
+ * Each peer has two 'slots', namely 'primary' and 'secondary', where
+ * keys can be installed. The key in the 'primary' slot is used for
+ * encryption, while both keys can be used for decryption by matching the
+ * key ID carried in the incoming packet.
+ *
+ * The user is responsible for rotating keys when necessary. The user
+ * may fetch peer traffic statistics via netlink in order to better
+ * identify the right time to rotate keys.
+ * The renegotiation follows these steps:
+ * 1. a new key is computed by the user and is installed in the 'secondary'
+ * slot
+ * 2. at user discretion (usually after a predetermined time) 'primary' and
+ * 'secondary' contents are swapped and the new key starts being used for
+ * encryption, while the old key is kept around for decryption of late
+ * packets.
+ *
+ * Return: 0 on success or a negative error code otherwise.
+ */
int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
{
- return -EOPNOTSUPP;
+ struct nlattr *attrs[OVPN_A_KEYCONF_MAX + 1];
+ struct ovpn_priv *ovpn = info->user_ptr[0];
+ struct ovpn_peer_key_reset pkr;
+ struct ovpn_peer *peer;
+ u32 peer_id;
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, OVPN_A_KEYCONF))
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,
+ info->attrs[OVPN_A_KEYCONF],
+ ovpn_keyconf_nl_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_PEER_ID))
+ return -EINVAL;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_SLOT) ||
+ NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_KEY_ID) ||
+ NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_CIPHER_ALG) ||
+ NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_ENCRYPT_DIR) ||
+ NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_DECRYPT_DIR))
+ return -EINVAL;
+
+ pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
+ pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
+ pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
+
+ ret = ovpn_nl_get_key_dir(info, attrs[OVPN_A_KEYCONF_ENCRYPT_DIR],
+ pkr.key.cipher_alg, &pkr.key.encrypt);
+ if (ret < 0)
+ return ret;
+
+ ret = ovpn_nl_get_key_dir(info, attrs[OVPN_A_KEYCONF_DECRYPT_DIR],
+ pkr.key.cipher_alg, &pkr.key.decrypt);
+ if (ret < 0)
+ return ret;
+
+ peer_id = nla_get_u32(attrs[OVPN_A_KEYCONF_PEER_ID]);
+ peer = ovpn_peer_get_by_id(ovpn, peer_id);
+ if (!peer) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "no peer with id %u to set key for",
+ peer_id);
+ return -ENOENT;
+ }
+
+ ret = ovpn_crypto_state_reset(&peer->crypto, &pkr);
+ if (ret < 0) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot install new key for peer %u",
+ peer_id);
+ goto out;
+ }
+
+ netdev_dbg(ovpn->dev, "new key installed (id=%u) for peer %u\n",
+ pkr.key.key_id, peer_id);
+out:
+ ovpn_peer_put(peer);
+ return ret;
+}
+
+static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
+ u32 peer_id, enum ovpn_key_slot slot,
+ const struct ovpn_key_config *keyconf)
+{
+ struct nlattr *attr;
+ void *hdr;
+
+ hdr = genlmsg_put(skb, info->snd_portid, info->snd_seq, &ovpn_nl_family,
+ 0, OVPN_CMD_KEY_GET);
+ if (!hdr)
+ return -ENOBUFS;
+
+ attr = nla_nest_start(skb, OVPN_A_KEYCONF);
+ if (!attr)
+ goto err;
+
+ if (nla_put_u32(skb, OVPN_A_KEYCONF_PEER_ID, peer_id))
+ goto err;
+
+ if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
+ nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
+ nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
+ goto err;
+
+ nla_nest_end(skb, attr);
+ genlmsg_end(skb, hdr);
+
+ return 0;
+err:
+ genlmsg_cancel(skb, hdr);
+ return -EMSGSIZE;
}
int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
{
- return -EOPNOTSUPP;
+ struct nlattr *attrs[OVPN_A_KEYCONF_MAX + 1];
+ struct ovpn_priv *ovpn = info->user_ptr[0];
+ struct ovpn_key_config keyconf = { 0 };
+ enum ovpn_key_slot slot;
+ struct ovpn_peer *peer;
+ struct sk_buff *msg;
+ u32 peer_id;
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, OVPN_A_KEYCONF))
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,
+ info->attrs[OVPN_A_KEYCONF],
+ ovpn_keyconf_nl_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_PEER_ID))
+ return -EINVAL;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_SLOT))
+ return -EINVAL;
+
+ peer_id = nla_get_u32(attrs[OVPN_A_KEYCONF_PEER_ID]);
+ peer = ovpn_peer_get_by_id(ovpn, peer_id);
+ if (!peer) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot find peer with id %u", peer_id);
+ return -ENOENT;
+ }
+
+ slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
+
+ ret = ovpn_crypto_config_get(&peer->crypto, slot, &keyconf);
+ if (ret < 0) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "cannot extract key from slot %u for peer %u",
+ slot, peer_id);
+ goto err;
+ }
+
+ msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!msg) {
+ ret = -ENOMEM;
+ goto err;
+ }
+
+ ret = ovpn_nl_send_key(msg, info, peer->id, slot, &keyconf);
+ if (ret < 0) {
+ nlmsg_free(msg);
+ goto err;
+ }
+
+ ret = genlmsg_reply(msg, info);
+err:
+ ovpn_peer_put(peer);
+ return ret;
}
int ovpn_nl_key_swap_doit(struct sk_buff *skb, struct genl_info *info)
{
- return -EOPNOTSUPP;
+ struct ovpn_priv *ovpn = info->user_ptr[0];
+ struct nlattr *attrs[OVPN_A_PEER_MAX + 1];
+ struct ovpn_peer *peer;
+ u32 peer_id;
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, OVPN_A_KEYCONF))
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,
+ info->attrs[OVPN_A_KEYCONF],
+ ovpn_keyconf_nl_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_PEER_ID))
+ return -EINVAL;
+
+ peer_id = nla_get_u32(attrs[OVPN_A_KEYCONF_PEER_ID]);
+ peer = ovpn_peer_get_by_id(ovpn, peer_id);
+ if (!peer) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "no peer with id %u to swap keys for",
+ peer_id);
+ return -ENOENT;
+ }
+
+ ovpn_crypto_key_slots_swap(&peer->crypto);
+ ovpn_peer_put(peer);
+
+ return 0;
}
int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
{
- return -EOPNOTSUPP;
+ struct nlattr *attrs[OVPN_A_KEYCONF_MAX + 1];
+ struct ovpn_priv *ovpn = info->user_ptr[0];
+ enum ovpn_key_slot slot;
+ struct ovpn_peer *peer;
+ u32 peer_id;
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, OVPN_A_KEYCONF))
+ return -EINVAL;
+
+ ret = nla_parse_nested(attrs, OVPN_A_KEYCONF_MAX,
+ info->attrs[OVPN_A_KEYCONF],
+ ovpn_keyconf_nl_policy, info->extack);
+ if (ret)
+ return ret;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_PEER_ID))
+ return -EINVAL;
+
+ if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
+ OVPN_A_KEYCONF_SLOT))
+ return -EINVAL;
+
+ peer_id = nla_get_u32(attrs[OVPN_A_KEYCONF_PEER_ID]);
+ slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
+
+ peer = ovpn_peer_get_by_id(ovpn, peer_id);
+ if (!peer) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "no peer with id %u to delete key for",
+ peer_id);
+ return -ENOENT;
+ }
+
+ ovpn_crypto_key_slot_delete(&peer->crypto, slot);
+ ovpn_peer_put(peer);
+
+ return 0;
}
/**
--
2.45.32025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
> int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
> {
...
> + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
> + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
> + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
[...]
> +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
> + u32 peer_id, enum ovpn_key_slot slot,
> + const struct ovpn_key_config *keyconf)
> +{
...
> + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
> + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
> + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
That's a bit inconsistent. nla_put_u32 matches the generated policy,
but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
key_id" it's getting stored into).
[also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
> int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
> {
...
> + slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
> int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
> {
...
> + slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
A few more inconsistencies:
* OVPN_A_IFNAME is defined in the uapi but never used (I guess
leftover from genl link creation)
* OVPN_A_PEER_DEL_REASON (u32 vs u8)
drivers/net/ovpn/netlink-gen.c:52: [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4),
drivers/net/ovpn/netlink.c:1131: if (nla_put_u8(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))
* OVPN_A_PEER_LINK_TX_PACKETS/OVPN_A_PEER_LINK_RX_PACKETS (uint vs u32):
drivers/net/ovpn/netlink-gen.c-57- [OVPN_A_PEER_LINK_RX_BYTES] = { .type = NLA_UINT, },
drivers/net/ovpn/netlink-gen.c-58- [OVPN_A_PEER_LINK_TX_BYTES] = { .type = NLA_UINT, },
drivers/net/ovpn/netlink-gen.c-59- [OVPN_A_PEER_LINK_RX_PACKETS] = { .type = NLA_U32, },
drivers/net/ovpn/netlink-gen.c:60: [OVPN_A_PEER_LINK_TX_PACKETS] = { .type = NLA_U32, },
--
drivers/net/ovpn/netlink.c-618- /* link RX stats */
drivers/net/ovpn/netlink.c-619- nla_put_uint(skb, OVPN_A_PEER_LINK_RX_BYTES,
drivers/net/ovpn/netlink.c-620- atomic64_read(&peer->link_stats.rx.bytes)) ||
drivers/net/ovpn/netlink.c-621- nla_put_uint(skb, OVPN_A_PEER_LINK_RX_PACKETS,
drivers/net/ovpn/netlink.c-622- atomic64_read(&peer->link_stats.rx.packets)) ||
drivers/net/ovpn/netlink.c-623- /* link TX stats */
drivers/net/ovpn/netlink.c-624- nla_put_uint(skb, OVPN_A_PEER_LINK_TX_BYTES,
drivers/net/ovpn/netlink.c-625- atomic64_read(&peer->link_stats.tx.bytes)) ||
drivers/net/ovpn/netlink.c:626: nla_put_uint(skb, OVPN_A_PEER_LINK_TX_PACKETS,
drivers/net/ovpn/netlink.c-627- atomic64_read(&peer->link_stats.tx.packets)))
I guess all the stats should be UINT.
--
Sabrina
On 04/03/2025 13:00, Sabrina Dubroca wrote:
> 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
>> int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
>> {
> ...
>> + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>> + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
>> + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
>
>
> [...]
>> +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
>> + u32 peer_id, enum ovpn_key_slot slot,
>> + const struct ovpn_key_config *keyconf)
>> +{
> ...
>> + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
>> + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
>> + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
>
> That's a bit inconsistent. nla_put_u32 matches the generated policy,
> but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
> key_id" it's getting stored into).
>
> [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
From
https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
"Note that types smaller than 32 bit should be avoided as using them
does not save any memory in Netlink messages (due to alignment)."
Hence I went for u32 attributes, although values stored into them are
much smaller.
>
>
>
>> int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
>> {
> ...
>> + slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
>
>
>
>> int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
>> {
> ...
>> + slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
Right. Since actual values are smaller I sometimes used the smaller macro.
I guess I better convert them all to u32.
>
>
>
> A few more inconsistencies:
>
> * OVPN_A_IFNAME is defined in the uapi but never used (I guess
> leftover from genl link creation)
Ouch, I guess you're right about it being a leftover.
>
> * OVPN_A_PEER_DEL_REASON (u32 vs u8)
> drivers/net/ovpn/netlink-gen.c:52: [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4),
> drivers/net/ovpn/netlink.c:1131: if (nla_put_u8(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))
Like above, probably better to convert them all to u32.
>
> * OVPN_A_PEER_LINK_TX_PACKETS/OVPN_A_PEER_LINK_RX_PACKETS (uint vs u32):
> drivers/net/ovpn/netlink-gen.c-57- [OVPN_A_PEER_LINK_RX_BYTES] = { .type = NLA_UINT, },
> drivers/net/ovpn/netlink-gen.c-58- [OVPN_A_PEER_LINK_TX_BYTES] = { .type = NLA_UINT, },
> drivers/net/ovpn/netlink-gen.c-59- [OVPN_A_PEER_LINK_RX_PACKETS] = { .type = NLA_U32, },
> drivers/net/ovpn/netlink-gen.c:60: [OVPN_A_PEER_LINK_TX_PACKETS] = { .type = NLA_U32, },
> --
> drivers/net/ovpn/netlink.c-618- /* link RX stats */
> drivers/net/ovpn/netlink.c-619- nla_put_uint(skb, OVPN_A_PEER_LINK_RX_BYTES,
> drivers/net/ovpn/netlink.c-620- atomic64_read(&peer->link_stats.rx.bytes)) ||
> drivers/net/ovpn/netlink.c-621- nla_put_uint(skb, OVPN_A_PEER_LINK_RX_PACKETS,
> drivers/net/ovpn/netlink.c-622- atomic64_read(&peer->link_stats.rx.packets)) ||
> drivers/net/ovpn/netlink.c-623- /* link TX stats */
> drivers/net/ovpn/netlink.c-624- nla_put_uint(skb, OVPN_A_PEER_LINK_TX_BYTES,
> drivers/net/ovpn/netlink.c-625- atomic64_read(&peer->link_stats.tx.bytes)) ||
> drivers/net/ovpn/netlink.c:626: nla_put_uint(skb, OVPN_A_PEER_LINK_TX_PACKETS,
> drivers/net/ovpn/netlink.c-627- atomic64_read(&peer->link_stats.tx.packets)))
>
> I guess all the stats should be UINT.
u32 should be enough for packets - but yeah, going UINT is better as I
don't need to care if it really fits 4B or not.
Will do.
Thanks!
--
Antonio Quartulli
OpenVPN Inc.
2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
> On 04/03/2025 13:00, Sabrina Dubroca wrote:
> > 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
> > > int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
> > > {
> > ...
> > > + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
> > > + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
> > > + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
> >
> >
> > [...]
> > > +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
> > > + u32 peer_id, enum ovpn_key_slot slot,
> > > + const struct ovpn_key_config *keyconf)
> > > +{
> > ...
> > > + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
> > > + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
> > > + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
> >
> > That's a bit inconsistent. nla_put_u32 matches the generated policy,
> > but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
> > key_id" it's getting stored into).
> >
> > [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
>
> From https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
>
> "Note that types smaller than 32 bit should be avoided as using them does
> not save any memory in Netlink messages (due to alignment)."
>
> Hence I went for u32 attributes, although values stored into them are much
> smaller.
Right.
> > > int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > {
> > ...
> > > + slot = nla_get_u32(attrs[OVPN_A_KEYCONF_SLOT]);
> >
> >
> >
> > > int ovpn_nl_key_del_doit(struct sk_buff *skb, struct genl_info *info)
> > > {
> > ...
> > > + slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>
> Right. Since actual values are smaller I sometimes used the smaller macro.
Which only works on little-endian if the other side is using the full
size?
LE BE
put_u32(1) -> 01000000 00000001
get_u8 ^^ ^^
=1 =0
(and put_u8(1) vs get_u32 would result in fetching 0x1000000 in BE)
> I guess I better convert them all to u32.
SGTM
> > A few more inconsistencies:
> >
> > * OVPN_A_IFNAME is defined in the uapi but never used (I guess
> > leftover from genl link creation)
>
> Ouch, I guess you're right about it being a leftover.
>
> >
> > * OVPN_A_PEER_DEL_REASON (u32 vs u8)
> > drivers/net/ovpn/netlink-gen.c:52: [OVPN_A_PEER_DEL_REASON] = NLA_POLICY_MAX(NLA_U32, 4),
> > drivers/net/ovpn/netlink.c:1131: if (nla_put_u8(msg, OVPN_A_PEER_DEL_REASON, peer->delete_reason))
>
> Like above, probably better to convert them all to u32.
[...]
>
> u32 should be enough for packets - but yeah, going UINT is better as I don't
> need to care if it really fits 4B or not.
And that SGTM too. Thanks.
--
Sabrina
On 05/03/2025 00:09, Sabrina Dubroca wrote:
> 2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
>> On 04/03/2025 13:00, Sabrina Dubroca wrote:
>>> 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
>>>> int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
>>>> {
>>> ...
>>>> + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>>>> + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
>>>> + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
>>>
>>>
>>> [...]
>>>> +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
>>>> + u32 peer_id, enum ovpn_key_slot slot,
>>>> + const struct ovpn_key_config *keyconf)
>>>> +{
>>> ...
>>>> + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
>>>> + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
>>>> + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
>>>
>>> That's a bit inconsistent. nla_put_u32 matches the generated policy,
>>> but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
>>> key_id" it's getting stored into).
>>>
>>> [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
>>
>> From https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
>>
>> "Note that types smaller than 32 bit should be avoided as using them does
>> not save any memory in Netlink messages (due to alignment)."
>>
>> Hence I went for u32 attributes, although values stored into them are much
>> smaller.
>
> Right.
What's wrong with key_id being u8 tough?
I am a bit reluctant to change all key_id fields/variables to u32, just
because the netlink APIs prefers using u32 instead of u8.
Keeping variables/fields u8 allows to understand what values we're going
to store internally.
And thanks to the netlink policy we know that no larger value will be
attempted to be saved, even if the field is actually u32.
Cheers,
--
Antonio Quartulli
OpenVPN Inc.
2025-03-05, 02:00:21 +0100, Antonio Quartulli wrote:
> On 05/03/2025 00:09, Sabrina Dubroca wrote:
> > 2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
> > > On 04/03/2025 13:00, Sabrina Dubroca wrote:
> > > > 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
> > > > > int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
> > > > > {
> > > > ...
> > > > > + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
> > > > > + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
> > > > > + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
> > > >
> > > >
> > > > [...]
> > > > > +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
> > > > > + u32 peer_id, enum ovpn_key_slot slot,
> > > > > + const struct ovpn_key_config *keyconf)
> > > > > +{
> > > > ...
> > > > > + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
> > > > > + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
> > > > > + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
> > > >
> > > > That's a bit inconsistent. nla_put_u32 matches the generated policy,
> > > > but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
> > > > key_id" it's getting stored into).
> > > >
> > > > [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
> > >
> > > From https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
> > >
> > > "Note that types smaller than 32 bit should be avoided as using them does
> > > not save any memory in Netlink messages (due to alignment)."
> > >
> > > Hence I went for u32 attributes, although values stored into them are much
> > > smaller.
> >
> > Right.
>
> What's wrong with key_id being u8 tough?
Nothing. It would make a little bit more sense to use nla_get_u16 if
key_id was u16 (even with OVPN_A_KEYCONF_KEY_ID defined as U32), or to
use nla_get_u8 for u8, but here it was just 3 different int sizes and
that triggered my "uh? what?" :)
> I am a bit reluctant to change all key_id fields/variables to u32, just
> because the netlink APIs prefers using u32 instead of u8.
>
> Keeping variables/fields u8 allows to understand what values we're going to
> store internally.
Sure.
> And thanks to the netlink policy we know that no larger value will be
> attempted to be saved, even if the field is actually u32.
Yes.
--
Sabrina
On 05/03/2025 11:11, Sabrina Dubroca wrote:
> 2025-03-05, 02:00:21 +0100, Antonio Quartulli wrote:
>> On 05/03/2025 00:09, Sabrina Dubroca wrote:
>>> 2025-03-04, 13:11:28 +0100, Antonio Quartulli wrote:
>>>> On 04/03/2025 13:00, Sabrina Dubroca wrote:
>>>>> 2025-03-04, 01:33:50 +0100, Antonio Quartulli wrote:
>>>>>> int ovpn_nl_key_new_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>> {
>>>>> ...
>>>>>> + pkr.slot = nla_get_u8(attrs[OVPN_A_KEYCONF_SLOT]);
>>>>>> + pkr.key.key_id = nla_get_u16(attrs[OVPN_A_KEYCONF_KEY_ID]);
>>>>>> + pkr.key.cipher_alg = nla_get_u16(attrs[OVPN_A_KEYCONF_CIPHER_ALG]);
>>>>>
>>>>>
>>>>> [...]
>>>>>> +static int ovpn_nl_send_key(struct sk_buff *skb, const struct genl_info *info,
>>>>>> + u32 peer_id, enum ovpn_key_slot slot,
>>>>>> + const struct ovpn_key_config *keyconf)
>>>>>> +{
>>>>> ...
>>>>>> + if (nla_put_u32(skb, OVPN_A_KEYCONF_SLOT, slot) ||
>>>>>> + nla_put_u32(skb, OVPN_A_KEYCONF_KEY_ID, keyconf->key_id) ||
>>>>>> + nla_put_u32(skb, OVPN_A_KEYCONF_CIPHER_ALG, keyconf->cipher_alg))
>>>>>
>>>>> That's a bit inconsistent. nla_put_u32 matches the generated policy,
>>>>> but the nla_get_u{8,16} don't (and nla_get_u16 also doesn't match "u8
>>>>> key_id" it's getting stored into).
>>>>>
>>>>> [also kind of curious that the policy/spec uses U32 with max values of 1/2/7]
>>>>
>>>> From https://www.kernel.org/doc/html/next/userspace-api/netlink/specs.html#fix-width-integer-types
>>>>
>>>> "Note that types smaller than 32 bit should be avoided as using them does
>>>> not save any memory in Netlink messages (due to alignment)."
>>>>
>>>> Hence I went for u32 attributes, although values stored into them are much
>>>> smaller.
>>>
>>> Right.
>>
>> What's wrong with key_id being u8 tough?
>
> Nothing. It would make a little bit more sense to use nla_get_u16 if
> key_id was u16 (even with OVPN_A_KEYCONF_KEY_ID defined as U32), or to
> use nla_get_u8 for u8, but here it was just 3 different int sizes and
> that triggered my "uh? what?" :)
>
>> I am a bit reluctant to change all key_id fields/variables to u32, just
>> because the netlink APIs prefers using u32 instead of u8.
>>
>> Keeping variables/fields u8 allows to understand what values we're going to
>> store internally.
>
> Sure.
>
>> And thanks to the netlink policy we know that no larger value will be
>> attempted to be saved, even if the field is actually u32.
>
> Yes.
>
Ok :)
Then I'll make sure they all use nla_get/put_u32, but will still store
the key_id in a u8 field.
Cheers!
--
Antonio Quartulli
OpenVPN Inc.
© 2016 - 2025 Red Hat, Inc.