Re: [PATCH 5/5] libvduse: Add support for reconnecting

[Stefan Hajnoczi]

On Tue, Feb 08, 2022 at 03:35:27PM +0800, Yongji Xie wrote:
> On Mon, Feb 7, 2022 at 10:39 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, Jan 25, 2022 at 09:18:00PM +0800, Xie Yongji wrote:
> > > +static void *vduse_log_get(const char *dir, const char *name, size_t 
> > > size)
> > > +{
> > > +    void *ptr = MAP_FAILED;
> > > +    char *path;
> > > +    int fd;
> > > +
> > > +    path = (char *)malloc(strlen(dir) + strlen(name) +
> > > +                          strlen("/vduse-log-") + 1);
> > > +    if (!path) {
> > > +        return ptr;
> > > +    }
> > > +    sprintf(path, "%s/vduse-log-%s", dir, name);
> >
> > Please use g_strdup_printf() and g_autofree in QEMU code. In libvduse
> > code it's okay to use malloc(3), but regular QEMU code should use glib.
> >
> 
> But this code resides in libvduse currently.

Oops, I thought we were in block/export/vduse-blk.c. Then it's fine to
use malloc(3).

> > > +static int vduse_queue_check_inflights(VduseVirtq *vq)
> > > +{
> > > +    int i = 0;
> > > +    VduseDev *dev = vq->dev;
> > > +
> > > +    vq->used_idx = vq->vring.used->idx;
> >
> > Is this reading struct vring_used->idx without le16toh()?
> >
> > > +    vq->resubmit_num = 0;
> > > +    vq->resubmit_list = NULL;
> > > +    vq->counter = 0;
> > > +
> > > +    if (unlikely(vq->log->inflight.used_idx != vq->used_idx)) {
> > > +        
> > > vq->log->inflight.desc[vq->log->inflight.last_batch_head].inflight = 0;
> >
> > I suggest validating vq->log->inflight fields before using them.
> > last_batch_head must be less than the virtqueue size. Although the log
> > file is somewhat trusted, there may still be ways to corrupt it or
> > confuse the new process that loads it.
> >
> 
> I can validate the last_batch_head field. But it's hard to validate
> the inflight field, so we might still meet some issues if the file is
> corrupted.

It's okay if the log tells us to resubmit virtqueue buffers that have
garbage vring descriptors because the vring code needs to handle garbage
descriptors anyway.

But we cannot load dest[untrusted_input] or do anything else that could
crash, corrupt memory, etc.

> > > @@ -988,6 +1212,12 @@ VduseDev *vduse_dev_create(const char *name, 
> > > uint32_t device_id,
> > >          vqs[i].index = i;
> > >          vqs[i].dev = dev;
> > >          vqs[i].fd = -1;
> > > +        if (log) {
> > > +            vqs[i].log = log;
> > > +            vqs[i].log->inflight.desc_num = VIRTQUEUE_MAX_SIZE;
> > > +            log = (void *)((char *)log +
> > > +                  vduse_vq_log_size(VIRTQUEUE_MAX_SIZE));
> >
> > The size of the log needs to be verified. The file is mmapped but
> > there's no guarantee that the size matches num_queues *
> > vduse_vq_log_size(VIRTQUEUE_MAX_SIZE).
> >
> 
> We will call ftruncate() in vduse_log_get(). Is it enough?

Yes, I think so.

Thanks,
Stefan

signature.asc
Description: PGP signature