| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZdiOo6sIzQ5p0l_v@nanopsycho>
Date: Fri, 23 Feb 2024 13:25:07 +0100
From: Jiri Pirko <jiri@...nulli.us>
To: Eric Dumazet <edumazet@...gle.com>
Cc: "David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
netdev@...r.kernel.org, eric.dumazet@...il.com,
Jiri Pirko <jiri@...dia.com>,
Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>,
Vadim Fedorenko <vadim.fedorenko@...ux.dev>
Subject: Re: [PATCH net] dpll: rely on rcu for netdev_dpll_pin()
Fri, Feb 23, 2024 at 12:51:43PM CET, edumazet@...gle.com wrote:
>On Fri, Feb 23, 2024 at 10:00 AM Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Thu, Feb 22, 2024 at 05:48:51PM CET, edumazet@...gle.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?
>
>Absolutely, this came while I was working on my RTNL->RCU conversion
>of "ip l" in net-next.
>
>
>
>>
>>
>> >
>> >Fixes: 5f1842692880 ("netdev: expose DPLL pin handle for netdevice")
>> >Signed-off-by: Eric Dumazet <edumazet@...gle.com>
>> >Cc: Jiri Pirko <jiri@...dia.com>
>> >Cc: Arkadiusz Kubalewski <arkadiusz.kubalewski@...el.com>
>> >Cc: Vadim Fedorenko <vadim.fedorenko@...ux.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?
>
>Because of the rcu_dereference_rtnl() call, not available to
>include/linux/netdevice.h
>
>Adding the missing include would have been more painful.
>include/linux/netdevice.h being included everywhere and being bloated already,
>I think moving netdev_dpll_pin() helper in dpll.h is better (should
>not increase compile time for the kernel)
Fair enough.
>
>>
>>
>> >+}
>> >+
>> > #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()?
>
>This is because I discovered the issue while working on net-next tree,
>and basically had to fix the bug in net tree, I had to prune my WIP to
>get to the fix.
>
>You are right, the rcu_read_lock() is not really needed here.
Okay.
Powered by blists - more mailing lists