[PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[Jeongjun Park]

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(-)

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);
--

Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[Paolo Abeni]

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

Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[Jakub Kicinski]

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()

Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[Jeongjun Park]

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

Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[Jakub Kicinski]

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.

Re: [PATCH net] net/xen-netback: prevent UAF in xenvif_flush_hash()

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html