Re: [Qemu-devel] [PATCH v3 7/9] raw: Prohibit dangerous writes for probed images

[Dr. David Alan Gilbert]

* Kevin Wolf (address@hidden) wrote:
> Am 20.11.2014 um 21:08 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (address@hidden) wrote:
> > 
> > 
> > > diff --git a/block/raw_bsd.c b/block/raw_bsd.c
> > > index 401b967..2ce5409 100644
> > > --- a/block/raw_bsd.c
> > > +++ b/block/raw_bsd.c
> > > @@ -58,8 +58,58 @@ static int coroutine_fn raw_co_readv(BlockDriverState 
> > > *bs, int64_t sector_num,
> > >  static int coroutine_fn raw_co_writev(BlockDriverState *bs, int64_t 
> > > sector_num,
> > >                                        int nb_sectors, QEMUIOVector *qiov)
> > >  {
> > > +    void *buf = NULL;
> > > +    BlockDriver *drv;
> > > +    QEMUIOVector local_qiov;
> > > +    int ret;
> > > +
> > > +    if (bs->probed && sector_num == 0) {
> > > +        /* As long as these conditions are true, we can't get partial 
> > > writes to
> > > +         * the probe buffer and can just directly check the request. */
> > > +        QEMU_BUILD_BUG_ON(BLOCK_PROBE_BUF_SIZE != 512);
> > > +        QEMU_BUILD_BUG_ON(BDRV_SECTOR_SIZE != 512);
> > > +
> > > +        if (nb_sectors == 0) {
> > > +            /* qemu_iovec_to_buf() would fail, but we want to return 
> > > success
> > > +             * instead of -EINVAL in this case. */
> > > +            return 0;
> > > +        }
> > > +
> > > +        buf = qemu_try_blockalign(bs->file, 512);
> > > +        if (!buf) {
> > > +            ret = -ENOMEM;
> > > +            goto fail;
> > > +        }
> > > +
> > > +        ret = qemu_iovec_to_buf(qiov, 0, buf, 512);
> > > +        if (ret != 512) {
> > > +            ret = -EINVAL;
> > > +            goto fail;
> > > +        }
> > > +
> > > +        drv = bdrv_probe_all(buf, 512, NULL);
> > > +        if (drv != bs->drv) {
> > > +            ret = -EPERM;
> > > +            goto fail;
> > > +        }
> > 
> > Two things about this worry me:
> >    1) It allows a running guest to prod at the probing code potentially 
> > quite
> > hard; if there is anything nasty that can be done during probing it would
> > potentially make it easier for a guest to find it.
> 
> The probing functions are trivial. You can audit them in no time even
> with no previous block layer experience. They just do a few tests on the
> passed buffer.

Hmm, OK.

> >    2) We don't log anything when this failure happens so if someone hits
> > this by accident for some reason it'll confuse them no end.  Could we add
> > a (1 time?) error_report/printf just so that there's something to work with 
> > ?
> 
> We already log a warning on bdrv_open(). Don't you think that should be
> enough?

Imagine a place that's got lots of QEMUs running already, they'll install
the QEMU that has this fix in it and carry on; they won't look at their logs.
Then if they hit a problem with a write failure they'll get confused; the logs
in bdrv_open just tell them they should be doing something safely, they don't
tell them they've actually hit a case fo something trying to do the write.

Dave

> Kevin
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK