Hi Roy !Hmm, wouldn't this bring us back the bug again? ia == NULL for a non-local source addresses (generated via pfil_run_hooks-NAT operation) and IP_FORWARDING is not set as tcp_input.c:syn_cache_respond does rightfully not set IP_FORWARDING and pfil_run_hooks has no means to set that flag. That gives us error == -1 with your sequence. So we would return EADDRNOTAVAIL breaking packet filter NAT action again, if I didn't overlook something.
From what I understand this code originally attempted to avoid sending from invalid/unusable local address (e. g. duplicate IP - error, tentative and detached should just be dropped). No validation can be done for non-local addresses at all. IP_FORWARDING formerly used to be used to suppress infinite recursion on mcast forwarding, but it seems the semantics where extended a little bit in the mean time (like here to suppress a check).
So I cannot say something about the intentions for the IP_FORWARDING check.For correct packet filter operation we need to distinguish the local and non-local address case here. If we keep the previous logic for IP_FORWARDING and ip_ifaddrvalid() then
the new expression
if (ia != NULL && (flags & IP_FORWARDING) == 0 &&
(error = ip_ifaddrvalid(ia)) != 0)
correctly limits the check to local interface addresses only keeping the
original intention/implementation for IP_FORWARDING and validity checks.
If then IP_FORWARDING and validity check needs to be adjusted that part must be limited to local interface addresses only. (ia != NULL).
That's how I understand that code section or the intention there in the normal and packet filter context.
I didn't check IPv6. Frank On 05/07/17 21:50, Roy Marples wrote:
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 seemsto 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