| Message ID | 20200727074328.2279-1-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net: check the existence of peer before accesing its config | expand |
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>
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
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
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
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(-)
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(-)
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
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
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
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
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(-)
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(-)
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(-)
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); } }
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(-)