[Qemu-devel] [Bug 1626972] Fwd: [PATCH] vhost: secure vhost shared log files using argv paremeter

[Rafael David Tinoco]

> Begin forwarded message:
> 
> From: Rafael David Tinoco 
> Subject: Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using 
> argv paremeter
> Date: October 22, 2016 at 19:52:31 GMT-2
> To: Marc-André Lureau 
> Cc: Rafael David Tinoco , qemu-devel 
> 
> 
> Hello,
> 
>> On Oct 22, 2016, at 05:18, Marc-André Lureau  
>> wrote:
>> 
>> Hi
>> 
>> On Sat, Oct 22, 2016 at 10:01 AM 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).
>> 
>> Could you make this a seperate patch?
> 
> Just did, in another e-mail, cc'ing you.
> 
>> 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
>> 
>> Could it be only a filename? This would simplify testing.
> 
> It could. Should I keep the /tmp/ logic if no vhostlog arg is present 
> ? Or you think it should fail if no arg is given ? I'm afraid of backward 
> compatibility when back-porting this to older qemu versions on stable 
> releases (like my case: I'll backport this to ~3 different versions). 
> 
>> 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.
>> 
>> 
>> Regarding testing, you add utility code mmap-file, could you make this a 
>> seperate commit, with unit tests?
>> 
> 
> Sure, I'll work on it.
> 
>> thanks
> 
> Thank u!
> 
> -Rafael Tinoco

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1626972

Title:
  QEMU memfd_create fallback mechanism change for security drivers

Status in QEMU:
  In Progress

Bug description:
  And, when libvirt starts using apparmor, and creating apparmor
  profiles for every virtual machine created in the compute nodes,
  mitaka qemu (2.5 - and upstream also) uses a fallback mechanism for
  creating shared memory for live-migrations. This fall back mechanism,
  on kernels 3.13 - that don't have memfd_create() system-call, try to
  create files on /tmp/ directory and fails.. causing live-migration not
  to work.

  Trusty with kernel 3.13 + Mitaka with qemu 2.5 + apparmor capability =
  can't live migrate.

  From qemu 2.5, logic is on :

  void *qemu_memfd_alloc(const char *name, size_t size, unsigned int seals, int 
*fd)
  {
  if (memfd_create)... ### only works with HWE kernels

  else ### 3.13 kernels, gets blocked by apparmor
 tmpdir = g_get_tmp_dir
 ...
 mfd = mkstemp(fname)
  }

  And you can see the errors:

  From the host trying to send the virtual machine:

  2016-08-15 16:36:26.160 1974 ERROR nova.virt.libvirt.driver 
[req-0cac612b-8d53-4610-b773-d07ad6bacb91 691a581cfa7046278380ce82b1c38ddd 
133ebc3585c041aebaead8c062cd6511 - - -] [instance: 
2afa1131-bc8c-43d2-9c4a-962c1bf7723e] Migration operation has aborted
  2016-08-15 16:36:26.248 1974 ERROR nova.virt.libvirt.driver 
[req-0cac612b-8d53-4610-b773-d07ad6bacb91 691a581cfa7046278380ce82b1c38ddd 
133ebc3585c041aebaead8c062cd6511 - - -] [instance: 
2afa1131-bc8c-43d2-9c4a-962c1bf7723e] Live Migration failure: internal error: 
unable to execute QEMU command 'migrate': Migration disabled: failed to 
allocate shared memory

  From the host trying to receive the virtual machine:

  Aug 15 16:36:19 tkcompute01 kernel: [ 1194.356794] type=1400 
audit(1471289779.791:72): apparmor="STATUS" operation="profile_load" 
profile="unconfined" name="libvirt-2afa1131-bc8c-43d2-9c4a-962c1bf7723e" 
pid=12565 comm="apparmor_parser"
  Aug 15 16:36:19 tkcompute01 kernel: [ 1194.357048] type=1400 
audit(1471289779.791:73): apparmor="STATUS" operation="profile_load" 
profile="unconfined" name="qemu_bridge_helper" pid=12565 comm="apparmor_parser"
  Aug 15 16:36:20 tkcompute01 kernel: [ 1194.877027] type=1400 
audit(1471289780.311:74): apparmor="STATUS" operation="profile_replace" 
profile="unconfined" name="libvirt-2afa1131-bc8c-43d2-9c4a-962c1bf7723e" 
pid=12613 comm="apparmor_parser"
  Aug 15 16:36:20 tkcompute01 kernel: [ 1194.904407] type=1400 
audit(1471289780.343:75): apparmor="STATUS" operation="profile_replace" 
profile="unconfined" name="qemu_bridge_helper" pid=12613 comm="apparmor_parser"
  Aug 15 16:36:20 tkcompute01 kernel: [ 1194.973064] type=1400 
audit(1471289780.407:76): apparmor="DENIED" operation="mknod" 
profile="libvirt-2afa1131-bc8c-43d2-9c4a-962c1bf7723e" name="/tmp/memfd-tNpKSj" 
pid=12625 comm="qemu-system-x86" requested_mask="c" denied_mask="c" fsuid=107 
ouid=107
  Aug 15 16:36:20 tkcompute01 

[Qemu-devel] [Bug 1626972] Fwd: [PATCH] vhost: secure vhost shared log files using argv paremeter

[Rafael David Tinoco]

> Begin forwarded message:
> 
> From: Marc-André Lureau 
> Subject: Re: [Qemu-devel] [PATCH] vhost: secure vhost shared log files using 
> argv paremeter
> Date: October 22, 2016 at 05:18:02 GMT-2
> To: Rafael David Tinoco 
> Cc: QEMU 
> 
> Hi
> 
> On Sat, Oct 22, 2016 at 10:01 AM Rafael David Tinoco 
> mailto:rafael.tin...@canonical.com>> 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).
> 
> Could you make this a seperate patch?
>  
> 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: /memfd-XX.
> 
> 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
> 
> Could it be only a filename? This would simplify testing.
>  
> 
> 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.
> 
> 
> Regarding testing, you add utility code mmap-file, could you make this a 
> seperate commit, with unit tests?
> 
> thanks
> 
> Signed-off-by: Rafael David Tinoco  >
> ---
>  hw/net/vhost_net.c|   4 +-
>  hw/scsi/vhost-scsi.c  |   2 +-
>  hw/virtio/vhost-vsock.c   |   2 +-
>  hw/virtio/vhost.c |  41 +++--
>  include/hw/virtio/vhost.h |   4 +-
>  include/net/vhost_net.h   |   1 +
>  include/qemu/mmap-file.h  |  10 +++
>  net/tap.c |   6 ++
>  qapi-schema.json  |   3 +
>  qemu-options.hx   |   3 +-
>  util/Makefile.objs|   1 +
>  util/mmap-file.c  | 153 
> ++
>  12 files changed, 207 insertions(+), 23 deletions(-)
>  create mode 100644 include/qemu/mmap-file.h
>  create mode 100644 util/mmap-file.c
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index f2d49ad..d650c92 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -171,8 +171,8 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
>  net->dev.vq_index = net->nc->queue_index * net->dev.nvqs;
>  }
> 
> -r = vhost_dev_init(&net->dev, options->opaque,
> -   options->backend_type, options->busyloop_timeout);
> +r = vhost_dev_init(&net->dev, options->opaque, options->backend_type,
> +   options->busyloop_timeout, options->vhostlog);
>  if (r < 0) {
>  goto fail;
>  }
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 5b26946..5dc3d30 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -248,7 +248,7 @@ static void vhost_scsi_realize(DeviceState *dev, Error 
> **errp)
>  s->dev.backend_features = 0;
> 
>  ret = vhost_dev_init(&s->dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, NULL);
>  if (ret < 0) {
>  error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> strerror(-ret));
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index b481562..6cf6081 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -342,7 +342,7 @@ static void vhost_vsock_device_realize(DeviceState *dev, 
> Error **errp)
>  vsock->vhost_dev.nvqs = ARRAY_SIZE(vsock->vhost_vqs);
>  vsock->vhost_dev.vqs = vsock->vhost_vqs;
>  ret = vhost_dev_init(&vsock->vhost_dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, NULL);
>  if (ret < 0) {
>  error_setg_errno(errp, -ret, "vhost-vsock: vhost_dev_init failed");
>  goto err_virtio;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index bd051ab..d874ebb 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -20,7 +20,7 @@
>  #include "qemu/atomic.h"
>  #include "qemu/range.h"
>  #include "qemu/error-report.h"
> -#include "qemu/memfd.h"
> +#include "