Re: [PATCH] system/physmem: fix use-after-free with dispatch

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 24/7/25 18:11, Pierrick Bouvier wrote:
> A use-after-free bug was reported when booting a Linux kernel during the
> pci setup phase. It's quite hard to reproduce (needs smp, and favored by
> having several pci devices with BAR and specific Linux config, which
> is Debian default one in this case).
> 
> After investigation (see the associated bug ticket), it appears that,
> under specific conditions, we might access a cached AddressSpaceDispatch
> that was reclaimed by RCU thread meanwhile.
> In the Linux boot scenario, during the pci phase, memory region are
> destroyed/recreated, resulting in exposition of the bug.
> 
> The core of the issue is that we cache the dispatch associated to
> current cpu in cpu->cpu_ases[asidx].memory_dispatch. It is updated with
> tcg_commit, which runs asynchronously on a given cpu.
> At some point, we leave the rcu critial section, and the RCU thread
> starts reclaiming it, but tcg_commit is not yet invoked, resulting in
> the use-after-free.
> 
> It's not the first problem around this area, and this patch [1] already
> tried to address it. It did a good job, but it seems that we found a
> specific situation where it's not enough.
> 
> This patch takes a simple approach: remove the cached value creating the
> issue, and make sure we always get the current mapping for address
> space, using address_space_to_dispatch(cpu->cpu_ases[asidx].as).

Very nice.

> It's equivalent to qatomic_rcu_read(&as->current_map)->dispatch;
> This is not really costly, we just need two dereferences,
> including one atomic (rcu) read, which is negligible considering we are
> already on mmu slow path anyway.
> 
> Note that tcg_commit is still needed, as it's taking care of flushing
> TLB, removing previously mapped entries.
> 
> Another solution would be to cache directly values under the dispatch
> (dispatch themselves are not ref counted), keep an active reference on
> associated memory section, and release it when appropriate (tricky).
> Given the time already spent debugging this area now and previously, I
> strongly prefer eliminating the root of the issue, instead of adding
> more complexity for a hypothetical performance gain. RCU is precisely
> used to ensure good performance when reading data, so caching is not as
> beneficial as it might seem IMHO.
> 
> [1] https://gitlab.com/qemu-project/qemu/-/commit/0d58c660689f6da1e3feff8a997014003d928b3b
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/3040
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   system/physmem.c | 15 +++------------
>   1 file changed, 3 insertions(+), 12 deletions(-)

Patch queued, thanks!