diff mbox series

qemu: don't reject virtiofs for qemu:///session

Message ID 632ce101c4a83a61a80ddbecb64e48d8e8d9d87c.1616776100.git.crobinso@redhat.com
State New
Headers show
Series qemu: don't reject virtiofs for qemu:///session | expand

Commit Message

Cole Robinson March 26, 2021, 4:34 p.m. UTC 
Currently libvirt rejects attempts to use virtiofs with
qemu:///session. Indeed virtiofs does not have a chance of working
with typical session usage, because virtiofsd needs multiple linux
capabilities to perform its job. The only way to do this with out
of the box qemu packaging is to run virtiofsd as root, so libvirtd
must run as root, so qemu:///system is required.

But it's possible that a custom environment could setuid or set
file capabilities on the virtiofsd binary. In this case qemu:///session
would work fine. This may be an option for kubevirt to help them
transition to using qemu:///session everywhere

Drop the two pieces that block virtiofs for qemu:///session. Attempts
to use virtiofs for stock qemu:///session will fail at qemu startup,
though it's a bit opaque:

error: Failed to start domain 'f32'
error: internal error: qemu unexpectedly closed the monitor: 2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: Failed to write msg. Wrote -1 instead of 12.
2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: vhost_dev_init failed: Operation not permitted

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
The SetUID/SetGID bits don't seem to be necessary for qemu:///system
usage AFAICT, but it's a bit tough to decode virSetUIDGIDWithCaps.
virtiofsd is meticulous about managing its capability set though

 src/qemu/qemu_validate.c | 7 +------
 src/qemu/qemu_virtiofs.c | 4 ----
 2 files changed, 1 insertion(+), 10 deletions(-)

-- 
2.30.2

Comments

Ján Tomko March 29, 2021, 10:52 a.m. UTC | #1 
On a Friday in 2021, Cole Robinson wrote:
>Currently libvirt rejects attempts to use virtiofs with

>qemu:///session. Indeed virtiofs does not have a chance of working

>with typical session usage, because virtiofsd needs multiple linux

>capabilities to perform its job. The only way to do this with out

>of the box qemu packaging is to run virtiofsd as root, so libvirtd

>must run as root, so qemu:///system is required.

>

>But it's possible that a custom environment could setuid or set

>file capabilities on the virtiofsd binary. In this case qemu:///session

>would work fine. This may be an option for kubevirt to help them

>transition to using qemu:///session everywhere

>

>Drop the two pieces that block virtiofs for qemu:///session. Attempts

>to use virtiofs for stock qemu:///session will fail at qemu startup,

>though it's a bit opaque:

>

>error: Failed to start domain 'f32'

>error: internal error: qemu unexpectedly closed the monitor: 2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: Failed to write msg. Wrote -1 instead of 12.

>2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: vhost_dev_init failed: Operation not permitted

>


That is not a friendly error message for regular users.

Some alternatives come to mind:
* XML element telling libvirt to ignore the check/do not set the UID.
   The downside is, that we usually do this via:
     <seclabel relabel='no'/>
   and the seclabel code makes my head hurt.
* Introduce a QEMU capability for this, that kubevirt could set via
   <qemu:capabilities>
   https://libvirt.org/drvqemu.html#xmlnsfeatures
* Introduce the capability to 50-qemu-virtiofsd.json
* Wait until the lockdown eases up and I finally post the patches
   for externally launched virtiofsd
   https://bugzilla.redhat.com/show_bug.cgi?id=1855789

>Signed-off-by: Cole Robinson <crobinso@redhat.com>

>---

>The SetUID/SetGID bits don't seem to be necessary for qemu:///system

>usage AFAICT, but it's a bit tough to decode virSetUIDGIDWithCaps.


I *think* the only difference without the two virCommandSet?ID calls
is that we error out if UID/GID 0 can't be set. But yes it's tough
to read.

Jano

>virtiofsd is meticulous about managing its capability set though

>

> src/qemu/qemu_validate.c | 7 +------

> src/qemu/qemu_virtiofs.c | 4 ----

> 2 files changed, 1 insertion(+), 10 deletions(-)

>

>diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c

>index 6043f974ce..d4079f6b67 100644

>--- a/src/qemu/qemu_validate.c

>+++ b/src/qemu/qemu_validate.c

>@@ -4053,7 +4053,7 @@ qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,

> static int

> qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,

>                               const virDomainDef *def,

>-                              virQEMUDriverPtr driver,

>+                              virQEMUDriverPtr driver G_GNUC_UNUSED,

>                               virQEMUCapsPtr qemuCaps)

> {

>     if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {

>@@ -4107,11 +4107,6 @@ qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,

>                            _("virtiofs does not yet support read-only mode"));

>             return -1;

>         }

>-        if (!driver->privileged) {

>-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

>-                           _("virtiofs is not yet supported in session mode"));

>-            return -1;

>-        }

>         if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {

>             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

>                            _("virtiofs only supports passthrough accessmode"));

>diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c

>index 2e239cad66..0bb4e3c0d1 100644

>--- a/src/qemu/qemu_virtiofs.c

>+++ b/src/qemu/qemu_virtiofs.c

>@@ -214,10 +214,6 @@ qemuVirtioFSStart(virLogManagerPtr logManager,

>     if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd)))

>         goto cleanup;

>

>-    /* so far only running as root is supported */

>-    virCommandSetUID(cmd, 0);

>-    virCommandSetGID(cmd, 0);

>-

>     virCommandSetPidFile(cmd, pidfile);

>     virCommandSetOutputFD(cmd, &logfd);

>     virCommandSetErrorFD(cmd, &logfd);

>-- 

>2.30.2

>
Michal Prívozník March 29, 2021, 2:21 p.m. UTC | #2 
On 3/29/21 12:52 PM, Ján Tomko wrote:
> On a Friday in 2021, Cole Robinson wrote:

>> Currently libvirt rejects attempts to use virtiofs with

>> qemu:///session. Indeed virtiofs does not have a chance of working

>> with typical session usage, because virtiofsd needs multiple linux

>> capabilities to perform its job. The only way to do this with out

>> of the box qemu packaging is to run virtiofsd as root, so libvirtd

>> must run as root, so qemu:///system is required.

>>

>> But it's possible that a custom environment could setuid or set

>> file capabilities on the virtiofsd binary. In this case qemu:///session

>> would work fine. This may be an option for kubevirt to help them

>> transition to using qemu:///session everywhere

>>

>> Drop the two pieces that block virtiofs for qemu:///session. Attempts

>> to use virtiofs for stock qemu:///session will fail at qemu startup,

>> though it's a bit opaque:

>>

>> error: Failed to start domain 'f32'

>> error: internal error: qemu unexpectedly closed the monitor: 

>> 2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device 

>> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: 

>> Failed to write msg. Wrote -1 instead of 12.

>> 2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device 

>> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0: 

>> vhost_dev_init failed: Operation not permitted

>>

> 

> That is not a friendly error message for regular users.

> 

> Some alternatives come to mind:

> * XML element telling libvirt to ignore the check/do not set the UID.

>    The downside is, that we usually do this via:

>      <seclabel relabel='no'/>

>    and the seclabel code makes my head hurt.


No, seclabel shouldn't be used for processes IMO. The fact that we use 
an external process should be hidden from user (as an implementation 
detail) and therefore libvirt should do all magic necessary so that it 
just works (TM). Of course it has capabilities to do so. Otherwise - I'd 
say we can expect users to configure all bits necessary.

Also, it might be misleading - does seclabel apply to the process or 
perms visible from inside the guest? Wouldn't it recursively chown() the 
directory exposed?

> * Introduce a QEMU capability for this, that kubevirt could set via

>    <qemu:capabilities>

>    https://libvirt.org/drvqemu.html#xmlnsfeatures


I'd rather not do this. Such capability would be useless, the only thing 
it would help with is nicer error message.

> * Introduce the capability to 50-qemu-virtiofsd.json

> * Wait until the lockdown eases up and I finally post the patches

>    for externally launched virtiofsd

>    https://bugzilla.redhat.com/show_bug.cgi?id=1855789


How are you dealing with this situation in your patches? I mean, what if 
externally launched virtiofs doesn't have enough permissions?

Michal
Cole Robinson March 29, 2021, 7:34 p.m. UTC | #3 
On 3/29/21 6:52 AM, Ján Tomko wrote:
> On a Friday in 2021, Cole Robinson wrote:

>> Currently libvirt rejects attempts to use virtiofs with

>> qemu:///session. Indeed virtiofs does not have a chance of working

>> with typical session usage, because virtiofsd needs multiple linux

>> capabilities to perform its job. The only way to do this with out

>> of the box qemu packaging is to run virtiofsd as root, so libvirtd

>> must run as root, so qemu:///system is required.

>>

>> But it's possible that a custom environment could setuid or set

>> file capabilities on the virtiofsd binary. In this case qemu:///session

>> would work fine. This may be an option for kubevirt to help them

>> transition to using qemu:///session everywhere

>>

>> Drop the two pieces that block virtiofs for qemu:///session. Attempts

>> to use virtiofs for stock qemu:///session will fail at qemu startup,

>> though it's a bit opaque:

>>

>> error: Failed to start domain 'f32'

>> error: internal error: qemu unexpectedly closed the monitor:

>> 2021-03-26T16:26:12.459293Z qemu-system-x86_64: -device

>> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0:

>> Failed to write msg. Wrote -1 instead of 12.

>> 2021-03-26T16:26:12.459317Z qemu-system-x86_64: -device

>> vhost-user-fs-pci,chardev=chr-vu-fs0,tag=/foovirtiofs,bus=pci.10,addr=0x0:

>> vhost_dev_init failed: Operation not permitted

>>

> 

> That is not a friendly error message for regular users.

> 

> Some alternatives come to mind:

> * XML element telling libvirt to ignore the check/do not set the UID.

>   The downside is, that we usually do this via:

>     <seclabel relabel='no'/>

>   and the seclabel code makes my head hurt.

> * Introduce a QEMU capability for this, that kubevirt could set via

>   <qemu:capabilities>

>   https://libvirt.org/drvqemu.html#xmlnsfeatures

> * Introduce the capability to 50-qemu-virtiofsd.json

> * Wait until the lockdown eases up and I finally post the patches

>   for externally launched virtiofsd

>   https://bugzilla.redhat.com/show_bug.cgi?id=1855789

> 


I don't have much of an opinion. The latter will be useful for CNV
someday but it will take some rearchitechting on their part

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>> The SetUID/SetGID bits don't seem to be necessary for qemu:///system

>> usage AFAICT, but it's a bit tough to decode virSetUIDGIDWithCaps.

> 

> I *think* the only difference without the two virCommandSet?ID calls

> is that we error out if UID/GID 0 can't be set. But yes it's tough

> to read.

> 


I just tested with the qemu_validate.c removal only, so SetUID still in
place. The error:

error: Failed to start domain 'f32'
error: internal error: process exited while connecting to monitor:
2021-03-29T19:11:41.217583Z qemu-system-x86_64: -chardev
socket,id=chr-vu-fs0,path=/home/crobinso/.config/libvirt/qemu/lib/domain-3-f32/fs0-fs.sock:
Failed to connect to
'/home/crobinso/.config/libvirt/qemu/lib/domain-3-f32/fs0-fs.sock':
Connection refused

virtiofsd log has:
libvirt:  error : internal error: cannot apply process capabilities -5

So seems like libvirt error reporting here needs some tweaks regardless.
Maybe if catching virtiofsd startup error is improved it will improve
the first error case as well.

- Cole
diff mbox series

Patch

diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
index 6043f974ce..d4079f6b67 100644
--- a/src/qemu/qemu_validate.c
+++ b/src/qemu/qemu_validate.c
@@ -4053,7 +4053,7 @@  qemuValidateDomainDeviceDefGraphics(const virDomainGraphicsDef *graphics,
 static int
 qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
                               const virDomainDef *def,
-                              virQEMUDriverPtr driver,
+                              virQEMUDriverPtr driver G_GNUC_UNUSED,
                               virQEMUCapsPtr qemuCaps)
 {
     if (fs->type != VIR_DOMAIN_FS_TYPE_MOUNT) {
@@ -4107,11 +4107,6 @@  qemuValidateDomainDeviceDefFS(virDomainFSDefPtr fs,
                            _("virtiofs does not yet support read-only mode"));
             return -1;
         }
-        if (!driver->privileged) {
-            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-                           _("virtiofs is not yet supported in session mode"));
-            return -1;
-        }
         if (fs->accessmode != VIR_DOMAIN_FS_ACCESSMODE_PASSTHROUGH) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("virtiofs only supports passthrough accessmode"));
diff --git a/src/qemu/qemu_virtiofs.c b/src/qemu/qemu_virtiofs.c
index 2e239cad66..0bb4e3c0d1 100644
--- a/src/qemu/qemu_virtiofs.c
+++ b/src/qemu/qemu_virtiofs.c
@@ -214,10 +214,6 @@  qemuVirtioFSStart(virLogManagerPtr logManager,
     if (!(cmd = qemuVirtioFSBuildCommandLine(cfg, fs, &fd)))
         goto cleanup;
 
-    /* so far only running as root is supported */
-    virCommandSetUID(cmd, 0);
-    virCommandSetGID(cmd, 0);
-
     virCommandSetPidFile(cmd, pidfile);
     virCommandSetOutputFD(cmd, &logfd);
     virCommandSetErrorFD(cmd, &logfd);