Bug 30524

Lars Eggert <larse@isi.edu> wrote:
> --- plain       Wed Sep 12 09:47:58 2001
> +++ ip_icmp.c   Wed Sep 12 09:37:54 2001
> @@ -639,9 +639,26 @@
>         /*
>          * The following happens if the packet was not addressed to us,
>          * and was received on an interface with no IP address.
> +        *
> +        * Prior versions simply set ia = in_ifaddrhead.tqh_first if it
> +        * was zero here. When in_ifaddrhead.tqh_first is also zero
> +        * (pointer gets dereferenced below), the kernel panics. It looks
> +        * like this can happen with PC-card interfaces, but I have not
> +        * investigated this fully.
> +        *
> +        * Instead, iterate over the TAILQ to find the first non-zero
> +        * interface address, and use that. If none can be found, skip
> +        * sending the ICMP packet.
> +        *                                              larse@isi.edu
>          */
> -       if (ia == (struct in_ifaddr *)0)
> -               ia = in_ifaddrhead.tqh_first;
> +       if (ia == (struct in_ifaddr *)0) {
> +               TAILQ_FOREACH(ia, &in_ifaddrhead, ia_link)
> +                       if (ia != (struct in_ifaddr *)0)
> +                               break;

This check doesn't make sense, because TAILQ_FOREACH expands
(effectively) into:

	for (ia = in_ifaddrhead.tqh_first; ia != NULL; ia = ia->tqh_next)
		/* I might've gotten some variable names wrong here. */

Thus, the if() inside it will *always* fail.  Essentially what you
want to do is check if the TAILQ is empty, and fail otherwise.  But...

Right now, the code in question assumes that if a packet gets to it,
there must be a valid interface.  However, this assumption was broken
in rev. 1.114 of ip_input.c, which says:

    ----------------------------
    revision 1.114
    date: 1999/02/09 16:55:46;  author: wollman;  state: Exp;  lines: +8 -10
    After wading in the cesspool of ip_input for an hour, I have managed to
    convince myself that nothing will break if we permit IP input while
    interface addresses are unconfigured.  (At worst, they will hit some
    ULP's PCB scan and fail if nobody is listening.)  So, remove the restriction
    that addresses must be configured before packets can be input.  Assume
    that any unicast packet we receive while unconfigured is potentially ours.
    ----------------------------

Unforunately, the commit log doesn't provide any rationale for the
change, just reassurance that it probably doesn't break anything.
Garrett (cc'd), can you provide any insight on this?  Was your
intention to filter down TAILQ_EMPTY(&in_ifaddrhead) checks below
in_input, and, if so, why?  If the only recourse in this case is to
bail out, why shouldn't it be done in in_input?  Or should this code
try to do something else?


> +               if (ia == (struct in_ifaddr *)0)
> +                       /* did not find any valid interface address */

You would also need to free `m' here.

> +                       goto done;
> +       }
>         t = IA_SIN(ia)->sin_addr;
>         ip->ip_src = t;
>         ip->ip_ttl = ip_defttl;

Dima,

thanks for the quick reply!

> This check doesn't make sense, because TAILQ_FOREACH expands
> (effectively) into:
>
> 	for (ia = in_ifaddrhead.tqh_first; ia != NULL; ia = ia->tqh_next)
> 		/* I might've gotten some variable names wrong here. */
>
> Thus, the if() inside it will *always* fail.  Essentially what you
> want to do is check if the TAILQ is empty, and fail otherwise.  But...

Ooops. You're right of course.

> > +               if (ia == (struct in_ifaddr *)0)
> > +                       /* did not find any valid interface address */
>
> You would also need to free `m' here.

Both times :-)

Lars
--
Lars Eggert <larse@isi.edu>               Information Sciences Institute
http://www.isi.edu/larse/              University of Southern California

Garrett Wollman <wollman@khavrinen.lcs.mit.edu> wrote:
> <<On Thu, 13 Sep 2001 04:50:02 -0700 (PDT), Dima Dorfman <dima@unixfreak.org>
>  said:
> 
> >  Unforunately, the commit log doesn't provide any rationale for the
> >  change, just reassurance that it probably doesn't break anything.
> >  Garrett (cc'd), can you provide any insight on this?
> 
> The standard requires that hosts accept broadcasts and multicasts
> while unconfigured, just as they are supposed to be able to send them
> (with source address 0.0.0.0).  4.2BSD got this wrong.  This was one
> of the issues in the way of correct DHCP client operation without
> requiring BPF.

Okay, please review the attached patch for the icmp case.  It's
adpated from NetBSD, who have been using it for three years.  It fixes
the case described in this PR and the one in 29337.

Thanks.

Index: ip_icmp.c
===================================================================
RCS file: /ref/cvsf/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.59
diff -u -r1.59 ip_icmp.c
--- ip_icmp.c	3 Sep 2001 20:03:54 -0000	1.59
+++ ip_icmp.c	17 Sep 2001 09:46:46 -0000
@@ -624,9 +624,22 @@
 	/*
 	 * The following happens if the packet was not addressed to us,
 	 * and was received on an interface with no IP address.
+	 * We find the first address on the first non-loopback
+	 * interface in the hope of it having a route back to the
+	 * source.
 	 */
 	if (ia == (struct in_ifaddr *)0)
-		ia = TAILQ_FIRST(&in_ifaddrhead);
+		TAILQ_FOREACH(ia, &in_ifaddrhead, ia_link)
+			if (!(ia->ia_ifp->if_flags & IFF_LOOPBACK))
+				break;
+	/*
+	 * If we still don't have an address, punt.  We probably have
+	 * an interface up and receiving packets with no addresses.
+	 */
+	if (ia == (struct in_ifaddr *)0) {
+		m_freem(m);
+		goto done;
+	}
 	t = IA_SIN(ia)->sin_addr;
 	ip->ip_src = t;
 	ip->ip_ttl = ip_defttl;

FWIW, here's the updated patch.

--- /usr/src/sys/netinet/ip_icmp.c.orig Mon Sep 17 09:57:37 2001
+++ /usr/src/sys/netinet/ip_icmp.c      Mon Sep 17 10:14:32 2001
@@ -640,8 +640,13 @@
         * The following happens if the packet was not addressed to us,
         * and was received on an interface with no IP address.
         */
-       if (ia == (struct in_ifaddr *)0)
+       if (ia == (struct in_ifaddr *)0) {
+               if (TAILQ_EMPTY(&in_ifaddrhead)) {
+                       m_freem(m);     /* Cannot find interface address
*/
+                       goto done;      /* to send ICMP packet from. */
+               }
                ia = in_ifaddrhead.tqh_first;
+       }
        t = IA_SIN(ia)->sin_addr;
        ip->ip_src = t;
        ip->ip_ttl = ip_defttl;

Will this issue be resolved before 4.4? Either with this patch, or with a
better solution?

Thanks,
Lars
--
Lars Eggert <larse@isi.edu>               Information Sciences Institute
http://www.isi.edu/larse/              University of Southern California

This also occurs in 4.4-RELEASE using a machine as a bridge and
putting:

net.link.ether.bridge=1

in /etc/sysctl.conf which brings up the bridging before the
network interfaces get configured.

Regards, Matthew

-- 
Matthew Frost                                         http://www.frost.org/
                                                  "A Invalid argument, 10:2"

Comment 6 dd 2001-10-01 13:45:53 UTC

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

I'm working on this.

Comment 7 dd 2001-11-27 20:03:29 UTC

State Changed
From-To: open->closed

Fixed in rev. 1.63 of ip_icmp.c.  I'll MFC this after a few weeks. 
Thanks!