[libvirt] [PATCH v4 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

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 v2->v4:
 - Rename iptables(Add|Remove)ForwardDontMasquerade to
          iptables(Add|Remove)DontMasquerade [Laine].

 src/network/bridge_driver_linux.c | 70 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)

diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c
index 80430a8..066779a 100644
--- a/src/network/bridge_driver_linux.c
+++ b/src/network/bridge_driver_linux.c
@@ -127,6 +127,9 @@ out:
     return ret;
 }
 
+static const char networkLocalMulticast[] = "224.0.0.0/24";
+static const char networkLocalBroadcast[] = "255.255.255.255/32";
+
 int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
                                         virNetworkIpDefPtr ipdef)
 {
@@ -167,11 +170,20 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
     /*
      * 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. protocol=tcp with sport mapping restriction
-     *  2. protocol=udp with sport mapping restriction
-     *  3. generic any protocol
+     *  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.
+     *
+     * 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.
@@ -238,8 +250,50 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network,
         goto masqerr5;
     }
 
+    /* exempt local network broadcast address as destination */
+    if (iptablesAddDontMasquerade(&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 (iptablesAddDontMasquerade(&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:
+    iptablesRemoveDontMasquerade(&ipdef->address,
+                                 prefix,
+                                 forwardIf,
+                                 networkLocalBroadcast);
+ masqerr6:
+    iptablesRemoveForwardMasquerade(&ipdef->address,
+                                    prefix,
+                                    forwardIf,
+                                    &network->def->forward.addr,
+                                    &network->def->forward.port,
+                                    "tcp");
  masqerr5:
     iptablesRemoveForwardMasquerade(&ipdef->address,
                                     prefix,
@@ -275,6 +329,14 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network,
     const char *forwardIf = virNetworkDefForwardIf(network->def, 0);
 
     if (prefix >= 0) {
+        iptablesRemoveDontMasquerade(&ipdef->address,
+                                     prefix,
+                                     forwardIf,
+                                     networkLocalMulticast);
+        iptablesRemoveDontMasquerade(&ipdef->address,
+                                     prefix,
+                                     forwardIf,
+                                     networkLocalBroadcast);
         iptablesRemoveForwardMasquerade(&ipdef->address,
                                         prefix,
                                         forwardIf,
-- 
1.8.3.1