[ovs-dev] [PATCH] Add Nicira extension to OpenFlow for dropping spoofed ARP packets.

[Ben Pfaff]

On Wed, Aug 25, 2010 at 09:33:08PM -0700, Jesse Gross wrote:
> On Tue, Aug 24, 2010 at 7:03 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> Even with this and appropriate MAC and IP flows you can still poison
> >> someone's ARP cache (by responding with someone else's IP).  You may not
> >> be
> >> able to see their traffic but you can DoS them.
> >
> > The IP fields in the ARP packet are part of the flow, so it's the
> > controller's problem to ensure that the host can't poison someone's ARP
> > cache, by using flows to restrict the possible responses.  We only need
> > this action because the MAC fields in the ARP packet are not part of the
> > flow.
> 
> Right, it completely slipped my mind that we were already copying the
> IP addresses into the flow.
> 
> >> If this is used on a datapath that doesn't support this action (i.e. the
> >> userspace datapath) it will be silently ignored, right?  That makes me
> >> nervous.
> >
> > The best I can do is to implement it in the userspace datapath, unless
> > you have a better idea.  I've done that now.
> 
> Good enough.
> 
> The new version looks good with one little issue:
> 
> > +       arp = (struct arp_eth_header *)skb_network_header(skb);
> > +       return (arp->ar_hrd != htons(ARPHRD_ETHER) ||
> > +               arp->ar_pro != htons(ETH_P_IP) ||
> > +               arp->ar_hln != ETH_ALEN ||
> > +               arp->ar_pln != 4 ||
> > +               !compare_ether_addr(arp->ar_sha, eth_hdr(skb)->h_source));
> 
> compare_ether_addr() returns zero on equality so this will have the
> opposite effect...

Argh, I looked at that twice but it seems that I should have looked at
it three times.

> I know that you were waiting to hear more comments on this before
> pushing but I think it is worth doing now.  We've had a lot of
> requests for this and it is definitely an improvement over what we
> have now.

I got adequate feedback from Paul.  I'll push it after I fix another bug
that Justin reported to me.