On Tue, Feb 8, 2022 at 4:09 PM Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> 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.
>
Makes sense to me.
Thanks,
Yongji
