From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54337) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkKzP-0000bo-No for qemu-devel@nongnu.org; Tue, 13 May 2014 18:16:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkKzH-00008Q-Mi for qemu-devel@nongnu.org; Tue, 13 May 2014 18:15:59 -0400 Received: from toccata.ens-lyon.fr ([140.77.166.68]:41965 helo=toccata.ens-lyon.org) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkKzH-000073-8o for qemu-devel@nongnu.org; Tue, 13 May 2014 18:15:51 -0400 Date: Wed, 14 May 2014 00:15:47 +0200 From: Samuel Thibault Message-ID: <20140513221547.GE6302@type.youpi.perso.aquilenet.fr> References: <1396218189-14422-1-git-send-email-samuel.thibault@ens-lyon.org> <1396218189-14422-2-git-send-email-samuel.thibault@ens-lyon.org> <20140507221509.GA3302@type.youpi.perso.aquilenet.fr> <20140508061018.GB7523@hostname> <20140508065033.GT6261@type.youpi.perso.aquilenet.fr> <20140508065921.GH7381@hostname> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20140508065921.GH7381@hostname> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH, DoS] slirp (arp): do not special-case bogus IP addresses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: qemu-devel@nongnu.org Edgar E. Iglesias, le Thu 08 May 2014 06:59:22 +0000, a =E9crit : > 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 =E9crit : > > > The search part looks OK to me but when adding to the arp table, do= n't > > > you at least want to avoid adding mappings for 0.0.0.0/32? > >=20 > > I don't see the gain, actually. It would mean burning some CPU all t= he > > time just to save a small potential memory loss and CPU burning in th= e > > rare case when the guest behaves oddly. > >=20 > > > to avoid for ex garps to pollute the cache with invalid entries? > >=20 > > Only one entry will be created and updated by garps. The guest alread= y > > has a lot of ways to pollute the cache :) >=20 > 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 :) 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? 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, uin= t8_t ethaddr[ETH_ALEN]) ethaddr[0], ethaddr[1], ethaddr[2], ethaddr[3], ethaddr[4], ethaddr[5])); =20 - /* Check 0.0.0.0/8 invalid source-only addresses */ - if ((ip_addr & htonl(~(0xfU << 28))) =3D=3D 0) { - return; - } - - if (ip_addr =3D=3D 0xffffffff || ip_addr =3D=3D broadcast_addr) { + if (ip_addr =3D=3D 0 || ip_addr =3D=3D 0xffffffff || ip_addr =3D=3D = 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 =3D 0x%x", ip_addr); =20 - /* Check 0.0.0.0/8 invalid source-only addresses */ - assert((ip_addr & htonl(~(0xfU << 28))) !=3D 0); - /* If broadcast address */ - if (ip_addr =3D=3D 0xffffffff || ip_addr =3D=3D broadcast_addr) { + if (ip_addr =3D=3D 0 || ip_addr =3D=3D 0xffffffff || ip_addr =3D=3D = broadcast_addr) { /* return Ethernet broadcast address */ memset(out_ethaddr, 0xff, ETH_ALEN); return 1;