On 09/23/2013 08:03 PM, Laszlo Ersek wrote: > 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 > - eg. 192.168.122.255/32 (ie. address- and netmask-dependent (= directed) > local network broadcast address) As you pointed out in an email responding to my request for this - it's not necessary as it is already covered by all of the rules that *do* want masquerading being qualified with "! -d 192.168.122.0/24" (for example). So you can/should take it out. Sorry for creating the extra work. > 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@xxxxxxxxxx> > --- > v2->v3: > - also prevent masquerading of directed broadcast [Laine] > > src/network/bridge_driver_linux.c | 151 +++++++++++++++++++++++++++++++++++++- > 1 file changed, 147 insertions(+), 4 deletions(-) > > diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c > index 80430a8..093cba1 100644 > --- a/src/network/bridge_driver_linux.c > +++ b/src/network/bridge_driver_linux.c > @@ -127,11 +127,64 @@ out: > return ret; > } > > +/* Fill in preallocated virPfxSocketAddr objects with masquerading exceptions: > + * > + * 1. do not masquerade packets targeting 224.0.0.0/24 > + * 2. do not masquerade packets targeting 255.255.255.255/32 > + * 3. do not masquerade packets targeting the directed local broadcast > + * address > + * > + * 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 directed local broadcast address looks like 192.168.122.255/32, and > + * behaves like the generic broadcast address 255.255.255.255/32. > + * > + * Returns 0 on success and -1 on failure. > + */ > +static int networkFillMasqExceptions(const char *bridgeName, > + const virPfxSocketAddr *bridge, > + virPfxSocketAddr *localMulticast, > + virPfxSocketAddr *genericBroadcast, > + virPfxSocketAddr *directedBroadcast) > +{ > + int result; > + > + localMulticast->prefix = 24; > + result = virSocketAddrParseIPv4(&localMulticast->addr, > + "224.0.0.0"); > + sa_assert(result != -1); You must have accidentally left this in. libvirt is a library, so it must never assert. In a case where the called function is guaranteed to never fail (due to the args passed in), you can enclose it in ignore_value(): ignore_value(cirSocketAddrParseIPv4(.......) > + > + genericBroadcast->prefix = 32; > + result = virSocketAddrParseIPv4(&genericBroadcast->addr, > + "255.255.255.255"); > + sa_assert(result != -1); > + > + directedBroadcast->prefix = 32; > + result = virSocketAddrBroadcastByPrefix(&bridge->addr, bridge->prefix, > + &directedBroadcast->addr); > + if (result == -1) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Couldn't form directed broadcast address for '%s'"), > + bridgeName); > + return -1; > + } > + return 0; > +} > + > int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, > virNetworkIpDefPtr ipdef) > { > int prefix = virNetworkIpDefPrefix(ipdef); > const char *forwardIf = virNetworkDefForwardIf(network->def, 0); > + virPfxSocketAddr bridgePfxAddr, > + localMulticast, > + genericBroadcast, > + directedBroadcast; > > if (prefix < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > @@ -140,6 +193,15 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, > goto masqerr1; > } > > + bridgePfxAddr.addr = ipdef->address; > + bridgePfxAddr.prefix = prefix; > + if (networkFillMasqExceptions(network->def->bridge, > + &bridgePfxAddr, > + &localMulticast, > + &genericBroadcast, > + &directedBroadcast) == -1) > + goto masqerr1; > + > /* allow forwarding packets from the bridge interface */ > if (iptablesAddForwardAllowOut(&ipdef->address, > prefix, > @@ -167,11 +229,12 @@ 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 6 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-3. masquerading exceptions > + * 4. masquerade protocol=tcp with sport mapping restriction > + * 5. masquerade protocol=udp with sport mapping restriction > + * 6. generic, masquerade any protocol > * > * The sport mappings are required, because default IPtables > * MASQUERADE maintain port numbers unchanged where possible. > @@ -238,8 +301,65 @@ int networkAddMasqueradingFirewallRules(virNetworkObjPtr network, > goto masqerr5; > } > > + /* exempt generic broadcast address as destination */ > + if (iptablesAddDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &genericBroadcast) == -1) { > + if (forwardIf) > + virReportError(VIR_ERR_SYSTEM_ERROR, > + _("failed to add iptables rule to prevent generic broadcast masquerading on %s"), > + forwardIf); > + else > + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", > + _("failed to add iptables rule to prevent generic broadcast masquerading")); > + goto masqerr6; > + } > + > + /* exempt directed broadcast address as destination */ > + if (iptablesAddDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &directedBroadcast) == -1) { > + if (forwardIf) > + virReportError(VIR_ERR_SYSTEM_ERROR, > + _("failed to add iptables rule to prevent directed broadcast masquerading on %s"), > + forwardIf); > + else > + virReportError(VIR_ERR_SYSTEM_ERROR, "%s", > + _("failed to add iptables rule to prevent directed broadcast masquerading")); > + goto masqerr7; > + } > + > + /* exempt local multicast range as destination */ > + if (iptablesAddDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &localMulticast) == -1) { > + 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 masqerr8; > + } > + > return 0; > > + masqerr8: > + iptablesRemoveDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &directedBroadcast); > + masqerr7: > + iptablesRemoveDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &genericBroadcast); > + masqerr6: > + iptablesRemoveForwardMasquerade(&ipdef->address, > + prefix, > + forwardIf, > + &network->def->forward.addr, > + &network->def->forward.port, > + "tcp"); > masqerr5: > iptablesRemoveForwardMasquerade(&ipdef->address, > prefix, > @@ -275,6 +395,29 @@ void networkRemoveMasqueradingFirewallRules(virNetworkObjPtr network, > const char *forwardIf = virNetworkDefForwardIf(network->def, 0); > > if (prefix >= 0) { > + virPfxSocketAddr bridgePfxAddr, > + localMulticast, > + genericBroadcast, > + directedBroadcast; > + > + bridgePfxAddr.addr = ipdef->address; > + bridgePfxAddr.prefix = prefix; > + if (networkFillMasqExceptions(network->def->bridge, > + &bridgePfxAddr, > + &localMulticast, > + &genericBroadcast, > + &directedBroadcast) == 0) { > + iptablesRemoveDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &localMulticast); > + iptablesRemoveDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &directedBroadcast); > + iptablesRemoveDontMasquerade(&bridgePfxAddr, > + forwardIf, > + &genericBroadcast); > + } > + > iptablesRemoveForwardMasquerade(&ipdef->address, > prefix, > forwardIf, ACK once the ill-fated directed broadcast rule and sa_asserts are removed. -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list