Re: [Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.
On 2012-11-12 15:41, Nickolai Zeldovich wrote: > On Mon, Nov 12, 2012 at 4:37 AM, Jan Kiszka wrote: >> On 2012-11-12 01:59, Nickolai Zeldovich wrote: >>> LWIP can generate packets with a source of 0.0.0.0, which triggers an >>> assertion failure in arp_table_add(). Instead of crashing, simply return >>> to avoid adding an invalid ARP table entry. >> >> I would prefer to filter out such invalid packets at a different level. >> Did you analyzed which path it takes through the stack? > > The particular packet that crashed qemu for me was a gratuitous ARP, > though it looks like all three calls to arp_table_add() in arp_input() > can trigger this. > > Popping up one level, I'm not sure why arp_table_add() and > arp_table_search() need a special case for 0.0.0.0/8 in the first > place. I couldn't find any other code that assumes the ARP table > cannot contain 0.0.0.0/8 entries. Would anything break if the check > for 0.0.0.0/8 was removed from arp_table_add() and arp_table_search() > altogether? 0.0.0.0/8 are source-only, invalid as destination. So they have no place in the ARP table. OK, let's follow your path and filter them in arp_table_add. Just add the missing braces and resend. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.
On Mon, Nov 12, 2012 at 4:37 AM, Jan Kiszka wrote: > On 2012-11-12 01:59, Nickolai Zeldovich wrote: >> LWIP can generate packets with a source of 0.0.0.0, which triggers an >> assertion failure in arp_table_add(). Instead of crashing, simply return >> to avoid adding an invalid ARP table entry. > > I would prefer to filter out such invalid packets at a different level. > Did you analyzed which path it takes through the stack? The particular packet that crashed qemu for me was a gratuitous ARP, though it looks like all three calls to arp_table_add() in arp_input() can trigger this. Popping up one level, I'm not sure why arp_table_add() and arp_table_search() need a special case for 0.0.0.0/8 in the first place. I couldn't find any other code that assumes the ARP table cannot contain 0.0.0.0/8 entries. Would anything break if the check for 0.0.0.0/8 was removed from arp_table_add() and arp_table_search() altogether? Nickolai.
Re: [Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.
On 2012-11-12 01:59, Nickolai Zeldovich wrote: > LWIP can generate packets with a source of 0.0.0.0, which triggers an > assertion failure in arp_table_add(). Instead of crashing, simply return > to avoid adding an invalid ARP table entry. I would prefer to filter out such invalid packets at a different level. Did you analyzed which path it takes through the stack? > > Signed-off-by: Nickolai Zeldovich > --- > slirp/arp_table.c |3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/slirp/arp_table.c b/slirp/arp_table.c > index 5d7b8ac..3318ce9 100644 > --- a/slirp/arp_table.c > +++ b/slirp/arp_table.c > @@ -38,7 +38,8 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t > ethaddr[ETH_ALEN]) > ethaddr[3], ethaddr[4], ethaddr[5])); > > /* Check 0.0.0.0/8 invalid source-only addresses */ > -assert((ip_addr & htonl(~(0xf << 28))) != 0); > +if ((ip_addr & htonl(~(0xf << 28))) == 0) > +return; Please follow our coding style. There is also checkpatch.pl to help you. > > if (ip_addr == 0x || ip_addr == broadcast_addr) { > /* Do not register broadcast addresses */ > Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux
[Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.
LWIP can generate packets with a source of 0.0.0.0, which triggers an assertion failure in arp_table_add(). Instead of crashing, simply return to avoid adding an invalid ARP table entry. Signed-off-by: Nickolai Zeldovich --- slirp/arp_table.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/slirp/arp_table.c b/slirp/arp_table.c index 5d7b8ac..3318ce9 100644 --- a/slirp/arp_table.c +++ b/slirp/arp_table.c @@ -38,7 +38,8 @@ void arp_table_add(Slirp *slirp, uint32_t ip_addr, uint8_t ethaddr[ETH_ALEN]) ethaddr[3], ethaddr[4], ethaddr[5])); /* Check 0.0.0.0/8 invalid source-only addresses */ -assert((ip_addr & htonl(~(0xf << 28))) != 0); +if ((ip_addr & htonl(~(0xf << 28))) == 0) +return; if (ip_addr == 0x || ip_addr == broadcast_addr) { /* Do not register broadcast addresses */ -- 1.7.10.4