NetBSD Problem Report #56052

From kardel@kardel.name  Fri Mar 12 08:58:46 2021
Return-Path: <kardel@kardel.name>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 7384D1A9217
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 12 Mar 2021 08:58:46 +0000 (UTC)
Message-Id: <20210312085828.A279228C03F@mail.kardel.name>
Date: Fri, 12 Mar 2021 08:58:28 +0000 (UTC)
From: kardel@netbsd.org
Reply-To: kardel@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: NPF: block return... processes the return packet again
X-Send-Pr-Version: 3.95

>Number:         56052
>Category:       kern
>Synopsis:       NPF: block return... processes the return packet again
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Mar 12 09:00:00 +0000 2021
>Closed-Date:    Sun Jun 18 19:32:20 +0000 2023
>Last-Modified:  Sun Jun 18 19:32:20 +0000 2023
>Originator:     Frank Kardel
>Release:        NetBSD 9.99.81
>Organization:

>Environment:


System: NetBSD gateway 9.99.81 NetBSD 9.99.81 (GATEWAY) #9: Fri Mar 12 07:41:46 UTC 2021 kardel@gateway:/src/NetBSD/cur/src/obj.amd64/sys/arch/amd64/compile/GATEWAY amd64
Architecture: x86_64
Machine: amd64
>Description:
	A ruleset like
group "name" on <interface> {
	pass in final ...
	pass in final ...
        block return in final all apply "log"
}

	will not send out any TCP-RST or ICMP(,6} unreachable pakets as the
	block-return packets pass again through the NPF filter engine on
	their way out. block-return packets are sent via ip{,6}_output() and
	icmp{,6}_error().
	This can be observed via the npflogX interface.

	block-return packets must skip outgoing packet rules to avoid
	to add workaround rules like

# allow for block return packets
pass out final proto tcp all apply "log"
pass out final proto icmp all apply "log"

	before the block-return rule. this workaround allows also unwanted
	communication. more precise rules require pcap-filter
	rules and may still be incomplete.

	as the workarounds are hard to manage and hard to explain to
	users NPF needs to be fixed to work like other packet filters
	so that is does not examine its own block-return packets.

	see Fix: below

	I will commit the fix if no objections arise.

>How-To-Repeat:
	Have a ruleset like
group "name" on <interface> {
        pass in final ...
        pass in final ...
        block return in final all apply "log"
}

	send packets that are not matched by the pass in rules. observe
	via npflogX that the block-return packet pass the filter rules
	in out direct and they get finally dropped.

>Fix:
	to fix the iss the block-return packets need to be marked
	with the PACKET_TAG_NPF tag so the NPF filter engine will
	automatically pass these packets without rule processing.
	there is an issue in ip_icmp.c (IPv4 ICMP) in icmp_error.
	as it create a new packet the tags must be transferred. the
	the current code only knows about PACKAGE_TAG_PF. This needs to
	be extended.

	see the following patch for fixing the NPF block-return
	functionality.

Index: sys/net/npf/npf.h
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf.h,v
retrieving revision 1.63
diff -u -r1.63 npf.h
--- sys/net/npf/npf.h	30 May 2020 14:16:56 -0000	1.63
+++ sys/net/npf/npf.h	12 Mar 2021 08:00:18 -0000
@@ -121,7 +121,9 @@
 void *		nbuf_ensure_writable(nbuf_t *, size_t);

 bool		nbuf_cksum_barrier(nbuf_t *, int);
+int		mbuf_add_tag(nbuf_t *, struct mbuf *, uint32_t);
 int		nbuf_add_tag(nbuf_t *, uint32_t);
+int		mbuf_find_tag(nbuf_t *, struct mbuf *, uint32_t *);
 int		nbuf_find_tag(nbuf_t *, uint32_t *);

 /*
Index: sys/net/npf/npf_mbuf.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_mbuf.c,v
retrieving revision 1.24
diff -u -r1.24 npf_mbuf.c
--- sys/net/npf/npf_mbuf.c	30 May 2020 14:16:56 -0000	1.24
+++ sys/net/npf/npf_mbuf.c	12 Mar 2021 08:00:18 -0000
@@ -297,14 +297,13 @@
 }

 /*
- * nbuf_add_tag: associate a tag with the network buffer.
+ * mbuf_add_tag: associate a tag with the network buffer.
  *
  * => Returns 0 on success or error number on failure.
  */
 int
-nbuf_add_tag(nbuf_t *nbuf, uint32_t val)
+mbuf_add_tag(nbuf_t *nbuf, struct mbuf *m, uint32_t val)
 {
-	struct mbuf *m = nbuf->nb_mbuf0;
 #ifdef _KERNEL
 	struct m_tag *mt;
 	uint32_t *dat;
@@ -328,14 +327,25 @@
 }

 /*
- * nbuf_find_tag: find a tag associated with a network buffer.
+ * nbuf_add_tag: associate a tag with the network buffer.
  *
  * => Returns 0 on success or error number on failure.
  */
 int
-nbuf_find_tag(nbuf_t *nbuf, uint32_t *val)
+nbuf_add_tag(nbuf_t *nbuf, uint32_t val)
 {
 	struct mbuf *m = nbuf->nb_mbuf0;
+	return mbuf_add_tag(nbuf, m, val);
+}
+
+/*
+ * mbuf_find_tag: find a tag associated with a network buffer.
+ *
+ * => Returns 0 on success or error number on failure.
+ */
+int
+mbuf_find_tag(nbuf_t *nbuf, struct mbuf *m, uint32_t *val)
+{
 #ifdef _KERNEL
 	struct m_tag *mt;

@@ -354,3 +364,15 @@
 	return nbuf->nb_mops->get_tag(m, val);
 #endif
 }
+
+/*
+ * nbuf_find_tag: find a tag associated with a network buffer.
+ *
+ * => Returns 0 on success or error number on failure.
+ */
+int
+nbuf_find_tag(nbuf_t *nbuf, uint32_t *val)
+{
+	struct mbuf *m = nbuf->nb_mbuf0;
+	return mbuf_find_tag(nbuf, m, val);
+}
Index: sys/net/npf/npf_sendpkt.c
===================================================================
RCS file: /cvsroot/src/sys/net/npf/npf_sendpkt.c,v
retrieving revision 1.22
diff -u -r1.22 npf_sendpkt.c
--- sys/net/npf/npf_sendpkt.c	30 May 2020 14:16:56 -0000	1.22
+++ sys/net/npf/npf_sendpkt.c	12 Mar 2021 08:00:18 -0000
@@ -197,6 +197,9 @@
 		}
 	}

+	/* don't look at our generated reject packets going out */
+	(void)mbuf_add_tag(npc->npc_nbuf, m, NPF_NTAG_PASS);
+
 	/* Pass to IP layer. */
 	if (npf_iscached(npc, NPC_IP4)) {
 		return ip_output(m, NULL, NULL, IP_FORWARDING, NULL, NULL);
@@ -215,6 +218,9 @@
 {
 	struct mbuf *m = nbuf_head_mbuf(npc->npc_nbuf);

+	/* don't look at our generated reject packets going out */
+	(void)mbuf_add_tag(npc->npc_nbuf, m, NPF_NTAG_PASS);
+
 	if (npf_iscached(npc, NPC_IP4)) {
 		icmp_error(m, ICMP_UNREACH, ICMP_UNREACH_ADMIN_PROHIBIT, 0, 0);
 		return 0;
Index: sys/netinet/ip_icmp.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/ip_icmp.c,v
retrieving revision 1.177
diff -u -r1.177 ip_icmp.c
--- sys/netinet/ip_icmp.c	22 Dec 2018 14:28:57 -0000	1.177
+++ sys/netinet/ip_icmp.c	12 Mar 2021 08:00:19 -0000
@@ -165,6 +165,10 @@
 static int icmp_redirtimeout = 600;
 static struct rttimer_queue *icmp_redirect_timeout_q = NULL;

+/* packet filter m_tag helpers */
+static void move_mtag(int, struct mbuf*, struct mbuf *);
+static void clear_non_pf_mtags(struct mbuf *);
+
 /* Protect mtudisc and redirect stuff */
 static kmutex_t icmp_mtx __cacheline_aligned;

@@ -237,6 +241,22 @@
 }

 /*
+ * Move a specified m_tag from soure mbuf to target mbuf
+ */
+static void
+move_mtag(int type, struct mbuf *s, struct mbuf *t)
+{
+	struct m_tag *mtag;
+
+	mtag = m_tag_find(s, type);
+
+	if (mtag != NULL) {
+		m_tag_unlink(s, mtag);
+		m_tag_prepend(t, mtag);
+	}
+}
+
+/*
  * Generate an error packet of type error in response to a bad IP packet. 'n'
  * contains this packet. We create 'm' and send it.
  * 
@@ -254,7 +274,6 @@
 	const unsigned oiphlen = oip->ip_hl << 2;
 	struct icmp *icp;
 	struct mbuf *m;
-	struct m_tag *mtag;
 	unsigned datalen, mblen;
 	int totlen;

@@ -381,12 +400,10 @@
 	nip->ip_p = IPPROTO_ICMP;
 	nip->ip_src = oip->ip_src;
 	nip->ip_dst = oip->ip_dst;
-	/* move PF m_tag to new packet, if it exists */
-	mtag = m_tag_find(n, PACKET_TAG_PF);
-	if (mtag != NULL) {
-		m_tag_unlink(n, mtag);
-		m_tag_prepend(m, mtag);
-	}
+
+	/* move packet filter tags */
+	move_mtag(PACKET_TAG_NPF, n, m);
+	move_mtag(PACKET_TAG_PF, n, m);

 	icmp_reflect(m);

@@ -699,6 +716,39 @@
 }

 /*
+ * clear all non packet filter mtags
+ */
+static void
+clear_non_pf_mtags(struct mbuf *m)
+{
+	struct m_tag * mtag_npf = NULL;
+	struct m_tag * mtag_pf = NULL;
+
+	mtag_npf = m_tag_find(m, PACKET_TAG_NPF);
+	if (mtag_npf != NULL) {
+		m_tag_unlink(m, mtag_npf);
+	}
+
+	mtag_pf = m_tag_find(m, PACKET_TAG_PF);
+	if (mtag_pf != NULL) {
+		m_tag_unlink(m, mtag_pf);
+	}
+
+	/* remove all other tags */
+	m_tag_delete_chain(m);
+
+	/* recover tag for PF */
+	if (mtag_pf != NULL) {
+		m_tag_prepend(m, mtag_pf);
+	}
+
+	/* recover tag for NPF */
+	if (mtag_npf != NULL) {
+		m_tag_prepend(m, mtag_npf);
+	}
+}
+
+/*
  * Reflect the ip packet back to the source
  */
 void
@@ -919,7 +969,10 @@
 		memmove(ip + 1, (char *)ip + optlen,
 		    (unsigned)(m->m_len - sizeof(struct ip)));
 	}
-	m_tag_delete_chain(m);
+
+	/* remove all non packet filter mtags */
+	clear_non_pf_mtags(m);
+
 	m->m_flags &= ~(M_BCAST|M_MCAST);

 	/*

>Release-Note:

>Audit-Trail:
From: "Frank Kardel" <kardel@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56052 CVS commit: src/sys/net/npf
Date: Sun, 12 Feb 2023 13:38:37 +0000

 Module Name:	src
 Committed By:	kardel
 Date:		Sun Feb 12 13:38:37 UTC 2023

 Modified Files:
 	src/sys/net/npf: npf.h npf_mbuf.c npf_sendpkt.c

 Log Message:
 PR kern/56052:
 allow block-return packets passed through without rule matching.
 Included up-stream as https://github.com/rmind/npf/pull/115


 To generate a diff of this commit:
 cvs rdiff -u -r1.63 -r1.64 src/sys/net/npf/npf.h
 cvs rdiff -u -r1.24 -r1.25 src/sys/net/npf/npf_mbuf.c
 cvs rdiff -u -r1.22 -r1.23 src/sys/net/npf/npf_sendpkt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: kardel@NetBSD.org
State-Changed-When: Tue, 14 Mar 2023 07:55:17 +0000
State-Changed-Why:
pullups for -10 and -9 requested


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56052 CVS commit: [netbsd-10] src/sys/net/npf
Date: Tue, 14 Mar 2023 17:09:21 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Mar 14 17:09:21 UTC 2023

 Modified Files:
 	src/sys/net/npf [netbsd-10]: npf.h npf_mbuf.c npf_sendpkt.c

 Log Message:
 Pull up following revision(s) (requested by kardel in ticket #119):

 	sys/net/npf/npf_mbuf.c: revision 1.25
 	sys/net/npf/npf.h: revision 1.64
 	sys/net/npf/npf_sendpkt.c: revision 1.23

 PR kern/56052:
 allow block-return packets passed through without rule matching.
 Included up-stream ashttps://github.com/rmind/npf/pull/115


 To generate a diff of this commit:
 cvs rdiff -u -r1.63 -r1.63.20.1 src/sys/net/npf/npf.h
 cvs rdiff -u -r1.24 -r1.24.20.1 src/sys/net/npf/npf_mbuf.c
 cvs rdiff -u -r1.22 -r1.22.20.1 src/sys/net/npf/npf_sendpkt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56052 CVS commit: [netbsd-9] src/sys/net/npf
Date: Tue, 14 Mar 2023 17:11:13 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Mar 14 17:11:13 UTC 2023

 Modified Files:
 	src/sys/net/npf [netbsd-9]: npf.h npf_mbuf.c npf_sendpkt.c

 Log Message:
 Pull up following revision(s) (requested by kardel in ticket #119):

 	sys/net/npf/npf_mbuf.c: revision 1.25
 	sys/net/npf/npf.h: revision 1.64
 	sys/net/npf/npf_sendpkt.c: revision 1.23

 PR kern/56052:
 allow block-return packets passed through without rule matching.
 Included up-stream ashttps://github.com/rmind/npf/pull/115


 To generate a diff of this commit:
 cvs rdiff -u -r1.60.2.3 -r1.60.2.4 src/sys/net/npf/npf.h
 cvs rdiff -u -r1.22.4.1 -r1.22.4.2 src/sys/net/npf/npf_mbuf.c
 cvs rdiff -u -r1.21.4.1 -r1.21.4.2 src/sys/net/npf/npf_sendpkt.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: kern-bug-people->closed
Responsible-Changed-By: gutteridge@NetBSD.org
Responsible-Changed-When: Sun, 18 Jun 2023 19:26:35 +0000
Responsible-Changed-Why:
Pullups were completed, closing.

Responsible-Changed-From-To: closed->kern-bug-people
Responsible-Changed-By: gutteridge@NetBSD.org
Responsible-Changed-When: Sun, 18 Jun 2023 19:32:00 +0000
Responsible-Changed-Why:
Part one of fixing previous entry error, restore Responsible entry.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: gutteridge@NetBSD.org
State-Changed-When: Sun, 18 Jun 2023 19:32:20 +0000
State-Changed-Why:
Really close this ticket.

>Unformatted:
 pullup-10
 pullup-9

Home	PR Database Search