Re: [PATCH net] dpll: rely on rcu for netdev_dpll_pin()

[Jiri Pirko <jiri@resnulli.us>]

Thu, Feb 22, 2024 at 05:48:51PM CET, edumazet@google.com wrote:
>This fixes a possible UAF in if_nlmsg_size(),
>which can run without RTNL.
>
>Add rcu protection to "struct dpll_pin"
>
>Note: This looks possible to no longer acquire RTNL in
>netdev_dpll_pin_assign().

Yeah, looks like no longer needed. Will you do a follow-up for net-next
once this is applied to -net?


>
>Fixes: 5f1842692880 ("netdev: expose DPLL pin handle for netdevice")
>Signed-off-by: Eric Dumazet <edumazet@google.com>
>Cc: Jiri Pirko <jiri@nvidia.com>
>Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@intel.com>
>Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>
>---
> drivers/dpll/dpll_core.c  |  2 +-
> drivers/dpll/dpll_core.h  |  2 ++
> include/linux/dpll.h      | 11 +++++++++++
> include/linux/netdevice.h | 11 +----------
> net/core/dev.c            |  2 +-
> net/core/rtnetlink.c      |  2 ++
> 6 files changed, 18 insertions(+), 12 deletions(-)
>
>diff --git a/drivers/dpll/dpll_core.c b/drivers/dpll/dpll_core.c
>index 5152bd1b0daf599869195e81805fbb2709dbe6b4..4c2bb27c99fe4e517b0d92c4ae3db83a679c7d11 100644
>--- a/drivers/dpll/dpll_core.c
>+++ b/drivers/dpll/dpll_core.c
>@@ -564,7 +564,7 @@ void dpll_pin_put(struct dpll_pin *pin)
> 		xa_destroy(&pin->parent_refs);
> 		xa_erase(&dpll_pin_xa, pin->id);
> 		dpll_pin_prop_free(&pin->prop);
>-		kfree(pin);
>+		kfree_rcu(pin, rcu);
> 	}
> 	mutex_unlock(&dpll_lock);
> }
>diff --git a/drivers/dpll/dpll_core.h b/drivers/dpll/dpll_core.h
>index 717f715015c742238d5585fddc5cd267fbb0db9f..2b6d8ef1cdf36cff24328e497c49d667659dd0e6 100644
>--- a/drivers/dpll/dpll_core.h
>+++ b/drivers/dpll/dpll_core.h
>@@ -47,6 +47,7 @@ struct dpll_device {
>  * @prop:		pin properties copied from the registerer
>  * @rclk_dev_name:	holds name of device when pin can recover clock from it
>  * @refcount:		refcount
>+ * @rcu:		rcu_head for kfree_rcu()
>  **/
> struct dpll_pin {
> 	u32 id;
>@@ -57,6 +58,7 @@ struct dpll_pin {
> 	struct xarray parent_refs;
> 	struct dpll_pin_properties prop;
> 	refcount_t refcount;
>+	struct rcu_head rcu;
> };
> 
> /**
>diff --git a/include/linux/dpll.h b/include/linux/dpll.h
>index 9cf896ea1d4122f3bc7094e46a5af81b999937dc..4ec2fe9caf5a3f284afd0cfe4fc7c2bf42cbbc60 100644
>--- a/include/linux/dpll.h
>+++ b/include/linux/dpll.h
>@@ -10,6 +10,8 @@
> #include <uapi/linux/dpll.h>
> #include <linux/device.h>
> #include <linux/netlink.h>
>+#include <linux/netdevice.h>
>+#include <linux/rtnetlink.h>
> 
> struct dpll_device;
> struct dpll_pin;
>@@ -167,4 +169,13 @@ int dpll_device_change_ntf(struct dpll_device *dpll);
> 
> int dpll_pin_change_ntf(struct dpll_pin *pin);
> 
>+static inline struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
>+{
>+#if IS_ENABLED(CONFIG_DPLL)
>+	return rcu_dereference_rtnl(dev->dpll_pin);
>+#else
>+	return NULL;
>+#endif

Why you moved netdev_dpll_pin() here?


>+}
>+
> #endif
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index ef7bfbb9849733fa7f1f097ba53a36a68cc3384b..a9c973b92294bb110cf3cd336485972127b01b58 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -2469,7 +2469,7 @@ struct net_device {
> 	struct devlink_port	*devlink_port;
> 
> #if IS_ENABLED(CONFIG_DPLL)
>-	struct dpll_pin		*dpll_pin;
>+	struct dpll_pin	__rcu	*dpll_pin;
> #endif
> #if IS_ENABLED(CONFIG_PAGE_POOL)
> 	/** @page_pools: page pools created for this netdevice */
>@@ -4035,15 +4035,6 @@ bool netdev_port_same_parent_id(struct net_device *a, struct net_device *b);
> void netdev_dpll_pin_set(struct net_device *dev, struct dpll_pin *dpll_pin);
> void netdev_dpll_pin_clear(struct net_device *dev);
> 
>-static inline struct dpll_pin *netdev_dpll_pin(const struct net_device *dev)
>-{
>-#if IS_ENABLED(CONFIG_DPLL)
>-	return dev->dpll_pin;
>-#else
>-	return NULL;
>-#endif
>-}
>-
> struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev, bool *again);
> struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev,
> 				    struct netdev_queue *txq, int *ret);
>diff --git a/net/core/dev.c b/net/core/dev.c
>index 73a0219730075e666c4f11f668a50dbf9f9afa97..0230391c78f71e22d3c0e925ff8d3d792aa54a32 100644
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -9078,7 +9078,7 @@ static void netdev_dpll_pin_assign(struct net_device *dev, struct dpll_pin *dpll
> {
> #if IS_ENABLED(CONFIG_DPLL)
> 	rtnl_lock();
>-	dev->dpll_pin = dpll_pin;
>+	rcu_assign_pointer(dev->dpll_pin, dpll_pin);
> 	rtnl_unlock();
> #endif
> }
>diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>index 9c4f427f3a5057b52ec05405e8b15b8ca2246b4b..6239aa62a29cb8752a53e3f75a48a1e2fdd3b0ec 100644
>--- a/net/core/rtnetlink.c
>+++ b/net/core/rtnetlink.c
>@@ -1057,7 +1057,9 @@ static size_t rtnl_dpll_pin_size(const struct net_device *dev)
> {
> 	size_t size = nla_total_size(0); /* nest IFLA_DPLL_PIN */
> 
>+	rcu_read_lock();

Why do you need to take the rcu read lock here? Isn't this called
with either rtnl held of rcu read lock held?

And if you need to take rcu read lock here, why you use
rcu_dereference_rtnl() instead of rcu_dereference() in
netdev_dpll_pin()?



> 	size += dpll_msg_pin_handle_size(netdev_dpll_pin(dev));
>+	rcu_read_unlock();
> 
> 	return size;
> }
>-- 
>2.44.0.rc1.240.g4c46232300-goog
>
>