[libvirt] [PATCHv2 2/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4

[John Ferlan]

On 2/18/19 6:21 PM, Laine Stump wrote:
> dnsmasq documentation says that the *IPv4* prefix/network
> address/broadcast address sent to dhcp clients will be automatically
> determined by dnsmasq by looking at the interface it's listening on,
> so the original libvirt code did not add a netmask to the dnsmasq
> commandline (or later, the dnsmasq conf file).
> 
> For *IPv6* however, dnsmasq apparently cannot automatically determine
> the prefix (functionally the same as a netmask), and it must be
> explicitly provided in the conf file (as a part of the dhcp-range
> option). So many years after IPv4 DHCP support had been added, when
> IPv6 dhcp support was added the prefix was included at the end of the
> dhcp-range setting, but only for IPv6.
> 
> Recently a user reported (privately, because they suspected a possible
> security implication, which turned out to be unfounded) a bug on a
> host where one of the interfaces was a superset of the libvirt network
> where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and
> the libvirt network is 10.10.0.1/24). For some reason dnsmasq was
> supplying the netmask for the /8 network to clients requesting an
> address on the /24 interface.
> 
> This seems like a bug in dnsmasq, but even if/when it gets fixed
> there, it looks like there is no harm in just always adding the
> netmask to all IPv4 dhcp-range options similar to how prefix is added
> to all IPv6 dhcp-range options.
> 
> Signed-off-by: Laine Stump <laine at laine.org>
> ---
>  src/network/bridge_driver.c                   | 27 +++++++++++++++----
>  .../dhcp6-nat-network.conf                    |  2 +-
>  .../networkxml2confdata/isolated-network.conf |  2 +-
>  .../nat-network-dns-srv-record-minimal.conf   |  2 +-
>  .../nat-network-dns-srv-record.conf           |  2 +-
>  .../nat-network-dns-txt-record.conf           |  2 +-
>  .../networkxml2confdata/nat-network-mtu.conf  |  2 +-
>  .../nat-network-name-with-quotes.conf         |  2 +-
>  tests/networkxml2confdata/nat-network.conf    |  2 +-
>  .../networkxml2confdata/netboot-network.conf  |  2 +-
>  .../netboot-proxy-network.conf                |  2 +-
>  .../networkxml2confdata/ptr-domains-auto.conf |  2 +-
>  12 files changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 6d80818e40..9fa902896b 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj,
>                  !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end)))
>                  goto cleanup;
>  
> -            virBufferAsprintf(&configbuf, "dhcp-range=%s,%s",
> -                              saddr, eaddr);
> -            if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6))
> -                virBufferAsprintf(&configbuf, ",%d", prefix);
> -            virBufferAddLit(&configbuf, "\n");
> +            if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) {
> +               virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n",
> +                                 saddr, eaddr, prefix);
> +            } else {
> +                /* IPv4 - dnsmasq requires a netmask rather than prefix */
> +                virSocketAddr netmask;

Does it matter if this isn't initialized? e.g. "= { 0 };"

> +                char *netmaskStr;

    VIR_AUTOFREE(char *) netmaskStr = NULL;

> +
> +                if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR,
> +                                   _("Failed to translate bridge '%s' "
> +                                     "prefix %d to netmask"),
> +                                   def->bridge, prefix);
> +                    goto cleanup;
> +                }
> +
> +                if (!(netmaskStr = virSocketAddrFormat(&netmask)))
> +                    goto cleanup;
> +                virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n",
> +                                  saddr, eaddr, netmaskStr);
> +                VIR_FREE(netmaskStr);

w/ VIR_AUTOFREE, this isn't necessary

I know bridge_driver doesn't use it yet, but doesn't harm to start.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

[...]