Message ID | 20140507221509.GA3302@type.youpi.perso.aquilenet.fr |
---|---|
State | New |
Headers | show |
On Thu, May 08, 2014 at 12:15:09AM +0200, Samuel Thibault wrote: > Do not special-case addresses with zero host part, as we do not > necessarily know how big it is, and the guest can fake them anyway. Hi Samuel, 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? to avoid for ex garps to pollute the cache with invalid entries? Cheers, Edgar > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > --- > > This is particularly bad actually, one can for instance simply do this > inside a Linux guest > > ip addr add 192.0.0.0/1 dev eth0 > > and crash qemu (thus a DoS) by just emitting a packet (thus from > 192.0.0.0), getting: > > qemu-system-x86_64: /usr/src/qemu/slirp/arp_table.c:77: arp_table_search: Assertion `(ip_addr & __bswap_32 (~(0xfU << 28))) != 0' failed. > > so it should probably go to all stable maintained versions. > > arp_table.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/slirp/arp_table.c b/slirp/arp_table.c > index ecdb0ba..243cbbc 100644 > --- a/slirp/arp_table.c > +++ b/slirp/arp_table.c > @@ -37,11 +37,6 @@ 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) { > /* Do not register broadcast addresses */ > return; > @@ -73,9 +68,6 @@ 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) { > /* return Ethernet broadcast address */ >
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 :) samuel
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 :) Hi, I was under the impression that entries for 0.0.0.0 are strictly invalid (not about performance). I might be wrong though. Cheers, Edgar
diff --git a/slirp/arp_table.c b/slirp/arp_table.c index ecdb0ba..243cbbc 100644 --- a/slirp/arp_table.c +++ b/slirp/arp_table.c @@ -37,11 +37,6 @@ 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) { /* Do not register broadcast addresses */ return; @@ -73,9 +68,6 @@ 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) { /* return Ethernet broadcast address */
Do not special-case addresses with zero host part, as we do not necessarily know how big it is, and the guest can fake them anyway. Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> --- This is particularly bad actually, one can for instance simply do this inside a Linux guest ip addr add 192.0.0.0/1 dev eth0 and crash qemu (thus a DoS) by just emitting a packet (thus from 192.0.0.0), getting: qemu-system-x86_64: /usr/src/qemu/slirp/arp_table.c:77: arp_table_search: Assertion `(ip_addr & __bswap_32 (~(0xfU << 28))) != 0' failed. so it should probably go to all stable maintained versions. arp_table.c | 8 -------- 1 file changed, 8 deletions(-)