igmp: possible NULL dereference after GFP_ATOMIC allocation?

igmpv3_newpack() uses alloc_skb() with GFP_ATOMIC. It fails, igmpv3_newpack() returns NULL. add_grhead(), sees NULL, returns NULL.

At one place add_grhead() return value fed into skb_put() which dereferences it.

net/ipv4/igmp.c:
454 if (first) {
455 skb = add_grhead(skb, pmc, type, &pgr);
456 first = 0;
457 }
458 psrc = (u32 *)skb_put(skb, sizeof(u32));

Alexey,

        I think you're correct-- looks like it needs:

        if (!skb)
                return NULL;

just before the skb_put(), since an allocation (and failure) could occur in either the igmpv3_newpack() call or in add_grhead().
Also, similar code in net/ipv6/mcast..c.

Will you be submitting the patch?

+-DLS

One box here can semi-reliably reproduce oops which is attributed to these allocation failures. I'll tell if patch works or not soon.

BUG: unable to handle kernel NULL pointer dereference at virtual address 00000064
 EIP is at add_grec+0x22b/0x372
 eax: 00000000   ebx: 00000000   ecx: f567cb20   edx: f567cb20
 esi: e91ad540   edi: 00000001   ebp: 00000000   esp: c0787f54
 ds: 007b   es: 007b   ss: 0068
 Process swapper (pid: 0, veid: 0, ti=c0787000 task=c0670be0 task.ti=c073e000)
 Stack: c0787f80 00000001 00000005 e08a8920 c930a800 00000000 e08a892c 00000000
        00000000 00000000 00000000 00000000 e08a8920 e2000ea0 00000000 00000005
        c05f1510 00000000 00000001 00000000 c0678580 c07fbb00 00000100 c05f13ac
 Call Trace:
  [<c05f1510>] igmp_ifc_timer_expire+0x164/0x1db
  [<c042cd17>] run_timer_softirq+0x116/0x18d
  [<c0427ea8>] __do_softirq+0x84/0x109
  [<c0406234>] do_softirq+0x55/0xad
  =======================
 Code: fa ff ff 89 c3 c7 44 24 1c 00 00 00 00 eb 04 85 ff 74 18 8b 4c 24 08 8d 44 24 2c 8b 54 24 0c 89 04 24 89 d8 e8 66 fc ff ff 89 c3 <83> 7b 64 00 8b bb a0 00 00 00 74 0b 0f 0b 66 b8 5a 03 b8 52 e9
 EIP: [<c05f1265>] add_grec+0x22b/0x372 SS:ESP 0068:c0787f54
 Kernel panic - not syncing: Fatal exception in interrupt

I also think this "if" is needed there and hope you are right but, actually, skb allocating errors shouldn't be so semi-reliable.

I don't know what is the kernel version of this box, but probably another reason could be in_device not fully initialized, so you could check for David's Stevens patch to devinet.c.

Regards,
Jarek P.

OK, now that we aren't seeing crashes which can be attributed to these NULL dereferences any longer.
--------------------------------------
add_grhead() allocates memory with GFP_ATOMIC and in at least two places skb from it passed to skb_put() without checking.

---

 net/ipv4/igmp.c  |    2 ++
 net/ipv6/mcast.c |    2 ++
 2 files changed, 4 insertions(+)

@@ -455,6 +455,8 @@ static struct sk_buff *add_grec(struct s
 			skb = add_grhead(skb, pmc, type, &pgr);
 			first = 0;
 		}
+		if (!skb)
+			return NULL;
 		psrc = (__be32 *)skb_put(skb, sizeof(__be32));
 		*psrc = psf->sf_inaddr;
 		scount++; stotal++;

@@ -1582,6 +1582,8 @@ static struct sk_buff *add_grec(struct s
 			skb = add_grhead(skb, pmc, type, &pgr);
 			first = 0;
 		}
+		if (!skb)
+			return NULL;
 		psrc = (struct in6_addr *)skb_put(skb, sizeof(*psrc));
 		*psrc = psf->sf_addr;
 		scount++; stotal++;

From: Alexey Dobriyan <[email protected]>
Date: Mon, 5 Feb 2007 18:04:33 +0300

Thanks for the patch and for testing Alexey.

Applied, and I'll push this to -stable too.

Thanks again.