Re: [Qemu-devel] [PATCH] util: secure memfd_create fallback mechanism

[Rafael David Tinoco]

> On Sep 27, 2016, at 05:36, Daniel P. Berrange <address@hidden> wrote:
> 
> On Tue, Sep 27, 2016 at 03:06:21AM +0000, Rafael David Tinoco wrote:
>> Commit: 35f9b6ef3acc9d0546c395a566b04e63ca84e302 added a fallback
>> mechanism for systems not supporting memfd_create syscall (started
>> being supported since 3.17).
> 
> This is really dubious code in general and IMHO should just
> be reverted.

There are numerous people relying on older kernels in openstack 
deployments - sometimes with specific drivers (ovswitch, dpdk, 
infiniband) holding kernel upgrades - but still in need of upgrading 
userland (e.g. newer releases). Having a fallback mechanism seems 
appropriate for those cases.

> 
> We have a golden rule that any time QEMU needs to be able to
> create a file on disk, then the path should be explicitly
> provided as a command line argument so that mgmt apps can
> control the location used.
> 
>> 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.
>> 
>> It is more appropriate to include UUID and/or VM names in the
>> temporary filename, allowing security driver rules to be applied
>> while maintaining the required unpredictability with mkstemp.
> 
> We should not have QEMU creating unpredictabile filenames in the
> first place - any filenames should be determined by libvirt
> explicitly.

Note that the filename, per se, is not as important as other files, 
since qemu won't provide it for being accessed by external programs, and,
deletes the file, while keeping the descriptor, right after its creation
(due to its nature, that is probably why it was created in /tmp).

Having libvirt to define a filename that would not be used for recent
kernels (> 3.17) and would exist for a fraction of second doesn't seem
right to me. 

> 
>> This change will allow libvirt to know exact memfd file to be created
>> for vhost log AND to create appropriate security rules to allow access
>> per instance (instead of a opened rule like <tmpdir>/memfd-*).
> 
> Even with this change it is bad - we don't want driver backends
> creating arbitrary files in the shared /tmp directory.

On the other hand, if we are creating a tmp file, like I said, I see 
benefit on having unpredictability (mkstemp), but providing predictable
parts to allow security driver to apply rules per instance basis 
(/tmp/memfd-UUID*, /tmp/memfd-VMname*). 

Looking forward to a decision so I can backport correct behaviour
(with or without memfd file).  

Thank you!

Best Regards,
Rafael