Re: kern/52074: -current npf map directive broken

[David Holland <dholland-bugs%netbsd.org@localhost>]

The following reply was made to PR kern/52074; it has been noted by GNATS.

From: David Holland <dholland-bugs%netbsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/52074: -current npf map directive broken
Date: Tue, 16 May 2017 06:10:53 +0000

 Not sent to gnats.
 
    ------
 
 From: Roy Marples <roy%marples.name@localhost>
 To: Mindaugas Rasiukevicius <rmind%netbsd.org@localhost>, Frank Kardel
 	<kardel%netbsd.org@localhost>
 Cc: netbsd-bugs%netbsd.org@localhost, gnats-admin%netbsd.org@localhost
 Subject: Re: kern/52074: -current npf map directive broken
 Date: Sun, 7 May 2017 20:50:07 +0100
 
 On 07/05/2017 17:28, Mindaugas Rasiukevicius wrote:
 > >  The condition in 620 currently is (see if()):
 > >       ia = in_get_ia_psref(ip->ip_src, &psref_ia);
 > > 
 > >       /* Ensure we only send from a valid address. */==>
 > >       if ((ia != NULL || (flags & IP_FORWARDING) == 0) && <<<<!
 > >           (error = ip_ifaddrvalid(ia)) != 0)
 > >       {
 > >           ARPLOG(LOG_ERR,
 > >               "refusing to send from invalid address %s (pid %d)\n",
 > >               ARPLOGADDR(ip->ip_src), curproc->p_pid);
 > >           IP_STATINC(IP_STAT_ODROPPED);
 > >           if (error == 1)
 > >               /*
 > >                * Address exists, but is tentative or detached.
 > >                * We can't send from it because it's invalid,
 > >                * so we drop the packet.
 > >                */
 > >               error = 0;
 > >           else
 > >               error = EADDRNOTAVAIL;
 > >           goto bad;
 > >       }
 > > 
 > >  Proposed fix is to replace the || inf the if with && as this also seems
 > >  to have been the original intention by the author.
 > 
 > Good catch.
 > 
 > Looks like the regression was introduced 7 months ago, as part of the
 > ip_output.c rev 1.261.  Roy, would you like to have a look into this?
 
 I  think xtos already comitted a fix, but I'm unsure his fix is correct.
 I think the workflow should be this:
 
 if (ia != NULL)
 	error = ip_ifaddrvalid(ia);
 else
 	error = flags & IP_FORWARDING ? 0 : -1;
 if (error != 0) { ...
 
 The idea is that if we claim to send from an address it has to be valid, but
 allow the NULL address if forwarded from the filter.
 
 Does this make sense?
 The same path probably needs adjustment in inet6.
 
 Roy