On Mon, 2009-10-05 at 13:19 +0100, Daniel P. Berrange wrote:
> On Fri, Oct 02, 2009 at 03:48:36PM +0200, Gerhard Stenzel wrote:
> > This patch adds MAC address based port filtering to the qemu driver.
> >
> > Signed-off-by: Gerhard Stenzel <gerhard.stenzel@xxxxxxxxxx>
> > ---
> >
> > src/qemu/qemu.conf | 3 +++
> > src/qemu/qemu_conf.c | 14 ++++++++++++++
> > src/qemu/qemu_conf.h | 2 ++
> > src/qemu/qemu_driver.c | 23 +++++++++++++++++++++++
> > 4 files changed, 42 insertions(+), 0 deletions(-)
>
>
> > @@ -1193,6 +1197,16 @@ qemudNetworkIfaceConnect(virConnectPtr conn,
> > tapfd = -1;
> > }
> >
> > + if (driver->macFilter) {
> > + virNetworkPtr network = virNetworkLookupByName(conn,
> > + net->data.network.name);
> > + if ((err = virNetworkAllowMacOnPort(network, brname, net->ifname, net->mac))) {
> > + virReportSystemError(conn, err,
> > + _("failed to add ebtables rule to allow MAC address on '%s'"),
> > + net->ifname);
> > + }
> > + }
>
> This will crash in a large number of scenarios, since it is
> only valid to deference net->data.network fields once you
> have verified net->type == VIR_DOMAIN_NET_TYPE_NETWORK. It
> is also failing to check for virNetworkLookupByName() returning
> NULL.
>
> This is why the MAC filtering should not be part of the
> virNetwork API set. The QEMU driver should be directly
> calling the ebtables APIs you added in patch 1, rather
> then indirectly via virNetwork.. This would allow this
> MAC filtering to work with bridged network modes too.
>
> > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> > index 155e4a3..a95c867 100644
> > --- a/src/qemu/qemu_driver.c
> > +++ b/src/qemu/qemu_driver.c
> > @@ -239,6 +239,14 @@ qemudAutostartConfigs(struct qemud_driver *driver) {
> > }
> > virDomainObjUnlock(vm);
> > }
> > + if (qemu_driver->macFilter) {
> > + fprintf(stderr,"%s:%s:%d macFilter=%d\n", __FILE__, __FUNCTION__, __LINE__, qemu_driver->macFilter);
>
> There's a VIR_DEBUG() macro available for logging.
>
>
> > + if ((errno = virNetworkDisableAllFrames(conn))) {
> > + virReportSystemError(conn, errno,
> > + _("failed to add rule to drop all frames in '%s'"), __FILE__);
> > + }
> > + }
> > +
> > qemuDriverUnlock(driver);
> >
> > if (conn)
> > @@ -2167,8 +2175,23 @@ cleanup:
> > static void qemudShutdownVMDaemon(virConnectPtr conn,
> > struct qemud_driver *driver,
> > virDomainObjPtr vm) {
> > +
> > int ret;
> > int retries = 0;
> > + char *brname;
> > +
> > + virDomainNetDefPtr net = vm->def->nets[0];
>
> This assumes the guest has exactly one NIC - it'll crash if there
> are no NICs, and it'll miss cleanup steps if there are multiple NICs
>
> > + virNetworkPtr network = virNetworkLookupByName(conn,
> > + net->data.network.name);
> > + brname = virNetworkGetBridgeName(network);
> > +
> > + if (driver->macFilter) {
> > + if ((errno = virNetworkDisallowMacOnPort(network, brname, net->ifname, net->mac))) {
> > + virReportSystemError(conn, errno,
> > + _("failed to add ebtables rule to allow MAC address on '%s'"),
> > + net->ifname);
> > + }
> > + }
>
> Same comment as before about not using virNetwork for any of this
>
> Regards,
> Daniel
Thanks for the feedback so far.
I know there is more to be done like handling multiple NICs and some
more error checking, but is the following more in the direction you
would like to have it?
Index: libvirt/src/qemu/qemu_conf.c
===================================================================
--- libvirt.orig/src/qemu/qemu_conf.c
+++ libvirt/src/qemu/qemu_conf.c
@@ -37,6 +37,7 @@
#include <sys/utsname.h>
#include <mntent.h>
+#include "ebtables.h"
#include "c-ctype.h"
#include "virterror_internal.h"
#include "qemu_conf.h"
@@ -318,6 +319,10 @@ int qemudLoadDriverConfig(struct qemud_d
}
}
+ p = virConfGetValue (conf, "mac_filter");
+ CHECK_TYPE ("mac_filter", VIR_CONF_LONG);
+ if (p) driver->macFilter = p->l;
+
virConfFree (conf);
return 0;
}
@@ -1118,6 +1123,38 @@ int qemudExtractVersion(virConnectPtr co
}
+static int
+networkAllowMacOnPort(virConnectPtr conn,
+ struct qemud_driver *driver,
+ char * ifname,
+ unsigned char * mac) {
+
+ int err;
+ char *macaddr;
+
+ if (virAsprintf(&macaddr,
+ "%02x:%02x:%02x:%02x:%02x:%02x",
+ mac[0], mac[1],
+ mac[2], mac[3],
+ mac[4], mac[5]) < 0) {
+ virReportOOMError(conn);
+ return -1;
+ }
+ /* allow this combination of macaddr and ifname */
+
+ ebtablesContext * ebtablescontext = driver->ebtables;
+ if ((err = ebtablesAddForwardAllowIn(ebtablescontext,
+ ifname,
+ macaddr))) {
+ virReportSystemError(conn, err,
+ _("failed to add ebtables rule to allow
routing to '%s'"),
+ ifname);
+ }
+
+ return 0;
+}
+
+
int
qemudNetworkIfaceConnect(virConnectPtr conn,
struct qemud_driver *driver,
@@ -1193,6 +1230,14 @@ qemudNetworkIfaceConnect(virConnectPtr c
tapfd = -1;
}
+ if (driver->macFilter) {
+ if ((err = networkAllowMacOnPort(conn, driver, net->ifname,
net->mac))) {
+ virReportSystemError(conn, err,
+ _("failed to add ebtables rule to
allow MAC address on '%s'"),
+ net->ifname);
+ }
+ }
+
cleanup:
VIR_FREE(brname);
Index: libvirt/src/qemu/qemu_driver.c
===================================================================
--- libvirt.orig/src/qemu/qemu_driver.c
+++ libvirt/src/qemu/qemu_driver.c
@@ -50,6 +50,7 @@
#include <sched.h>
#endif
+#include "ebtables.h"
#include "virterror_internal.h"
#include "logging.h"
#include "datatypes.h"
@@ -200,6 +201,74 @@ qemudLogReadFD(virConnectPtr conn, const
return fd;
}
+static int
+networkAddEbtablesRules(virConnectPtr conn,
+ struct qemud_driver *driver) {
+ int err;
+
+ /* Set forward policy to DROP */
+ if ((err = ebtablesAddForwardPolicyReject(driver->ebtables))) {
+ virReportSystemError(conn, err,
+ _("failed to add ebtables rule to set
default policy to drop on '%s'"),
+ __FILE__);
+ return err;
+ }
+ ebtablesSaveRules(driver->ebtables);
+
+ return 0;
+}
+
+static int
+networkDisableAllFrames(virConnectPtr conn,
+ struct qemud_driver *driver) {
+ int err;
+
+ /* Set forward policy to DROP */
+ if ((err = !networkAddEbtablesRules(conn, driver))) {
+ virReportSystemError(conn, err,
+ _("cannot filter mac addresses on bridge
'%s'"),
+ __FILE__);
+ return err;
+ }
+/* if ((err = ebtablesAddForwardPolicyReject(netdriver->ebtables,
"dummy"))) {
+ virReportSystemError(conn, err,
+ _("failed to add ebtables rule to set
default policy to drop on '%s'"),
+ "dummy");
+ }
+*/
+ return 0;
+}
+
+static int
+networkDisallowMacOnPort(virConnectPtr conn,
+ struct qemud_driver *driver,
+ char * ifname,
+ unsigned char * mac) {
+
+ int err;
+ char *macaddr;
+
+ if (virAsprintf(&macaddr,
+ "%02x:%02x:%02x:%02x:%02x:%02x",
+ mac[0], mac[1],
+ mac[2], mac[3],
+ mac[4], mac[5]) < 0) {
+ virReportOOMError(conn);
+ return -1;
+ }
+ /* allow this combination of macaddr and ifname */
+
+ ebtablesContext * ebtablescontext = driver->ebtables;
+ if ((err = ebtablesRemoveForwardAllowIn(ebtablescontext,
+ ifname,
+ macaddr))) {
+ virReportSystemError(conn, err,
+ _("failed to add ebtables rule to allow
routing to '%s'"),
+ ifname);
+ }
+
+ return 0;
+}
static void
qemudAutostartConfigs(struct qemud_driver *driver) {
@@ -240,6 +309,17 @@ qemudAutostartConfigs(struct qemud_drive
}
virDomainObjUnlock(vm);
}
+ if (qemu_driver->macFilter) {
+ if (!(driver->ebtables = ebtablesContextNew())) {
+ qemu_driver->macFilter = 0; // TODO: we need to report an error here
+ }
+
+ if ((errno = networkDisableAllFrames(conn, driver))) {
+ virReportSystemError(conn, errno,
+ _("failed to add rule to drop all
frames in '%s'"), __FILE__);
+ }
+ }
+
qemuDriverUnlock(driver);
if (conn)
@@ -2168,8 +2248,18 @@ cleanup:
static void qemudShutdownVMDaemon(virConnectPtr conn,
struct qemud_driver *driver,
virDomainObjPtr vm) {
int ret;
int retries = 0;
+
+ virDomainNetDefPtr net = vm->def->nets[0];
+
+ if (driver->macFilter) {
+ if ((errno = networkDisallowMacOnPort(conn, driver, net->ifname,
net->mac))) {
+ virReportSystemError(conn, errno,
+ _("failed to add ebtables rule to
allow MAC address on '%s'"),
+ net->ifname);
+ }
+ }
if (!virDomainIsActive(vm))
return;
Index: libvirt/src/qemu/qemu.conf
===================================================================
--- libvirt.orig/src/qemu/qemu.conf
+++ libvirt/src/qemu/qemu.conf
@@ -152,3 +152,6 @@
# in a location of $MOUNTPOINT/libvirt/qemu
# hugetlbfs_mount = "/dev/hugepages"
+
+mac_filter = 1
+
Index: libvirt/src/qemu/qemu_conf.h
===================================================================
--- libvirt.orig/src/qemu/qemu_conf.h
+++ libvirt/src/qemu/qemu_conf.h
@@ -26,6 +26,7 @@
#include <config.h>
+#include "ebtables.h"
#include "internal.h"
#include "bridge.h"
#include "capabilities.h"
@@ -112,6 +113,9 @@ struct qemud_driver {
char *hugetlbfs_mount;
char *hugepage_path;
+ unsigned int macFilter : 1;
+ ebtablesContext *ebtables;
+
virCapsPtr caps;
/* An array of callbacks */
--
Best regards,
Gerhard Stenzel,
-----------------------------------------------------------------------------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martin Jetter
Geschäftsführung: Erich Baier
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list