Project

General

Profile

Bug #8903

IPPF paths have bad logic

Added by Dan McDonald over 2 years ago. Updated 11 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
networking
Start date:
2017-12-07
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
needs-triage
Gerrit CR:

Description

Per https://github.com/illumos/illumos-gate/pull/27

It seems that the author meant to use && rather than || in these cases. A variable will always be non-equal to at least one of two different constants, so the current expression doesn't make sense.

That said, I can't vouch for the effect of this change because I'm unfamiliar with the codebase. It's likely that it restores the intended behavior of the condition, but someone knowledgeable about the code should verify that.

It's indeed broken, but it's on the ipqosconf(1M) path, which in spite of its richer enforcement semantics, appears to have been supplanted by flowadm(1M). In other words, this bug is in a rarely-used codepath.


Files

test (1.65 KB) test input to ipqosconf(1M) -> `ipqosconf -a test` Dan McDonald, 2019-08-02 10:03 PM
8903.d (532 Bytes) 8903.d DTrace script monitoring ip_process(). Dan McDonald, 2019-08-02 10:37 PM

History

#1

Updated by Dan McDonald over 2 years ago

From 1aa03c6406a2d70c98546d572dda34d901beccf7 Mon Sep 17 00:00:00 2001
From: Michael McConville <mmcco@mykolab.com>
Date: Wed, 6 Dec 2017 15:12:06 -0700
Subject: [PATCH] Fix probable boolean logic errors

---
 usr/src/uts/common/inet/ip/ip6_input.c | 4 ++--
 usr/src/uts/common/inet/ip/ip_input.c  | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/usr/src/uts/common/inet/ip/ip6_input.c b/usr/src/uts/common/inet/ip/ip6_input.c
index c7c241f9441..57b7e704e03 100644
--- a/usr/src/uts/common/inet/ip/ip6_input.c
+++ b/usr/src/uts/common/inet/ip/ip6_input.c
@@ -2021,8 +2021,8 @@ ip_fanout_v6(mblk_t *mp, ip6_t *ip6h, ip_recv_attr_t *ira)
      */
     if (IPP_ENABLED(IPP_LOCAL_IN, ipst) &&
         !(iraflags & IRAF_LOOPBACK) &&
-        (protocol != IPPROTO_ESP || protocol != IPPROTO_AH ||
-        protocol != IPPROTO_DSTOPTS || protocol != IPPROTO_ROUTING ||
+        (protocol != IPPROTO_ESP && protocol != IPPROTO_AH &&
+        protocol != IPPROTO_DSTOPTS && protocol != IPPROTO_ROUTING &&
         protocol != IPPROTO_FRAGMENT)) {
         /*
          * Use the interface on which the packet arrived - not where
diff --git a/usr/src/uts/common/inet/ip/ip_input.c b/usr/src/uts/common/inet/ip/ip_input.c
index 6aa70b014a9..a6e9898c916 100644
--- a/usr/src/uts/common/inet/ip/ip_input.c
+++ b/usr/src/uts/common/inet/ip/ip_input.c
@@ -2361,7 +2361,7 @@ ip_fanout_v4(mblk_t *mp, ipha_t *ipha, ip_recv_attr_t *ira)
      */
     if (IPP_ENABLED(IPP_LOCAL_IN, ipst) &&
         !(iraflags & IRAF_LOOPBACK) &&
-        (protocol != IPPROTO_ESP || protocol != IPPROTO_AH)) {
+        (protocol != IPPROTO_ESP && protocol != IPPROTO_AH)) {
         /*
          * Use the interface on which the packet arrived - not where
          * the IP address is hosted.

#2

Updated by Dan McDonald 11 months ago

Well, I manged to both identify the bug AND test this fix on the IPv4 path.

1.) Set up ipqosconf(1M). This may involve installing a package on IPS-based systems. Luckily `ipqos` is the package name mentioned in-gate. A config file ("test") is attached, and you may not even need to alter the IP address in the file.

2.) Run the attached 8903.d DTrace script.

3.) Set up IPsec between the netstack under test and an OFF-LINK-PEER. illumos has same-physical-machine bypasses that set up the LOOPBACK properties of packets that won't trigger the codepaths in question.

4.) Send an IPsec-protected packet to the netstack under test.

5.) Without this fix, the D script will produce output indicating an AH or ESP packet reached ip_process(). With this fix, ip_process() does not called with an AH or ESP packet.

#3

Updated by Dan McDonald 11 months ago

  • File 8903.d added
  • File test test added

Attachments here.

#4

Updated by Dan McDonald 11 months ago

IPv6 testing is simpler, as fragmented packets will trigger the DTrace probe N+1 times for a packet consisting of N segments.

Repeat steps 1 and 2 of the IPv4 testing, making sure the probe for IPv6 has protocol 44 in it as well.

Don't bother with IPsec but do send a fragmented packet (large pings work well) to the netstack under test. The probe will fire N+1 times without the fix, and 1 time with it (as the sampling of "protocol" in the packet happens at function's start, not during the looping that the ip_fanout_v6() does).

#5

Updated by Dan McDonald 11 months ago

  • File deleted (8903.d)
#6

Updated by Dan McDonald 11 months ago

#7

Updated by Electric Monk 11 months ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 94a5108e6d17b76907472153e979d6261be71c37

commit  94a5108e6d17b76907472153e979d6261be71c37
Author: Michael McConville <mmcco@mykolab.com>
Date:   2019-08-02T22:43:39.000Z

    8903 IPPF paths have bad logic
    Reviewed by: Dan McDonald <danmcd@joyent.com>
    Reviewed by: John Levon <levon@movementarian.org>
    Reviewed by: Gergő Doma <domag02@gmail.com>
    Approved by: Joshua M. Clulow <josh@sysmgr.org>

Also available in: Atom PDF