| Message ID | 20240822181109.2577354-1-aha310510@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0fa5e94a1811d68fbffa0725efe6d4ca62c03d12 |
| Headers | show |
| Series | [net] net/xen-netback: prevent UAF in xenvif_flush_hash() | expand |
On 8/22/24 20:11, Jeongjun Park wrote: > During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, > kfree_rcu does not exist inside the rcu read critical section, so if The above wording is confusing, do you mean "kfree_rcu does not exit from "...? > kfree_rcu is called when the rcu grace period ends during the iteration, > UAF occurs when accessing head->next after the entry becomes free. The loop runs with irq disabled, the RCU critical section extends over it, uninterrupted. Do you have a splat for the reported UAF? This does not look the correct solution. Thanks, Paolo
On Tue, 27 Aug 2024 13:19:59 +0200 Paolo Abeni wrote: > On 8/22/24 20:11, Jeongjun Park wrote: > > During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, > > kfree_rcu does not exist inside the rcu read critical section, so if > > The above wording is confusing, do you mean "kfree_rcu does not exit > from "...? I think they mean that kfree_rcu() is called without holding RCU read lock.. > > kfree_rcu is called when the rcu grace period ends during the iteration, > > UAF occurs when accessing head->next after the entry becomes free. .. so it can run immediately. Therefore the loop fetching head->next may cause a UAF. > The loop runs with irq disabled, the RCU critical section extends over > it, uninterrupted. Is this an official RCU rule? I remember Paul told us it's the case for softirq, but IDK if it is also for local IRQ disable. > Do you have a splat for the reported UAF? > > This does not look the correct solution. The problem may not exist, but FWIW the change makes sense to me :) We hold the write lock, and modify the list. for_each_entry_safe() seems like a better fit than for_each_entry_rcu()
On Tue, 27 Aug 2024 13:19:59 +0200 Paolo Abeni wrote: > On 8/22/24 20:11, Jeongjun Park wrote: > > During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, > > kfree_rcu does not exist inside the rcu read critical section, so if > > The above wording is confusing, do you mean "kfree_rcu does not exit > from "...? > > > kfree_rcu is called when the rcu grace period ends during the iteration, > > UAF occurs when accessing head->next after the entry becomes free. > > The loop runs with irq disabled, the RCU critical section extends over > it, uninterrupted. Basically, list_for_each_entry_rcu is specified to be used under the protection of rcu_read_lock(), but this is not the case with xenvif_new_hash(). If it is used without the protection of rcu_read_lock(), kfree is called immediately after the grace period ends after the call to kfree_rcu() inside list_for_each_entry_rcu, so the entry is released, and a UAF occurs when fetching with ->next thereafter. Regards, Jeongjun Park
On Wed, 28 Aug 2024 21:52:12 +0900 Jeongjun Park wrote: > > The loop runs with irq disabled, the RCU critical section extends over > > it, uninterrupted. > > Basically, list_for_each_entry_rcu is specified to be used under the protection > of rcu_read_lock(), but this is not the case with xenvif_new_hash(). If it is > used without the protection of rcu_read_lock(), kfree is called immediately > after the grace period ends after the call to kfree_rcu() inside > list_for_each_entry_rcu, so the entry is released, and a UAF occurs when > fetching with ->next thereafter. You cut off and didn't answer Paolo's question whether you have a splat / saw this actually cause a crash or a KASAN warning.
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 23 Aug 2024 03:11:09 +0900 you wrote: > During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, > kfree_rcu does not exist inside the rcu read critical section, so if > kfree_rcu is called when the rcu grace period ends during the iteration, > UAF occurs when accessing head->next after the entry becomes free. > > Therefore, to solve this, you need to change it to list_for_each_entry_safe. > > [...] Here is the summary with links: - [net] net/xen-netback: prevent UAF in xenvif_flush_hash() https://git.kernel.org/netdev/net-next/c/0fa5e94a1811 You are awesome, thank you!
diff --git a/drivers/net/xen-netback/hash.c b/drivers/net/xen-netback/hash.c index ff96f22648ef..45ddce35f6d2 100644 --- a/drivers/net/xen-netback/hash.c +++ b/drivers/net/xen-netback/hash.c @@ -95,7 +95,7 @@ static u32 xenvif_new_hash(struct xenvif *vif, const u8 *data, static void xenvif_flush_hash(struct xenvif *vif) { - struct xenvif_hash_cache_entry *entry; + struct xenvif_hash_cache_entry *entry, *n; unsigned long flags; if (xenvif_hash_cache_size == 0) @@ -103,8 +103,7 @@ static void xenvif_flush_hash(struct xenvif *vif) spin_lock_irqsave(&vif->hash.cache.lock, flags); - list_for_each_entry_rcu(entry, &vif->hash.cache.list, link, - lockdep_is_held(&vif->hash.cache.lock)) { + list_for_each_entry_safe(entry, n, &vif->hash.cache.list, link) { list_del_rcu(&entry->link); vif->hash.cache.count--; kfree_rcu(entry, rcu);
During the list_for_each_entry_rcu iteration call of xenvif_flush_hash, kfree_rcu does not exist inside the rcu read critical section, so if kfree_rcu is called when the rcu grace period ends during the iteration, UAF occurs when accessing head->next after the entry becomes free. Therefore, to solve this, you need to change it to list_for_each_entry_safe. Fixes: f3265971ded9 ("net: xen-netback: hash.c: Use built-in RCU list checking") Signed-off-by: Jeongjun Park <aha310510@gmail.com> --- drivers/net/xen-netback/hash.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) --