Re: [Qemu-devel] [PATCH, DoS] slirp (arp): do not special-case bogus IP addresses

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Wed, May 14, 2014 at 12:15:47AM +0200, Samuel Thibault wrote:
> Edgar E. Iglesias, le Thu 08 May 2014 06:59:22 +0000, a écrit :
> > On Thu, May 08, 2014 at 08:50:33AM +0200, Samuel Thibault wrote:
> > > Edgar E. Iglesias, le Thu 08 May 2014 06:10:18 +0000, a écrit :
> > > > The search part looks OK to me but when adding to the arp table, don't
> > > > you at least want to avoid adding mappings for 0.0.0.0/32?
> > > 
> > > I don't see the gain, actually.  It would mean burning some CPU all the
> > > time just to save a small potential memory loss and CPU burning in the
> > > rare case when the guest behaves oddly.
> > > 
> > > > to avoid for ex garps to pollute the cache with invalid entries?
> > > 
> > > Only one entry will be created and updated by garps. The guest already
> > > has a lot of ways to pollute the cache :)
> > 
> > I was under the impression that entries for 0.0.0.0 are strictly
> > invalid (not about performance). I might be wrong though.
> 
> I'd tend to think that, but what should be done? I don't think we want
> an assert failure :)
> 

Right, I didn't mean that the assert should stay.

> At best I could think of using the patch below, which avoids registering
> anything for 0.0.0.0, and use a broadcast to answer a guest which
> would have used 0.0.0.0 as a source for whatever reason.  I don't find
> anything else reasonable.  What would be preferred?

Specs are not super clear on this but rfc1700 says that 0.0.0.0 is a source only address.
http://www.rfc-editor.org/rfc/rfc1700.txt Page 3
http://tools.ietf.org/html/draft-iana-special-ipv4-04 Page 1

What I was trying to suggest was a mix between your two versions.
Removing the assert in table_search and avoid adding 0.0.0.0/32 to the cache
in table_add. We might need to complement with something that drops datagrams
destined to 0.0.0.0 in upper layers so we dont keep trying, not sure.
Does something like that make sense?

Cheers,
Edgar


> 
> Samuel
> 
> diff --git a/slirp/arp_table.c b/slirp/arp_table.c
> index ecdb0ba..d160cfc 100644
> --- a/slirp/arp_table.c
> +++ b/slirp/arp_table.c
> @@ -37,12 +37,7 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN])
>                  ethaddr[0], ethaddr[1], ethaddr[2],
>                  ethaddr[3], ethaddr[4], ethaddr[5]));
>  
> -    /* Check 0.0.0.0/8 invalid source-only addresses */
> -    if ((ip_addr & htonl(~(0xfU << 28))) == 0) {
> -        return;
> -    }
> -
> -    if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
> +    if (ip_addr == 0 || ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
>          /* Do not register broadcast addresses */
>          return;
>      }
> @@ -73,11 +68,8 @@ bool arp_table_search(Slirp *slirp, uint32_t ip_addr,
>      DEBUG_CALL("arp_table_search");
>      DEBUG_ARG("ip = 0x%x", ip_addr);
>  
> -    /* Check 0.0.0.0/8 invalid source-only addresses */
> -    assert((ip_addr & htonl(~(0xfU << 28))) != 0);
> -
>      /* If broadcast address */
> -    if (ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
> +    if (ip_addr == 0 || ip_addr == 0xffffffff || ip_addr == broadcast_addr) {
>          /* return Ethernet broadcast address */
>          memset(out_ethaddr, 0xff, ETH_ALEN);
>          return 1;