Re: [libvirt] [PATCH V1 2/3] nwfilter: accept broadcasted DHCP replies in DHCP snooping code

Some DHCP servers send their DHCP replies to the broadcast MAC address rather than to the MAC address of the VM. The existing DHCP snooping code assumes that the reply always goes to the MAC address of the VM thus filtering the traffic of some DHCP servers' replies. The below patch adapts the code to 1) filter DHCP replies by comparing the MAC address in the reply against the MAC address of the VM (held in the snoop request) 2) adapts the pcap filter for traffic towards the VM to accept DHCP replies sent to any MAC address; for further filtering we rely on 1) 3) creates initial rules that are active while waiting for DHCP replies; these rules now accept DHCP replies to the VM's MAC address or to the MAC broadcast address --- src/nwfilter/nwfilter_dhcpsnoop.c | 36 +++++++++++++++++++++++------- src/nwfilter/nwfilter_ebiptables_driver.c | 35 +++++++++++++++++------------ 2 files changed, 49 insertions(+), 22 deletions(-) Index: libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_dhcpsnoop.c +++ libvirt-acl/src/nwfilter/nwfilter_dhcpsnoop.c @@ -1010,6 +1010,17 @@ virNWFilterSnoopDHCPDecode(virNWFilterSn if (len < 0) return -2; /* invalid packet length */ + /* + * some DHCP servers send their responses as MAC broadcast replies + * filter messages from the server also by the destination MAC + * inside the DHCP response + */ + if (!fromVM) { + if (virMacAddrCmpRaw(&req->macaddr, + (unsigned char *)&pd->d_chaddr) != 0) + return -2; + } + if (virNWFilterSnoopDHCPGetOpt(pd, len, &mtype, &leasetime) < 0) return -2; @@ -1069,7 +1080,6 @@ virNWFilterSnoopDHCPOpen(const char *ifn char pcap_errbuf[PCAP_ERRBUF_SIZE]; char *ext_filter = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; - const char *ext; virMacAddrFormat(mac, macaddr); @@ -1080,14 +1090,24 @@ virNWFilterSnoopDHCPOpen(const char *ifn * extend the filter with the macaddr of the VM; filter the * more unlikely parameters first, then go for the MAC */ - ext = "and ether src"; + if (virAsprintf(&ext_filter, + "%s and ether src %s", filter, macaddr) < 0) { + virReportOOMError(); + return NULL; + } } else { - ext = "and ether dst"; - } - - if (virAsprintf(&ext_filter, "%s %s %s", filter, ext, macaddr) < 0) { - virReportOOMError(); - return NULL; + /* + * Some DHCP servers respond via MAC broadcast; we rely on later + * filtering of responses by comparing the MAC address inside the + * DHCP response against the one of the VM. Assuming that the + * bridge learns the VM's MAC address quickly this should not + * generate much more traffic than if we filtered by VM and + * braodcast MAC as well + */ + if (virAsprintf(&ext_filter, "%s", filter) < 0) { + virReportOOMError(); + return NULL; + } } handle = pcap_create(ifname, pcap_errbuf); Index: libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c =================================================================== --- libvirt-acl.orig/src/nwfilter/nwfilter_ebiptables_driver.c +++ libvirt-acl/src/nwfilter/nwfilter_ebiptables_driver.c @@ -3345,6 +3345,7 @@ ebtablesApplyDHCPOnlyRules(const char *i while (true) { char *srcIPParam = NULL; + int ctr; if (idx < num_dhcpsrvrs) { const char *dhcpserver; @@ -3357,20 +3358,26 @@ ebtablesApplyDHCPOnlyRules(const char *i } } - virBufferAsprintf(&buf, - CMD_DEF("$EBT -t nat -A %s" - " -d %s" - " -p ipv4 --ip-protocol udp" - " %s" - " --ip-sport 67 --ip-dport 68" - " -j ACCEPT") CMD_SEPARATOR - CMD_EXEC - "%s", - - chain_out, - macaddr_str, - srcIPParam != NULL ? srcIPParam : "", - CMD_STOPONERR(1)); + /* + * create two rules allowing response to MAC address of VM + * or to broadcast MAC address + */ + for (ctr = 0; ctr < 2; ctr++) { + virBufferAsprintf(&buf, + CMD_DEF("$EBT -t nat -A %s" + " -d %s" + " -p ipv4 --ip-protocol udp" + " %s" + " --ip-sport 67 --ip-dport 68" + " -j ACCEPT") CMD_SEPARATOR + CMD_EXEC + "%s", + + chain_out, + (ctr == 0) ? macaddr_str : "ff:ff:ff:ff:ff:ff", + srcIPParam != NULL ? srcIPParam : "", + CMD_STOPONERR(1)); + } VIR_FREE(srcIPParam);