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 <l...@redhat.com>
> > > Tested-by: Cornelia Huck <coh...@redhat.com>
> > > Signed-off-by: Jason Wang <jasow...@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
