From: Jason Wang <jasowang@redhat.com>
To: Stefano Garzarella <sgarzare@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>
Cc: virtualization@lists.linux-foundation.org,
Parav Pandit <parav@nvidia.com>, Eli Cohen <elic@nvidia.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] vdpa/mlx5: fix param validation in mlx5_vdpa_get_config()
Date: Thu, 18 Feb 2021 13:47:24 +0800 [thread overview]
Message-ID: <5b748533-7091-cba1-50c6-3da2049bc354@redhat.com> (raw)
In-Reply-To: <20210210100821.aaye2cgmrpwhhzgn@steredhat>
On 2021/2/10 下午6:08, Stefano Garzarella wrote:
> On Tue, Feb 09, 2021 at 04:31:23AM -0500, Michael S. Tsirkin wrote:
>> On Tue, Feb 09, 2021 at 11:24:03AM +0800, Jason Wang wrote:
>>>
>>> On 2021/2/9 上午2:38, Michael S. Tsirkin wrote:
>>> > On Mon, Feb 08, 2021 at 05:17:41PM +0100, Stefano Garzarella wrote:
>>> > > It's legal to have 'offset + len' equal to
>>> > > sizeof(struct virtio_net_config), since 'ndev->config' is a
>>> > > 'struct virtio_net_config', so we can safely copy its content under
>>> > > this condition.
>>> > >
>>> > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported
>>> mlx5 devices")
>>> > > Cc: stable@vger.kernel.org
>>> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>>> > > ---
>>> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 2 +-
>>> > > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> > >
>>> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> > > index dc88559a8d49..10e9b09932eb 100644
>>> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> > > @@ -1820,7 +1820,7 @@ static void mlx5_vdpa_get_config(struct
>>> vdpa_device *vdev, unsigned int offset,
>>> > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> > > - if (offset + len < sizeof(struct virtio_net_config))
>>> > > + if (offset + len <= sizeof(struct virtio_net_config))
>>> > > memcpy(buf, (u8 *)&ndev->config + offset, len);
>>> > > }
>>> > Actually first I am not sure we need these checks at all.
>>> > vhost_vdpa_config_validate already validates the values, right?
>>>
>>>
>>> I think they're working at different levels. There's no guarantee that
>>> vhost-vdpa is the driver for this vdpa device.
>>
>> In fact, get_config returns void, so userspace can easily get
>> trash if it passes incorrect parameters by mistake, there is
>> no way for userspace to find out whether that is the case :(
>>
>> Any objections to returning the # of bytes copied, or -1
>> on error?
>
> Make sense for me, but are we sure we don't break userspace if we
> return the number of bytes instead of 0 on success?
>
> I had a quick look at QEMU and it looks like we consider success if
> the return value is >= 0, but I need to check further.
So I think in the vdpa bus level, we can return #bytes and in vhost uAPI
level, we can return error if the size is not expected otherwise zero?
Thanks
>
>>
>>>
>>> >
>>> > Second, what will happen when we extend the struct and then
>>> > run new userspace on an old kernel? Looks like it will just
>>> > fail right? So what is the plan?
>>>
>>>
>>> In this case, get_config() should match the spec behaviour. That is
>>> to say
>>> the size of config space depends on the feature negotiated.
>>>
>>> Thanks
>>
>> Yes but spec says config space can be bigger than specified by features:
>>
>> Drivers MUST NOT limit structure size and device configuration
>> space size. Instead, drivers SHOULD only
>> check that device configuration space is large enough to contain
>> the fields necessary for device operation.
>>
>
> So IIUC in the driver we should copy as much as we can.
>
> If you agree, I can send an RFC series and we can continue the
> discussion on it, but I think we should queue this patch for stable
> branches.
>
> Thanks,
> Stefano
>
next prev parent reply other threads:[~2021-02-18 6:11 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-08 16:17 [PATCH] vdpa/mlx5: fix param validation in mlx5_vdpa_get_config() Stefano Garzarella
2021-02-08 18:38 ` Michael S. Tsirkin
2021-02-09 3:24 ` Jason Wang
2021-02-09 8:58 ` Stefano Garzarella
2021-02-09 9:31 ` Michael S. Tsirkin
2021-02-10 10:08 ` Stefano Garzarella
2021-02-18 5:47 ` Jason Wang [this message]
2021-02-09 3:21 ` Jason Wang
2021-02-09 5:43 ` Eli Cohen
2021-02-09 9:00 ` Stefano Garzarella
2021-02-10 4:17 ` Jason Wang
2021-02-10 12:12 ` Michael S. Tsirkin
2021-02-11 15:39 ` Stefano Garzarella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5b748533-7091-cba1-50c6-3da2049bc354@redhat.com \
--to=jasowang@redhat.com \
--cc=elic@nvidia.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mst@redhat.com \
--cc=parav@nvidia.com \
--cc=sgarzare@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).