net: ip: fix promiscuous mode altering packets

Always clone net_pkt to pass to promiscuous queue. Previously, net_pkt was passed to L2 before conditionally cloning. But L2 would in some cases strip ethernet headers, so cloned net_pkt received through promiscuous interface would be missing headers. Signed-off-by: Kieran Tyrrell <[email protected]>
zephyrproject-rtos · Oct 3, 2023 · a714b42 · a714b42
1 parent d68db1e
commit a714b42
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 37 deletions.
diff --git a/subsys/net/ip/net_if.c b/subsys/net/ip/net_if.c
@@ -4120,45 +4120,13 @@ enum net_verdict net_if_recv_data(struct net_if *iface, struct net_pkt *pkt)
 {
 	if (IS_ENABLED(CONFIG_NET_PROMISCUOUS_MODE) &&
 	    net_if_is_promisc(iface)) {
-		/* If the packet is not for us and the promiscuous
-		 * mode is enabled, then increase the ref count so
-		 * that net_core.c:processing_data() will not free it.
-		 * The promiscuous mode handler must free the packet
-		 * after it has finished working with it.
-		 *
-		 * If packet is for us, then NET_CONTINUE is returned.
-		 * In this case we must clone the packet, as the packet
-		 * could be manipulated by other part of the stack.
-		 */
-		enum net_verdict verdict;
 		struct net_pkt *new_pkt;
 
-		/* This protects pkt so that it will not be freed by L2 recv()
-		 */
-		net_pkt_ref(pkt);
+		new_pkt = net_pkt_clone(pkt, K_NO_WAIT);
 
-		verdict = net_if_l2(iface)->recv(iface, pkt);
-		if (verdict == NET_CONTINUE) {
-			new_pkt = net_pkt_clone(pkt, K_NO_WAIT);
-		} else {
-			new_pkt = net_pkt_ref(pkt);
+		if (net_promisc_mode_input(new_pkt) == NET_DROP) {
+			net_pkt_unref(new_pkt);
 		}
-
-		/* L2 has modified the buffer starting point, it is easier
-		 * to re-initialize the cursor rather than updating it.
-		 */
-		if (new_pkt) {
-			net_pkt_cursor_init(new_pkt);
-
-			if (net_promisc_mode_input(new_pkt) == NET_DROP) {
-				net_pkt_unref(new_pkt);
-			}
-		} else {
-			NET_WARN("promiscuous packet dropped, unable to clone packet");
-		}
-		net_pkt_unref(pkt);
-
-		return verdict;
 	}
 
 	return net_if_l2(iface)->recv(iface, pkt);

diff --git a/tests/net/promiscuous/src/main.c b/tests/net/promiscuous/src/main.c
@@ -352,6 +352,8 @@ static void _recv_data(struct net_if *iface, struct net_pkt **pkt)
 	*pkt = net_pkt_rx_alloc_with_buffer(iface, sizeof(data),
 					    AF_UNSPEC, 0, K_FOREVER);
 
+	net_pkt_ref(*pkt);
+
 	net_pkt_write(*pkt, data, sizeof(data));
 
 	ret = net_recv_data(iface, *pkt);
@@ -372,11 +374,25 @@ static void test_verify_data(void)
 	struct net_pkt *pkt;
 
 	pkt = net_promisc_mode_wait_data(K_SECONDS(1));
-	zassert_equal_ptr(pkt, pkt1, "pkt %p != %p", pkt, pkt1);
+	zassert_not_null(pkt, "pkt");
+	zassert_not_null(pkt->buffer, "pkt->buffer");
+	zassert_not_null(pkt1, "pkt1");
+	zassert_not_null(pkt1->buffer, "pkt1->buffer");
+	zassert_equal(pkt->buffer->len, pkt1->buffer->len, "packet length differs");
+	zassert_not_null(pkt->buffer->data, "pkt->buffer->data");
+	zassert_not_null(pkt1->buffer->data, "pkt1->buffer->data");
+	zassert_mem_equal(pkt->buffer->data, pkt1->buffer->data, pkt1->buffer->len);
 	net_pkt_unref(pkt);
 
 	pkt = net_promisc_mode_wait_data(K_SECONDS(1));
-	zassert_equal_ptr(pkt, pkt2, "pkt %p != %p", pkt, pkt2);
+	zassert_not_null(pkt, "pkt");
+	zassert_not_null(pkt->buffer, "pkt->buffer");
+	zassert_not_null(pkt2, "pkt2");
+	zassert_not_null(pkt2->buffer, "pkt2->buffer");
+	zassert_equal(pkt->buffer->len, pkt2->buffer->len, "packet length differs");
+	zassert_not_null(pkt->buffer->data, "pkt->buffer->data");
+	zassert_not_null(pkt2->buffer->data, "pkt2->buffer->data");
+	zassert_mem_equal(pkt->buffer->data, pkt2->buffer->data, pkt2->buffer->len);
 	net_pkt_unref(pkt);
 }