9:25 a.m.
The <interface type='bridge'> is working mostly because of a bad design
decision in Linux. Ideally, QEMU would run with an empty capability
bounding set and would not be able to do any privileged operation
(not even by running a helper program). This is not the case because
dropping capabilities from the bounding set requires a capability of its
own, CAP_SETPCAP; thus QEMU does *not* run with an empty bounding set if
invoked via qemu:///session.
This series lets libvirtd invoke the privileged helper program on its own,
which is a cleaner design that would work even if the above Linux bug
was not there. Also, this adds a <target dev='tap0'/> element to the
XML of an active domain using <interface type='bridge'>.
libvirt now needs to know the path to the helper (patch
3), and must not set permitted/effective capabilities on children when
running unprivileged (patches 1/2). Apart from this, the recvfd and
virCommand APIs make the task almost trivial.
Paolo Bonzini (5):
util: simplify virSetUIDGIDWithCaps
util: allow using virCommandAllowCap with setuid helpers
qemu_conf: add new configuration key bridge_helper
virnetdevtap: add virNetDevTapGetName
qemu: launch bridge helper from libvirtd
src/libvirt_private.syms | 1 +
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 8 +++
src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++----------
src/qemu/qemu_command.h | 1 -
src/qemu/qemu_conf.c | 3 +
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_hotplug.c | 25 +++----
src/qemu/test_libvirtd_qemu.aug.in | 1 +
src/util/virnetdevtap.c | 33 +++++++++
src/util/virnetdevtap.h | 3 +
src/util/virutil.c | 36 +++++++---
12 files changed, 183 insertions(+), 63 deletions(-)
--
1.8.1.4
9:25 a.m.
New subject: [libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps
The need_prctl variable is not really needed. If it is false,
capng_apply will be called twice with the same set, causing
a little extra work but no problem. This keeps the code a bit
simpler.
It is also clearer to invoke capng_apply(CAPNG_SELECT_BOUNDS)
separately, to make sure it is done while we have CAP_SETPCAP.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/util/virutil.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 42b4295..36e6cee 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -3003,7 +3003,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
{
int ii, capng_ret, ret = -1;
bool need_setgid = false, need_setuid = false;
- bool need_prctl = false, need_setpcap = false;
+ bool need_setpcap = false;
/* First drop all caps (unless the requested uid is "unchanged" or
* root and clearExistingCaps wasn't requested), then add back
@@ -3043,17 +3043,15 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
# endif
- need_prctl = capBits || need_setgid || need_setuid || need_setpcap;
-
/* Tell system we want to keep caps across uid change */
- if (need_prctl && prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
+ if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) {
virReportSystemError(errno, "%s",
_("prctl failed to set KEEPCAPS"));
goto cleanup;
}
/* Change to the temp capabilities */
- if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
+ if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot apply process capabilities %d"), capng_ret);
goto cleanup;
@@ -3063,12 +3061,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
goto cleanup;
/* Tell it we are done keeping capabilities */
- if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
+ if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
virReportSystemError(errno, "%s",
_("prctl failed to reset KEEPCAPS"));
goto cleanup;
}
+ /* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot
+ * do this if we failed to get the capability above, so ignore the
+ * return value.
+ */
+ capng_apply(CAPNG_SELECT_BOUNDS);
+
/* Drop the caps that allow setuid/gid (unless they were requested) */
if (need_setgid)
capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
@@ -3078,7 +3082,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
if (need_setpcap)
capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
- if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
+ if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot apply process capabilities %d"), capng_ret);
ret = -1;
--
1.8.1.4
5:36 p.m.
New subject: [libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps
On 03/25/2013 08:25 AM, Paolo Bonzini wrote:
The need_prctl variable is not really needed. If it is false,
capng_apply will be called twice with the same set, causing
a little extra work but no problem. This keeps the code a bit
simpler.
It is also clearer to invoke capng_apply(CAPNG_SELECT_BOUNDS)
separately, to make sure it is done while we have CAP_SETPCAP.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/util/virutil.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
/* Change to the temp capabilities */
- if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
+ if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
Beforehand, we limited both caps and bounding set, with an overlarge
set, now you are limiting just caps...
virReportError(VIR_ERR_INTERNAL_ERROR,
_("cannot apply process capabilities %d"), capng_ret);
goto cleanup;
@@ -3063,12 +3061,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
goto cleanup;
/* Tell it we are done keeping capabilities */
- if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
+ if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
virReportSystemError(errno, "%s",
_("prctl failed to reset KEEPCAPS"));
goto cleanup;
}
+ /* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot
+ * do this if we failed to get the capability above, so ignore the
+ * return value.
+ */
+ capng_apply(CAPNG_SELECT_BOUNDS);
...and then separately limiting bounds, but still while having an
overlarge set.
+
/* Drop the caps that allow setuid/gid (unless they were requested) */
if (need_setgid)
capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
@@ -3078,7 +3082,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
if (need_setpcap)
capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
Here, the set is now pruned to size...
- if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
+ if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
...but you are now only limiting caps, not the bounding set. Is that
correct?
Does this need to be considered for 1.0.4, or can we delay it to
post-release?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:29 a.m.
New subject: [libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps
On Wed, Mar 27, 2013 at 04:36:38PM -0600, Eric Blake wrote:
On 03/25/2013 08:25 AM, Paolo Bonzini wrote:
> The need_prctl variable is not really needed. If it is false,
> capng_apply will be called twice with the same set, causing
> a little extra work but no problem. This keeps the code a bit
> simpler.
>
> It is also clearer to invoke capng_apply(CAPNG_SELECT_BOUNDS)
> separately, to make sure it is done while we have CAP_SETPCAP.
>
> Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
> ---
> src/util/virutil.c | 18 +++++++++++-------
> 1 file changed, 11 insertions(+), 7 deletions(-)
> /* Change to the temp capabilities */
> - if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
> + if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
Beforehand, we limited both caps and bounding set, with an overlarge
set, now you are limiting just caps...
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot apply process capabilities %d"),
capng_ret);
> goto cleanup;
> @@ -3063,12 +3061,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long
long capBits,
> goto cleanup;
>
> /* Tell it we are done keeping capabilities */
> - if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> + if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> virReportSystemError(errno, "%s",
> _("prctl failed to reset KEEPCAPS"));
> goto cleanup;
> }
>
> + /* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot
> + * do this if we failed to get the capability above, so ignore the
> + * return value.
> + */
> + capng_apply(CAPNG_SELECT_BOUNDS);
...and then separately limiting bounds, but still while having an
overlarge set.
> +
> /* Drop the caps that allow setuid/gid (unless they were requested) */
> if (need_setgid)
> capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
> @@ -3078,7 +3082,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
> if (need_setpcap)
> capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
Here, the set is now pruned to size...
>
> - if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) <
0)) {
> + if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
...but you are now only limiting caps, not the bounding set. Is that
correct?
This method is derived from code in libcap-ng capng_change_id. Paulo's
changes actually make the libvirt code closer to what capng_change_id
does, so I think it is OK.
Does this need to be considered for 1.0.4, or can we delay it to
post-release?
I think we can wait for this whole series - it is really feature
work rather than critical bugfix.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
5:04 a.m.
New subject: [libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps
> /* Change to the temp capabilities */
> - if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
> + if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
Beforehand, we limited both caps and bounding set, with an overlarge
set, now you are limiting just caps...
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("cannot apply process capabilities %d"),
> capng_ret);
> goto cleanup;
> @@ -3063,12 +3061,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid,
> unsigned long long capBits,
> goto cleanup;
>
> /* Tell it we are done keeping capabilities */
> - if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> + if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> virReportSystemError(errno, "%s",
> _("prctl failed to reset KEEPCAPS"));
> goto cleanup;
> }
>
> + /* Set bounding set while we have CAP_SETPCAP. Unfortunately we cannot
> + * do this if we failed to get the capability above, so ignore the
> + * return value.
> + */
> + capng_apply(CAPNG_SELECT_BOUNDS);
...and then separately limiting bounds, but still while having an
overlarge set.
> +
> /* Drop the caps that allow setuid/gid (unless they were
> requested) */
> if (need_setgid)
> capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
> CAP_SETGID);
> @@ -3078,7 +3082,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid,
> unsigned long long capBits,
> if (need_setpcap)
> capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED,
> CAP_SETPCAP);
Here, the set is now pruned to size...
>
> - if (need_prctl && ((capng_ret =
> capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
> + if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
...but you are now only limiting caps, not the bounding set. Is that
correct?
Yes, the code after capng(CAPNG_SELECT_BOUNDS) does not affect the
bounding set.
Does this need to be considered for 1.0.4, or can we delay it to
post-release?
Post-release, absolutely.
Paolo
3:48 p.m.
New subject: [libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps
On 03/28/2013 04:04 AM, Paolo Bonzini wrote:
>> /* Change to the temp capabilities */
>> - if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
>> + if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
>
> Beforehand, we limited both caps and bounding set, with an overlarge
> set, now you are limiting just caps...
>
>> + /* Set bounding set while we have CAP_SETPCAP.
Unfortunately we cannot
>> + * do this if we failed to get the capability above, so ignore the
>> + * return value.
>> + */
>> + capng_apply(CAPNG_SELECT_BOUNDS);
>
> ...and then separately limiting bounds, but still while having an
> overlarge set.
>
>> capng_update(CAPNG_DROP,
CAPNG_EFFECTIVE|CAPNG_PERMITTED,
>> CAP_SETPCAP);
>
> Here, the set is now pruned to size...
>
>>
>> - if (need_prctl && ((capng_ret =
>> capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
>> + if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
>
> ...but you are now only limiting caps, not the bounding set. Is that
> correct?
Yes, the code after capng(CAPNG_SELECT_BOUNDS) does not affect the
bounding set.
Ah, I see now - the CAPNG_* flags to the second parameter of
capng_update did not alter CAPNG_BOUNDING_SET.
ACK; I've gone ahead and pushed this patch.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
9:25 a.m.
New subject: [libvirt] [PATCH 2/5] util: allow using virCommandAllowCap with setuid helpers
When running unprivileged, virSetUIDGIDWithCaps will fail because it
tries to add the requested capabilities to the permitted and effective
sets.
Detect this case, and invoke the child with cleared permitted and
effective sets. If it is a setuid program, it will get them.
Some care is needed also because you cannot drop capabilities from the
bounding set without CAP_SETPCAP. Because of that, ignore errors from
setting the bounding set.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/util/virutil.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 36e6cee..8104e89 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -3052,9 +3052,21 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
/* Change to the temp capabilities */
if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot apply process capabilities %d"), capng_ret);
- goto cleanup;
+ /* Failed. If we are running unprivileged, and the arguments make sense
+ * for this scenario, assume we're starting some kind of setuid helper:
+ * do not set any of capBits in the permitted or effective sets, and let
+ * the program get them on its own.
+ *
+ * (Too bad we cannot restrict the bounding set to the capabilities we
+ * would like the helper to have!).
+ */
+ if (getuid() > 0 && clearExistingCaps && !need_setuid
&& !need_setgid) {
+ capng_clear(CAPNG_SELECT_CAPS);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot apply process capabilities %d"),
capng_ret);
+ goto cleanup;
+ }
}
if (virSetUIDGID(uid, gid) < 0)
--
1.8.1.4
5:46 p.m.
New subject: [libvirt] [PATCH 2/5] util: allow using virCommandAllowCap with setuid helpers
On 03/25/2013 08:25 AM, Paolo Bonzini wrote:
When running unprivileged, virSetUIDGIDWithCaps will fail because it
tries to add the requested capabilities to the permitted and effective
sets.
Detect this case, and invoke the child with cleared permitted and
effective sets. If it is a setuid program, it will get them.
Some care is needed also because you cannot drop capabilities from the
bounding set without CAP_SETPCAP. Because of that, ignore errors from
setting the bounding set.
That seems like a kernel flaw - it makes sense that you can't _add_
capabilities without CAP_SETPCAP, but being unable to _drop_
capabilities without first acquiring a capability seems backwards. I
wonder if lkml would accept a patch that makes CAP_SETPCAP unnecessary
for the restriction case, and only require it for the case of gaining
capabilities. But regardless of that thought experiment, I agree that
the behavior is what it is in released kernels, so we must deal with it.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/util/virutil.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/util/virutil.c b/src/util/virutil.c
index 36e6cee..8104e89 100644
--- a/src/util/virutil.c
+++ b/src/util/virutil.c
@@ -3052,9 +3052,21 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long
capBits,
/* Change to the temp capabilities */
if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("cannot apply process capabilities %d"), capng_ret);
- goto cleanup;
+ /* Failed. If we are running unprivileged, and the arguments make sense
+ * for this scenario, assume we're starting some kind of setuid helper:
+ * do not set any of capBits in the permitted or effective sets, and let
+ * the program get them on its own.
+ *
+ * (Too bad we cannot restrict the bounding set to the capabilities we
+ * would like the helper to have!).
+ */
+ if (getuid() > 0 && clearExistingCaps && !need_setuid
&& !need_setgid) {
+ capng_clear(CAPNG_SELECT_CAPS);
+ } else {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("cannot apply process capabilities %d"),
capng_ret);
+ goto cleanup;
+ }
Hmm, this seems like we may want it for 1.0.4, but I still had questions
on 1/5 which needs to be applied first.
As written, the patch makes sense.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
5:09 a.m.
New subject: [libvirt] [PATCH 2/5] util: allow using virCommandAllowCap with setuid helpers
That seems like a kernel flaw - it makes sense that you can't
_add_
capabilities without CAP_SETPCAP, but being unable to _drop_
capabilities without first acquiring a capability seems backwards.
You cannot add capabilities to the bounding set at all. It is a
one-way street.
/me learned a lot of things while writing these two patches.
In fact, capng_apply(CAPNG_SELECT_BOUNDS) will never fail, but I
preferred to be conservative in patch 1 just in case this changes
in the future.
Hmm, this seems like we may want it for 1.0.4
I do not think so, there should not be any cases right now where
unprivileged libvirt calls a setuid helper.
Paolo
3:54 p.m.
New subject: [libvirt] [PATCH 2/5] util: allow using virCommandAllowCap with setuid helpers
On 03/27/2013 04:46 PM, Eric Blake wrote:
On 03/25/2013 08:25 AM, Paolo Bonzini wrote:
> When running unprivileged, virSetUIDGIDWithCaps will fail because it
> tries to add the requested capabilities to the permitted and effective
> sets.
>
> Detect this case, and invoke the child with cleared permitted and
> effective sets. If it is a setuid program, it will get them.
>
> Some care is needed also because you cannot drop capabilities from the
> bounding set without CAP_SETPCAP. Because of that, ignore errors from
> setting the bounding set.
As written, the patch makes sense.
ACK and pushed.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
2:26 a.m.
New subject: [libvirt] [PATCH 2/5] util: allow using virCommandAllowCap with setuid helpers
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Il 27/03/2013 23:46, Eric Blake ha scritto:
That seems like a kernel flaw - it makes sense that you can't
_add_ capabilities without CAP_SETPCAP, but being unable to _drop_
capabilities without first acquiring a capability seems backwards.
I wonder if lkml would accept a patch that makes CAP_SETPCAP
unnecessary for the restriction case, and only require it for the
case of gaining capabilities.
The worry here is that dropping _some_ caps but not all lets you
exploit untested error paths in suid binaries.
The solution could be to install libvirtd as suid-root and drop all
capabilities except CAP_SETPCAP when running unprivileged.
Alternatively, you could use file capabilities to the same effect.
Paolo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
iQIcBAEBAgAGBQJRcPGaAAoJEBvWZb6bTYbyYOQP/A8pZ1uDgwpeF23KMQdg2Rnl
5tt064KyjVs5NhQC8ntC1vulRQnEr0/qZt2NA7t4RiF3H2X9DVH5QmxMl6ELjxJI
/GjScuswYOP5cmdj1xHKAJdmEV76C5f5NDra1CSw9iooWOdvrYLBgEqIEEp89i2s
PKWN2GUbMesdFDgxf7mYaSqGYiIUI4UvON6bsPqJsuShyRvMsqffP+OQHbN7qNE3
O6TecxkpDFEnPJxOGKmemvLPDzTywPABaCJwozonB/xKzqWZ4w1EDB4czAlbhqVR
zxtbWXFKJmZ1D72wgyTLeXtstJJnnh6DG9eHxGRf04+jVRGtMk7kOgZ8FE+WrsZQ
VoAh8nAUI3kaZ9DfNPNuZ7Y7oT0venUWu5tCjnnO1hEMtFeSIfnP4h/BcV95wt+B
I7jYco0pxCkBvWrwEXGkgHxMr+LMDav6nxo2ES5ToJUUGUAH6J8yPF7XYzppJ92k
pFXueVMEIDgKZzRcuUyaaaDeZj8XQvGUN9I4s2x0CbRdQpOD+wdp0t10xoDXXeVw
1Sngs/yLptZqG5FY0g7Vt8Tnc21VrVShi2/dkacybjMCfHostQCzyLUfKR+4rQI1
oXvajO5lHpgnETLLdfo8Udjy90uWas09Hh940TKtA1dX6h8FVFyvplwm41HyMxnS
oHZNxdw01qc/OmpoGYuP
=Ob6t
-----END PGP SIGNATURE-----
9:25 a.m.
New subject: [libvirt] [PATCH 3/5] qemu_conf: add new configuration key bridge_helper
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 8 ++++++++
src/qemu/qemu_conf.c | 3 +++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
5 files changed, 14 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 91f5f77..61740a9 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -60,6 +60,7 @@ module Libvirtd_qemu =
let process_entry = str_entry "hugetlbfs_mount"
| bool_entry "clear_emulator_capabilities"
+ | str_entry "bridge_helper"
| bool_entry "set_process_name"
| int_entry "max_processes"
| int_entry "max_files"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index dd853c8..87bdf70 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -301,6 +301,14 @@
#hugetlbfs_mount = "/dev/hugepages"
+# Path to the setuid helper for creating tap devices. This executable
+# is used to create <source type='bridge'> interfaces when libvirtd is
+# running unprivileged. libvirt invokes the helper directly, instead
+# of using "-netdev bridge", for security reasons.
+#bridge_helper = "/usr/libexec/qemu-bridge-helper"
+
+
+
# If clear_emulator_capabilities is enabled, libvirt will drop all
# privileged capabilities of the QEmu/KVM emulator. This is enabled by
# default.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c2e2e10..f648fc3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -248,6 +248,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
}
}
#endif
+ cfg->bridgeHelperName = strdup("/usr/libexec/qemu-bridge-helper");
cfg->clearEmulatorCapabilities = true;
@@ -297,6 +298,7 @@ static void virQEMUDriverConfigDispose(void *obj)
VIR_FREE(cfg->hugetlbfsMount);
VIR_FREE(cfg->hugepagePath);
+ VIR_FREE(cfg->bridgeHelperName);
VIR_FREE(cfg->saveImageFormat);
VIR_FREE(cfg->dumpImageFormat);
@@ -509,6 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
GET_VALUE_BOOL("auto_start_bypass_cache", cfg->autoStartBypassCache);
GET_VALUE_STR("hugetlbfs_mount", cfg->hugetlbfsMount);
+ GET_VALUE_STR("bridge_helper", cfg->bridgeHelperName);
GET_VALUE_BOOL("mac_filter", cfg->macFilter);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index c5ddaad..666ac33 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -117,6 +117,7 @@ struct _virQEMUDriverConfig {
char *hugetlbfsMount;
char *hugepagePath;
+ char *bridgeHelperName;
bool macFilter;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 2892204..0aec997 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -49,6 +49,7 @@ module Test_libvirtd_qemu =
{ "auto_dump_bypass_cache" = "0" }
{ "auto_start_bypass_cache" = "0" }
{ "hugetlbfs_mount" = "/dev/hugepages" }
+{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
{ "clear_emulator_capabilities" = "1" }
{ "set_process_name" = "1" }
{ "max_processes" = "0" }
--
1.8.1.4
12:35 p.m.
New subject: [libvirt] [PATCH 3/5] qemu_conf: add new configuration key bridge_helper
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 8 ++++++++
src/qemu/qemu_conf.c | 3 +++
src/qemu/qemu_conf.h | 1 +
src/qemu/test_libvirtd_qemu.aug.in | 1 +
5 files changed, 14 insertions(+)
diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
index 91f5f77..61740a9 100644
--- a/src/qemu/libvirtd_qemu.aug
+++ b/src/qemu/libvirtd_qemu.aug
@@ -60,6 +60,7 @@ module Libvirtd_qemu =
let process_entry = str_entry "hugetlbfs_mount"
| bool_entry "clear_emulator_capabilities"
+ | str_entry "bridge_helper"
| bool_entry "set_process_name"
| int_entry "max_processes"
| int_entry "max_files"
diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
index dd853c8..87bdf70 100644
--- a/src/qemu/qemu.conf
+++ b/src/qemu/qemu.conf
@@ -301,6 +301,14 @@
#hugetlbfs_mount = "/dev/hugepages"
+# Path to the setuid helper for creating tap devices. This executable
+# is used to create <source type='bridge'> interfaces when libvirtd is
+# running unprivileged. libvirt invokes the helper directly, instead
+# of using "-netdev bridge", for security reasons.
+#bridge_helper = "/usr/libexec/qemu-bridge-helper"
+
+
Are we sure we want to allow this to be configured? That could lead to
some "interesting" troubleshooting incidents :-)
On the other hand, I guess the path to qemu itself is right there in the
domain config file, so how much worse could this be...
+
# If clear_emulator_capabilities is enabled, libvirt will drop all
# privileged capabilities of the QEmu/KVM emulator. This is enabled by
# default.
diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
index c2e2e10..f648fc3 100644
--- a/src/qemu/qemu_conf.c
+++ b/src/qemu/qemu_conf.c
@@ -248,6 +248,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
}
}
#endif
+ cfg->bridgeHelperName = strdup("/usr/libexec/qemu-bridge-helper");
cfg->clearEmulatorCapabilities = true;
@@ -297,6 +298,7 @@ static void virQEMUDriverConfigDispose(void *obj)
VIR_FREE(cfg->hugetlbfsMount);
VIR_FREE(cfg->hugepagePath);
+ VIR_FREE(cfg->bridgeHelperName);
VIR_FREE(cfg->saveImageFormat);
VIR_FREE(cfg->dumpImageFormat);
@@ -509,6 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
GET_VALUE_BOOL("auto_start_bypass_cache", cfg->autoStartBypassCache);
GET_VALUE_STR("hugetlbfs_mount", cfg->hugetlbfsMount);
+ GET_VALUE_STR("bridge_helper", cfg->bridgeHelperName);
GET_VALUE_BOOL("mac_filter", cfg->macFilter);
diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
index c5ddaad..666ac33 100644
--- a/src/qemu/qemu_conf.h
+++ b/src/qemu/qemu_conf.h
@@ -117,6 +117,7 @@ struct _virQEMUDriverConfig {
char *hugetlbfsMount;
char *hugepagePath;
+ char *bridgeHelperName;
bool macFilter;
diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in
index 2892204..0aec997 100644
--- a/src/qemu/test_libvirtd_qemu.aug.in
+++ b/src/qemu/test_libvirtd_qemu.aug.in
@@ -49,6 +49,7 @@ module Test_libvirtd_qemu =
{ "auto_dump_bypass_cache" = "0" }
{ "auto_start_bypass_cache" = "0" }
{ "hugetlbfs_mount" = "/dev/hugepages" }
+{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
{ "clear_emulator_capabilities" = "1" }
{ "set_process_name" = "1" }
{ "max_processes" = "0" }
ACK. (But I'd like at least one other ACK from someone else due to the
fact that this is polluting the config namespace with something we would
like to eventually eliminate.)
4:06 p.m.
New subject: [libvirt] [PATCH 3/5] qemu_conf: add new configuration key bridge_helper
On 04/18/2013 11:35 AM, Laine Stump wrote:
> +# Path to the setuid helper for creating tap devices. This
executable
> +# is used to create <source type='bridge'> interfaces when libvirtd
is
> +# running unprivileged. libvirt invokes the helper directly, instead
> +# of using "-netdev bridge", for security reasons.
> +#bridge_helper = "/usr/libexec/qemu-bridge-helper"
> +
> +
Are we sure we want to allow this to be configured? That could lead to
some "interesting" troubleshooting incidents :-)
About the only time it would be configured is if qemu is installed in an
alternate location.
On the other hand, I guess the path to qemu itself is right there in the
domain config file, so how much worse could this be...
Yeah, sometimes we've got to just trust the user to not be insane.
ACK. (But I'd like at least one other ACK from someone else due to the
fact that this is polluting the config namespace with something we would
like to eventually eliminate.)
Even if we add a way for libvirt to get the tap device without depending
on qemu's helper program, we'll have to leave the config item present
(so we don't reject an older .conf file as invalid), but we can then
ignore the entry at that point. I can live with this change going in,
so I agree with your ACK, and have pushed it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
4:44 a.m.
New subject: [libvirt] [PATCH 3/5] qemu_conf: add new configuration key bridge_helper
On Thu, Apr 18, 2013 at 01:35:51PM -0400, Laine Stump wrote:
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
> ---
> src/qemu/libvirtd_qemu.aug | 1 +
> src/qemu/qemu.conf | 8 ++++++++
> src/qemu/qemu_conf.c | 3 +++
> src/qemu/qemu_conf.h | 1 +
> src/qemu/test_libvirtd_qemu.aug.in | 1 +
> 5 files changed, 14 insertions(+)
>
> diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug
> index 91f5f77..61740a9 100644
> --- a/src/qemu/libvirtd_qemu.aug
> +++ b/src/qemu/libvirtd_qemu.aug
> @@ -60,6 +60,7 @@ module Libvirtd_qemu =
>
> let process_entry = str_entry "hugetlbfs_mount"
> | bool_entry "clear_emulator_capabilities"
> + | str_entry "bridge_helper"
> | bool_entry "set_process_name"
> | int_entry "max_processes"
> | int_entry "max_files"
> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
> index dd853c8..87bdf70 100644
> --- a/src/qemu/qemu.conf
> +++ b/src/qemu/qemu.conf
> @@ -301,6 +301,14 @@
> #hugetlbfs_mount = "/dev/hugepages"
>
>
> +# Path to the setuid helper for creating tap devices. This executable
> +# is used to create <source type='bridge'> interfaces when libvirtd
is
> +# running unprivileged. libvirt invokes the helper directly, instead
> +# of using "-netdev bridge", for security reasons.
> +#bridge_helper = "/usr/libexec/qemu-bridge-helper"
> +
> +
Are we sure we want to allow this to be configured? That could lead to
some "interesting" troubleshooting incidents :-)
On the other hand, I guess the path to qemu itself is right there in the
domain config file, so how much worse could this be...
> +
> # If clear_emulator_capabilities is enabled, libvirt will drop all
> # privileged capabilities of the QEmu/KVM emulator. This is enabled by
> # default.
> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
> index c2e2e10..f648fc3 100644
> --- a/src/qemu/qemu_conf.c
> +++ b/src/qemu/qemu_conf.c
> @@ -248,6 +248,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged)
> }
> }
> #endif
> + cfg->bridgeHelperName =
strdup("/usr/libexec/qemu-bridge-helper");
>
> cfg->clearEmulatorCapabilities = true;
>
> @@ -297,6 +298,7 @@ static void virQEMUDriverConfigDispose(void *obj)
>
> VIR_FREE(cfg->hugetlbfsMount);
> VIR_FREE(cfg->hugepagePath);
> + VIR_FREE(cfg->bridgeHelperName);
>
> VIR_FREE(cfg->saveImageFormat);
> VIR_FREE(cfg->dumpImageFormat);
> @@ -509,6 +511,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
> GET_VALUE_BOOL("auto_start_bypass_cache",
cfg->autoStartBypassCache);
>
> GET_VALUE_STR("hugetlbfs_mount", cfg->hugetlbfsMount);
> + GET_VALUE_STR("bridge_helper", cfg->bridgeHelperName);
>
> GET_VALUE_BOOL("mac_filter", cfg->macFilter);
>
> diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h
> index c5ddaad..666ac33 100644
> --- a/src/qemu/qemu_conf.h
> +++ b/src/qemu/qemu_conf.h
> @@ -117,6 +117,7 @@ struct _virQEMUDriverConfig {
>
> char *hugetlbfsMount;
> char *hugepagePath;
> + char *bridgeHelperName;
>
> bool macFilter;
>
> diff --git a/src/qemu/test_libvirtd_qemu.aug.in
b/src/qemu/test_libvirtd_qemu.aug.in
> index 2892204..0aec997 100644
> --- a/src/qemu/test_libvirtd_qemu.aug.in
> +++ b/src/qemu/test_libvirtd_qemu.aug.in
> @@ -49,6 +49,7 @@ module Test_libvirtd_qemu =
> { "auto_dump_bypass_cache" = "0" }
> { "auto_start_bypass_cache" = "0" }
> { "hugetlbfs_mount" = "/dev/hugepages" }
> +{ "bridge_helper" = "/usr/libexec/qemu-bridge-helper" }
> { "clear_emulator_capabilities" = "1" }
> { "set_process_name" = "1" }
> { "max_processes" = "0" }
ACK. (But I'd like at least one other ACK from someone else due to the
fact that this is polluting the config namespace with something we would
like to eventually eliminate.)
We have to support this really, since we need to cope with people
installing into non-/usr locations
ACK
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:25 a.m.
New subject: [libvirt] [PATCH 4/5] virnetdevtap: add virNetDevTapGetName
This will be used on a tap file descriptor returned by the bridge helper
to populate the <target> element, because the helper does not provide
the interface name.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virnetdevtap.c | 33 +++++++++++++++++++++++++++++++++
src/util/virnetdevtap.h | 3 +++
3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f241ec4..06085ba 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1515,6 +1515,7 @@ virNetDevOpenvswitchSetMigrateData;
virNetDevTapCreate;
virNetDevTapCreateInBridgePort;
virNetDevTapDelete;
+virNetDevTapGetName;
# util/virnetdevveth.h
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index a884de1..e9fddf1 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -44,6 +44,39 @@
#define VIR_FROM_THIS VIR_FROM_NONE
/**
+ * virNetDevTapGetName:
+ * @tapfd: a tun/tap file descriptor
+ * @ifname: a pointer that will receive the interface name
+ *
+ * Retrieve the interface name given a file descriptor for a tun/tap
+ * interface.
+ *
+ * Returns 0 if the interface name is successfully queried, -1 otherwise
+ */
+int
+virNetDevTapGetName(int tapfd, char **ifname)
+{
+#ifdef TUNGETIFF
+ struct ifreq ifr;
+
+ /* The kernel will always return -1 at this point.
+ * If TUNGETIFF is not implemented then errno == EBADFD.
+ */
+ if (ioctl(tapfd, TUNGETIFF, &ifr) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to query tap interface name"));
+ return -1;
+ }
+
+ *ifname = strdup(ifr.ifr_name);
+ return 0;
+#else
+ return -1;
+#endif
+}
+
+
+/**
* virNetDevProbeVnetHdr:
* @tapfd: a tun/tap file descriptor
*
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 980db61..6bfc80c 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -35,6 +35,9 @@ int virNetDevTapCreate(char **ifname,
int virNetDevTapDelete(const char *ifname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virNetDevTapGetName(int tapfd, char **ifname)
+ ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
typedef enum {
VIR_NETDEV_TAP_CREATE_NONE = 0,
/* Bring the interface up */
--
1.8.1.4
11:48 a.m.
New subject: [libvirt] [PATCH 4/5] virnetdevtap: add virNetDevTapGetName
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
This will be used on a tap file descriptor returned by the bridge
helper
to populate the <target> element, because the helper does not provide
the interface name.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virnetdevtap.c | 33 +++++++++++++++++++++++++++++++++
src/util/virnetdevtap.h | 3 +++
3 files changed, 37 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index f241ec4..06085ba 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1515,6 +1515,7 @@ virNetDevOpenvswitchSetMigrateData;
virNetDevTapCreate;
virNetDevTapCreateInBridgePort;
virNetDevTapDelete;
+virNetDevTapGetName;
# util/virnetdevveth.h
diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
index a884de1..e9fddf1 100644
--- a/src/util/virnetdevtap.c
+++ b/src/util/virnetdevtap.c
@@ -44,6 +44,39 @@
#define VIR_FROM_THIS VIR_FROM_NONE
/**
+ * virNetDevTapGetName:
+ * @tapfd: a tun/tap file descriptor
+ * @ifname: a pointer that will receive the interface name
+ *
+ * Retrieve the interface name given a file descriptor for a tun/tap
+ * interface.
+ *
+ * Returns 0 if the interface name is successfully queried, -1 otherwise
+ */
+int
+virNetDevTapGetName(int tapfd, char **ifname)
+{
+#ifdef TUNGETIFF
+ struct ifreq ifr;
+
+ /* The kernel will always return -1 at this point.
+ * If TUNGETIFF is not implemented then errno == EBADFD.
+ */
+ if (ioctl(tapfd, TUNGETIFF, &ifr) < 0) {
+ virReportSystemError(errno, "%s",
+ _("Unable to query tap interface name"));
+ return -1;
+ }
+
+ *ifname = strdup(ifr.ifr_name);
You need to check for OOM here. (actually there are patches pending
somewhere to replace strdup() with a VIR_STRDUP() or something like that
which does the error reporting for you, but you'll still need to return
-1 if it fails).
+ return 0;
+#else
+ return -1;
+#endif
+}
+
+
+/**
* virNetDevProbeVnetHdr:
* @tapfd: a tun/tap file descriptor
*
diff --git a/src/util/virnetdevtap.h b/src/util/virnetdevtap.h
index 980db61..6bfc80c 100644
--- a/src/util/virnetdevtap.h
+++ b/src/util/virnetdevtap.h
@@ -35,6 +35,9 @@ int virNetDevTapCreate(char **ifname,
int virNetDevTapDelete(const char *ifname)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;
+int virNetDevTapGetName(int tapfd, char **ifname)
+ ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK;
+
typedef enum {
VIR_NETDEV_TAP_CREATE_NONE = 0,
/* Bring the interface up */
9:25 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
<source type='bridge'> uses a helper application to do the necessary
TUN/TAP setup to use an existing network bridge, thus letting
unprivileged users use TUN/TAP interfaces.
However, libvirt should be preventing QEMU from running any setuid
programs at all, which would include this helper program. From
a security POV, any setuid helper needs to be run by libvirtd itself,
not QEMU.
This is what this patch does. libvirt now invokes the setuid helper,
gets the TAP fd and then passes it to QEMU in the normal manner.
The path to the helper is specified in qemu.conf.
As a small advantage, this adds a <target dev='tap0'/> element to the
XML of an active domain using <interface type='bridge'>.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++++++++-------------
src/qemu/qemu_command.h | 1 -
src/qemu/qemu_hotplug.c | 25 +++------
3 files changed, 106 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a0c278f..fa31102 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -28,12 +28,14 @@
#include "qemu_capabilities.h"
#include "qemu_bridge_filter.h"
#include "cpu/cpu.h"
+#include "passfd.h"
#include "viralloc.h"
#include "virlog.h"
#include "virarch.h"
#include "virerror.h"
#include "virutil.h"
#include "virfile.h"
+#include "virnetdev.h"
#include "virstring.h"
#include "viruuid.h"
#include "c-ctype.h"
@@ -46,6 +48,9 @@
#include "base64.h"
#include "device_conf.h"
#include "virstoragefile.h"
+#if defined(__linux__)
+# include <linux/capability.h>
+#endif
#include <sys/stat.h>
#include <fcntl.h>
@@ -193,6 +198,77 @@ error:
}
+/**
+ * qemuCreateInBridgePortWithHelper:
+ * @cfg: the configuration object in which the helper name is looked up
+ * @brname: the bridge name
+ * @ifname: the returned interface name
+ * @macaddr: the returned MAC address
+ * @tapfd: file descriptor return value for the new tap device
+ * @flags: OR of virNetDevTapCreateFlags:
+
+ * VIR_NETDEV_TAP_CREATE_VNET_HDR
+ * - Enable IFF_VNET_HDR on the tap device
+ *
+ * This function creates a new tap device on a bridge using an external
+ * helper. The final name for the bridge will be stored in @ifname.
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
+ const char *brname,
+ char **ifname,
+ int *tapfd,
+ unsigned int flags)
+{
+ virCommandPtr cmd;
+ int status;
+ int pair[2] = { -1, -1 };
+
+ if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP)
+ return -1;
+
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
+ virReportSystemError(errno, "%s", _("failed to create
socket"));
+ return -1;
+ }
+
+ cmd = virCommandNew(cfg->bridgeHelperName);
+ if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR)
+ virCommandAddArgFormat(cmd, "--use-vnet");
+ virCommandAddArgFormat(cmd, "--br=%s", brname);
+ virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
+ virCommandTransferFD(cmd, pair[1]);
+ virCommandClearCaps(cmd);
+#ifdef CAP_NET_ADMIN
+ virCommandAllowCap(cmd, CAP_NET_ADMIN);
+#endif
+ if (virCommandRunAsync(cmd, NULL) < 0) {
+ *tapfd = -1;
+ goto out;
+ }
+
+ do {
+ *tapfd = recvfd(pair[0], 0);
+ } while (*tapfd < 0 && errno == EINTR);
+ if (*tapfd < 0) {
+ virReportSystemError(errno, "%s",
+ _("failed to retrieve file descriptor for
interface"));
+ goto out;
+ }
+
+ if (virNetDevTapGetName(*tapfd, ifname) < 0 ||
+ virCommandWait(cmd, &status) < 0) {
+ VIR_FORCE_CLOSE(*tapfd);
+ *tapfd = -1;
+ }
+
+out:
+ virCommandFree(cmd);
+ VIR_FORCE_CLOSE(pair[0]);
+ return *tapfd < 0 ? -1 : 0;
+}
+
int
qemuNetworkIfaceConnect(virDomainDefPtr def,
virConnectPtr conn,
@@ -270,11 +346,17 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
}
- err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
- def->uuid, &tapfd,
- virDomainNetGetActualVirtPortProfile(net),
- virDomainNetGetActualVlan(net),
- tap_create_flags);
+ if (cfg->privileged)
+ err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
+ def->uuid, &tapfd,
+ virDomainNetGetActualVirtPortProfile(net),
+ virDomainNetGetActualVlan(net),
+ tap_create_flags);
+ else
+ err = qemuCreateInBridgePortWithHelper(cfg, brname,
+ &net->ifname,
+ &tapfd, tap_create_flags);
+
virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
if (err < 0) {
if (template_ifname)
@@ -3746,7 +3828,6 @@ error:
char *
qemuBuildHostNetStr(virDomainNetDefPtr net,
virQEMUDriverPtr driver,
- virQEMUCapsPtr qemuCaps,
char type_sep,
int vlan,
const char *tapfd,
@@ -3755,7 +3836,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
bool is_tap = false;
virBuffer buf = VIR_BUFFER_INITIALIZER;
enum virDomainNetType netType = virDomainNetGetActualType(net);
- const char *brname = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
@@ -3773,14 +3853,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
* through, -net tap,fd
*/
case VIR_DOMAIN_NET_TYPE_BRIDGE:
- if (!cfg->privileged &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) {
- brname = virDomainNetGetActualBridgeName(net);
- virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep, brname);
- type_sep = ',';
- is_tap = true;
- break;
- }
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_DIRECT:
virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd);
@@ -6681,26 +6753,17 @@ qemuBuildCommandLine(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
- /*
- * If type='bridge' then we attempt to allocate the tap fd here
only if
- * running under a privilged user or -netdev bridge option is not
- * supported.
- */
- if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- cfg->privileged ||
- (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
- int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
- qemuCaps);
- if (tapfd < 0)
- goto error;
+ int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
+ qemuCaps);
+ if (tapfd < 0)
+ goto error;
- last_good_net = i;
- virCommandTransferFD(cmd, tapfd);
+ last_good_net = i;
+ virCommandTransferFD(cmd, tapfd);
- if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
- tapfd) >= sizeof(tapfd_name))
- goto no_memory;
- }
+ if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
+ tapfd) >= sizeof(tapfd_name))
+ goto no_memory;
} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
int tapfd = qemuPhysIfaceConnect(def, driver, net,
qemuCaps, vmop);
@@ -6744,7 +6807,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
virCommandAddArg(cmd, "-netdev");
- if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
+ if (!(host = qemuBuildHostNetStr(net, driver,
',', vlan, tapfd_name,
vhostfd_name)))
goto error;
@@ -6768,7 +6831,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
virCommandAddArg(cmd, "-net");
- if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
+ if (!(host = qemuBuildHostNetStr(net, driver,
',', vlan, tapfd_name,
vhostfd_name)))
goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 17687f4..267a96e 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -71,7 +71,6 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial,
/* With vlan == -1, use netdev syntax, else old hostnet */
char * qemuBuildHostNetStr(virDomainNetDefPtr net,
virQEMUDriverPtr driver,
- virQEMUCapsPtr qemuCaps,
char type_sep,
int vlan,
const char *tapfd,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de9edd4..6891efd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -729,21 +729,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
- /*
- * If type=bridge then we attempt to allocate the tap fd here only if
- * running under a privilged user or -netdev bridge option is not
- * supported.
- */
- if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- cfg->privileged ||
- (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
- if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
- priv->qemuCaps)) < 0)
- goto cleanup;
- iface_connected = true;
- if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) <
0)
- goto cleanup;
- }
+ if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
+ priv->qemuCaps)) < 0)
+ goto cleanup;
+ iface_connected = true;
+ if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0)
+ goto cleanup;
} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net,
priv->qemuCaps,
@@ -803,12 +794,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
- if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
+ if (!(netstr = qemuBuildHostNetStr(net, driver,
',', -1, tapfd_name,
vhostfd_name)))
goto cleanup;
} else {
- if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
+ if (!(netstr = qemuBuildHostNetStr(net, driver,
' ', vlan, tapfd_name,
vhostfd_name)))
goto cleanup;
--
1.8.1.4
12:32 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
<source type='bridge'> uses a helper application to do
the necessary
TUN/TAP setup to use an existing network bridge, thus letting
unprivileged users use TUN/TAP interfaces.
However, libvirt should be preventing QEMU from running any setuid
programs at all, which would include this helper program. From
a security POV, any setuid helper needs to be run by libvirtd itself,
not QEMU.
This is what this patch does. libvirt now invokes the setuid helper,
gets the TAP fd and then passes it to QEMU in the normal manner.
The path to the helper is specified in qemu.conf.
As a small advantage, this adds a <target dev='tap0'/> element to the
XML of an active domain using <interface type='bridge'>.
Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
---
src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++++++++-------------
src/qemu/qemu_command.h | 1 -
src/qemu/qemu_hotplug.c | 25 +++------
3 files changed, 106 insertions(+), 53 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index a0c278f..fa31102 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -28,12 +28,14 @@
#include "qemu_capabilities.h"
#include "qemu_bridge_filter.h"
#include "cpu/cpu.h"
+#include "passfd.h"
#include "viralloc.h"
#include "virlog.h"
#include "virarch.h"
#include "virerror.h"
#include "virutil.h"
#include "virfile.h"
+#include "virnetdev.h"
#include "virstring.h"
#include "viruuid.h"
#include "c-ctype.h"
@@ -46,6 +48,9 @@
#include "base64.h"
#include "device_conf.h"
#include "virstoragefile.h"
+#if defined(__linux__)
+# include <linux/capability.h>
+#endif
#include <sys/stat.h>
#include <fcntl.h>
@@ -193,6 +198,77 @@ error:
}
+/**
+ * qemuCreateInBridgePortWithHelper:
+ * @cfg: the configuration object in which the helper name is looked up
+ * @brname: the bridge name
+ * @ifname: the returned interface name
+ * @macaddr: the returned MAC address
+ * @tapfd: file descriptor return value for the new tap device
+ * @flags: OR of virNetDevTapCreateFlags:
+
+ * VIR_NETDEV_TAP_CREATE_VNET_HDR
+ * - Enable IFF_VNET_HDR on the tap device
+ *
+ * This function creates a new tap device on a bridge using an external
+ * helper. The final name for the bridge will be stored in @ifname.
+ *
+ * Returns 0 in case of success or -1 on failure
+ */
+static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
+ const char *brname,
+ char **ifname,
+ int *tapfd,
+ unsigned int flags)
+{
+ virCommandPtr cmd;
+ int status;
+ int pair[2] = { -1, -1 };
+
+ if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) != VIR_NETDEV_TAP_CREATE_IFUP)
+ return -1;
+
+ if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
+ virReportSystemError(errno, "%s", _("failed to create
socket"));
+ return -1;
+ }
+
+ cmd = virCommandNew(cfg->bridgeHelperName);
+ if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR)
+ virCommandAddArgFormat(cmd, "--use-vnet");
+ virCommandAddArgFormat(cmd, "--br=%s", brname);
+ virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
+ virCommandTransferFD(cmd, pair[1]);
+ virCommandClearCaps(cmd);
+#ifdef CAP_NET_ADMIN
+ virCommandAllowCap(cmd, CAP_NET_ADMIN);
I thought you had said that attempts to set caps would fail because
CAP_SETPCAP was cleared for the non-privileged libvirtd.
+#endif
+ if (virCommandRunAsync(cmd, NULL) < 0) {
+ *tapfd = -1;
+ goto out;
+ }
+
+ do {
+ *tapfd = recvfd(pair[0], 0);
+ } while (*tapfd < 0 && errno == EINTR);
+ if (*tapfd < 0) {
+ virReportSystemError(errno, "%s",
+ _("failed to retrieve file descriptor for
interface"));
+ goto out;
+ }
+
+ if (virNetDevTapGetName(*tapfd, ifname) < 0 ||
+ virCommandWait(cmd, &status) < 0) {
+ VIR_FORCE_CLOSE(*tapfd);
+ *tapfd = -1;
+ }
+
+out:
We prefer to name these labels "cleanup" rather that "out" (although
there are some occurrences of "out" still in the code).
+ virCommandFree(cmd);
+ VIR_FORCE_CLOSE(pair[0]);
+ return *tapfd < 0 ? -1 : 0;
+}
+
int
qemuNetworkIfaceConnect(virDomainDefPtr def,
virConnectPtr conn,
@@ -270,11 +346,17 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
}
- err = virNetDevTapCreateInBridgePort(brname, &net->ifname, &net->mac,
- def->uuid, &tapfd,
- virDomainNetGetActualVirtPortProfile(net),
- virDomainNetGetActualVlan(net),
- tap_create_flags);
+ if (cfg->privileged)
+ err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
+ def->uuid, &tapfd,
+ virDomainNetGetActualVirtPortProfile(net),
+ virDomainNetGetActualVlan(net),
+ tap_create_flags);
+ else
+ err = qemuCreateInBridgePortWithHelper(cfg, brname,
+ &net->ifname,
+ &tapfd, tap_create_flags);
+
virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
if (err < 0) {
if (template_ifname)
@@ -3746,7 +3828,6 @@ error:
char *
qemuBuildHostNetStr(virDomainNetDefPtr net,
virQEMUDriverPtr driver,
- virQEMUCapsPtr qemuCaps,
qemuCaps might again become useful for this function in the future, so
you may want to leave it here (marked as ATTRIBUTE_UNUSED) to reduce
code churn.
char type_sep,
int vlan,
const char *tapfd,
@@ -3755,7 +3836,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
bool is_tap = false;
virBuffer buf = VIR_BUFFER_INITIALIZER;
enum virDomainNetType netType = virDomainNetGetActualType(net);
- const char *brname = NULL;
virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
@@ -3773,14 +3853,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
* through, -net tap,fd
*/
case VIR_DOMAIN_NET_TYPE_BRIDGE:
- if (!cfg->privileged &&
- virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) {
- brname = virDomainNetGetActualBridgeName(net);
- virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep, brname);
- type_sep = ',';
- is_tap = true;
- break;
- }
case VIR_DOMAIN_NET_TYPE_NETWORK:
case VIR_DOMAIN_NET_TYPE_DIRECT:
virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd);
@@ -6681,26 +6753,17 @@ qemuBuildCommandLine(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
- /*
- * If type='bridge' then we attempt to allocate the tap fd here
only if
- * running under a privilged user or -netdev bridge option is not
- * supported.
- */
- if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- cfg->privileged ||
- (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
- int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
- qemuCaps);
- if (tapfd < 0)
- goto error;
+ int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
+ qemuCaps);
+ if (tapfd < 0)
+ goto error;
- last_good_net = i;
- virCommandTransferFD(cmd, tapfd);
+ last_good_net = i;
+ virCommandTransferFD(cmd, tapfd);
- if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
- tapfd) >= sizeof(tapfd_name))
- goto no_memory;
- }
+ if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
+ tapfd) >= sizeof(tapfd_name))
+ goto no_memory;
} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
int tapfd = qemuPhysIfaceConnect(def, driver, net,
qemuCaps, vmop);
@@ -6744,7 +6807,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
virCommandAddArg(cmd, "-netdev");
- if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
+ if (!(host = qemuBuildHostNetStr(net, driver,
',', vlan, tapfd_name,
vhostfd_name)))
goto error;
@@ -6768,7 +6831,7 @@ qemuBuildCommandLine(virConnectPtr conn,
if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
virCommandAddArg(cmd, "-net");
- if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
+ if (!(host = qemuBuildHostNetStr(net, driver,
',', vlan, tapfd_name,
vhostfd_name)))
goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
index 17687f4..267a96e 100644
--- a/src/qemu/qemu_command.h
+++ b/src/qemu/qemu_command.h
@@ -71,7 +71,6 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial,
/* With vlan == -1, use netdev syntax, else old hostnet */
char * qemuBuildHostNetStr(virDomainNetDefPtr net,
virQEMUDriverPtr driver,
- virQEMUCapsPtr qemuCaps,
char type_sep,
int vlan,
const char *tapfd,
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index de9edd4..6891efd 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -729,21 +729,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
- /*
- * If type=bridge then we attempt to allocate the tap fd here only if
- * running under a privilged user or -netdev bridge option is not
- * supported.
- */
- if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
- cfg->privileged ||
- (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
- if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
- priv->qemuCaps)) < 0)
- goto cleanup;
- iface_connected = true;
- if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) <
0)
- goto cleanup;
- }
+ if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
+ priv->qemuCaps)) < 0)
+ goto cleanup;
+ iface_connected = true;
+ if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) < 0)
+ goto cleanup;
} else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net,
priv->qemuCaps,
@@ -803,12 +794,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
- if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
+ if (!(netstr = qemuBuildHostNetStr(net, driver,
',', -1, tapfd_name,
vhostfd_name)))
goto cleanup;
} else {
- if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
+ if (!(netstr = qemuBuildHostNetStr(net, driver,
' ', vlan, tapfd_name,
vhostfd_name)))
goto cleanup;
I still don't like using qemu-bridge-helper, but this is better than the
alternative of having qemu call it (although, due to the way that
process capabilities works, we are unable to prevent a rogue qemu
started by unprivileged libvirtd from calling it :-(
ACK to this patch (I think I would prefer you left the qemuCaps arg in,
but others may disagree with me.)
4:12 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
On 04/18/2013 11:32 AM, Laine Stump wrote:
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
> <source type='bridge'> uses a helper application to do the necessary
> TUN/TAP setup to use an existing network bridge, thus letting
> unprivileged users use TUN/TAP interfaces.
>
> @@ -3746,7 +3828,6 @@ error:
> char *
> qemuBuildHostNetStr(virDomainNetDefPtr net,
> virQEMUDriverPtr driver,
> - virQEMUCapsPtr qemuCaps,
qemuCaps might again become useful for this function in the future, so
you may want to leave it here (marked as ATTRIBUTE_UNUSED) to reduce
code churn.
For that matter, do we need a qemu capability bit that says whether the
bridge helper even exists, or do you give a nice error message to
someone using qemu:///session on a machine with too-old qemu without the
need for a capability bit??
I still don't like using qemu-bridge-helper, but this is better than the
alternative of having qemu call it (although, due to the way that
process capabilities works, we are unable to prevent a rogue qemu
started by unprivileged libvirtd from calling it :-(
ACK to this patch (I think I would prefer you left the qemuCaps arg in,
but others may disagree with me.)
I haven't pushed 4 or 5 (4 needed an OOM fix, and there's the question
of whether a capability bit is still useful). I don't have a strong
preference whether to leave qemuCaps in; we have version control to
inspect if we need to figure out how to add it back in later. I prefer
to limit ATTRIBUTE_UNUSED markers to functions that have a required
signature (generally, callbacks to be called from some other generic
code site); but this doesn't feel like we have any reason to be locked
into leaving qemuCaps in the interface.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
8:47 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
[snip]
I still don't like using qemu-bridge-helper, but this is better than the
alternative of having qemu call it (although, due to the way that
process capabilities works, we are unable to prevent a rogue qemu
started by unprivileged libvirtd from calling it :-(
Maybe we can introduce a tighter seccomp sandbox environment that
doesn't allow the QEMU process to call exec(), open(), socket() (and
anything else?) on top of the syscalls that are already not included in
the -sandbox whitelist. This would require fd's to be passed from
libvirt. Eduardo's going to work on adding functionality in this area
in case you have any suggestions.
--
Regards,
Corey Bryant
ACK to this patch (I think I would prefer you left the qemuCaps arg in,
but others may disagree with me.)
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
8:51 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
On Fri, Apr 19, 2013 at 09:47:05AM -0400, Corey Bryant wrote:
[snip]
>
>I still don't like using qemu-bridge-helper, but this is better than the
>alternative of having qemu call it (although, due to the way that
>process capabilities works, we are unable to prevent a rogue qemu
>started by unprivileged libvirtd from calling it :-(
Maybe we can introduce a tighter seccomp sandbox environment that
doesn't allow the QEMU process to call exec(), open(), socket() (and
anything else?) on top of the syscalls that are already not included
in the -sandbox whitelist. This would require fd's to be passed
from libvirt. Eduardo's going to work on adding functionality in
this area in case you have any suggestions.
I'd certainly like to see us have a profile that prevents execve() in
the near future. Already today there's no reason why a QEMU managed by
libvirt should need to exec anything. Eventually we'll get to the place
where we can consider blocking open/socket too, but I fear that's still
quite a way off in the distance.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
9:05 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
On 04/19/2013 09:51 AM, Daniel P. Berrange wrote:
On Fri, Apr 19, 2013 at 09:47:05AM -0400, Corey Bryant wrote:
>
> [snip]
>>
>> I still don't like using qemu-bridge-helper, but this is better than the
>> alternative of having qemu call it (although, due to the way that
>> process capabilities works, we are unable to prevent a rogue qemu
>> started by unprivileged libvirtd from calling it :-(
>
> Maybe we can introduce a tighter seccomp sandbox environment that
> doesn't allow the QEMU process to call exec(), open(), socket() (and
> anything else?) on top of the syscalls that are already not included
> in the -sandbox whitelist. This would require fd's to be passed
> from libvirt. Eduardo's going to work on adding functionality in
> this area in case you have any suggestions.
I'd certainly like to see us have a profile that prevents execve() in
the near future. Already today there's no reason why a QEMU managed by
libvirt should need to exec anything. Eventually we'll get to the place
where we can consider blocking open/socket too, but I fear that's still
quite a way off in the distance.
Daniel
Thanks for the input. The problem we run into next is that QEMU already
calls execve() so I assume we'd need to support 2 seccomp syscall
whitelists, the existing whitelist that allows execve() and another that
doesn't. Perhaps we can have a -sandbox,strict that ends up being a
much more locked down environment than the current default. Or perhaps
we can allow libvirt to define and pass a whitelist to QEMU.
--
Regards,
Corey Bryant
9:11 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
On Fri, Apr 19, 2013 at 10:05:33AM -0400, Corey Bryant wrote:
On 04/19/2013 09:51 AM, Daniel P. Berrange wrote:
>On Fri, Apr 19, 2013 at 09:47:05AM -0400, Corey Bryant wrote:
>>
>>[snip]
>>>
>>>I still don't like using qemu-bridge-helper, but this is better than the
>>>alternative of having qemu call it (although, due to the way that
>>>process capabilities works, we are unable to prevent a rogue qemu
>>>started by unprivileged libvirtd from calling it :-(
>>
>>Maybe we can introduce a tighter seccomp sandbox environment that
>>doesn't allow the QEMU process to call exec(), open(), socket() (and
>>anything else?) on top of the syscalls that are already not included
>>in the -sandbox whitelist. This would require fd's to be passed
>>from libvirt. Eduardo's going to work on adding functionality in
>>this area in case you have any suggestions.
>
>I'd certainly like to see us have a profile that prevents execve() in
>the near future. Already today there's no reason why a QEMU managed by
>libvirt should need to exec anything. Eventually we'll get to the place
>where we can consider blocking open/socket too, but I fear that's still
>quite a way off in the distance.
>
>Daniel
>
Thanks for the input. The problem we run into next is that QEMU
already calls execve() so I assume we'd need to support 2 seccomp
syscall whitelists, the existing whitelist that allows execve() and
another that doesn't. Perhaps we can have a -sandbox,strict that
ends up being a much more locked down environment than the current
default. Or perhaps we can allow libvirt to define and pass a
whitelist to QEMU.
I think it is inevitable that we will need to have multiple seccomp
whitelist profiles, because we can't assume that everyone who wants
to use seccomp with QEMU, will also be using libvirt. Minimising the
number of profiles we need to maintain though is key, perhaps to just
2 or 3 profiles. One that is fairly permissive to allow all QEMU
features to 'just work', and one+ that is strict requiring apps to
avoid certain features in favour of FD passing.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4:13 p.m.
New subject: [libvirt] [PATCH 5/5] qemu: launch bridge helper from libvirtd
Il 18/04/2013 19:32, Laine Stump ha scritto:
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
> <source type='bridge'> uses a helper application to do the necessary
> TUN/TAP setup to use an existing network bridge, thus letting
> unprivileged users use TUN/TAP interfaces.
>
> However, libvirt should be preventing QEMU from running any setuid
> programs at all, which would include this helper program. From
> a security POV, any setuid helper needs to be run by libvirtd itself,
> not QEMU.
>
> This is what this patch does. libvirt now invokes the setuid helper,
> gets the TAP fd and then passes it to QEMU in the normal manner.
> The path to the helper is specified in qemu.conf.
>
> As a small advantage, this adds a <target dev='tap0'/> element to the
> XML of an active domain using <interface type='bridge'>.
>
> Signed-off-by: Paolo Bonzini <pbonzini(a)redhat.com>
> ---
> src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++++++++++-------------
> src/qemu/qemu_command.h | 1 -
> src/qemu/qemu_hotplug.c | 25 +++------
> 3 files changed, 106 insertions(+), 53 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a0c278f..fa31102 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -28,12 +28,14 @@
> #include "qemu_capabilities.h"
> #include "qemu_bridge_filter.h"
> #include "cpu/cpu.h"
> +#include "passfd.h"
> #include "viralloc.h"
> #include "virlog.h"
> #include "virarch.h"
> #include "virerror.h"
> #include "virutil.h"
> #include "virfile.h"
> +#include "virnetdev.h"
> #include "virstring.h"
> #include "viruuid.h"
> #include "c-ctype.h"
> @@ -46,6 +48,9 @@
> #include "base64.h"
> #include "device_conf.h"
> #include "virstoragefile.h"
> +#if defined(__linux__)
> +# include <linux/capability.h>
> +#endif
>
> #include <sys/stat.h>
> #include <fcntl.h>
> @@ -193,6 +198,77 @@ error:
> }
>
>
> +/**
> + * qemuCreateInBridgePortWithHelper:
> + * @cfg: the configuration object in which the helper name is looked up
> + * @brname: the bridge name
> + * @ifname: the returned interface name
> + * @macaddr: the returned MAC address
> + * @tapfd: file descriptor return value for the new tap device
> + * @flags: OR of virNetDevTapCreateFlags:
> +
> + * VIR_NETDEV_TAP_CREATE_VNET_HDR
> + * - Enable IFF_VNET_HDR on the tap device
> + *
> + * This function creates a new tap device on a bridge using an external
> + * helper. The final name for the bridge will be stored in @ifname.
> + *
> + * Returns 0 in case of success or -1 on failure
> + */
> +static int qemuCreateInBridgePortWithHelper(virQEMUDriverConfigPtr cfg,
> + const char *brname,
> + char **ifname,
> + int *tapfd,
> + unsigned int flags)
> +{
> + virCommandPtr cmd;
> + int status;
> + int pair[2] = { -1, -1 };
> +
> + if ((flags & ~VIR_NETDEV_TAP_CREATE_VNET_HDR) !=
VIR_NETDEV_TAP_CREATE_IFUP)
> + return -1;
> +
> + if (socketpair(AF_UNIX, SOCK_STREAM, 0, pair) < 0) {
> + virReportSystemError(errno, "%s", _("failed to create
socket"));
> + return -1;
> + }
> +
> + cmd = virCommandNew(cfg->bridgeHelperName);
> + if (flags & VIR_NETDEV_TAP_CREATE_VNET_HDR)
> + virCommandAddArgFormat(cmd, "--use-vnet");
> + virCommandAddArgFormat(cmd, "--br=%s", brname);
> + virCommandAddArgFormat(cmd, "--fd=%d", pair[1]);
> + virCommandTransferFD(cmd, pair[1]);
> + virCommandClearCaps(cmd);
> +#ifdef CAP_NET_ADMIN
> + virCommandAllowCap(cmd, CAP_NET_ADMIN);
I thought you had said that attempts to set caps would fail because
CAP_SETPCAP was cleared for the non-privileged libvirtd.
I still prefer to note down the capabilities. This way running libvirt
with CAP_SETPCAP as a permitted file capability (and only that one + the
effective bit set) should do the right thing.
I tested this by forcing this path for the system libvirt.
> +#endif
> + if (virCommandRunAsync(cmd, NULL) < 0) {
> + *tapfd = -1;
> + goto out;
> + }
> +
> + do {
> + *tapfd = recvfd(pair[0], 0);
> + } while (*tapfd < 0 && errno == EINTR);
> + if (*tapfd < 0) {
> + virReportSystemError(errno, "%s",
> + _("failed to retrieve file descriptor for
interface"));
> + goto out;
> + }
> +
> + if (virNetDevTapGetName(*tapfd, ifname) < 0 ||
> + virCommandWait(cmd, &status) < 0) {
> + VIR_FORCE_CLOSE(*tapfd);
> + *tapfd = -1;
> + }
> +
> +out:
We prefer to name these labels "cleanup" rather that "out" (although
there are some occurrences of "out" still in the code).
Ok.
Paolo
> + virCommandFree(cmd);
> + VIR_FORCE_CLOSE(pair[0]);
> + return *tapfd < 0 ? -1 : 0;
> +}
> +
> int
> qemuNetworkIfaceConnect(virDomainDefPtr def,
> virConnectPtr conn,
> @@ -270,11 +346,17 @@ qemuNetworkIfaceConnect(virDomainDefPtr def,
> tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
> }
>
> - err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
> - def->uuid, &tapfd,
> - virDomainNetGetActualVirtPortProfile(net),
> - virDomainNetGetActualVlan(net),
> - tap_create_flags);
> + if (cfg->privileged)
> + err = virNetDevTapCreateInBridgePort(brname, &net->ifname,
&net->mac,
> + def->uuid, &tapfd,
> +
virDomainNetGetActualVirtPortProfile(net),
> + virDomainNetGetActualVlan(net),
> + tap_create_flags);
> + else
> + err = qemuCreateInBridgePortWithHelper(cfg, brname,
> + &net->ifname,
> + &tapfd, tap_create_flags);
> +
> virDomainAuditNetDevice(def, net, "/dev/net/tun", tapfd >= 0);
> if (err < 0) {
> if (template_ifname)
> @@ -3746,7 +3828,6 @@ error:
> char *
> qemuBuildHostNetStr(virDomainNetDefPtr net,
> virQEMUDriverPtr driver,
> - virQEMUCapsPtr qemuCaps,
qemuCaps might again become useful for this function in the future, so
you may want to leave it here (marked as ATTRIBUTE_UNUSED) to reduce
code churn.
> char type_sep,
> int vlan,
> const char *tapfd,
> @@ -3755,7 +3836,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> bool is_tap = false;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> enum virDomainNetType netType = virDomainNetGetActualType(net);
> - const char *brname = NULL;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>
> if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
> @@ -3773,14 +3853,6 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
> * through, -net tap,fd
> */
> case VIR_DOMAIN_NET_TYPE_BRIDGE:
> - if (!cfg->privileged &&
> - virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE)) {
> - brname = virDomainNetGetActualBridgeName(net);
> - virBufferAsprintf(&buf, "bridge%cbr=%s", type_sep,
brname);
> - type_sep = ',';
> - is_tap = true;
> - break;
> - }
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> case VIR_DOMAIN_NET_TYPE_DIRECT:
> virBufferAsprintf(&buf, "tap%cfd=%s", type_sep, tapfd);
> @@ -6681,26 +6753,17 @@ qemuBuildCommandLine(virConnectPtr conn,
>
> if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> actualType == VIR_DOMAIN_NET_TYPE_BRIDGE) {
> - /*
> - * If type='bridge' then we attempt to allocate the tap fd
here only if
> - * running under a privilged user or -netdev bridge option is not
> - * supported.
> - */
> - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - cfg->privileged ||
> - (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
> - int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
> - qemuCaps);
> - if (tapfd < 0)
> - goto error;
> + int tapfd = qemuNetworkIfaceConnect(def, conn, driver, net,
> + qemuCaps);
> + if (tapfd < 0)
> + goto error;
>
> - last_good_net = i;
> - virCommandTransferFD(cmd, tapfd);
> + last_good_net = i;
> + virCommandTransferFD(cmd, tapfd);
>
> - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> - tapfd) >= sizeof(tapfd_name))
> - goto no_memory;
> - }
> + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d",
> + tapfd) >= sizeof(tapfd_name))
> + goto no_memory;
> } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> int tapfd = qemuPhysIfaceConnect(def, driver, net,
> qemuCaps, vmop);
> @@ -6744,7 +6807,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) {
> virCommandAddArg(cmd, "-netdev");
> - if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
> + if (!(host = qemuBuildHostNetStr(net, driver,
> ',', vlan, tapfd_name,
> vhostfd_name)))
> goto error;
> @@ -6768,7 +6831,7 @@ qemuBuildCommandLine(virConnectPtr conn,
> if (!(virQEMUCapsGet(qemuCaps, QEMU_CAPS_NETDEV) &&
> virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE))) {
> virCommandAddArg(cmd, "-net");
> - if (!(host = qemuBuildHostNetStr(net, driver, qemuCaps,
> + if (!(host = qemuBuildHostNetStr(net, driver,
> ',', vlan, tapfd_name,
> vhostfd_name)))
> goto error;
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 17687f4..267a96e 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -71,7 +71,6 @@ qemuBuildChrDeviceStr (virDomainChrDefPtr serial,
> /* With vlan == -1, use netdev syntax, else old hostnet */
> char * qemuBuildHostNetStr(virDomainNetDefPtr net,
> virQEMUDriverPtr driver,
> - virQEMUCapsPtr qemuCaps,
> char type_sep,
> int vlan,
> const char *tapfd,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index de9edd4..6891efd 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -729,21 +729,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>
> if (actualType == VIR_DOMAIN_NET_TYPE_BRIDGE ||
> actualType == VIR_DOMAIN_NET_TYPE_NETWORK) {
> - /*
> - * If type=bridge then we attempt to allocate the tap fd here only if
> - * running under a privilged user or -netdev bridge option is not
> - * supported.
> - */
> - if (actualType == VIR_DOMAIN_NET_TYPE_NETWORK ||
> - cfg->privileged ||
> - (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV_BRIDGE))) {
> - if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> - priv->qemuCaps)) < 0)
> - goto cleanup;
> - iface_connected = true;
> - if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd)
< 0)
> - goto cleanup;
> - }
> + if ((tapfd = qemuNetworkIfaceConnect(vm->def, conn, driver, net,
> + priv->qemuCaps)) < 0)
> + goto cleanup;
> + iface_connected = true;
> + if (qemuOpenVhostNet(vm->def, net, priv->qemuCaps, &vhostfd) <
0)
> + goto cleanup;
> } else if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
> if ((tapfd = qemuPhysIfaceConnect(vm->def, driver, net,
> priv->qemuCaps,
> @@ -803,12 +794,12 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>
> if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_NETDEV) &&
> virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE)) {
> - if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
> + if (!(netstr = qemuBuildHostNetStr(net, driver,
> ',', -1, tapfd_name,
> vhostfd_name)))
> goto cleanup;
> } else {
> - if (!(netstr = qemuBuildHostNetStr(net, driver, priv->qemuCaps,
> + if (!(netstr = qemuBuildHostNetStr(net, driver,
> ' ', vlan, tapfd_name,
> vhostfd_name)))
> goto cleanup;
I still don't like using qemu-bridge-helper, but this is better than the
alternative of having qemu call it (although, due to the way that
process capabilities works, we are unable to prevent a rogue qemu
started by unprivileged libvirtd from calling it :-(
ACK to this patch (I think I would prefer you left the qemuCaps arg in,
but others may disagree with me.)
2:30 p.m.
On 03/25/2013 10:25 AM, Paolo Bonzini wrote:
The <interface type='bridge'> is working mostly because
of a bad design
decision in Linux. Ideally, QEMU would run with an empty capability
bounding set and would not be able to do any privileged operation
(not even by running a helper program). This is not the case because
dropping capabilities from the bounding set requires a capability of its
own, CAP_SETPCAP; thus QEMU does *not* run with an empty bounding set if
invoked via qemu:///session.
Ewww. So what you're saying is that the qemu that's run from
qemu:///system is more locked down (and thus "more secure") than the
qemu that's run from qemu:///session? Basically this qemu can run any
setuid application it likes, and there's nothing that we can do about it.
12:37 a.m.
Il 28/03/2013 20:30, Laine Stump ha scritto:
> The <interface type='bridge'> is working mostly
because of a bad design
> decision in Linux. Ideally, QEMU would run with an empty capability
> bounding set and would not be able to do any privileged operation
> (not even by running a helper program). This is not the case because
> dropping capabilities from the bounding set requires a capability of its
> own, CAP_SETPCAP; thus QEMU does *not* run with an empty bounding set if
> invoked via qemu:///session.
Ewww. So what you're saying is that the qemu that's run from
qemu:///system is more locked down (and thus "more secure") than the
qemu that's run from qemu:///session? Basically this qemu can run any
setuid application it likes, and there's nothing that we can do about it.
Yes. However, seccompv2 can still prevent execve to be executed by qemu.
Paolo
12:36 a.m.
Il 25/03/2013 15:25, Paolo Bonzini ha scritto:
The <interface type='bridge'> is working mostly because
of a bad design
decision in Linux. Ideally, QEMU would run with an empty capability
bounding set and would not be able to do any privileged operation
(not even by running a helper program). This is not the case because
dropping capabilities from the bounding set requires a capability of its
own, CAP_SETPCAP; thus QEMU does *not* run with an empty bounding set if
invoked via qemu:///session.
This series lets libvirtd invoke the privileged helper program on its own,
which is a cleaner design that would work even if the above Linux bug
was not there. Also, this adds a <target dev='tap0'/> element to the
XML of an active domain using <interface type='bridge'>.
libvirt now needs to know the path to the helper (patch
3), and must not set permitted/effective capabilities on children when
running unprivileged (patches 1/2). Apart from this, the recvfd and
virCommand APIs make the task almost trivial.
Paolo Bonzini (5):
util: simplify virSetUIDGIDWithCaps
util: allow using virCommandAllowCap with setuid helpers
qemu_conf: add new configuration key bridge_helper
virnetdevtap: add virNetDevTapGetName
qemu: launch bridge helper from libvirtd
src/libvirt_private.syms | 1 +
src/qemu/libvirtd_qemu.aug | 1 +
src/qemu/qemu.conf | 8 +++
src/qemu/qemu_command.c | 133 +++++++++++++++++++++++++++----------
src/qemu/qemu_command.h | 1 -
src/qemu/qemu_conf.c | 3 +
src/qemu/qemu_conf.h | 1 +
src/qemu/qemu_hotplug.c | 25 +++----
src/qemu/test_libvirtd_qemu.aug.in | 1 +
src/util/virnetdevtap.c | 33 +++++++++
src/util/virnetdevtap.h | 3 +
src/util/virutil.c | 36 +++++++---
12 files changed, 183 insertions(+), 63 deletions(-)
Ping?
Paolo
4235
days inactive
4260
days old
27 comments
5 participants
participants (5)
-
Corey Bryant -
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Paolo Bonzini