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?
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.
--
Best regards,
Dmitry
