[libvirt] [PATCH 2/2] bridge driver: don't masquerade local subnet broadcast/multicast packets

[Laszlo Ersek]

Packets sent by guests on virbrN, *or* by dnsmasq on the same, to
- 255.255.255.255/32 (netmask-independent local network broadcast
  address), or to
- 224.0.0.0/24 (local subnetwork multicast range)
are never forwarded, hence it is not necessary to masquerade them.

In fact we must not masquerade them: translating their source addresses or
source ports (where applicable) may confuse receivers on virbrN.

One example is the DHCP client in OVMF (= UEFI firmware for virtual
machines):

  http://thread.gmane.org/gmane.comp.bios.tianocore.devel/1506/focus=2640

It expects DHCP replies to arrive from remote source port 67. Even though
dnsmasq conforms to that, the destination address (255.255.255.255) and
the source address (eg. 192.168.122.1) in the reply allow the UDP
masquerading rule to match, which rewrites the source port to or above
1024. This prevents the DHCP client in OVMF from accepting the packet.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=709418

(I read attachment 526549 in that BZ by Brian J. Murrell
<brian at interlinx.bc.ca>, and Laine Stump's review thereof, before starting
this patch.)

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 src/network/bridge_driver.c |   76 ++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 72 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index d5886fe..2b3b250 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -1542,6 +1542,9 @@ networkRefreshDaemons(struct network_driver *driver)
     }
 }
 
+static const char networkLocalMulticast[] = "224.0.0.0/24";
+static const char networkLocalBroadcast[] = "255.255.255.255/32";
+
 static int
 networkAddMasqueradingIptablesRules(struct network_driver *driver,
                                     virNetworkObjPtr network,
@@ -1586,11 +1589,20 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
     /*
      * Enable masquerading.
      *
-     * We need to end up with 3 rules in the table in this order
+     * We need to end up with 5 rules in the table in this order
+     *
+     *  1. do not masquerade packets targeting 224.0.0.0/24
+     *  2. do not masquerade packets targeting 255.255.255.255/32
+     *  3. masquerade protocol=tcp with sport mapping restriction
+     *  4. masquerade protocol=udp with sport mapping restriction
+     *  5. generic, masquerade any protocol
+     *
+     * 224.0.0.0/24 is the local network multicast range. Packets are not
+     * forwarded outside.
      *
-     *  1. protocol=tcp with sport mapping restriction
-     *  2. protocol=udp with sport mapping restriction
-     *  3. generic any protocol
+     * 255.255.255.255/32 is the broadcast address of any local network. Again,
+     * such packets are never forwarded, but strict DHCP clients don't accept
+     * DHCP replies with changed source ports.
      *
      * The sport mappings are required, because default IPtables
      * MASQUERADE maintain port numbers unchanged where possible.
@@ -1660,8 +1672,54 @@ networkAddMasqueradingIptablesRules(struct network_driver *driver,
         goto masqerr5;
     }
 
+    /* exempt local network broadcast address as destination */
+    if (iptablesAddForwardDontMasquerade(driver->iptables,
+                                         &ipdef->address,
+                                         prefix,
+                                         forwardIf,
+                                         networkLocalBroadcast) < 0) {
+        if (forwardIf)
+            virReportError(VIR_ERR_SYSTEM_ERROR,
+                           _("failed to add iptables rule to prevent local broadcast masquerading on %s"),
+                           forwardIf);
+        else
+            virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+                           _("failed to add iptables rule to prevent local broadcast masquerading"));
+        goto masqerr6;
+    }
+
+    /* exempt local multicast range as destination */
+    if (iptablesAddForwardDontMasquerade(driver->iptables,
+                                         &ipdef->address,
+                                         prefix,
+                                         forwardIf,
+                                         networkLocalMulticast) < 0) {
+        if (forwardIf)
+            virReportError(VIR_ERR_SYSTEM_ERROR,
+                           _("failed to add iptables rule to prevent local multicast masquerading on %s"),
+                           forwardIf);
+        else
+            virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
+                           _("failed to add iptables rule to prevent local multicast masquerading"));
+        goto masqerr7;
+    }
+
     return 0;
 
+ masqerr7:
+    iptablesRemoveForwardDontMasquerade(driver->iptables,
+                                        &ipdef->address,
+                                        prefix,
+                                        forwardIf,
+                                        networkLocalBroadcast);
+ masqerr6:
+    iptablesRemoveForwardMasquerade(driver->iptables,
+                                    &ipdef->address,
+                                    prefix,
+                                    forwardIf,
+                                    &network->def->forward.addr,
+                                    &network->def->forward.port,
+                                    "tcp");
  masqerr5:
     iptablesRemoveForwardMasquerade(driver->iptables,
                                     &ipdef->address,
@@ -1703,6 +1761,16 @@ networkRemoveMasqueradingIptablesRules(struct network_driver *driver,
     const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
 
     if (prefix >= 0) {
+        iptablesRemoveForwardDontMasquerade(driver->iptables,
+                                            &ipdef->address,
+                                            prefix,
+                                            forwardIf,
+                                            networkLocalMulticast);
+        iptablesRemoveForwardDontMasquerade(driver->iptables,
+                                            &ipdef->address,
+                                            prefix,
+                                            forwardIf,
+                                            networkLocalBroadcast);
         iptablesRemoveForwardMasquerade(driver->iptables,
                                         &ipdef->address,
                                         prefix,
-- 
1.7.1