linux-kernel - Re: [PATCH] KVM: arm64: vgic-its: Do not call vgic_put_irq() within vgic_its_inject_cached

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZxCxL8ZCHRm7RTCA@linux.dev>
Date: Wed, 16 Oct 2024 23:39:43 -0700
From: Oliver Upton <oliver.upton@...ux.dev>
To: WangYuli <wangyuli@...ontech.com>
Cc: maz@...nel.org, james.morse@....com, suzuki.poulose@....com,
	yuzenghui@...wei.com, catalin.marinas@....com, will@...nel.org,
	rdunlap@...radead.org, sebott@...hat.com,
	linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
	linux-kernel@...r.kernel.org, guanwentao@...ontech.com,
	zhanjun@...ontech.com, stable@...r.kernel.org
Subject: Re: [PATCH] KVM: arm64: vgic-its: Do not call vgic_put_irq() within
 vgic_its_inject_cached_translation()

Hi,

On Thu, Oct 17, 2024 at 02:13:34PM +0800, WangYuli wrote:
> There is a probability that the host machine will also restart
> when the virtual machine is restarting.

This is a start, but can you please describe in detail what the flow is
you're seeing that leads to the refcount bug / UAF?

> Commit ad362fe07fec ("KVM: arm64: vgic-its: Avoid potential UAF
> in LPI translation cache") released the reference count of an IRQ
> when it shouldn't have. This led to a situation where, when the
> system finally released the IRQ, it found that the structure had
> already been freed, triggering a
> 'refcount_t: underflow; use-after-free' error.
> 
> In fact, the function "vgic_put_irq" should be called by
> "vgic_its_inject_cached_translation" instead of
> "vgic_its_trigger_msi".

This line doesn't match your patch, and instead aligns with the upstream
behavior.

The put in vgic_its_inject_cached_translation() pairs with the get in
vgic_its_check_cache(). We need to do this because the LPI injection
fast path happens outside of the its_lock.

The slow path for LPI injection takes the its_lock and is safe because
the ITE holds a reference on the descriptor as well. Because of that,
vgic_its_trigger_msi() doesn't touch the refcount on the resolved IRQ.

So I'm not following how any of this leads to the underflow you observe.
Even if the put in vgic_its_inject_cached_translation() causes the
refcount to drop to 0, it is likely that an unbalanced put happened
somewhere else in the ITS emulation.

-- 
Thanks,
Oliver