Re: [Qemu-devel] [PATCH v2 2/2] xen: add pvUSB backend

[Juergen Gross <jgross@suse.com>]

On 05/05/16 12:13, Anthony PERARD wrote:
> On Wed, May 04, 2016 at 10:25:03AM +0200, Juergen Gross wrote:
>> On 03/05/16 17:06, Anthony PERARD wrote:
>>> On Thu, Mar 10, 2016 at 04:19:30PM +0100, Juergen Gross wrote:
>>>> +static void usbback_bh(void *opaque)
>>>> +{
>>>> +    struct usbback_info *usbif;
>>>> +    struct usbif_urb_back_ring *urb_ring;
>>>> +    struct usbback_req *usbback_req;
>>>> +    RING_IDX rc, rp;
>>>> +    unsigned int more_to_do;
>>>> +
>>>> +    usbif = opaque;
>>>> +    if (usbif->ring_error) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    urb_ring = &usbif->urb_ring;
>>>> +    rc = urb_ring->req_cons;
>>>> +    rp = urb_ring->sring->req_prod;
>>>
>>> Maybe use atomic_read() here to avoid req_prod been read more than once.
>>
>> Hmm. This isn't done in the other backends.
>>
>> TBH: what would happen if req_prod would be read multiple times? In the
>> worst case we would see a new request from the guest which we would have
>> missed without the atomic_read().
> 
> If the guest is misbehaving, it maybe could provoke QEMU to handle more
> request. I'm not sure.

I don't think this would add any risk to dom0. A misbehaving guest
writing arbitrary values to ->req_prod could influence qemu activity
in just the same way regardless whether atomic_read() is used on qemu
side or not. The only difference would be that with atomic_read() the
additional qemu activity would be delayed until the next invocation
of the function.

> For this use of atomic_read, I'm mostly refering to XSA-155[1] and a
> conversation[2].

The main problem with XSA-155 was modification of the request's contents
by the guest after verification by the backend happened. This is not
related to reading the producer's ring index.

I should use RING_COPY_REQUEST(), however.


Juergen