* [PATCH] virtio-net: check the existence of peer before accesing its config
@ 2020-07-27 7:43 Jason Wang
2020-07-27 8:44 ` Cornelia Huck
2020-07-27 9:41 ` Michael S. Tsirkin
0 siblings, 2 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 7:43 UTC (permalink / raw)
To: mst, jasowang; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
We try to get config from peer unconditionally which may lead NULL
pointer dereference. Add a check before trying to access the config.
Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
Cc: Cindy Lu <lulu@redhat.com>
Tested-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
hw/net/virtio-net.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4895af1cbe..935b9ef5c7 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
{
VirtIONet *n = VIRTIO_NET(vdev);
struct virtio_net_config netcfg;
+ NetClientState *nc = qemu_get_queue(n->nic);
int ret = 0;
memset(&netcfg, 0 , sizeof(struct virtio_net_config));
@@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
VIRTIO_NET_RSS_SUPPORTED_HASHES);
memcpy(config, &netcfg, n->config_size);
- NetClientState *nc = qemu_get_queue(n->nic);
- if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+ if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
- n->config_size);
- if (ret != -1) {
- memcpy(config, &netcfg, n->config_size);
- }
+ n->config_size);
+ if (ret != -1) {
+ memcpy(config, &netcfg, n->config_size);
+ }
}
}
@@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
{
VirtIONet *n = VIRTIO_NET(vdev);
struct virtio_net_config netcfg = {};
+ NetClientState *nc = qemu_get_queue(n->nic);
memcpy(&netcfg, config, n->config_size);
@@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
}
- NetClientState *nc = qemu_get_queue(n->nic);
- if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
- vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
- 0, n->config_size,
- VHOST_SET_CONFIG_TYPE_MASTER);
+ if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
+ vhost_net_set_config(get_vhost_net(nc->peer),
+ (uint8_t *)&netcfg, 0, n->config_size,
+ VHOST_SET_CONFIG_TYPE_MASTER);
}
}
--
2.20.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 7:43 [PATCH] virtio-net: check the existence of peer before accesing its config Jason Wang
@ 2020-07-27 8:44 ` Cornelia Huck
2020-07-27 8:51 ` Jason Wang
2020-07-27 9:41 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Cornelia Huck @ 2020-07-27 8:44 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, Cindy Lu, mst
On Mon, 27 Jul 2020 15:43:28 +0800
Jason Wang <jasowang@redhat.com> wrote:
typo in subject: s/accesing/accessing/
> We try to get config from peer unconditionally which may lead NULL
s/lead/lead to a/
> pointer dereference. Add a check before trying to access the config.
>
> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> Cc: Cindy Lu <lulu@redhat.com>
> Tested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> hw/net/virtio-net.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 8:44 ` Cornelia Huck
@ 2020-07-27 8:51 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 8:51 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-devel, Cindy Lu, mst
On 2020/7/27 下午4:44, Cornelia Huck wrote:
> On Mon, 27 Jul 2020 15:43:28 +0800
> Jason Wang <jasowang@redhat.com> wrote:
>
> typo in subject: s/accesing/accessing/
>
>> We try to get config from peer unconditionally which may lead NULL
> s/lead/lead to a/
>
>> pointer dereference. Add a check before trying to access the config.
>>
>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>> Cc: Cindy Lu <lulu@redhat.com>
>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> hw/net/virtio-net.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Applied with the above fixed.
Thanks
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 7:43 [PATCH] virtio-net: check the existence of peer before accesing its config Jason Wang
2020-07-27 8:44 ` Cornelia Huck
@ 2020-07-27 9:41 ` Michael S. Tsirkin
2020-07-27 9:49 ` Jason Wang
2020-07-27 9:53 ` Cornelia Huck
1 sibling, 2 replies; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 9:41 UTC (permalink / raw)
To: Jason Wang; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> We try to get config from peer unconditionally which may lead NULL
> pointer dereference. Add a check before trying to access the config.
>
> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> Cc: Cindy Lu <lulu@redhat.com>
> Tested-by: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
I am a bit lost here. Isn't this invoked
when guest attempts to read the config?
With no peer, what do we return to guest?
A code comment might be helpful here.
> ---
> hw/net/virtio-net.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 4895af1cbe..935b9ef5c7 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> struct virtio_net_config netcfg;
> + NetClientState *nc = qemu_get_queue(n->nic);
>
> int ret = 0;
> memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> VIRTIO_NET_RSS_SUPPORTED_HASHES);
> memcpy(config, &netcfg, n->config_size);
>
> - NetClientState *nc = qemu_get_queue(n->nic);
> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> - n->config_size);
> - if (ret != -1) {
> - memcpy(config, &netcfg, n->config_size);
> - }
> + n->config_size);
> + if (ret != -1) {
> + memcpy(config, &netcfg, n->config_size);
> + }
> }
> }
>
> @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> {
> VirtIONet *n = VIRTIO_NET(vdev);
> struct virtio_net_config netcfg = {};
> + NetClientState *nc = qemu_get_queue(n->nic);
>
> memcpy(&netcfg, config, n->config_size);
>
> @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> }
>
> - NetClientState *nc = qemu_get_queue(n->nic);
> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> - 0, n->config_size,
> - VHOST_SET_CONFIG_TYPE_MASTER);
> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> + vhost_net_set_config(get_vhost_net(nc->peer),
> + (uint8_t *)&netcfg, 0, n->config_size,
> + VHOST_SET_CONFIG_TYPE_MASTER);
> }
> }
>
> --
> 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 9:41 ` Michael S. Tsirkin
@ 2020-07-27 9:49 ` Jason Wang
2020-07-27 10:17 ` Michael S. Tsirkin
2020-07-27 10:21 ` Michael S. Tsirkin
2020-07-27 9:53 ` Cornelia Huck
1 sibling, 2 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 9:49 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On 2020/7/27 下午5:41, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
>> We try to get config from peer unconditionally which may lead NULL
>> pointer dereference. Add a check before trying to access the config.
>>
>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>> Cc: Cindy Lu <lulu@redhat.com>
>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I am a bit lost here. Isn't this invoked
> when guest attempts to read the config?
> With no peer, what do we return to guest?
With no peer, it just work as in the past (read from the qemu's own
emulated config space). With a vDPA as its peer, it tries to read it
from vhost-vDPA.
> A code comment might be helpful here.
Does something like above help?
Thanks
>
>> ---
>> hw/net/virtio-net.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 4895af1cbe..935b9ef5c7 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>> {
>> VirtIONet *n = VIRTIO_NET(vdev);
>> struct virtio_net_config netcfg;
>> + NetClientState *nc = qemu_get_queue(n->nic);
>>
>> int ret = 0;
>> memset(&netcfg, 0 , sizeof(struct virtio_net_config));
>> @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>> VIRTIO_NET_RSS_SUPPORTED_HASHES);
>> memcpy(config, &netcfg, n->config_size);
>>
>> - NetClientState *nc = qemu_get_queue(n->nic);
>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>> ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>> - n->config_size);
>> - if (ret != -1) {
>> - memcpy(config, &netcfg, n->config_size);
>> - }
>> + n->config_size);
>> + if (ret != -1) {
>> + memcpy(config, &netcfg, n->config_size);
>> + }
>> }
>> }
>>
>> @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>> {
>> VirtIONet *n = VIRTIO_NET(vdev);
>> struct virtio_net_config netcfg = {};
>> + NetClientState *nc = qemu_get_queue(n->nic);
>>
>> memcpy(&netcfg, config, n->config_size);
>>
>> @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>> }
>>
>> - NetClientState *nc = qemu_get_queue(n->nic);
>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>> - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>> - 0, n->config_size,
>> - VHOST_SET_CONFIG_TYPE_MASTER);
>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>> + vhost_net_set_config(get_vhost_net(nc->peer),
>> + (uint8_t *)&netcfg, 0, n->config_size,
>> + VHOST_SET_CONFIG_TYPE_MASTER);
>> }
>> }
>>
>> --
>> 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 9:41 ` Michael S. Tsirkin
2020-07-27 9:49 ` Jason Wang
@ 2020-07-27 9:53 ` Cornelia Huck
2020-07-27 10:13 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Cornelia Huck @ 2020-07-27 9:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jason Wang, qemu-devel, Cindy Lu
On Mon, 27 Jul 2020 05:41:17 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> > We try to get config from peer unconditionally which may lead NULL
> > pointer dereference. Add a check before trying to access the config.
> >
> > Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> > Cc: Cindy Lu <lulu@redhat.com>
> > Tested-by: Cornelia Huck <cohuck@redhat.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> I am a bit lost here. Isn't this invoked
> when guest attempts to read the config?
> With no peer, what do we return to guest?
Same as with a non-vdpa peer? It's the dereference that needs to be
guarded.
> A code comment might be helpful here.
>
> > ---
> > hw/net/virtio-net.c | 22 +++++++++++-----------
> > 1 file changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 9:53 ` Cornelia Huck
@ 2020-07-27 10:13 ` Michael S. Tsirkin
2020-07-27 10:26 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 10:13 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Jason Wang, qemu-devel, Cindy Lu
On Mon, Jul 27, 2020 at 11:53:22AM +0200, Cornelia Huck wrote:
> On Mon, 27 Jul 2020 05:41:17 -0400
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> > > We try to get config from peer unconditionally which may lead NULL
> > > pointer dereference. Add a check before trying to access the config.
> > >
> > > Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> > > Cc: Cindy Lu <lulu@redhat.com>
> > > Tested-by: Cornelia Huck <cohuck@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > I am a bit lost here. Isn't this invoked
> > when guest attempts to read the config?
> > With no peer, what do we return to guest?
>
> Same as with a non-vdpa peer? It's the dereference that needs to be
> guarded.
So vdpa has a GET_CONFIG ioctl which to me hints that a peer needs to be
notified on get config calls.
If we return config from qemu memory here, then I guess we
need to call GET_CONFIG on connect and validate it -
does this make sense?
Cindy, Jason?
> > A code comment might be helpful here.
> >
> > > ---
> > > hw/net/virtio-net.c | 22 +++++++++++-----------
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 9:49 ` Jason Wang
@ 2020-07-27 10:17 ` Michael S. Tsirkin
2020-07-27 10:22 ` Jason Wang
2020-07-27 10:21 ` Michael S. Tsirkin
1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 10:17 UTC (permalink / raw)
To: Jason Wang; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On Mon, Jul 27, 2020 at 05:49:47PM +0800, Jason Wang wrote:
>
> On 2020/7/27 下午5:41, Michael S. Tsirkin wrote:
> > On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> > > We try to get config from peer unconditionally which may lead NULL
> > > pointer dereference. Add a check before trying to access the config.
> > >
> > > Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> > > Cc: Cindy Lu <lulu@redhat.com>
> > > Tested-by: Cornelia Huck <cohuck@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > I am a bit lost here. Isn't this invoked
> > when guest attempts to read the config?
> > With no peer, what do we return to guest?
>
>
> With no peer, it just work as in the past (read from the qemu's own emulated
> config space). With a vDPA as its peer, it tries to read it from vhost-vDPA.
Are these scenarios where guest would sometimes get one and
sometimes another? E.g. does it happen on disconnect?
If yes that might become a problem ...
>
> > A code comment might be helpful here.
>
>
> Does something like above help?
>
> Thanks
>
>
> >
> > > ---
> > > hw/net/virtio-net.c | 22 +++++++++++-----------
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 4895af1cbe..935b9ef5c7 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > > {
> > > VirtIONet *n = VIRTIO_NET(vdev);
> > > struct virtio_net_config netcfg;
> > > + NetClientState *nc = qemu_get_queue(n->nic);
> > > int ret = 0;
> > > memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> > > @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > > VIRTIO_NET_RSS_SUPPORTED_HASHES);
> > > memcpy(config, &netcfg, n->config_size);
> > > - NetClientState *nc = qemu_get_queue(n->nic);
> > > - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> > > - n->config_size);
> > > - if (ret != -1) {
> > > - memcpy(config, &netcfg, n->config_size);
> > > - }
> > > + n->config_size);
> > > + if (ret != -1) {
> > > + memcpy(config, &netcfg, n->config_size);
> > > + }
> > > }
> > > }
> > > @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > {
> > > VirtIONet *n = VIRTIO_NET(vdev);
> > > struct virtio_net_config netcfg = {};
> > > + NetClientState *nc = qemu_get_queue(n->nic);
> > > memcpy(&netcfg, config, n->config_size);
> > > @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > > }
> > > - NetClientState *nc = qemu_get_queue(n->nic);
> > > - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> > > - 0, n->config_size,
> > > - VHOST_SET_CONFIG_TYPE_MASTER);
> > > + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > + vhost_net_set_config(get_vhost_net(nc->peer),
> > > + (uint8_t *)&netcfg, 0, n->config_size,
> > > + VHOST_SET_CONFIG_TYPE_MASTER);
> > > }
> > > }
> > > --
> > > 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 9:49 ` Jason Wang
2020-07-27 10:17 ` Michael S. Tsirkin
@ 2020-07-27 10:21 ` Michael S. Tsirkin
2020-07-27 10:23 ` Jason Wang
1 sibling, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 10:21 UTC (permalink / raw)
To: Jason Wang; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On Mon, Jul 27, 2020 at 05:49:47PM +0800, Jason Wang wrote:
>
> On 2020/7/27 下午5:41, Michael S. Tsirkin wrote:
> > On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> > > We try to get config from peer unconditionally which may lead NULL
> > > pointer dereference. Add a check before trying to access the config.
> > >
> > > Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> > > Cc: Cindy Lu <lulu@redhat.com>
> > > Tested-by: Cornelia Huck <cohuck@redhat.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > I am a bit lost here. Isn't this invoked
> > when guest attempts to read the config?
> > With no peer, what do we return to guest?
>
>
> With no peer, it just work as in the past (read from the qemu's own emulated
> config space). With a vDPA as its peer, it tries to read it from vhost-vDPA.
>
>
> > A code comment might be helpful here.
>
>
> Does something like above help?
>
> Thanks
I think this part of commit log caused confusion:
Add a check before trying to access the config.
looking more at the code, imho in fact here is a better description of
what is going on:
We try to check whether a peer is VDPA in order to get config from
there - with no peer, this leads to a NULL
pointer dereference. Add a check before trying to access the peer
type. No peer means not VDPA.
>
> >
> > > ---
> > > hw/net/virtio-net.c | 22 +++++++++++-----------
> > > 1 file changed, 11 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 4895af1cbe..935b9ef5c7 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > > {
> > > VirtIONet *n = VIRTIO_NET(vdev);
> > > struct virtio_net_config netcfg;
> > > + NetClientState *nc = qemu_get_queue(n->nic);
> > > int ret = 0;
> > > memset(&netcfg, 0 , sizeof(struct virtio_net_config));
> > > @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
> > > VIRTIO_NET_RSS_SUPPORTED_HASHES);
> > > memcpy(config, &netcfg, n->config_size);
> > > - NetClientState *nc = qemu_get_queue(n->nic);
> > > - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> > > - n->config_size);
> > > - if (ret != -1) {
> > > - memcpy(config, &netcfg, n->config_size);
> > > - }
> > > + n->config_size);
> > > + if (ret != -1) {
> > > + memcpy(config, &netcfg, n->config_size);
> > > + }
> > > }
> > > }
> > > @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > {
> > > VirtIONet *n = VIRTIO_NET(vdev);
> > > struct virtio_net_config netcfg = {};
> > > + NetClientState *nc = qemu_get_queue(n->nic);
> > > memcpy(&netcfg, config, n->config_size);
> > > @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
> > > qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
> > > }
> > > - NetClientState *nc = qemu_get_queue(n->nic);
> > > - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
> > > - 0, n->config_size,
> > > - VHOST_SET_CONFIG_TYPE_MASTER);
> > > + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> > > + vhost_net_set_config(get_vhost_net(nc->peer),
> > > + (uint8_t *)&netcfg, 0, n->config_size,
> > > + VHOST_SET_CONFIG_TYPE_MASTER);
> > > }
> > > }
> > > --
> > > 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 10:17 ` Michael S. Tsirkin
@ 2020-07-27 10:22 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 10:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On 2020/7/27 下午6:17, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 05:49:47PM +0800, Jason Wang wrote:
>> On 2020/7/27 下午5:41, Michael S. Tsirkin wrote:
>>> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
>>>> We try to get config from peer unconditionally which may lead NULL
>>>> pointer dereference. Add a check before trying to access the config.
>>>>
>>>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>>>> Cc: Cindy Lu <lulu@redhat.com>
>>>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> I am a bit lost here. Isn't this invoked
>>> when guest attempts to read the config?
>>> With no peer, what do we return to guest?
>>
>> With no peer, it just work as in the past (read from the qemu's own emulated
>> config space). With a vDPA as its peer, it tries to read it from vhost-vDPA.
> Are these scenarios where guest would sometimes get one and
> sometimes another? E.g. does it happen on disconnect?
> If yes that might become a problem ...
For disconnecting yes, but I wonder if we need to care about that case
anyway.
Thanks
>
>>> A code comment might be helpful here.
>>
>> Does something like above help?
>>
>> Thanks
>>
>>
>>>> ---
>>>> hw/net/virtio-net.c | 22 +++++++++++-----------
>>>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 4895af1cbe..935b9ef5c7 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>> {
>>>> VirtIONet *n = VIRTIO_NET(vdev);
>>>> struct virtio_net_config netcfg;
>>>> + NetClientState *nc = qemu_get_queue(n->nic);
>>>> int ret = 0;
>>>> memset(&netcfg, 0 , sizeof(struct virtio_net_config));
>>>> @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>> VIRTIO_NET_RSS_SUPPORTED_HASHES);
>>>> memcpy(config, &netcfg, n->config_size);
>>>> - NetClientState *nc = qemu_get_queue(n->nic);
>>>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>>>> - n->config_size);
>>>> - if (ret != -1) {
>>>> - memcpy(config, &netcfg, n->config_size);
>>>> - }
>>>> + n->config_size);
>>>> + if (ret != -1) {
>>>> + memcpy(config, &netcfg, n->config_size);
>>>> + }
>>>> }
>>>> }
>>>> @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>> {
>>>> VirtIONet *n = VIRTIO_NET(vdev);
>>>> struct virtio_net_config netcfg = {};
>>>> + NetClientState *nc = qemu_get_queue(n->nic);
>>>> memcpy(&netcfg, config, n->config_size);
>>>> @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>>>> }
>>>> - NetClientState *nc = qemu_get_queue(n->nic);
>>>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>>>> - 0, n->config_size,
>>>> - VHOST_SET_CONFIG_TYPE_MASTER);
>>>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> + vhost_net_set_config(get_vhost_net(nc->peer),
>>>> + (uint8_t *)&netcfg, 0, n->config_size,
>>>> + VHOST_SET_CONFIG_TYPE_MASTER);
>>>> }
>>>> }
>>>> --
>>>> 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 10:21 ` Michael S. Tsirkin
@ 2020-07-27 10:23 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 10:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On 2020/7/27 下午6:21, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 05:49:47PM +0800, Jason Wang wrote:
>> On 2020/7/27 下午5:41, Michael S. Tsirkin wrote:
>>> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
>>>> We try to get config from peer unconditionally which may lead NULL
>>>> pointer dereference. Add a check before trying to access the config.
>>>>
>>>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>>>> Cc: Cindy Lu <lulu@redhat.com>
>>>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> I am a bit lost here. Isn't this invoked
>>> when guest attempts to read the config?
>>> With no peer, what do we return to guest?
>>
>> With no peer, it just work as in the past (read from the qemu's own emulated
>> config space). With a vDPA as its peer, it tries to read it from vhost-vDPA.
>>
>>
>>> A code comment might be helpful here.
>>
>> Does something like above help?
>>
>> Thanks
> I think this part of commit log caused confusion:
>
> Add a check before trying to access the config.
>
> looking more at the code, imho in fact here is a better description of
> what is going on:
>
> We try to check whether a peer is VDPA in order to get config from
> there - with no peer, this leads to a NULL
> pointer dereference. Add a check before trying to access the peer
> type. No peer means not VDPA.
Yes, this looks much better.
Thanks
>
>
>>>> ---
>>>> hw/net/virtio-net.c | 22 +++++++++++-----------
>>>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 4895af1cbe..935b9ef5c7 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -125,6 +125,7 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>> {
>>>> VirtIONet *n = VIRTIO_NET(vdev);
>>>> struct virtio_net_config netcfg;
>>>> + NetClientState *nc = qemu_get_queue(n->nic);
>>>> int ret = 0;
>>>> memset(&netcfg, 0 , sizeof(struct virtio_net_config));
>>>> @@ -142,13 +143,12 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>> VIRTIO_NET_RSS_SUPPORTED_HASHES);
>>>> memcpy(config, &netcfg, n->config_size);
>>>> - NetClientState *nc = qemu_get_queue(n->nic);
>>>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>>>> - n->config_size);
>>>> - if (ret != -1) {
>>>> - memcpy(config, &netcfg, n->config_size);
>>>> - }
>>>> + n->config_size);
>>>> + if (ret != -1) {
>>>> + memcpy(config, &netcfg, n->config_size);
>>>> + }
>>>> }
>>>> }
>>>> @@ -156,6 +156,7 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>> {
>>>> VirtIONet *n = VIRTIO_NET(vdev);
>>>> struct virtio_net_config netcfg = {};
>>>> + NetClientState *nc = qemu_get_queue(n->nic);
>>>> memcpy(&netcfg, config, n->config_size);
>>>> @@ -166,11 +167,10 @@ static void virtio_net_set_config(VirtIODevice *vdev, const uint8_t *config)
>>>> qemu_format_nic_info_str(qemu_get_queue(n->nic), n->mac);
>>>> }
>>>> - NetClientState *nc = qemu_get_queue(n->nic);
>>>> - if (nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> - vhost_net_set_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,
>>>> - 0, n->config_size,
>>>> - VHOST_SET_CONFIG_TYPE_MASTER);
>>>> + if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> + vhost_net_set_config(get_vhost_net(nc->peer),
>>>> + (uint8_t *)&netcfg, 0, n->config_size,
>>>> + VHOST_SET_CONFIG_TYPE_MASTER);
>>>> }
>>>> }
>>>> --
>>>> 2.20.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 10:13 ` Michael S. Tsirkin
@ 2020-07-27 10:26 ` Jason Wang
2020-07-27 11:41 ` Michael S. Tsirkin
0 siblings, 1 reply; 14+ messages in thread
From: Jason Wang @ 2020-07-27 10:26 UTC (permalink / raw)
To: Michael S. Tsirkin, Cornelia Huck; +Cc: qemu-devel, Cindy Lu
On 2020/7/27 下午6:13, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 11:53:22AM +0200, Cornelia Huck wrote:
>> On Mon, 27 Jul 2020 05:41:17 -0400
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>
>>> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
>>>> We try to get config from peer unconditionally which may lead NULL
>>>> pointer dereference. Add a check before trying to access the config.
>>>>
>>>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>>>> Cc: Cindy Lu <lulu@redhat.com>
>>>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> I am a bit lost here. Isn't this invoked
>>> when guest attempts to read the config?
>>> With no peer, what do we return to guest?
>> Same as with a non-vdpa peer? It's the dereference that needs to be
>> guarded.
> So vdpa has a GET_CONFIG ioctl which to me hints that a peer needs to be
> notified on get config calls.
> If we return config from qemu memory here, then I guess we
> need to call GET_CONFIG on connect and validate it -
> does this make sense?
>
> Cindy, Jason?
For "connect" you meant connecting virtio-net to its peer (vDPA)? AFAIK,
if we start with no peer, there's no way to set a peer afterwards.
Thanks
>
>>> A code comment might be helpful here.
>>>
>>>> ---
>>>> hw/net/virtio-net.c | 22 +++++++++++-----------
>>>> 1 file changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 10:26 ` Jason Wang
@ 2020-07-27 11:41 ` Michael S. Tsirkin
2020-07-27 12:44 ` Jason Wang
0 siblings, 1 reply; 14+ messages in thread
From: Michael S. Tsirkin @ 2020-07-27 11:41 UTC (permalink / raw)
To: Jason Wang; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On Mon, Jul 27, 2020 at 06:26:37PM +0800, Jason Wang wrote:
>
> On 2020/7/27 下午6:13, Michael S. Tsirkin wrote:
> > On Mon, Jul 27, 2020 at 11:53:22AM +0200, Cornelia Huck wrote:
> > > On Mon, 27 Jul 2020 05:41:17 -0400
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
> > > > > We try to get config from peer unconditionally which may lead NULL
> > > > > pointer dereference. Add a check before trying to access the config.
> > > > >
> > > > > Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> > > > > Cc: Cindy Lu <lulu@redhat.com>
> > > > > Tested-by: Cornelia Huck <cohuck@redhat.com>
> > > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > > I am a bit lost here. Isn't this invoked
> > > > when guest attempts to read the config?
> > > > With no peer, what do we return to guest?
> > > Same as with a non-vdpa peer? It's the dereference that needs to be
> > > guarded.
> > So vdpa has a GET_CONFIG ioctl which to me hints that a peer needs to be
> > notified on get config calls.
> > If we return config from qemu memory here, then I guess we
> > need to call GET_CONFIG on connect and validate it -
> > does this make sense?
> >
> > Cindy, Jason?
>
>
> For "connect" you meant connecting virtio-net to its peer (vDPA)? AFAIK, if
> we start with no peer, there's no way to set a peer afterwards.
>
> Thanks
That would be a good sentence to add in a code comment:
/*
* Is this VDPA? No peer means not VDPA: there's no way to
* disconnect/reconnect a VDPA peer.
*/
>
> >
> > > > A code comment might be helpful here.
> > > >
> > > > > ---
> > > > > hw/net/virtio-net.c | 22 +++++++++++-----------
> > > > > 1 file changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] virtio-net: check the existence of peer before accesing its config
2020-07-27 11:41 ` Michael S. Tsirkin
@ 2020-07-27 12:44 ` Jason Wang
0 siblings, 0 replies; 14+ messages in thread
From: Jason Wang @ 2020-07-27 12:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Cornelia Huck, qemu-devel, Cindy Lu
On 2020/7/27 下午7:41, Michael S. Tsirkin wrote:
> On Mon, Jul 27, 2020 at 06:26:37PM +0800, Jason Wang wrote:
>> On 2020/7/27 下午6:13, Michael S. Tsirkin wrote:
>>> On Mon, Jul 27, 2020 at 11:53:22AM +0200, Cornelia Huck wrote:
>>>> On Mon, 27 Jul 2020 05:41:17 -0400
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>
>>>>> On Mon, Jul 27, 2020 at 03:43:28PM +0800, Jason Wang wrote:
>>>>>> We try to get config from peer unconditionally which may lead NULL
>>>>>> pointer dereference. Add a check before trying to access the config.
>>>>>>
>>>>>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>>>>>> Cc: Cindy Lu <lulu@redhat.com>
>>>>>> Tested-by: Cornelia Huck <cohuck@redhat.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> I am a bit lost here. Isn't this invoked
>>>>> when guest attempts to read the config?
>>>>> With no peer, what do we return to guest?
>>>> Same as with a non-vdpa peer? It's the dereference that needs to be
>>>> guarded.
>>> So vdpa has a GET_CONFIG ioctl which to me hints that a peer needs to be
>>> notified on get config calls.
>>> If we return config from qemu memory here, then I guess we
>>> need to call GET_CONFIG on connect and validate it -
>>> does this make sense?
>>>
>>> Cindy, Jason?
>>
>> For "connect" you meant connecting virtio-net to its peer (vDPA)? AFAIK, if
>> we start with no peer, there's no way to set a peer afterwards.
>>
>> Thanks
>
> That would be a good sentence to add in a code comment:
>
> /*
> * Is this VDPA? No peer means not VDPA: there's no way to
> * disconnect/reconnect a VDPA peer.
> */
Sure.
Thanks
>
>>>>> A code comment might be helpful here.
>>>>>
>>>>>> ---
>>>>>> hw/net/virtio-net.c | 22 +++++++++++-----------
>>>>>> 1 file changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-07-27 12:46 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-27 7:43 [PATCH] virtio-net: check the existence of peer before accesing its config Jason Wang
2020-07-27 8:44 ` Cornelia Huck
2020-07-27 8:51 ` Jason Wang
2020-07-27 9:41 ` Michael S. Tsirkin
2020-07-27 9:49 ` Jason Wang
2020-07-27 10:17 ` Michael S. Tsirkin
2020-07-27 10:22 ` Jason Wang
2020-07-27 10:21 ` Michael S. Tsirkin
2020-07-27 10:23 ` Jason Wang
2020-07-27 9:53 ` Cornelia Huck
2020-07-27 10:13 ` Michael S. Tsirkin
2020-07-27 10:26 ` Jason Wang
2020-07-27 11:41 ` Michael S. Tsirkin
2020-07-27 12:44 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.