Bug 32806

Summary:	Reproducible panic in ipfw
Product:	Base System	Reporter:	Dag-Erling Smorgrav <des>
Component:	kern	Assignee:	ru <ru>
Status:	Closed FIXED
Severity:	Affects Only Me
Priority:	Normal
Version:	5.0-CURRENT
Hardware:	Any
OS:	Any

Description Dag-Erling Smorgrav 2001-12-13 17:20:01 UTC

If an outgoing packet originating on the local machine hits an "unreach" rule
in ipfw, a panic ensues in icmp_reflect() because there is no receiving
interface on which to transmit the ICMP unreachable.

Fix: 

The code directly above the KASSERT already handles the case where the packet
that triggers the rule is destined for a local address.  Similar code should
be added to handle the case where the source address is a local address.
How-To-Repeat: 
# ipfw add 1 unreach host ip from any to 10.0.0.0/8
00001 unreach host up from any to 10.0.0.0/8
# ifconfig dc0 inet 10.0.0.1 netmask 0xff000000
# telnet 10.0.0.2
Trying 10.0.0.2...
panic: icmp_reflect: NULL rcvif

The panic comes from the KASSERT on line 612 of sys/netinet/ip_icmp.c.

Comment 1 Sheldon Hearn freebsd_committer

2001-12-30 12:31:25 UTC

Responsible Changed
From-To: freebsd-bugs->ru

Ruslan, can you take a look at this one?

Comment 2 Maxim Konovalov 2001-12-31 21:51:18 UTC

Please review the patch below:

Index: ip_icmp.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.65
diff -u -r1.65 ip_icmp.c
--- ip_icmp.c	14 Dec 2001 19:32:47 -0000	1.65
+++ ip_icmp.c	31 Dec 2001 21:47:47 -0000
@@ -601,14 +601,23 @@
 	t = ip->ip_dst;
 	ip->ip_dst = ip->ip_src;
 	/*
-	 * If the incoming packet was addressed directly to us,
-	 * use dst as the src for the reply.  Otherwise (broadcast
-	 * or anonymous), use the address which corresponds
-	 * to the incoming interface.
+	 * The incoming packet was addressed directly to us,
+	 * use dst as the src for the reply.
 	 */
 	LIST_FOREACH(ia, INADDR_HASH(t.s_addr), ia_hash)
 		if (t.s_addr == IA_SIN(ia)->sin_addr.s_addr)
 			goto match;
+	/*
+	 * Our outgoing packet hits an "unreach" ipfw rule,
+	 * use src as the src for the reply.
+	 */
+	LIST_FOREACH(ia, INADDR_HASH(ip->ip_src.s_addr), ia_hash)
+		if (ip->ip_src.s_addr == IA_SIN(ia)->sin_addr.s_addr)
+			goto match;
+	/*
+	 * The incoming packet is a broadcast or anonymous, use
+	 * the address which corresponds to the incoming interface.
+	 */
 	KASSERT(m->m_pkthdr.rcvif != NULL, ("icmp_reflect: NULL rcvif"));
 	if (m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
 		TAILQ_FOREACH(ifa, &m->m_pkthdr.rcvif->if_addrhead, ifa_link) {

-- 
Maxim Konovalov, MAcomnet, Internet-Intranet Dept., system engineer
phone: +7 (095) 796-9079, mailto: maxim@macomnet.ru

Comment 3 ru freebsd_committer

2002-01-11 10:56:25 UTC

On Tue, Jan 01, 2002 at 05:29:38PM +0300, Maxim Konovalov wrote:
> 
> Hello,
> 
> Luigi and Ruslan, could you please review the patch?
> 
I have a much simpler solution to this problem.

Index: ip_icmp.c
===================================================================
RCS file: /home/ncvs/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.65
diff -u -p -r1.65 ip_icmp.c
--- ip_icmp.c	2001/12/14 19:32:47	1.65
+++ ip_icmp.c	2002/01/11 10:50:08
@@ -609,8 +609,8 @@ icmp_reflect(m)
 	LIST_FOREACH(ia, INADDR_HASH(t.s_addr), ia_hash)
 		if (t.s_addr == IA_SIN(ia)->sin_addr.s_addr)
 			goto match;
-	KASSERT(m->m_pkthdr.rcvif != NULL, ("icmp_reflect: NULL rcvif"));
-	if (m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
+	if (m->m_pkthdr.rcvif != NULL &&
+	    m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
 		TAILQ_FOREACH(ifa, &m->m_pkthdr.rcvif->if_addrhead, ifa_link) {
 			if (ifa->ifa_addr->sa_family != AF_INET)
 				continue;

The case that causes a panic condition is then handled by
a routing table lookup below.  I think that this KASSERT()
was bogus in the first place.

The same patch applies cleanly to -STABLE as well.

I'd also like to MFC this change ASAP, hence the re@ CC:ed.


Cheers,
-- 
Ruslan Ermilov		Oracle Developer/DBA,
ru@sunbay.com		Sunbay Software AG,
ru@FreeBSD.org		FreeBSD committer,
+380.652.512.251	Simferopol, Ukraine

http://www.FreeBSD.org	The Power To Serve
http://www.oracle.com	Enabling The Information Age

Comment 4 Luigi Rizzo freebsd_committer

2002-01-11 11:00:32 UTC

fine with me

	luigi

On Fri, Jan 11, 2002 at 12:56:25PM +0200, Ruslan Ermilov wrote:
> On Tue, Jan 01, 2002 at 05:29:38PM +0300, Maxim Konovalov wrote:
> > 
> > Hello,
> > 
> > Luigi and Ruslan, could you please review the patch?
> > 
> I have a much simpler solution to this problem.
> 
> Index: ip_icmp.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 ip_icmp.c
> --- ip_icmp.c	2001/12/14 19:32:47	1.65
> +++ ip_icmp.c	2002/01/11 10:50:08
> @@ -609,8 +609,8 @@ icmp_reflect(m)
>  	LIST_FOREACH(ia, INADDR_HASH(t.s_addr), ia_hash)
>  		if (t.s_addr == IA_SIN(ia)->sin_addr.s_addr)
>  			goto match;
> -	KASSERT(m->m_pkthdr.rcvif != NULL, ("icmp_reflect: NULL rcvif"));
> -	if (m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
> +	if (m->m_pkthdr.rcvif != NULL &&
> +	    m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
>  		TAILQ_FOREACH(ifa, &m->m_pkthdr.rcvif->if_addrhead, ifa_link) {
>  			if (ifa->ifa_addr->sa_family != AF_INET)
>  				continue;
> 
> The case that causes a panic condition is then handled by
> a routing table lookup below.  I think that this KASSERT()
> was bogus in the first place.
> 
> The same patch applies cleanly to -STABLE as well.
> 
> I'd also like to MFC this change ASAP, hence the re@ CC:ed.
> 
> 
> Cheers,
> -- 
> Ruslan Ermilov		Oracle Developer/DBA,
> ru@sunbay.com		Sunbay Software AG,
> ru@FreeBSD.org		FreeBSD committer,
> +380.652.512.251	Simferopol, Ukraine
> 
> http://www.FreeBSD.org	The Power To Serve
> http://www.oracle.com	Enabling The Information Age

Comment 5 Maxim Konovalov 2002-01-11 11:15:19 UTC

On 12:56+0200, Jan 11, 2002, Ruslan Ermilov wrote:

> On Tue, Jan 01, 2002 at 05:29:38PM +0300, Maxim Konovalov wrote:
> >
> > Hello,
> >
> > Luigi and Ruslan, could you please review the patch?
> >
> I have a much simpler solution to this problem.

Yep, it is better than mine.

> Index: ip_icmp.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/netinet/ip_icmp.c,v
> retrieving revision 1.65
> diff -u -p -r1.65 ip_icmp.c
> --- ip_icmp.c	2001/12/14 19:32:47	1.65
> +++ ip_icmp.c	2002/01/11 10:50:08
> @@ -609,8 +609,8 @@ icmp_reflect(m)
>  	LIST_FOREACH(ia, INADDR_HASH(t.s_addr), ia_hash)
>  		if (t.s_addr == IA_SIN(ia)->sin_addr.s_addr)
>  			goto match;
> -	KASSERT(m->m_pkthdr.rcvif != NULL, ("icmp_reflect: NULL rcvif"));
> -	if (m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
> +	if (m->m_pkthdr.rcvif != NULL &&
> +	    m->m_pkthdr.rcvif->if_flags & IFF_BROADCAST) {
>  		TAILQ_FOREACH(ifa, &m->m_pkthdr.rcvif->if_addrhead, ifa_link) {
>  			if (ifa->ifa_addr->sa_family != AF_INET)
>  				continue;
>
> The case that causes a panic condition is then handled by
> a routing table lookup below.  I think that this KASSERT()
> was bogus in the first place.
>
> The same patch applies cleanly to -STABLE as well.
>
> I'd also like to MFC this change ASAP, hence the re@ CC:ed.
>
>
> Cheers,
>

-- 
Maxim Konovalov, MAcomnet, Internet-Intranet Dept., system engineer
phone: +7 (095) 796-9079, mailto: maxim@macomnet.ru

Comment 6 Erwin Lansing 2002-01-11 11:48:36 UTC

On Fri, Jan 11, 2002 at 12:56:25PM +0200, Ruslan Ermilov wrote:
> On Tue, Jan 01, 2002 at 05:29:38PM +0300, Maxim Konovalov wrote:
> > 
> > Hello,

Hep,

> > 
> > Luigi and Ruslan, could you please review the patch?
> > 
> I have a much simpler solution to this problem.
> 
[snip patch]
> 
> The case that causes a panic condition is then handled by
> a routing table lookup below.  I think that this KASSERT()
> was bogus in the first place.
> 
> The same patch applies cleanly to -STABLE as well.
> 
> I'd also like to MFC this change ASAP, hence the re@ CC:ed.
> 
This new patch works like a charm

/erwin

-- 
Erwin Lansing 		-- 		http://droso.org

I love deadlines.                       -- Douglas Adams
I love the whooshing sound they make as the fly by.

Comment 7 ru freebsd_committer

2002-01-11 12:15:15 UTC

State Changed
From-To: open->feedback

Fix committed into 5.0-CURRENT (sys/netinet/ip_icmp.c,v 1.66). 

Awaiting approval from re@ to MFC.

Comment 8 ru freebsd_committer

2002-01-14 07:54:43 UTC

State Changed
From-To: feedback->closed

Fixed in 4.5-RC, src/sys/netinet/ip_icmp.c,v 1.39.2.14.