Akihiko Odaki <akihiko.od...@daynix.com> writes: > On 2025/05/22 16:31, Manos Pitsidianakis wrote: >> On Thu, May 22, 2025 at 10:03 AM Akihiko Odaki <akihiko.od...@daynix.com> >> wrote: >>> >>> On 2025/05/22 15:45, Alex Bennée wrote: >>>> Akihiko Odaki <akihiko.od...@daynix.com> writes: >>>> >>>>> On 2025/05/22 1:42, Alex Bennée wrote: >>>>>> From: Manos Pitsidianakis <manos.pitsidiana...@linaro.org> >>>>>> This commit fixes an indefinite hang when using VIRTIO GPU blob >>>>>> objects >>>>>> under TCG in certain conditions. >>>>>> The VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB VIRTIO command creates a >>>>>> MemoryRegion and attaches it to an offset on a PCI BAR of the >>>>>> VirtIOGPUdevice. The VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB command unmaps >>>>>> it. >>>>>> Because virglrenderer commands are not thread-safe they are only >>>>>> called on the main context and QEMU performs the cleanup in three steps >>>>>> to prevent a use-after-free scenario where the guest can access the >>>>>> region after it’s unmapped: >>>>>> 1. From the main context, the region’s field finish_unmapping is >>>>>> false >>>>>> by default, so it sets a variable cmd_suspended, increases the >>>>>> renderer_blocked variable, deletes the blob subregion, and >>>>>> unparents >>>>>> the blob subregion causing its reference count to decrement. >>>>>> 2. From an RCU context, the MemoryView gets freed, the FlatView gets >>>>>> recalculated, the free callback of the blob region >>>>>> virtio_gpu_virgl_hostmem_region_free is called which sets the >>>>>> region’s field finish_unmapping to true, allowing the main thread >>>>>> context to finish replying to the command >>>>>> 3. From the main context, the command is processed again, but this >>>>>> time >>>>>> finish_unmapping is true, so virgl_renderer_resource_unmap can be >>>>>> called and a response is sent to the guest. >>>>>> It happens so that under TCG, if the guest has no timers configured >>>>>> (and >>>>>> thus no interrupt will cause the CPU to exit), the RCU thread does not >>>>>> have enough time to grab the locks and recalculate the FlatView. >>>>>> That’s not a big problem in practice since most guests will assume a >>>>>> response will happen later in time and go on to do different things, >>>>>> potentially triggering interrupts and allowing the RCU context to run. >>>>>> If the guest waits for the unmap command to complete though, it blocks >>>>>> indefinitely. Attaching to the QEMU monitor and force quitting the guest >>>>>> allows the cleanup to continue. >>>>>> There's no reason why the FlatView recalculation can't occur right >>>>>> away >>>>>> when we delete the blob subregion, however. It does not, because when we >>>>>> create the subregion we set the object as its own parent: >>>>>> memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data); >>>>>> The extra reference is what prevents freeing the memory region >>>>>> object in >>>>>> the memory transaction of deleting the subregion. >>>>>> This commit changes the owner object to the device, which removes >>>>>> the >>>>>> extra owner reference in the memory region and causes the MR to be >>>>>> freed right away in the main context. >>>>>> Acked-by: Michael S. Tsirkin <m...@redhat.com> >>>>>> Signed-off-by: Manos Pitsidianakis <manos.pitsidiana...@linaro.org> >>>>>> Reviewed-by: Alex Bennée <alex.ben...@linaro.org> >>>>>> Tested-by: Alex Bennée <alex.ben...@linaro.org> >>>>>> Message-Id: <20250410122643.1747913-3-manos.pitsidiana...@linaro.org> >>>>>> Cc: qemu-sta...@nongnu.org >>>>>> --- >>>>>> hw/display/virtio-gpu-virgl.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> diff --git a/hw/display/virtio-gpu-virgl.c >>>>>> b/hw/display/virtio-gpu-virgl.c >>>>>> index 71a7500de9..8fbe4e70cc 100644 >>>>>> --- a/hw/display/virtio-gpu-virgl.c >>>>>> +++ b/hw/display/virtio-gpu-virgl.c >>>>>> @@ -112,7 +112,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, >>>>>> vmr->g = g; >>>>>> mr = g_new0(MemoryRegion, 1); >>>>>> - memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, >>>>>> data); >>>>>> + memory_region_init_ram_ptr(mr, OBJECT(g), "blob", size, data); >>>>>> memory_region_add_subregion(&b->hostmem, offset, mr); >>>>>> memory_region_set_enabled(mr, true); >>>>>> >>>>> >>>>> I suggest dropping this patch for now due to the reason I pointed out >>>>> for the first version of this series. >>>> >>>> This fixes an actual bug - without it we get a hang. >>>> >>> >>> I understand that but it also introduces a regression; "[PATCH v3 14/20] >>> ui/gtk-gl-area: Remove extra draw call in refresh" is also a similar case. >>> >>> Ideally such a bug should be fixed without regression, but I understand >>> it is sometimes difficult to do that and postponing the bug resolution >>> until figuring out the correct way does not make sense. >>> >>> In such a case, a bug should be fixed minimizing the regression and the >>> documentation of the regression should be left in the code. >>> >>> In particular, this patch can cause use-after-free whether TCG is used >>> or not. Instead, I suggest to avoid freeing memory regions at all on >>> TCG. It will surely leak memory, but won't result in use-after-free at >>> least and the other accelerators will be unaffected. >>> >>> Regards, >>> Akihiko Odaki >> We tested this fix with ASAN and didn't see anything. Do you have a >> test case in mind that can reproduce this use-after-free? It'd help >> make a certain decision on whether to drop this patch or not. I'm not >> doubting that this can cause a use-after-free by the way, it's just >> that it is hypothetical only. If it causes a use-after-free for sure >> we should definitely drop it. > > No, I don't have a test case and it should rarely occur. More > concretely, a UAF occurs if the guest accesses the memory region while > trying to unmap it. It is just a theory indeed, but the theory says > the UAF is possible.
I have a test case this fixes which I think trumps a theoretical UAF without a test case. Why would the guest attempt to access after triggering the free itself? Wouldn't it be correct to fault the guest for violating its own memory safety rules? >>> Instead, I suggest to avoid freeing memory regions at all on >>> TCG. It will surely leak memory, but won't result in use-after-free at >>> least and the other accelerators will be unaffected. >> Leaking memory for blob objects is also not ideal, since they are >> frequently allocated. It's memory-safe but the leak can accumulate >> over time. >> > > Memory safety and leak free cannot be compatible unless RCU is fixed. > We need to choose either of them. How can the guest access something that is now unmapped? The RCU should only run after the flatview has been updated. > > Regards, > Akihiko Odaki -- Alex Bennée Virtualisation Tech Lead @ Linaro
