[PATCH] virtio-net: check the existence of peer before accesing its config

[PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Cornelia Huck]

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>

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Michael S. Tsirkin]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Cornelia Huck]

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(-)

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Michael S. Tsirkin]

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(-)

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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(-)

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Michael S. Tsirkin]

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(-)

Re: [PATCH] virtio-net: check the existence of peer before accesing its config

[Jason Wang]

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(-)