Re: [Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.

2012-11-12 Thread Jan Kiszka 
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 nicko...@csail.mit.edu
 ---
  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

Re: [Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.

2012-11-12 Thread Nickolai Zeldovich 
On Mon, Nov 12, 2012 at 4:37 AM, Jan Kiszka jan.kis...@siemens.com 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.

2012-11-12 Thread Jan Kiszka 
On 2012-11-12 15:41, Nickolai Zeldovich wrote:
 On Mon, Nov 12, 2012 at 4:37 AM, Jan Kiszka jan.kis...@siemens.com 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

[Qemu-devel] [PATCH] slirp: Don't crash on packets from 0.0.0.0/8.

2012-11-11 Thread Nickolai Zeldovich 
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 nicko...@csail.mit.edu
---
 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