On 05/11/2024 11:16, Sabrina Dubroca wrote:
2024-10-29, 11:47:32 +0100, Antonio Quartulli wrote:
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 sensible data only), destroy and rotate session keys for a specific
nit: s/sensible/sensitive/
+int ovpn_crypto_config_get(struct ovpn_crypto_state *cs,
enum ovpn_key_slot slot,struct ovpn_key_config *keyconf)+{
[...]
- rcu_read_lock();
- ks = rcu_dereference(cs->slots[idx]);
- if (!ks || (ks && !ovpn_crypto_key_slot_hold(ks))) {
rcu_read_unlock();return -ENOENT;- }
- rcu_read_unlock();
You could stay under rcu_read_lock a little bit longer and avoid taking a reference just to release it immediately.
ACK.
- keyconf->cipher_alg = ovpn_aead_crypto_alg(ks);
- keyconf->key_id = ks->key_id;
- ovpn_crypto_key_slot_put(ks);
- return 0;
+}
[...]
int ovpn_nl_key_get_doit(struct sk_buff *skb, struct genl_info *info) {
[...]
- 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,"cannot find peer with id %u", 0);peer_id?
return -ENOENT;- }
- if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
OVPN_A_KEYCONF_SLOT))return -EINVAL;Move this check before ovpn_peer_get_by_id? We're leaking a reference on the peer.
ACK
- 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,
info->snd_portid, info->snd_seq, 0);info->snd_portid and info->snd_seq can be extracted from info directly in ovpn_nl_send_key since there's no other caller, and flags=0 can be skipped as well.
I tried to keep the signature similar to send_peer, but indeed they can both be simplified.
- if (ret < 0) {
nlmsg_free(msg);goto err;- }
- ret = genlmsg_reply(msg, info);
+err:
- ovpn_peer_put(peer);
- return ret; }
[...]
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_struct *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 (ret)
return ret;leftover?
very likely.
Thanks a lot
Regards,
- if (NL_REQ_ATTR_CHECK(info->extack, info->attrs[OVPN_A_KEYCONF], attrs,
OVPN_A_KEYCONF_SLOT))return -EINVAL;