linux-kernel - Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features requested

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0322ee9e-f6db-45ed-855f-9ebcdfca30a9@redhat.com>
Date: Fri, 5 Jan 2024 11:14:01 +0100
From: Maxime Coquelin <maxime.coquelin@...hat.com>
To: Eugenio Perez Martin <eperezma@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, mst@...hat.com,
 xuanzhuo@...ux.alibaba.com, xieyongji@...edance.com,
 virtualization@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
 david.marchand@...hat.com, lulu@...hat.com
Subject: Re: [PATCH v6 2/3] vduse: Temporarily fail if control queue features
 requested



On 1/5/24 10:59, Eugenio Perez Martin wrote:
> On Fri, Jan 5, 2024 at 9:12 AM Maxime Coquelin
> <maxime.coquelin@...hat.com> wrote:
>>
>>
>>
>> On 1/5/24 03:45, Jason Wang wrote:
>>> On Thu, Jan 4, 2024 at 11:38 PM Maxime Coquelin
>>> <maxime.coquelin@...hat.com> wrote:
>>>>
>>>> Virtio-net driver control queue implementation is not safe
>>>> when used with VDUSE. If the VDUSE application does not
>>>> reply to control queue messages, it currently ends up
>>>> hanging the kernel thread sending this command.
>>>>
>>>> Some work is on-going to make the control queue
>>>> implementation robust with VDUSE. Until it is completed,
>>>> let's fail features check if any control-queue related
>>>> feature is requested.
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@...hat.com>
>>>> ---
>>>>    drivers/vdpa/vdpa_user/vduse_dev.c | 13 +++++++++++++
>>>>    1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> index 0486ff672408..94f54ea2eb06 100644
>>>> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
>>>> @@ -28,6 +28,7 @@
>>>>    #include <uapi/linux/virtio_config.h>
>>>>    #include <uapi/linux/virtio_ids.h>
>>>>    #include <uapi/linux/virtio_blk.h>
>>>> +#include <uapi/linux/virtio_ring.h>
>>>>    #include <linux/mod_devicetable.h>
>>>>
>>>>    #include "iova_domain.h"
>>>> @@ -46,6 +47,15 @@
>>>>
>>>>    #define IRQ_UNBOUND -1
>>>>
>>>> +#define VDUSE_NET_INVALID_FEATURES_MASK         \
>>>> +       (BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |        \
>>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_RX)   |      \
>>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_VLAN) |      \
>>>> +        BIT_ULL(VIRTIO_NET_F_GUEST_ANNOUNCE) | \
>>>> +        BIT_ULL(VIRTIO_NET_F_MQ) |             \
>>>> +        BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |  \
>>>> +        BIT_ULL(VIRTIO_NET_F_RSS))
>>>
>>> We need to make this as well:
>>>
>>> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>
>> I missed it, and see others have been added in the Virtio spec
>> repository (BTW, I see this specific one is missing in the dependency
>> list [0], I will submit a patch).
>>
>> I wonder if it is not just simpler to just check for
>> VIRTIO_NET_F_CTRL_VQ is requested. As we fail instead of masking out,
>> the VDUSE driver won't be the one violating the spec so it should be
>> good?
>>
>> It will avoid having to update the mask if new features depending on it
>> are added (or forgetting to update it).
>>
>> WDYT?
>>
> 
> I think it is safer to work with a whitelist, instead of a blacklist.
> As any new feature might require code changes in QEMU. Is that
> possible?

Well, that's how it was done in previous revision. :)

I changed to a blacklist for consistency with block device's WCE feature
check after Jason's comment.

I'm not sure moving back to a whitelist brings much advantages when
compared to the effort of keeping it up to date. Just blacklisting
VIRTIO_NET_F_CTRL_VQ is enough in my opinion.

Thanks,
Maxime

>> Thanks,
>> Maxime
>>
>> [0]:
>> https://github.com/oasis-tcs/virtio-spec/blob/5fc35a7efb903fc352da81a6d2be5c01810b68d3/device-types/net/description.tex#L129
>>> Other than this,
>>>
>>> Acked-by: Jason Wang <jasowang@...hat.com>
>>>
>>> Thanks
>>>
>>>> +
>>>>    struct vduse_virtqueue {
>>>>           u16 index;
>>>>           u16 num_max;
>>>> @@ -1680,6 +1690,9 @@ static bool features_is_valid(struct vduse_dev_config *config)
>>>>           if ((config->device_id == VIRTIO_ID_BLOCK) &&
>>>>                           (config->features & (1ULL << VIRTIO_BLK_F_CONFIG_WCE)))
>>>>                   return false;
>>>> +       else if ((config->device_id == VIRTIO_ID_NET) &&
>>>> +                       (config->features & VDUSE_NET_INVALID_FEATURES_MASK))
>>>> +               return false;
>>>>
>>>>           return true;
>>>>    }
>>>> --
>>>> 2.43.0
>>>>
>>>
>>
>>
>