On Tue, Feb 08, 2022 at 03:35:27PM +0800, Yongji Xie wrote:
> On Mon, Feb 7, 2022 at 10:39 PM Stefan Hajnoczi <stefa...@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
