On 10/12/2011 03:50 PM, David L Stevens wrote:

This patch adds support for "continue" and "return" actions in filter rules.

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- src/conf/nwfilter_conf.c | 8 ++++++-- src/conf/nwfilter_conf.h | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index 08ede48..e0c2fb6 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -55,12 +55,16 @@ VIR_ENUM_IMPL(virNWFilterRuleAction, VIR_NWFILTER_RULE_ACTION_LAST, "drop", "accept", - "reject"); + "reject", + "return", + "continue");

VIR_ENUM_IMPL(virNWFilterJumpTarget, VIR_NWFILTER_RULE_ACTION_LAST, "DROP", "ACCEPT", - "REJECT"); + "REJECT", + "RETURN", + "CONTINUE");

VIR_ENUM_IMPL(virNWFilterRuleDirection, VIR_NWFILTER_RULE_DIRECTION_LAST, "in", diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 5306403..c96851a 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -299,6 +299,8 @@ enum virNWFilterRuleActionType { VIR_NWFILTER_RULE_ACTION_DROP = 0, VIR_NWFILTER_RULE_ACTION_ACCEPT, VIR_NWFILTER_RULE_ACTION_REJECT, + VIR_NWFILTER_RULE_ACTION_RETURN, + VIR_NWFILTER_RULE_ACTION_CONTINUE,

VIR_NWFILTER_RULE_ACTION_LAST, }; I'd ACK this. I'd like to also have test cases for the libvirt-tck.

Stefan

On 10/12/2011 03:50 PM, David L Stevens wrote:

[...]

diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index 23fd753..84aaa3c 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -3,12 +3,16 @@

FILTERS = \ allow-arp.xml \ + allow-arpip.xml \ + allow-arpmac.xml \ allow-dhcp-server.xml \ allow-dhcp.xml \ allow-incoming-ipv4.xml \ allow-ipv4.xml \ clean-traffic.xml \ no-arp-spoofing.xml \ + no-arpmac-spoofing.xml \ + no-arpip-spoofing.xml \ no-ip-multicast.xml \ no-ip-spoofing.xml \ no-mac-broadcast.xml \ diff --git a/examples/xml/nwfilter/allow-arp.xml b/examples/xml/nwfilter/allow-arp.xml index 63a92b2..6271ae4 100644 --- a/examples/xml/nwfilter/allow-arp.xml +++ b/examples/xml/nwfilter/allow-arp.xml @@ -1,3 +1,4 @@ -<filter name='allow-arp' chain='arp'> -<rule direction='inout' action='accept'/> +<filter name='allow-arp' chain='arpmac'> +<filterref filter='allow-arpmac.xml'/> +<filterref filter='allow-arpip.xml'/> </filter> So the intention here was to remove the 'arp' chain. With it staying now I suppose this patch and the allow-arpmac and allow-arpip aren't needed. diff --git a/examples/xml/nwfilter/allow-arpip.xml b/examples/xml/nwfilter/allow-arpip.xml new file mode 100644 index 0000000..6ddb6fe --- /dev/null +++ b/examples/xml/nwfilter/allow-arpip.xml @@ -0,0 +1,3 @@ +<filter name='allow-arpip' chain='arpip'> +<rule direction='inout' action='accept'/> +</filter> Seems no necessary following above. diff --git a/examples/xml/nwfilter/allow-arpmac.xml b/examples/xml/nwfilter/allow-arpmac.xml new file mode 100644 index 0000000..54f6714 --- /dev/null +++ b/examples/xml/nwfilter/allow-arpmac.xml @@ -0,0 +1,3 @@ +<filter name='allow-arpmac' chain='arpmac'> +<rule direction='inout' action='accept'/> +</filter> Seems not necessary following above. diff --git a/examples/xml/nwfilter/clean-traffic.xml b/examples/xml/nwfilter/clean-traffic.xml index 40f0ecb..9cee799 100644 --- a/examples/xml/nwfilter/clean-traffic.xml +++ b/examples/xml/nwfilter/clean-traffic.xml @@ -11,10 +11,10 @@ <!-- preventing ARP spoofing/poisoning --> <filterref filter='no-arp-spoofing'/>

-<!-- preventing any other traffic than IPv4 and ARP --> -<filterref filter='no-other-l2-traffic'/> - <!-- allow qemu to send a self-announce upon migration end --> <filterref filter='qemu-announce-self'/>

+<!-- preventing any other traffic than IPv4 and ARP --> +<filterref filter='no-other-l2-traffic'/> + </filter> This reshuffeling might make it more intuitive but isn't necessary.

diff --git a/examples/xml/nwfilter/no-arp-spoofing.xml b/examples/xml/nwfilter/no-arp-spoofing.xml index 3c83acd..1979b20 100644 --- a/examples/xml/nwfilter/no-arp-spoofing.xml +++ b/examples/xml/nwfilter/no-arp-spoofing.xml @@ -1,17 +1,4 @@ -<filter name='no-arp-spoofing' chain='arp'> -<uuid>f88f1932-debf-4aa1-9fbe-f10d3aa4bc95</uuid> -<rule action='drop' direction='out' priority='300'> -<mac match='no' srcmacaddr='$MAC'/> -</rule> - -<!-- no arp spoofing --> -<!-- drop if ipaddr or macaddr does not belong to guest --> -<rule action='drop' direction='out' priority='350'> -<arp match='no' arpsrcmacaddr='$MAC'/> -</rule> -<rule action='drop' direction='out' priority='400'> -<arp match='no' arpsrcipaddr='$IP' /> -</rule> -<!-- allow everything else --> -<rule action='accept' direction='in' priority='425' /> +<filter name='no-arp-spoofing'> +<filterref filter='no-arpmac-spoofing' /> +<filterref filter='no-arpip-spoofing' /> </filter> diff --git a/examples/xml/nwfilter/no-arpip-spoofing.xml b/examples/xml/nwfilter/no-arpip-spoofing.xml new file mode 100644 index 0000000..ee42d40 --- /dev/null +++ b/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -0,0 +1,12 @@ +<filter name='no-arpip-spoofing' chain='arpip'> +<!-- no arp spoofing --> +<!-- drop if ipaddr does not belong to guest --> +<rule action='return' direction='out' priority='400'> +<arp match='yes' arpsrcipaddr='$IP' /> +</rule> +<rule action='return' direction='out' priority='410'> +<arp match='yes' arpsrcipaddr='0.0.0.0' /> +</rule> Under what circumstances is the stack allowed to send a 0.0.0.0 as a response to an ARP request (presumably)? Form what I see 0.0.0.0 could be any machine whose interface is not configured. At least when using DHCP the VM would broadcast the request without prior sending of an ARP request (of course) and from what I remember the DHCP server then sends

In the meantime I took some of the hunks here and build a parallel set of filters (clean-traffic-new). I'll post that series soon. I am not sure whether we want to have a parallel set in the end, though. the response back to the MAC address it has received the request from also without ARP request.

+<!-- drop everything else --> +<rule action='drop' direction='out' priority='1000' /> +</filter> diff --git a/examples/xml/nwfilter/no-arpmac-spoofing.xml b/examples/xml/nwfilter/no-arpmac-spoofing.xml new file mode 100644 index 0000000..90499d3 --- /dev/null +++ b/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -0,0 +1,7 @@ +<filter name='no-arpmac-spoofing' chain='arpmac'> +<rule action='return' direction='out' priority='350'> +<arp match='yes' arpsrcmacaddr='$MAC'/> +</rule> +<!-- drop everything else --> +<rule action='drop' direction='out' priority='1000' /> +</filter> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index b8c94c8..84e8a5e 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -1,7 +1,9 @@ <filter name='no-ip-spoofing' chain='ipv4'>

<!-- drop if srcipaddr is not the IP address of the guest --> -<rule action='drop' direction='out'> -<ip match='no' srcipaddr='$IP' /> +<rule action='return' direction='out'> +<ip match='yes' srcipaddr='$IP' /> </rule> +<!-- drop any that don't match the source IP list --> +<rule action='drop' direction='out' /> </filter> diff --git a/examples/xml/nwfilter/no-mac-spoofing.xml b/examples/xml/nwfilter/no-mac-spoofing.xml index f210623..aee56c7 100644 --- a/examples/xml/nwfilter/no-mac-spoofing.xml +++ b/examples/xml/nwfilter/no-mac-spoofing.xml @@ -1,5 +1,9 @@ -<filter name='no-mac-spoofing' chain='ipv4'> -<rule action='drop' direction='out' priority='10'> -<mac match='no' srcmacaddr='$MAC' /> -</rule> +<filter name='no-mac-spoofing' chain='mac'> +<!-- no mac spoofing --> +<!-- drop if macaddr does not belong to guest --> +<rule action='return' direction='out' priority='350'> +<mac match='yes' srcmacaddr='$MAC'/> +</rule> +<!-- drop everything else --> +<rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-other-l2-traffic.xml b/examples/xml/nwfilter/no-other-l2-traffic.xml index 8bad86e..0501b1a 100644 --- a/examples/xml/nwfilter/no-other-l2-traffic.xml +++ b/examples/xml/nwfilter/no-other-l2-traffic.xml @@ -1,7 +1,12 @@ -<filter name='no-other-l2-traffic'> +<filter name='no-other-l2-traffic' chain='root'>

-<!-- drop all other l2 traffic than for which rules have been - written for; i.e., drop all other than arp and ipv4 traffic --> -<rule action='drop' direction='inout' priority='1000'/> +<!-- drop all other than arp and ipv4 traffic --> +<rule action='accept' direction='inout'> +<mac protocolid='0x800' /> +</rule> +<rule action='accept' direction='inout'> +<mac protocolid='0x806' /> +</rule> +<rule action='drop' direction='inout' priority='1000' />

With the pending patches, this one looks a little different now. Stefan

Stefan Berger<stefanb@linux.vnet.ibm.com> wrote on 10/25/2011 12:01:05 PM:

...

So the intention here was to remove the 'arp' chain. With it staying now I suppose this patch and the allow-arpmac and allow-arpip aren't needed.

...

diff --git a/examples/xml/nwfilter/allow-arpip.xml b/examples/xml/ nwfilter/allow-arpip.xml The intention was to separate MAC and IP address matching to be their own chains to make adding and removing addresses dynamically easy. If you do this in one chain with "IP=X and MAC=Y -j ACCEPT", then you need every combination of IP and MAC address as a rule. If you separate them The current (proposed) iterator does this, it produces every combination of IP and MAC list items. The question is whether this is the correct way of producing the filtering rules. On the one side one could say that

On 10/25/2011 03:41 PM, David Stevens wrote: the VM (or nested VMs) owns all the given MAC and IP addresses and can produce any combination of them. So we'll have m*n combinations -> O(n^2) evaluations. Another possibility would be to walk the lists of MAC and IP addresses parallel, i.e. produce a rule for $MAC[0] and $IP[0], another one for $MAC[n] and $IP[n], thus enforcing the association of IP and MAC addresses. We'd have O(n) evaluations, though the number of rules to evaluate should not drive how this works, but obviously 'correctness'. If we split the evaluation into two chains (one for IP , the other for MAC checking) we allow all combinations. Complexity is O(2*n) -> O(n). So what should be correct?Fixed IP - MAC association or allow any combination of them? Maybe we leave it as proposed (two chains) for now and modify the iterator to process multiple lists' elements in parallel...

into MAC matching and protocol matching chains, you need one rule per MAC and IP address. After doing that, the ARP chain was empty, since that's all the standard chain had.

Yes, fine. For backwards compatibility purposes the 'arp' chain will remain.

...

...

new file mode 100644 index 0000000..6ddb6fe --- /dev/null +++ b/examples/xml/nwfilter/allow-arpip.xml @@ -0,0 +1,3 @@ +<filter name='allow-arpip' chain='arpip'> +<rule direction='inout' action='accept'/> +</filter> Seems no necessary following above. I don't understand this comment.

...

This reshuffeling might make it more intuitive but isn't necessary. I didn't say it was the only way to support multiple addresses. :-) It uses nMAC + nIP rules, instead of nMAC X nIP rules to match every combination of MAC and IP addresses in the original ARP chain.

...

...

+</rule> +<rule action='return' direction='out' priority='410'> +<arp match='yes' arpsrcipaddr='0.0.0.0' /> +</rule> Under what circumstances is the stack allowed to send a 0.0.0.0 as a response to an ARP request (presumably)? Form what I see 0.0.0.0 could be any machine whose interface is not configured. At least when using DHCP the VM would broadcast the request without prior sending of an ARP request (of course) and from what I remember the DHCP server then sends the response back to the MAC address it has received the request from also without ARP request. This isn't a requirement, but in general, the filters are placing restrictions on packets that have nothing to do with spoofing and are not part of the underlying protocols. A host without an IP address can use source address "0.0.0.0" legally by the protocols in cases besides sending a DHCP request, DHCP requests need not be MAC broadcast or sent to the all-hosts broadcast address (e.g., if firmware configured a particular preferred server (unicast MAC and/or IP), or knows the subnet and uses a directed broadcast). ARP requests received from other machines need not be addressed to this VM to be processed and update the cache. I didn't test with a well-known DHCP server, so I was not sure whether

With the arp chain remaining we can keep the existing filter 'allow-arp' and don't need the one proposed in this patch. the rule would actually be used. Following the DHCP protocol fields one could broadcast and put the IP address of the server into the server address field in the DHCP protocol and if the protocol was to support it that way only that server could respond

By restricting these cases, the filters prevent VMs from doing protocol-legal things that have nothing to do with spoofing. It simply breaks unusual configurations unnecessarily.

I was planning to submit clean-up patches to remove all these restrictions I found after my primary concern (DHCP snooping) is done. I threw in some of them along with the multiple address support in the version that did that, but they really are separate fixes that have nothing to do with either multiple address support or DHCP snooping.

In the end, base anti-spoofing rules should prevent a VM from masquerading as someone else, and allow everything else.

+-DLS

On 10/12/2011 03:50 PM, David L Stevens wrote:

This patch simplifies the table rules by setting the protocol chains policy to be "DROP" and removes the explicit "-j DROP" entries that the protocol rules had previously. It also makes "no-other-rarp-traffic.xml" obsolete. I agree with Daniel's previous comments that this could introduce compatibility problems. It would be best not to change it or if really need be later on introduce an XML attribute for a chain that allows to choose whether the default policy is accept or drop.

Stefan

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- examples/xml/nwfilter/Makefile.am | 1 - examples/xml/nwfilter/no-arpip-spoofing.xml | 2 -- examples/xml/nwfilter/no-arpmac-spoofing.xml | 2 -- examples/xml/nwfilter/no-ip-spoofing.xml | 2 -- examples/xml/nwfilter/no-mac-spoofing.xml | 2 -- examples/xml/nwfilter/no-other-rarp-traffic.xml | 3 --- examples/xml/nwfilter/qemu-announce-self.xml | 1 - src/nwfilter/nwfilter_ebiptables_driver.c | 11 +---------- 8 files changed, 1 insertions(+), 23 deletions(-) delete mode 100644 examples/xml/nwfilter/no-other-rarp-traffic.xml

diff --git a/examples/xml/nwfilter/Makefile.am b/examples/xml/nwfilter/Makefile.am index 84aaa3c..67085fa 100644 --- a/examples/xml/nwfilter/Makefile.am +++ b/examples/xml/nwfilter/Makefile.am @@ -18,7 +18,6 @@ FILTERS = \ no-mac-broadcast.xml \ no-mac-spoofing.xml \ no-other-l2-traffic.xml \ - no-other-rarp-traffic.xml \ qemu-announce-self.xml \ qemu-announce-self-rarp.xml

diff --git a/examples/xml/nwfilter/no-arpip-spoofing.xml b/examples/xml/nwfilter/no-arpip-spoofing.xml index ee42d40..7ef6f0f 100644 --- a/examples/xml/nwfilter/no-arpip-spoofing.xml +++ b/examples/xml/nwfilter/no-arpip-spoofing.xml @@ -7,6 +7,4 @@ <rule action='return' direction='out' priority='410'> <arp match='yes' arpsrcipaddr='0.0.0.0' /> </rule> -<!-- drop everything else --> -<rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-arpmac-spoofing.xml b/examples/xml/nwfilter/no-arpmac-spoofing.xml index 90499d3..3834047 100644 --- a/examples/xml/nwfilter/no-arpmac-spoofing.xml +++ b/examples/xml/nwfilter/no-arpmac-spoofing.xml @@ -2,6 +2,4 @@ <rule action='return' direction='out' priority='350'> <arp match='yes' arpsrcmacaddr='$MAC'/> </rule> -<!-- drop everything else --> -<rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index 84e8a5e..2fccd12 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,6 +4,4 @@ <rule action='return' direction='out'> <ip match='yes' srcipaddr='$IP' /> </rule> -<!-- drop any that don't match the source IP list --> -<rule action='drop' direction='out' /> </filter> diff --git a/examples/xml/nwfilter/no-mac-spoofing.xml b/examples/xml/nwfilter/no-mac-spoofing.xml index aee56c7..e2e8c03 100644 --- a/examples/xml/nwfilter/no-mac-spoofing.xml +++ b/examples/xml/nwfilter/no-mac-spoofing.xml @@ -4,6 +4,4 @@ <rule action='return' direction='out' priority='350'> <mac match='yes' srcmacaddr='$MAC'/> </rule> -<!-- drop everything else --> -<rule action='drop' direction='out' priority='1000' /> </filter> diff --git a/examples/xml/nwfilter/no-other-rarp-traffic.xml b/examples/xml/nwfilter/no-other-rarp-traffic.xml deleted file mode 100644 index 7729996..0000000 --- a/examples/xml/nwfilter/no-other-rarp-traffic.xml +++ /dev/null @@ -1,3 +0,0 @@ -<filter name='no-other-rarp-traffic' chain='rarp'> -<rule action='drop' direction='inout' priority='1000'/> -</filter> diff --git a/examples/xml/nwfilter/qemu-announce-self.xml b/examples/xml/nwfilter/qemu-announce-self.xml index 352db50..12957b5 100644 --- a/examples/xml/nwfilter/qemu-announce-self.xml +++ b/examples/xml/nwfilter/qemu-announce-self.xml @@ -8,6 +8,5 @@

<!-- accept if it was changed to rarp --> <filterref filter='qemu-announce-self-rarp'/> -<filterref filter='no-other-rarp-traffic'/>

</filter> diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index 3c6fca7..e6a4880 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -2791,7 +2791,7 @@ ebtablesCreateTmpSubChain(virBufferPtr buf, protostr[0] = '\0';

virBufferAsprintf(buf, - CMD_DEF("%s -t %s -N %s") CMD_SEPARATOR + CMD_DEF("%s -t %s -N %s -P DROP") CMD_SEPARATOR CMD_EXEC "%s" CMD_DEF("%s -t %s -A %s %s -j %s") CMD_SEPARATOR @@ -3015,15 +3015,6 @@ ebtablesApplyBasicRules(const char *ifname,

PRINT_ROOT_CHAIN(chain, chainPrefix, ifname); virBufferAsprintf(&buf, - CMD_DEF("%s -t %s -A %s -s ! %s -j DROP") CMD_SEPARATOR - CMD_EXEC - "%s", - - ebtables_cmd_path, EBTABLES_DEFAULT_TABLE, - chain, macaddr_str, - CMD_STOPONERR(1)); - - virBufferAsprintf(&buf, CMD_DEF("%s -t %s -A %s -p IPv4 -j ACCEPT") CMD_SEPARATOR CMD_EXEC "%s",

On 10/12/2011 03:50 PM, David L Stevens wrote:

This patch adds the internal capability to add rules to existing chains instead of using temporary chains and to generate placeholders for chains that are referenced without generating a rule for them immediately. Finally, it includes variable matching for filter instantiation (i.e., instantiate only when a given variable is present in a filter, or only when it is not).

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- src/conf/nwfilter_conf.h | 4 +- src/nwfilter/nwfilter_ebiptables_driver.c | 93 +++++++++++++++++++++-------- src/nwfilter/nwfilter_gentech_driver.c | 32 +++++++++- 3 files changed, 100 insertions(+), 29 deletions(-)

diff --git a/src/conf/nwfilter_conf.h b/src/conf/nwfilter_conf.h index 17e954e..4348378 100644 --- a/src/conf/nwfilter_conf.h +++ b/src/conf/nwfilter_conf.h @@ -525,7 +525,9 @@ typedef int (*virNWFilterRuleCreateInstance)(virConnectPtr conn, virNWFilterRuleDefPtr rule, const char *ifname, virNWFilterHashTablePtr vars, - virNWFilterRuleInstPtr res); + virNWFilterRuleInstPtr res, + bool usetemp, + bool dummy);

typedef int (*virNWFilterRuleApplyNewRules)(virConnectPtr conn, const char *ifname, diff --git a/src/nwfilter/nwfilter_ebiptables_driver.c b/src/nwfilter/nwfilter_ebiptables_driver.c index e6a4880..918625c 100644 --- a/src/nwfilter/nwfilter_ebiptables_driver.c +++ b/src/nwfilter/nwfilter_ebiptables_driver.c @@ -1136,6 +1136,7 @@ iptablesEnforceDirection(int directionIn, * @isIPv6 : Whether this is an IPv6 rule * @maySkipICMP : whether this rule may under certain circumstances skip * the ICMP rule from being created + * @dummy : generate rule placeholder without installing * * Convert a single rule into its representation for later instantiation * @@ -1154,7 +1155,8 @@ _iptablesCreateRuleInstance(int directionIn, const char *match, bool defMatch, const char *accept_target, bool isIPv6, - bool maySkipICMP) + bool maySkipICMP, + bool dummy) { char chain[MAX_CHAINNAME_LENGTH]; char number[20]; @@ -1181,6 +1183,13 @@ _iptablesCreateRuleInstance(int directionIn,

PRINT_IPT_ROOT_CHAIN(chain, chainPrefix, ifname);

+ if (dummy) { + virBufferAsprintf(&buf, CMD_DEF_PRE "%s -- %s -%%c %s %%s", + "echo", iptables_cmd, chain); + bufUsed = virBufferUse(&buf); + goto prskip; + } + switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_TCP: case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: @@ -1521,6 +1530,8 @@ _iptablesCreateRuleInstance(int directionIn, return -1; }

+prskip: + if ((srcMacSkipped&& bufUsed == virBufferUse(&buf)) || skipRule) { virBufferFreeAndReset(&buf); @@ -1636,7 +1647,9 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool isIPv6) + bool isIPv6, + bool usetemp, + bool dummy) { int rc; int directionIn = 0; @@ -1668,7 +1681,7 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, return 1; }

- chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; if (create) { rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, @@ -1680,7 +1693,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy);

VIR_FREE(matchState); if (rc) @@ -1700,7 +1714,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, return 1; }

- chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : + CHAINPREFIX_HOST_OUT; if (create) { rc = _iptablesCreateRuleInstance(!directionIn, chainPrefix, @@ -1712,7 +1727,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "ACCEPT", isIPv6, - maySkipICMP); + maySkipICMP, + dummy);

VIR_FREE(matchState);

@@ -1736,7 +1752,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter,

if (create) { chainPrefix[0] = 'H'; - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : + CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1747,7 +1764,8 @@ iptablesCreateRuleInstanceStateCtrl(virNWFilterDefPtr nwfilter, matchState, false, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); VIR_FREE(matchState); }

@@ -1761,7 +1779,9 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool isIPv6) + bool isIPv6, + bool usetemp, + bool dummy) { int rc; int directionIn = 0; @@ -1777,7 +1797,9 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); }

if ((rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN) || @@ -1800,7 +1822,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, else matchState = NULL;

- chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1811,7 +1833,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); if (rc) return rc;

@@ -1822,7 +1845,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, else matchState = NULL;

- chainPrefix[1] = CHAINPREFIX_HOST_OUT_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : CHAINPREFIX_HOST_OUT; rc = _iptablesCreateRuleInstance(!directionIn, chainPrefix, nwfilter, @@ -1833,7 +1856,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "ACCEPT", isIPv6, - maySkipICMP); + maySkipICMP, + dummy); if (rc) return rc;

@@ -1844,7 +1868,7 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState = NULL;

chainPrefix[0] = 'H'; - chainPrefix[1] = CHAINPREFIX_HOST_IN_TEMP; + chainPrefix[1] = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; rc = _iptablesCreateRuleInstance(directionIn, chainPrefix, nwfilter, @@ -1855,7 +1879,8 @@ iptablesCreateRuleInstance(virNWFilterDefPtr nwfilter, matchState, true, "RETURN", isIPv6, - maySkipICMP); + maySkipICMP, + dummy);

return rc; } @@ -1886,7 +1911,8 @@ ebtablesCreateRuleInstance(char chainPrefix, const char *ifname, virNWFilterHashTablePtr vars, virNWFilterRuleInstPtr res, - bool reverse) + bool reverse, + bool dummy) { char macaddr[VIR_MAC_STRING_BUFLEN], ipaddr[INET_ADDRSTRLEN], @@ -1909,6 +1935,11 @@ ebtablesCreateRuleInstance(char chainPrefix, PRINT_CHAIN(chain, chainPrefix, ifname, virNWFilterChainSuffixTypeToString(nwfilter->chainsuffix));

+ if (dummy) { + virBufferAsprintf(&buf, CMD_DEF_PRE "%s -- -t %s -%%c %s %%s", + "echo", EBTABLES_DEFAULT_TABLE, chain); As above in the iptables case you should also add the ebtables_cmd into

Following the above I am not sure what this will be used for as part of this extension. the string (... %s -t %s ...).

+ goto prskip; + }

switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_MAC: @@ -2317,6 +2348,8 @@ ebtablesCreateRuleInstance(char chainPrefix, return -1; }

+prskip: + switch (rule->action) { case VIR_NWFILTER_RULE_ACTION_REJECT: /* REJECT not supported */ @@ -2374,11 +2407,15 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, virNWFilterRuleDefPtr rule, const char *ifname, virNWFilterHashTablePtr vars, - virNWFilterRuleInstPtr res) + virNWFilterRuleInstPtr res, + bool usetemp, + bool dummy) { int rc = 0; bool isIPv6; + char chainPrefix;

+ chainPrefix = usetemp ? CHAINPREFIX_HOST_IN_TEMP : CHAINPREFIX_HOST_IN; switch (rule->prtclType) { case VIR_NWFILTER_RULE_PROTOCOL_IP: case VIR_NWFILTER_RULE_PROTOCOL_MAC: @@ -2389,26 +2426,30 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED,

if (rule->tt == VIR_NWFILTER_RULE_DIRECTION_OUT || rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) { - rc = ebtablesCreateRuleInstance(CHAINPREFIX_HOST_IN_TEMP, + rc = ebtablesCreateRuleInstance(chainPrefix, nwfilter, rule, ifname, vars, res, - rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT); + rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT, + dummy); if (rc) return rc; }

+ chainPrefix = usetemp ? CHAINPREFIX_HOST_OUT_TEMP : + CHAINPREFIX_HOST_OUT; if (rule->tt == VIR_NWFILTER_RULE_DIRECTION_IN || rule->tt == VIR_NWFILTER_RULE_DIRECTION_INOUT) { - rc = ebtablesCreateRuleInstance(CHAINPREFIX_HOST_OUT_TEMP, + rc = ebtablesCreateRuleInstance(chainPrefix, nwfilter, rule, ifname, vars, res, - false); + false, + dummy); } break;

@@ -2427,7 +2468,9 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); break;

case VIR_NWFILTER_RULE_PROTOCOL_TCPoIPV6: @@ -2444,7 +2487,9 @@ ebiptablesCreateRuleInstance(virConnectPtr conn ATTRIBUTE_UNUSED, ifname, vars, res, - isIPv6); + isIPv6, + usetemp, + dummy); break;

case VIR_NWFILTER_RULE_PROTOCOL_LAST: diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 7891983..79350ac 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -284,7 +284,9 @@ virNWFilterRuleInstantiate(virConnectPtr conn, virNWFilterDefPtr filter, virNWFilterRuleDefPtr rule, const char *ifname, - virNWFilterHashTablePtr vars) + virNWFilterHashTablePtr vars, + bool usetemp, + bool dummy) { int rc; int i; @@ -297,8 +299,8 @@ virNWFilterRuleInstantiate(virConnectPtr conn,

ret->techdriver = techdriver;

- rc = techdriver->createRuleInstance(conn, nettype, filter, - rule, ifname, vars, ret); + rc = techdriver->createRuleInstance(conn, nettype, filter, rule, ifname, + vars, ret, usetemp, dummy);

if (rc) { for (i = 0; i< ret->ndata; i++) @@ -354,6 +356,8 @@ err_exit: * @ifname: The name of the interface to apply the rules to * @vars: A map holding variable names and values used for instantiating * the filter and its subfilters. + * @matchvar: if non-null, variable name to match + * @notmatch: if matchvar set, match filters that do not reference matchvar Can you write what happens if a match occurs versus if it does not occur? * @nEntries: number of virNWFilterInst objects collected * @insts: pointer to array for virNWFilterIns object pointers * @useNewFilter: instruct whether to use a newDef pointer rather than a @@ -377,6 +381,8 @@ _virNWFilterInstantiateRec(virConnectPtr conn, virNWFilterDefPtr filter, const char *ifname, virNWFilterHashTablePtr vars, + const char *matchvar, + bool notmatch, int *nEntries, virNWFilterRuleInstPtr **insts, enum instCase useNewFilter, bool *foundNewFilter, @@ -387,18 +393,34 @@ _virNWFilterInstantiateRec(virConnectPtr conn, int i; virNWFilterRuleInstPtr inst; virNWFilterDefPtr next_filter; + bool usetemp, dummy;

for (i = 0; i< filter->nentries; i++) { virNWFilterRuleDefPtr rule = filter->filterEntries[i]->rule; virNWFilterIncludeDefPtr inc = filter->filterEntries[i]->include; if (rule) { + usetemp = 1; + dummy = 0; + if (matchvar) { + int j; + + for (j = 0; j< rule->nvars; ++j) + if (strcmp(rule->vars[j], matchvar) == 0) + break; + /* use temp chains only on base rule install */ + usetemp = notmatch; + /* skip if not found; notmatch reverses the sense */ + dummy = (j == rule->nvars) ^ notmatch; + } inst = virNWFilterRuleInstantiate(conn, techdriver, nettype, filter, rule, ifname, - vars); + vars, + usetemp, + dummy); if (!inst) { rc = 1; break; @@ -456,6 +478,7 @@ _virNWFilterInstantiateRec(virConnectPtr conn, next_filter, ifname, tmpvars, + matchvar, notmatch, nEntries, insts, useNewFilter, foundNewFilter, @@ -684,6 +707,7 @@ virNWFilterInstantiate(virConnectPtr conn, filter, ifname, vars, + 0, 0, &nEntries,&insts, useNewFilter, foundNewFilter, driver);

On 10/12/2011 03:50 PM, David L Stevens wrote:

This patch adds DHCP Snooping support to libvirt.

Signed-off-by: David L Stevens<dlstevens@us.ibm.com> --- examples/xml/nwfilter/no-ip-spoofing.xml | 5 + src/Makefile.am | 2 + src/nwfilter/nwfilter_dhcpsnoop.c | 602 ++++++++++++++++++++++++++++++ src/nwfilter/nwfilter_dhcpsnoop.h | 36 ++ src/nwfilter/nwfilter_driver.c | 5 + src/nwfilter/nwfilter_gentech_driver.c | 83 +++-- 6 files changed, 708 insertions(+), 25 deletions(-) create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.c create mode 100644 src/nwfilter/nwfilter_dhcpsnoop.h

diff --git a/examples/xml/nwfilter/no-ip-spoofing.xml b/examples/xml/nwfilter/no-ip-spoofing.xml index 2fccd12..2ae9500 100644 --- a/examples/xml/nwfilter/no-ip-spoofing.xml +++ b/examples/xml/nwfilter/no-ip-spoofing.xml @@ -4,4 +4,9 @@ <rule action='return' direction='out'> <ip match='yes' srcipaddr='$IP' /> </rule> +<!-- allow DHCP requests --> +<rule action='return' direction='out'> +<ip match='yes' srcipaddr='0.0.0.0' protocol='udp' srcportstart='68' + srcportend='68' /> +</rule> </filter> diff --git a/src/Makefile.am b/src/Makefile.am index 738ee91..f6d3fdd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -473,6 +473,8 @@ NWFILTER_DRIVER_SOURCES = \ nwfilter/nwfilter_driver.h nwfilter/nwfilter_driver.c \ nwfilter/nwfilter_gentech_driver.c \ nwfilter/nwfilter_gentech_driver.h \ + nwfilter/nwfilter_dhcpsnoop.c \ + nwfilter/nwfilter_dhcpsnoop.h \ nwfilter/nwfilter_ebiptables_driver.c \ nwfilter/nwfilter_ebiptables_driver.h \ nwfilter/nwfilter_learnipaddr.c \ diff --git a/src/nwfilter/nwfilter_dhcpsnoop.c b/src/nwfilter/nwfilter_dhcpsnoop.c new file mode 100644 index 0000000..f784a29 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.c @@ -0,0 +1,602 @@ + +/* + * nwfilter_dhcpsnoop.c: support for DHCP snooping used by a VM + * on an interface + * + * Copyright (C) 2011 IBM Corp. + * Copyright (C) 2011 David L Stevens + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David L Stevens<dlstevens@us.ibm.com> + * Based in part on work by Stefan Berger<stefanb@us.ibm.com> + */ + +#include<config.h> + +#ifdef HAVE_LIBPCAP +#include<pcap.h> +#endif + +#include<fcntl.h> +#include<sys/ioctl.h> +#include<signal.h> + +#include<arpa/inet.h> +#include<net/ethernet.h> +#include<netinet/ip.h> +#include<netinet/udp.h> +#include<net/if.h> +#include<net/if_arp.h> +#include<intprops.h> + +#include "internal.h" + +#include "buf.h" +#include "memory.h" +#include "logging.h" +#include "datatypes.h" +#include "interface.h" +#include "virterror_internal.h" +#include "threads.h" +#include "conf/nwfilter_params.h" +#include "conf/domain_conf.h" +#include "nwfilter_gentech_driver.h" +#include "nwfilter_ebiptables_driver.h" +#include "nwfilter_dhcpsnoop.h" + +#define VIR_FROM_THIS VIR_FROM_NWFILTER + +static virHashTablePtr SnoopReqs; + +struct virNWFilterSnoopReq { + virConnectPtr conn; + virNWFilterTechDriverPtr techdriver; + enum virDomainNetType nettype; + virNWFilterDefPtr filter; + const char *ifname; + virNWFilterHashTablePtr vars; + virNWFilterDriverStatePtr driver; + pthread_t thread; + /* start and end of lease list, ordered by lease time */ + struct iplease *start; + struct iplease *end; + bool die; +}; + +#define POLL_INTERVAL 10*1000 /* 10 secs */ + +struct iplease { + uint32_t ipl_ipaddr; + uint32_t ipl_server; + struct virNWFilterSnoopReq *ipl_req; + unsigned int ipl_timeout; + /* timer list */ + struct iplease *ipl_prev; + struct iplease *ipl_next; +}; + +static struct iplease *ipl_getbyip(struct iplease *start, uint32_t ipaddr); +static void ipl_update(struct iplease *pl, uint32_t timeout); + + +/* + * ipl_ladd - add an IP lease to a list + */ +static void +ipl_ladd(struct iplease *plnew, struct iplease **start, struct iplease **end) +{ + struct iplease *pl; + + plnew->ipl_next = plnew->ipl_prev = 0; + if (!*start) { + plnew->ipl_prev = plnew->ipl_next = 0; same as done 2 lines above? + *start = *end = plnew; + return; + } + for (pl = *end; pl&& plnew->ipl_timeout< pl->ipl_timeout; + pl = pl->ipl_prev) + /* empty */ ; + if (!pl) { + plnew->ipl_next = *start; + *start = plnew; + } else { + plnew->ipl_next = pl->ipl_next; + pl->ipl_next = plnew; + } + plnew->ipl_prev = pl; + if (plnew->ipl_next) + plnew->ipl_next->ipl_prev = plnew; + else + *end = plnew; +} + +/* + * ipl_tadd - add an IP lease to the timer list + */ +static void +ipl_tadd(struct iplease *plnew) +{ + struct virNWFilterSnoopReq *req = plnew->ipl_req; + + ipl_ladd(plnew,&req->start,&req->end); +} + +/* + * ipl_add - create or update an IP lease + */ +static void +ipl_add(struct iplease *plnew) +{ + struct iplease *pl; + int rc; + char ipbuf[20]; /* dotted decimal IP addr string */ + char *ipstr; + + pl = ipl_getbyip(plnew->ipl_req->start, plnew->ipl_ipaddr); + if (pl) { + ipl_update(pl, plnew->ipl_timeout); + return; + } + if (VIR_ALLOC(pl)< 0) { + virReportOOMError(); + return; + } + *pl = *plnew; + + if (!inet_ntop(AF_INET,&pl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_add inet_ntop " "failed (0x%08X)"), + pl->ipl_ipaddr); + VIR_FREE(pl); + return; + + } + + ipstr = strdup(ipbuf); + if (!ipstr) { + VIR_FREE(pl); + virReportOOMError(); + return; + } + rc = virNWFilterChangeVar(pl->ipl_req->conn, + pl->ipl_req->techdriver, + pl->ipl_req->nettype, + pl->ipl_req->filter, + pl->ipl_req->ifname, + pl->ipl_req->vars, + pl->ipl_req->driver, "IP", ipstr, 0); + if (rc) { + VIR_FREE(ipstr); + VIR_FREE(pl); + return; + } + ipl_tadd(pl); +} + +/* + * ipl_tdel - remove an IP lease from a list + */ +static void +ipl_ldel(struct iplease *ipl, struct iplease **start, struct iplease **end) +{ + if (ipl->ipl_prev) + ipl->ipl_prev->ipl_next = ipl->ipl_next; + else + *start = ipl->ipl_next; + if (ipl->ipl_next) + ipl->ipl_next->ipl_prev = ipl->ipl_prev; + else + *end = ipl->ipl_prev; + ipl->ipl_next = ipl->ipl_prev = 0; +} + +/* + * ipl_tdel - remove an IP lease from the timer list + */ +static void +ipl_tdel(struct iplease *ipl) +{ + struct virNWFilterSnoopReq *req = ipl->ipl_req; + + ipl_ldel(ipl,&req->start,&req->end); + ipl->ipl_timeout = 0; +} + +/* + * ipl_del - delete an IP lease + */ +static void +ipl_del(struct iplease *ipl) +{ + struct virNWFilterSnoopReq *req; + char ipbuf[20]; /* dotted decimal IP addr string */ + char *ipstr; + + if (!ipl) + return; + + req = ipl->ipl_req; + + ipl_tdel(ipl); + + if (inet_ntop(AF_INET,&ipl->ipl_ipaddr, ipbuf, sizeof(ipbuf))) { + ipstr = strdup(ipbuf); + if (virNWFilterChangeVar(req->conn, + req->techdriver, + req->nettype, + req->filter, + req->ifname, + req->vars, req->driver, "IP", ipstr, 1)) + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_del virNWFilterChangeVar failed; " + "filter not deleted")); + } else + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("ipl_del inet_ntop " "failed (0x%08X)"), + ipl->ipl_ipaddr); + VIR_FREE(ipl); +} + +/* + * ipl_update - update the timeout on an IP lease + */ +static void +ipl_update(struct iplease *ipl, uint32_t timeout) +{ + ipl_tdel(ipl); + ipl->ipl_timeout = timeout; + ipl_tadd(ipl); + return; +} + +/* + * ipl_getbyip - lookup IP lease by IP address + */ +static struct iplease * +ipl_getbyip(struct iplease *start, uint32_t ipaddr) +{ + struct iplease *pl; + + for (pl = start; pl&& pl->ipl_ipaddr != ipaddr; pl = pl->ipl_next) + /* empty */ ; + return pl; +} + +#define GRACE 5 + +/* + * ipl_trun - run the IP lease timeout list + */ +static unsigned int +ipl_trun(struct virNWFilterSnoopReq *req) +{ + uint32_t now; + + now = time(0); + while (req->start&& req->start->ipl_timeout<= now) + ipl_del(req->start); + return 0; +} + +typedef unsigned char Eaddr[6]; + +struct eth { + Eaddr eh_dst; + Eaddr eh_src; + unsigned short eh_type; + unsigned char eh_data[0]; +}; + +struct dhcp { + unsigned char d_op; + unsigned char d_htype; + unsigned char d_hlen; + unsigned char d_hops; + unsigned int d_xid; + unsigned short d_secs; + unsigned short d_flags; + unsigned int d_ciaddr; + unsigned int d_yiaddr; + unsigned int d_siaddr; + unsigned int d_giaddr; + unsigned char d_chaddr[16]; + char d_sname[64]; + char d_file[128]; + unsigned char d_opts[0]; +}; + +/* DHCP options */ + +#define DHCPO_PAD 0 +#define DHCPO_LEASE 51 /* lease time in secs */ +#define DHCPO_MTYPE 53 /* message type */ +#define DHCPO_END 255 /* end of options */ + +/* DHCP message types */ +#define DHCPDECLINE 4 +#define DHCPACK 5 +#define DHCPRELEASE 7 + +unsigned char dhcp_magic[4] = { 99, 130, 83, 99 }; + +static int +dhcp_getopt(struct dhcp *pd, int len, int *pmtype, int *pleasetime) +{ + int oind, olen; + int oend; + + olen = len - sizeof *pd; + oind = 0; + + if (olen< 4) /* bad magic */ + return -1; + for (oind = 0; oind< sizeof dhcp_magic; ++oind) + if (pd->d_opts[oind] != dhcp_magic[oind]) + return -1; /* bad magic */ + Why not just use memcmp() ? + oend = 0; + + *pmtype = *pleasetime = 0; + + while (oind< olen) { + switch (pd->d_opts[oind]) { + case DHCPO_LEASE: + if (olen - oind< 6) + goto malformed; + if (*pleasetime) + return -1; /* duplicate lease time */ + *pleasetime = + ntohl(*(unsigned int *) (pd->d_opts + oind + 2)); I'd cast to (uint32_t *). + break; + case DHCPO_MTYPE: + if (olen - oind< 3) + goto malformed; + if (*pmtype) + return -1; /* duplicate message type */ + *pmtype = pd->d_opts[oind + 2]; + break; + case DHCPO_PAD: + oind++; + continue; + + case DHCPO_END: + oend = 1; + break; + default: + if (olen - oind< 2) + goto malformed; + } + if (oend) + break; + oind += pd->d_opts[oind + 1] + 2; + } + return 0; + malformed: + VIR_WARN("got lost in the options!"); + return -1; +} + +static void +dhcpdecode(struct virNWFilterSnoopReq *req, struct eth *pep, int len) +{ + struct iphdr *pip; + struct udphdr *pup; + struct dhcp *pd; + struct iplease ipl, *pipl; + int mtype, leasetime; + + /* go through the protocol headers */ + pip = (struct iphdr *) pep->eh_data; + len -= sizeof(*pep); + pup = (struct udphdr *) ((char *) pip + (pip->ihl<< 2)); + len -= pip->ihl<< 2; + pd = (struct dhcp *) ((char *) pup + sizeof(*pup)); + len -= sizeof(*pup); + if (len< 0) + return; /* dhcpdecode: invalid packet length */ + if (dhcp_getopt(pd, len,&mtype,&leasetime)< 0) + return; + + memset(&ipl, 0, sizeof(ipl)); + ipl.ipl_ipaddr = pd->d_yiaddr; + ipl.ipl_server = pd->d_siaddr; + if (leasetime == ~0) + ipl.ipl_timeout = ~0; + else + ipl.ipl_timeout = time(0) + leasetime; + ipl.ipl_req = req; + + switch (mtype) { + case DHCPACK: + ipl_add(&ipl); + break; + case DHCPDECLINE: + case DHCPRELEASE: + pipl = ipl_getbyip(req->start, ipl.ipl_ipaddr); + ipl_del(pipl); + break; + default: + break; + } +} + +#define PBUFSIZE 576 /*>= IP/TCP/DHCP headers */ + +static pcap_t * +dhcpopen(const char *intf) +{ + pcap_t *handle; + struct bpf_program fp; + char filter[64]; + char pcap_errbuf[PCAP_ERRBUF_SIZE]; + + handle = pcap_open_live(intf, PBUFSIZE, 0, POLL_INTERVAL, pcap_errbuf); + if (handle == NULL) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_open_live: %s"), pcap_errbuf); + return 0; + } + + sprintf(filter, "port 67 or dst port 68"); + if (pcap_compile(handle,&fp, filter, 1, PCAP_NETMASK_UNKNOWN) != 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_compile: %s"), pcap_geterr(handle)); + return 0; + } + if (pcap_setfilter(handle,&fp) != 0) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("pcap_setfilter: %s"), pcap_geterr(handle)); + return 0; + } + pcap_freecode(&fp); + return handle; +} + +static void * +virNWFilterDHCPSnoop(void *req0) +{ + struct virNWFilterSnoopReq *req = req0; + pcap_t *handle; + struct pcap_pkthdr hdr; + struct eth *packet; + struct iplease *ipl; + int ifindex; + + handle = dhcpopen(req->ifname); + if (!handle) + return 0; + + ifindex = if_nametoindex(req->ifname); + + while (1) { + if (req->die) + break; + ipl_trun(req); + + packet = (struct eth *) pcap_next(handle,&hdr); + + if (!packet) { + if (ifaceCheck(false, req->ifname, NULL, ifindex)) + virNWFilterDHCPSnoopEnd(req->ifname); + continue; + } + + dhcpdecode(req, packet, hdr.caplen); + } + /* free all leases */ + for (ipl = req->start; ipl; ipl = req->start) + ipl_del(ipl); + + /* free all req data */ + VIR_FREE(req->ifname); + virNWFilterHashTableFree(req->vars); + VIR_FREE(req); + return 0; +} + +int +virNWFilterDHCPSnoopReq(virConnectPtr conn, + virNWFilterTechDriverPtr techdriver ATTRIBUTE_UNUSED, + enum virDomainNetType nettype ATTRIBUTE_UNUSED, + virNWFilterDefPtr filter ATTRIBUTE_UNUSED, + const char *ifname ATTRIBUTE_UNUSED, + virNWFilterHashTablePtr vars ATTRIBUTE_UNUSED, + virNWFilterDriverStatePtr driver ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req; + + req = virHashLookup(SnoopReqs, ifname); + if (req) + return 0; + if (VIR_ALLOC(req)< 0) { + virReportOOMError(); + return 1; + } + + req->conn = conn; + req->techdriver = techdriver; + req->nettype = nettype; + req->filter = filter; + req->ifname = strdup(ifname); + req->vars = virNWFilterHashTableCreate(0); + if (!req->vars) { + virReportOOMError(); + Free req + req->ifname? return 1; + } + if (virNWFilterHashTablePutAll(vars, req->vars)) { + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq: can't copy variables" + " on if %s"), ifname); Same as above? Consider an error_exit label to goto. + return 1; + } + req->driver = driver; + req->start = req->end = 0; + + if (virHashAddEntry(SnoopReqs, ifname, req)) { Free req->ifname. + VIR_FREE(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq req add failed on" + "interface \"%s\""), ifname); + return 1; + } + if (pthread_create(&req->thread, NULL, virNWFilterDHCPSnoop, req) != 0) { + (void) virHashRemoveEntry(SnoopReqs, ifname); Free req->ifname. + VIR_FREE(req); + virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, + _("virNWFilterDHCPSnoopReq pthread_create failed" + " oninterface \"%s\""), ifname); + return 1; + } + return 0; +} + +/* + * freeReq - hash table free function to kill a request + */ +static void +freeReq(void *req0, const void *name ATTRIBUTE_UNUSED) +{ + struct virNWFilterSnoopReq *req = (struct virNWFilterSnoopReq *) req0; + + if (!req) + return; + + req->die = 1; +} + +int +virNWFilterDHCPSnoopInit(void) +{ + if (SnoopReqs) + return 0; + SnoopReqs = virHashCreate(0, freeReq); + if (!SnoopReqs) { + virReportOOMError(); + return -1; + } + return 0; +} + +void +virNWFilterDHCPSnoopEnd(const char *ifname) +{ + if (!SnoopReqs) + return; + if (ifname) + virHashRemoveEntry(SnoopReqs, ifname); + else /* free all of them */ + virHashFree(SnoopReqs); +} diff --git a/src/nwfilter/nwfilter_dhcpsnoop.h b/src/nwfilter/nwfilter_dhcpsnoop.h new file mode 100644 index 0000000..248b1a0 --- /dev/null +++ b/src/nwfilter/nwfilter_dhcpsnoop.h @@ -0,0 +1,36 @@ +/* + * nwfilter_dhcpsnoop.h: support DHCP snooping for a VM on an interface + * + * Copyright (C) 2010 IBM Corp. + * Copyright (C) 2010 David L Stevens + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Author: David L Stevens<dlstevens@us.ibm.com> + */ + +#ifndef __NWFILTER_DHCPSNOOP_H +# define __NWFILTER_DHCPSNOOP_H + +int virNWFilterDHCPSnoopInit(void); +int virNWFilterDHCPSnoopReq(virConnectPtr conn, + virNWFilterTechDriverPtr techdriver, + enum virDomainNetType, + virNWFilterDefPtr filter, + const char *ifname, + virNWFilterHashTablePtr vars, + virNWFilterDriverStatePtr driver); +void virNWFilterDHCPSnoopEnd(const char *ifname); +#endif /* __NWFILTER_DHCPSNOOP_H */ diff --git a/src/nwfilter/nwfilter_driver.c b/src/nwfilter/nwfilter_driver.c index a735059..57eb52c 100644 --- a/src/nwfilter/nwfilter_driver.c +++ b/src/nwfilter/nwfilter_driver.c @@ -39,6 +39,7 @@ #include "nwfilter_gentech_driver.h" #include "configmake.h"

+#include "nwfilter_dhcpsnoop.h" #include "nwfilter_learnipaddr.h"

#define VIR_FROM_THIS VIR_FROM_NWFILTER @@ -66,6 +67,8 @@ static int nwfilterDriverStartup(int privileged) { char *base = NULL;

+ if (virNWFilterDHCPSnoopInit()< 0) + return -1; if (virNWFilterLearnInit()< 0) return -1;

@@ -127,6 +130,7 @@ alloc_err_exit:

conf_init_err: virNWFilterTechDriversShutdown(); + virNWFilterDHCPSnoopEnd(0); Pass 'NULL' to indicate a pointer. virNWFilterLearnShutdown();

return -1; @@ -201,6 +205,7 @@ nwfilterDriverShutdown(void) {

virNWFilterConfLayerShutdown(); virNWFilterTechDriversShutdown(); + virNWFilterDHCPSnoopEnd(0); Same as above. virNWFilterLearnShutdown();

nwfilterDriverLock(driverState); diff --git a/src/nwfilter/nwfilter_gentech_driver.c b/src/nwfilter/nwfilter_gentech_driver.c index 563a1f3..577b48d 100644 --- a/src/nwfilter/nwfilter_gentech_driver.c +++ b/src/nwfilter/nwfilter_gentech_driver.c @@ -33,6 +33,7 @@ #include "virterror_internal.h" #include "nwfilter_gentech_driver.h" #include "nwfilter_ebiptables_driver.h" +#include "nwfilter_dhcpsnoop.h" #include "nwfilter_learnipaddr.h"

@@ -42,6 +43,8 @@ #define NWFILTER_STD_VAR_MAC "MAC" #define NWFILTER_STD_VAR_IP "IP"

+#define NWFILTER_DFLT_LEARN "DHCP" + static int _virNWFilterTeardownFilter(const char *ifname);

@@ -747,6 +750,8 @@ virNWFilterInstantiate(virConnectPtr conn, void **ptrs = NULL; int instantiate = 1; char *buf; + const char *learning = NWFILTER_DFLT_LEARN; + bool reportIP = false;

virNWFilterHashTablePtr missing_vars = virNWFilterHashTableCreate(0); if (!missing_vars) { @@ -764,39 +769,65 @@ virNWFilterInstantiate(virConnectPtr conn, if (rc) goto err_exit;

+ learning = virHashLookup(vars->hashTable, "ip_learning"); + if (virHashSize(missing_vars->hashTable) == 1) { if (virHashLookup(missing_vars->hashTable, NWFILTER_STD_VAR_IP) != NULL) { - if (virNWFilterLookupLearnReq(ifindex) == NULL) { - rc = virNWFilterLearnIPAddress(techdriver, - ifname, - ifindex, - linkdev, - nettype, macaddr, - filter->name, - vars, driver, - DETECT_DHCP|DETECT_STATIC); + if (strcasecmp(learning, "none") == 0) { /* no learning */ + reportIP = true; + goto err_unresolvable_vars; } - goto err_exit; - } - goto err_unresolvable_vars; + if (strcasecmp(learning, "dhcp") == 0) { + rc = _virNWFilterInstantiateRec(conn, + techdriver, + nettype, + filter, + ifname, + vars, + NWFILTER_STD_VAR_IP, 1, +&nEntries,&insts, + useNewFilter, foundNewFilter, + driver); + if (!rc) + rc = virNWFilterDHCPSnoopReq(conn, techdriver, nettype, + filter, ifname, vars, driver); + } else if (strcasecmp(learning, "any") == 0) { + if (virNWFilterLookupLearnReq(ifindex) == NULL) { + rc = virNWFilterLearnIPAddress(techdriver, + ifname, + ifindex, + linkdev, + nettype, macaddr, + filter->name, + vars, driver, + DETECT_DHCP|DETECT_STATIC); + } + goto err_exit; + } else { + rc = 1; + virNWFilterReportError(VIR_ERR_PARSE_FAILED, _("filter '%s' " + "learning value '%s' invalid."), + filter->name, learning); + } + } else + goto err_unresolvable_vars; } else if (virHashSize(missing_vars->hashTable)> 1) { goto err_unresolvable_vars; } else if (!forceWithPendingReq&& virNWFilterLookupLearnReq(ifindex) != NULL) { goto err_exit; - } - - rc = _virNWFilterInstantiateRec(conn, - techdriver, - nettype, - filter, - ifname, - vars, - 0, 0, -&nEntries,&insts, - useNewFilter, foundNewFilter, - driver); + } else + rc = _virNWFilterInstantiateRec(conn, + techdriver, + nettype, + filter, + ifname, + vars, + 0, 0, +&nEntries,&insts, + useNewFilter, foundNewFilter, + driver);

if (rc) goto err_exit; @@ -848,7 +879,7 @@ err_exit:

err_unresolvable_vars:

- buf = virNWFilterPrintVars(missing_vars->hashTable, ", ", false, false); + buf = virNWFilterPrintVars(missing_vars->hashTable, ", ", false, reportIP); if (buf) { virNWFilterReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot instantiate filter due to unresolvable " @@ -1180,6 +1211,8 @@ _virNWFilterTeardownFilter(const char *ifname) return 1; }

+ virNWFilterDHCPSnoopEnd(ifname); + virNWFilterTerminateLearnReq(ifname);

if (virNWFilterLockIface(ifname))

Some nits to address. Otherwise looks ok, but didn't try it. Stefan