Re: [PATCH v7 07/10] virtio-gpu: Handle resource blob commands

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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.

> 
> 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.
> 
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.

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