Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using argv paremeter

[Marc-André Lureau]

Hi

On Tue, Nov 8, 2016 at 4:49 PM Rafael David Tinoco <
address@hidden> wrote:

> Hello Michael, André,
>
> Could you do a quick review before a final submission ?
>
> http://paste.ubuntu.com/23446279/
>
> - I split the commits into 1) bugfix, 2) new util with test, 3) vhostlog
>
> The unit test is testing passing fds between 2 processes and asserting
> contents of mmap buffer coming from the "vhostlog" util (mmap-file).
>
> Your final comment on the "vhostlog" was:
>
> >> Argv examples:
> >>
> >>     -netdev tap,id=net0,vhost=on
> >>     -netdev tap,id=net0,vhost=on,vhostlog=/tmp/guest.log
> >>     -netdev tap,id=net0,vhost=on,vhostlog=/tmp
>
> (André) > Could it be only a filename? This would simplify testing.
> (Michael) > When vhostlog is not specified, can we just use memfd as we
> did?
>
>
Michael said:
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg08197.html
I think that the best approach is to allow passing in the fd, not the file
path. If not passed, use memfd.

I do agree :)

I'm going to change this to:
>
> 1 - if vhostlog is not provided shared log can't be used. Use memfd.
>
> 2 - for shared logs, vhostlog has to be provided as a "file" ?
>
> Should i keep vhostlog being a directory also ? (i know we are unlinking
> the
> file so might not be needed BUT a static file might have a race condition
> in
> between different instances and providing a directory - that creates random
> files on it - might be better approach).
>
> Is there anything else ?
>

Do we really need to give a path? (pass fd with -add-fd/qmp add-fd)

Thank you
>
> Rafael Tinoco
>
> On Mon, Oct 31, 2016 at 8:30 PM, Michael S. Tsirkin <address@hidden>
> wrote:
> > On Mon, Oct 31, 2016 at 08:35:33AM -0200, Rafael David Tinoco wrote:
> >> On Sun, Oct 30, 2016 at 5:26 PM, Michael S. Tsirkin <address@hidden>
> wrote:
> >> >
> >> > On Sat, Oct 22, 2016 at 07:00:41AM +0000, Rafael David Tinoco wrote:
> >> > > Commit 31190ed7 added a migration blocker in vhost_dev_init() to
> >> > > check if memfd would succeed. It is better if this blocker first
> >> > > checks if vhost backend requires shared log. This will avoid a
> >> > > situation where a blocker is added inappropriately (e.g. shared
> >> > > log allocation fails when vhost backend doesn't support it).
> >> >
> >> > Sounds like a bugfix but I'm not sure. Can this part be split
> >> > out in a patch by itself?
> >>
> >> Already sent some days ago (and pointed by Marc today).
> >>
> >> > > Commit: 35f9b6e added a fallback mechanism for systems not
> supporting
> >> > > memfd_create syscall (started being supported since 3.17).
> >> > >
> >> > > Backporting memfd_create might not be accepted for distros relying
> >> > > on older kernels. Nowadays there is no way for security driver
> >> > > to discover memfd filename to be created: <tmpdir>/memfd-XXXXXX.
> >> > >
> >> > > Also, because vhost log file descriptors can be passed to other
> >> > > processes, after discussion, we thought it is best to back mmap by
> >> > > using files that can be placed into a specific directory: this
> commit
> >> > > creates "vhostlog" argv parameter for such purpose. This will allow
> >> > > security drivers to operate on those files appropriately.
> >> > >
> >> > > Argv examples:
> >> > >
> >> > >     -netdev tap,id=net0,vhost=on
> >> > >     -netdev tap,id=net0,vhost=on,vhostlog=/tmp/guest.log
> >> > >     -netdev tap,id=net0,vhost=on,vhostlog=/tmp
> >> > >
> >> > > For vhost backends supporting shared logs, if vhostlog is
> non-existent,
> >> > > or a directory, random files are going to be created in the
> specified
> >> > > directory (or, for non-existent, in tmpdir). If vhostlog is
> specified,
> >> > > the filepath is always used when allocating vhost log files.
> >> >
> >> > When vhostlog is not specified, can we just use memfd as we did?
> >> >
> >>
> >> This was my approach on a "pastebin" example before this patch (in the
> >> discussion thread we had). Problem goes back to when vhost log file
> >> descriptor is shared with some vhost-user implementation - like the
> >> interface allows to - and the security driver labelling issue. IMO,
> >> yes, we could let vhostlog to specify a log file, and, if not
> >> specified, assume memfd is ok to be used.
> >>
> >> Please let me know if you - and Marc - want me to keep using memfd.
> >> I'll create the mmap-file tests and files in a different commit, like
> >> Marc has asked for, and will propose the patch again by the end of
> >> this week.
> >
> > I think that the best approach is to allow passing in the fd,
> > not the file path. If not passed, use memfd.
> >
> > --
> > MST
>
> --
Marc-André Lureau