Re: [PATCH v7 net-next 2/5] psp: add new netlink cmd for dev-assoc and dev-disassoc

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
psp: add new netlink cmd for dev-assoc and dev-disassoc

This commit adds new netlink commands (PSP_CMD_DEV_ASSOC and
PSP_CMD_DEV_DISASSOC) to associate non-PSP-capable devices (e.g. veth or
netkit) with a PSP device, enabling them to act as PSP-capable devices by
performing PSP encryption/decryption on the real PSP device.

> diff --git a/net/psp/psp_main.c b/net/psp/psp_main.c
> index 82de78a1d6bd..178b848989f1 100644
> --- a/net/psp/psp_main.c
> +++ b/net/psp/psp_main.c
> @@ -37,8 +37,18 @@ struct mutex psp_devs_lock;
>   */
>  int psp_dev_check_access(struct psp_dev *psd, struct net *net, bool admin)
>  {
> +	struct psp_assoc_dev *entry;
> +
>  	if (dev_net(psd->main_netdev) == net)
>  		return 0;
> +
> +	if (!admin) {
> +		list_for_each_entry(entry, &psd->assoc_dev_list, dev_list) {
> +			if (dev_net(entry->assoc_dev) == net)
> +				return 0;
> +		}
> +	}
> +
>  	return -ENOENT;
>  }

Does this list iteration have a race with psp_dev_unregister()?

The new list_for_each_entry iteration over psd->assoc_dev_list in
psp_dev_check_access is called from psp_assoc_device_get_locked
(net/psp/psp_nl.c:615) without holding psd->lock.

That caller obtains psd via psp_dev_get_for_sock which takes a refcount
but does not acquire psd->lock before calling psp_dev_check_access(psd,
..., false). Since admin is false, the new code path is entered.

Concurrently, psp_dev_unregister holds psd->lock and performs list_del +
kfree on the same assoc_dev_list entries:

net/psp/psp_main.c:psp_dev_unregister() {
	...
	list_for_each_entry_safe(entry, entry_tmp, &psd->assoc_dev_list,
				 dev_list) {
		list_del(&entry->dev_list);
		rcu_assign_pointer(entry->assoc_dev->psp_dev, NULL);
		netdev_put(entry->assoc_dev, &entry->dev_tracker);
		kfree(entry);
	}
	...
}

The list iterator in psp_dev_check_access can follow a next pointer into
freed memory, causing a use-after-free.

Other callers (psp_device_get_and_lock, psp_nl_dev_get_dumpit,
psp_nl_get_stats_dumpit) correctly hold psd->lock before calling this
function, so the race is specific to the psp_assoc_device_get_locked path.

> diff --git a/net/psp/psp_nl.c b/net/psp/psp_nl.c
> index b988f35412df..aa60a8277829 100644
> --- a/net/psp/psp_nl.c
> +++ b/net/psp/psp_nl.c

[ ... ]

> @@ -291,6 +446,140 @@ int psp_nl_key_rotate_doit(struct sk_buff *skb, struct genl_info *info)
>  	return err;
>  }
>
> +int psp_nl_dev_assoc_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct psp_dev *psd = info->user_ptr[0];
> +	struct psp_assoc_dev *psp_assoc_dev;
> +	struct net_device *assoc_dev;
> +	u32 assoc_ifindex;
> +	struct sk_buff *rsp;
> +	struct net *net;
> +	int nsid;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, PSP_A_DEV_IFINDEX))
> +		return -EINVAL;
> +
> +	if (info->attrs[PSP_A_DEV_NSID]) {
> +		nsid = nla_get_s32(info->attrs[PSP_A_DEV_NSID]);
> +
> +		net = get_net_ns_by_id(genl_info_net(info), nsid);
> +		if (!net) {
> +			NL_SET_BAD_ATTR(info->extack,
> +					info->attrs[PSP_A_DEV_NSID]);
> +			return -EINVAL;
> +		}
> +	} else {
> +		net = get_net(genl_info_net(info));
> +	}
> +
> +	psp_assoc_dev = kzalloc(sizeof(*psp_assoc_dev), GFP_KERNEL);
> +	if (!psp_assoc_dev) {
> +		put_net(net);
> +		return -ENOMEM;
> +	}
> +
> +	assoc_ifindex = nla_get_u32(info->attrs[PSP_A_DEV_IFINDEX]);
> +	assoc_dev = netdev_get_by_index(net, assoc_ifindex,
> +					&psp_assoc_dev->dev_tracker,
> +					GFP_KERNEL);
> +	if (!assoc_dev) {
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		NL_SET_BAD_ATTR(info->extack, info->attrs[PSP_A_DEV_IFINDEX]);
> +		return -ENODEV;
> +	}
> +
> +	/* Check if device is already associated with a PSP device */
> +	if (rcu_access_pointer(assoc_dev->psp_dev)) {
> +		NL_SET_ERR_MSG(info->extack,
> +			       "Device already associated with a PSP device");
> +		netdev_put(assoc_dev, &psp_assoc_dev->dev_tracker);
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		return -EBUSY;
> +	}
> +
> +	psp_assoc_dev->assoc_dev = assoc_dev;
> +	rsp = psp_nl_reply_new(info);
> +	if (!rsp) {
> +		netdev_put(assoc_dev, &psp_assoc_dev->dev_tracker);
> +		put_net(net);
> +		kfree(psp_assoc_dev);
> +		return -ENOMEM;
> +	}
> +
> +	rcu_assign_pointer(assoc_dev->psp_dev, psd);
> +	list_add_tail(&psp_assoc_dev->dev_list, &psd->assoc_dev_list);

Can two concurrent PSP_CMD_DEV_ASSOC operations for the same net_device but
different PSP devices both succeed?

The function checks rcu_access_pointer(assoc_dev->psp_dev) to verify the
target net_device is not already associated with a PSP device, then later
writes rcu_assign_pointer(assoc_dev->psp_dev, psd) and adds the wrapper to
psd->assoc_dev_list.

These operations are only serialized by psd->lock, which is a per-PSP-device
mutex acquired by the pre_doit callback psp_device_get_locked(). With
parallel_ops=true in the psp_nl_family (net/psp/psp-nl-gen.c:165), two
concurrent PSP_CMD_DEV_ASSOC operations targeting different PSP devices
(each holding their own distinct psd->lock) but specifying the same target
ifindex can both pass the rcu_access_pointer() NULL check.

Both then call rcu_assign_pointer(assoc_dev->psp_dev, psd) and
list_add_tail(), resulting in the same net_device being added to two
different PSP devices' assoc_dev_list. The second rcu_assign_pointer
silently overwrites the first.

This leaves the first PSP device's assoc_dev_list containing an entry whose
net_device->psp_dev points to the wrong PSP device:

Thread A (PSP device psd_A)        Thread B (PSP device psd_B)
================================   ================================
mutex_lock(&psd_A->lock)           mutex_lock(&psd_B->lock)
rcu_access_pointer(dev->psp_dev)
  -> NULL, proceed                 rcu_access_pointer(dev->psp_dev)
                                     -> NULL, proceed
rcu_assign_pointer(dev->psp_dev,   rcu_assign_pointer(dev->psp_dev,
                   psd_A)                             psd_B)
list_add_tail(entry_A,             list_add_tail(entry_B,
              &psd_A->list)                      &psd_B->list)

A subsequent disassociation from the first PSP device would find its stale
psp_assoc_dev entry, call rcu_assign_pointer(found->assoc_dev->psp_dev,
NULL), and clear the pointer that now belongs to the second PSP device,
corrupting cross-device state.

The data path check in psp_validate_xmit
(rcu_access_pointer(dev->psp_dev) == pas->psd) can then fail incorrectly.