|
| From: | Akihiko Odaki |
| Subject: | Re: [PATCH v7 07/10] virtio-gpu: Handle resource blob commands |
| Date: | Mon, 15 Apr 2024 19:05:06 +0900 |
| User-agent: | Mozilla Thunderbird |
On 2024/04/15 17:49, Dmitry Osipenko wrote:
On 4/15/24 11:13, Akihiko Odaki wrote:On 2024/04/15 17:03, Dmitry Osipenko wrote:Hello, On 4/13/24 14:57, Akihiko Odaki wrote: ...+static void +virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, + struct virtio_gpu_simple_resource *res) +{ + VirtIOGPUBase *b = VIRTIO_GPU_BASE(g); + + if (!res->mr) { + return; + } + + memory_region_set_enabled(res->mr, false); + memory_region_del_subregion(&b->hostmem, res->mr); + + /* memory region owns res->mr object and frees it when mr is released */ + res->mr = NULL; + + virgl_renderer_resource_unmap(res->resource_id);Hi, First, thanks for keeping working on this. This patch has some changes since the previous version, but it is still vulnerable to the race condition pointed out. The memory region is asynchronously unmapped from the guest address space, but the backing memory on the host address space is unmapped synchronously before that. This results in use-after-free. The whole unmapping operation needs to be implemented in an asynchronous manner.Thanks for the clarification! I missed this point from the previous discussion. Could you please clarify what do you mean by the "asynchronous manner"? Virglrenderer API works only in the virtio-gpu-gl context, it can't be accessed from other places. The memory_region_del_subregion() should remove the region as long as nobody references it, isn't it? On Linux guest nobody should reference hostmem regions besides virtio-gpu device on the unmap, don't know about other guests. We can claim it a guest's fault if MR lives after the deletion and in that case exit Qemu with a noisy error msg or leak resource. WDYT?We need to be prepared for a faulty guest for reliability and security as they are common goals of virtualization, and it is nice to have them after all. You need to call virgl_renderer_resource_unmap() after the MR actually gets freed. The virtio-gpu-gl context is just a context with BQL so it is fine to call virgl functions in most places.Do you have example of a legit use-case where hostmem MR could outlive resource mapping?
MR outliving after memory_region_del_subregion() is not a use-case, but a situation that occurs due to the limitation of the memory subsystem. It is not easy to answer how often such a situation happens.
I'm not sure what you mean by turning into an error condition. I doubt it's possible to emit errors when someone touches an unmapped region.Turning it into a error condition is much more reasonable to do than to to worry about edge case that nobody cares about, which can't be tested easily and that not trivial to support, IMO.
Reproducing this issue is not easy as it's often cases for use-after-free bugs, but fixing it is not that complicated in my opinion since you already have an implementation which asynchronously unmaps the region in v6. I write my suggestions to fix problems in v6:
- Remove ref member in virgl_gpu_resource, vres_get_ref(), vres_put_ref(), and virgl_resource_unmap().
- Change virtio_gpu_virgl_process_cmd(), virgl_cmd_resource_unmap_blob(), and virgl_cmd_resource_unref() to return a bool, which tells if the command was processed or suspended.
- In virtio_gpu_process_cmdq(), break if the command was suspended. - In virgl_resource_blob_async_unmap(), call virtio_gpu_gl_block(g, false).- In virgl_cmd_resource_unmap_blob() and virgl_cmd_resource_unref(), call memory_region_del_subregion() and virtio_gpu_gl_block(g, true), and tell that the command was suspended if the reference counter of MemoryRegion > 0. Free and unmap the MR otherwise.
Regards, Akihiko Odaki
| [Prev in Thread] | Current Thread | [Next in Thread] |