Re: [PATCH] rpmsg: virtio: don't let virtio core to validate used length

[Arnaud POULIQUEN]

On 11/24/21 3:12 AM, Jason Wang wrote:
> On Tue, Nov 23, 2021 at 9:31 PM Arnaud POULIQUEN
> <arnaud.pouliquen@xxxxxxxxxxx> wrote:
>>
>> Hello Mickael, Jason,
>>
>> On 11/23/21 7:15 AM, Michael S. Tsirkin wrote:
>>> On Mon, Nov 22, 2021 at 05:08:12PM +0100, Arnaud Pouliquen wrote:
>>>> For RX virtqueue, the used length is validated in all the three paths
>>>> (big, small and mergeable). For control vq, we never tries to use used
>>>> length. So this patch forbids the core to validate the used length.
>>>
>>> Jason commented on this. This is copy paste from virtio net
>>> where the change was merely an optimization.
>>>
>>
>> Right, I did it too fast last night (European time) to share the regression as
>> soon as possible.
>> For that, I copied and pasted the first commit I found related to the problem.
>> Need to rework this.
>>
>>>> Without patch the rpmsg client sample does not work.
>>>
>>> Hmm that's not enough of a description. Could you please
>>> provide more detail? Does rpmsg device set used length to a
>>> value > dma read buffer size? what kind of error message
>>> do you get? what are the plans to fix the device?
>>
>> Let's me explain the context.
>> I run the rpmsg client sample test to communicate with a remote processor
>> that runs a Zephyr FW designed to answer to the Linux kernel driver sample.
>>
>> The Zephyr is relying on OpenAMP library to implement the RPMsg and VirtIO layers.
>>
>> In TX direction (Linux to Zephyr) 8 buffers of 512 bytes are allocated.
>> The first 8 RPMsg sent are OK. But when virtio loop back the the TX buffer index
>> 0 (so already used and free one time) the following error occurs in
>> virtqueue_get_buf_ctx_split[1]:
>> " virtio_rpmsg_bus virtio0: output:used len 28 is larger than in buflen 0"
>>
>> I have investigated the problem further today. Here is my analysis
>>
>> rpmsg_send_offchannel_raw
>> -> virtqueue_add_outbuf
>>    -> virtqueue_add
>>       -> virtqueue_add_split
>>          Here we use the "out_sgs" (in_sgs == 0)
>>          buflen is not incremented in loop [2]
>>          We don't enter in loop [3] as "in_sgs == 0"
>>          consequence is that vq->buflen[head] is set to 0 [4]
>>
>> [1]
>> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L799
>> [2]
>> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L551
>> [3]
>> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L567
>> [4]
>> https://elixir.bootlin.com/linux/v5.16-rc2/source/drivers/virtio/virtio_ring.c#L622
>>
>>
>> An alternative to fix the issue is to set buflen in loop 2, but I'm not enough
>> expert to ensure that this will not have any side effect...
>>
>> @@ -559,10 +559,11 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
>>                          * table since it use stream DMA mapping.
>>                          */
>>                         i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
>>                                                      VRING_DESC_F_NEXT,
>>                                                      indirect);
>> +                       buflen += sg->length;
> 
> This is not what spec what:
> 
> "Each entry in the ring is a pair: id indicates the head entry of the
> descriptor chain describing the buffer (this matches an entry placed
> in the available ring by the guest earlier), and len the total of
> bytes written into the buffer."
> 
> For TX, the used length should be 0.
> 
>>                 }
>>         }
>>         for (; n < (out_sgs + in_sgs); n++) {
>>
>> So can you tell me if you prefer me to send a V2 updating the commit message or
>> a new message to fix virtio_ring (or both)?
> 
> See above, for the driver side, the suppress_used_validation is
> sufficient. For the device side, it needs to be fixed (0 for TX used
> length) too.

I confirm that set len to 0 in virtq_used_elem struct, when release the
in-buffer on device (remote processor) side, fixes the issue.

Need to improve the behaviour in OpenAMP lib but for legacy support of the
different Virtio libraries the suppress_used_validation seems necessary. I will
send a V2 with an update of the commit message

Thanks,
Arnaud

> 
> Thanks
> 
>>
>> Thanks,
>> Arnaud
>>
>>>
>>>> Fixes: 939779f5152d ("virtio_ring: validate used buffer length")
>>>>
>>>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
>>>> Cc: Jason Wang <jasowang@xxxxxxxxxx>
>>>> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
>>>> ---
>>>> base-commit: fa55b7dcdc43c1aa1ba12bca9d2dd4318c2a0dbf
>>>> ---
>>>>  drivers/rpmsg/virtio_rpmsg_bus.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> index 9c112aa65040..5f73f19c2c38 100644
>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>> @@ -1054,6 +1054,7 @@ static struct virtio_driver virtio_ipc_driver = {
>>>>      .feature_table_size = ARRAY_SIZE(features),
>>>>      .driver.name    = KBUILD_MODNAME,
>>>>      .driver.owner   = THIS_MODULE,
>>>> +    .suppress_used_validation = true,
>>>>      .id_table       = id_table,
>>>>      .probe          = rpmsg_probe,
>>>>      .remove         = rpmsg_remove,
>>>> --
>>>> 2.17.1
>>>
>>
>