Re: [Qemu-block] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Michael S. Tsirkin]

On Mon, Feb 04, 2019 at 11:16:14AM +0100, Stefano Garzarella wrote:
> On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote:
> > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote:
> > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <address@hidden> wrote:
> > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > > > In order to avoid migration issues, we enable DISCARD and
> > > > > WRITE ZEROES features only for machine type >= 4.0
> > > > >
> > > > > Suggested-by: Dr. David Alan Gilbert <address@hidden>
> > > > > Signed-off-by: Stefano Garzarella <address@hidden>
> > > > > ---
> > > > >  hw/block/virtio-blk.c          | 2 ++
> > > > >  hw/core/machine.c              | 1 +
> > > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > > >  3 files changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > index 8a6754d9a2..542ec52536 100644
> > > > > --- a/hw/block/virtio-blk.c
> > > > > +++ b/hw/block/virtio-blk.c
> > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 
> > > > > 128),
> > > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, 
> > > > > TYPE_IOTHREAD,
> > > > >                       IOThread *),
> > > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, 
> > > > > conf.discard_wzeroes, 0,
> > > > > +                     true),
> > > > >      DEFINE_PROP_END_OF_LIST(),
> > > > >  };
> > > > >
> > > >
> > > > Thinking about it, are there security implications for discard?
> > > > Should we make it default false?
> > > 
> > > Hi Michael,
> > > 
> > > I'm not completely sure but if the guest can write on a specific sector,
> > > discard or write_zeroes operations should not have a security implication.
> > > 
> > > Do I miss something?
> > 
> > Recently page cache attacks have been discussed in the Linux community:
> > https://arxiv.org/pdf/1901.01161.pdf
> > 
> > I guess the scenario Michael is thinking about involves either -drive
> > cache.direct=off (including cache=writeback or cache=writethrough) or
> > maybe a timing side-channel in the storage appliance.
> > 
> > The discard operation may allow a guest to evict the cache, an important
> > primitive for page cache attacks.
> > 
> > Most of the time disk images are not shared between guests at all.
> > Therefore the discard operation cannot be used to learn information
> > about other guests.
> > 
> > Let's focus on shared disk images: shared disk images are either
> > read-only (then discard isn't allowed anyway) or they are shared
> > writable (but this already implies a trust relationship between the
> > guests).
> > 
> > My opinion is that discard is safe because virtualization use cases are
> > quite different from the attacks possible with shared library files
> > between userspace processes.  I'm curious if anyone has figured out a
> > realistic scenario where it does matter though...
> 
> Many thanks for the explanation!
> 
> I'll wait to send the v3 in order to understand if Michael agrees to
> leave discard feature enabled to default.
> 
> Thanks,
> Stefano

OK. Maybe mention the above in the commit log.

-- 
MST