Re: kern/60491: arp creates state for invalid messages

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Wed, Oct 21, 2026 at 04:00:52PM +0200, Manuel Bouyer wrote:
> [ 19315.4531276] --- interrupt ---
> 
> The change is:
> -       KASSERT(op == ARPOP_REPLY || op == ARPOP_REQUEST);
> +       KASSERTMSG(op == ARPOP_REPLY || op == ARPOP_REQUEST,
> +           "op not REQUEST or REPLY: %d", op);
> 
> Looks like m_makewritable() may be at fault here.


Now running with this patch:

? sys/netinet/if_arp.c.assertmsg
? sys/netinet/if_arp.c.assertmsg2
Index: sys/netinet/if_arp.c
===================================================================
RCS file: /cvsroot/src/sys/netinet/if_arp.c,v
retrieving revision 1.321
diff -u -p -u -r1.321 if_arp.c
--- sys/netinet/if_arp.c	23 Jul 2026 21:49:19 -0000	1.321
+++ sys/netinet/if_arp.c	28 Oct 2026 13:56:37 -0000
@@ -742,6 +742,8 @@ arpintr(void *arg __unused)
 				ARP_STATINC(ARP_STAT_RCVBADPROTO);
 				goto free;
 			}
+			KASSERT(ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REPLY ||
+			    ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REQUEST);
 			in_arpinput(m);
 			continue;
 		default:
@@ -793,12 +795,16 @@ in_arpinput(struct mbuf *m)
 	int s;
 	char ipbuf[INET_ADDRSTRLEN];
 	bool find_source, do_dad;
+	struct mbuf *om = m;
 
+	KASSERT(ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REPLY ||
+	    ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REQUEST);
 	if (__predict_false(m_makewritable(&m, 0, m->m_pkthdr.len, M_DONTWAIT)))
 		goto out;
 	ah = mtod(m, struct arphdr *);
 	op = ntohs(ah->ar_op);
-	KASSERT(op == ARPOP_REPLY || op == ARPOP_REQUEST);
+	KASSERTMSG(op == ARPOP_REPLY || op == ARPOP_REQUEST,
+	    "op not REQUEST or REPLY: %d %p %p", op, m, om);
 
 	if (ah->ar_pln != sizeof(struct in_addr))
 		goto out;
@@ -1895,7 +1901,11 @@ in_revarpinput(struct mbuf *m)
 	switch (op) {
 	case ARPOP_REQUEST:
 	case ARPOP_REPLY:	/* per RFC */
+		KASSERT(ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REPLY ||
+		    ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REQUEST);
 		m_put_rcvif(rcvif, &s);
+		KASSERT(ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REPLY ||
+		    ntohs(mtod(m, struct arphdr *)->ar_op) == ARPOP_REQUEST);
 		in_arpinput(m);
 		return;
 	case ARPOP_REVREPLY:

I got this panic:
[ 603891.0584629] panic: kernel diagnostic assertion "op == ARPOP_REPLY || op == ARPOP_REQUEST" failed: file "/dsk/l1/misc/bouyer/HEAD/clean/src/sys/netinet/if_arp.c", line 806 op not REQUEST or REPLY: 65535 0xffff85ca8b4ff7e0 0xffff85ca73cdebc0
[ 603891.1323430] cpu0: Begin traceback...
[ 603891.1440383] vpanic() at netbsd:vpanic+0x18d
[ 603891.1584130] kern_assert() at netbsd:kern_assert+0x61
[ 603891.1722752] in_arpinput() at netbsd:in_arpinput+0x52c
[ 603891.1887326] arpintr() at netbsd:arpintr+0x11c
[ 603891.1995472] softint_dispatch() at netbsd:softint_dispatch+0x124

db{0}> show mbuf /v 0xffff85ca8b4ff7e0
MBUF(0) 0xffff85ca8b4ff7e0
  data=0xffff85ca8b4ff850, len=0, type=1, flags=0x102<PKTHDR,BCAST>
  owner=0x0, next=0xffff85ca73cdebc0, nextpkt=0x0
  leadingspace=0, trailingspace=400, readonly=0
  pktlen=46, rcvif=0xffff8f0001f66060, csum_flags=0, csum_data=0x0, segsz=0
ETHER: DST = 00:01:08:00:06:04
ETHER: SRC = 00:01:f4:cf:e2:d4
ETHER: TYPE = 0x4b00(unknown)
offset 0014: 84 e3 67 01
offset 0018: 00 00 00 00
offset 0022: 00 00 84 e3
offset 0026: 67 9e 00 00
offset 0030: 00 00 00 00
offset 0034: 00 00 00 00
offset 0038: 00 00 00 00
offset 0042: 00 00 00 00
db{0}> show mbuf /v 0xffff85ca73cdebc0
MBUF(0) 0xffff85ca73cdebc0
  data=0xffff85ca73ac680e, len=46, type=1, flags=0x9000101<EXT,BCAST,EXT_CLUSTER
,EXT_RW>
  owner=0x0, next=0x0, nextpkt=0x0
  leadingspace=14, trailingspace=1988, readonly=0
  ext_refcnt=1, ext_buf=0xffff85ca73ac6800, ext_size=2048, ext_free=0x0, ext_arg
=0x0
ETHER: DST = 00:01:08:00:06:04
ETHER: SRC = 00:01:f4:cf:e2:d4
ETHER: TYPE = 0x4b00(unknown)
offset 0014: 84 e3 67 01
offset 0018: 00 00 00 00
offset 0022: 00 00 84 e3
offset 0026: 67 9e 00 00
offset 0030: 00 00 00 00
offset 0034: 00 00 00 00
offset 0038: 00 00 00 00
offset 0042: 00 00 00 00

So it looks like that after m_makewritable(), we end up with a mbuf with no
data (len = 0), but which is chained with our old mbuf. But the data here don't
make much sense; these ethernet addresses are unknown on my network and
the ether type doesn't make sense either.
But if we look at  0xffff85ca73ac6800 (the start of the mbuf cluster area):
db{0}> x/x 0xffff85ca73ac6800
ffff85ca73ac6800:       ffffffff
db{0}> 
ffff85ca73ac6804:       cff4ffff
db{0}> 
ffff85ca73ac6808:       4bd4e2
db{0}> 
ffff85ca73ac680c:       1000608
db{0}> 
ffff85ca73ac6810:       4060008
db{0}> 
ffff85ca73ac6814:       cff40100
db{0}> 
ffff85ca73ac6818:       4bd4e2
db{0}> 
ffff85ca73ac681c:       167e384
db{0}> 
ffff85ca73ac6820:       0
db{0}> 
ffff85ca73ac6824:       e3840000


This gives:
DST: ff:ff:ff:ff:ff:ff
SRC: f4:cf:e2:d4:4b:00 (this is a known address on my network)
ETYPE: 0x0806 (this is ETHERTYPE_ARP)
ar_hrd: 0001 (ARPHRD_ETHER)

This makes more sense if we assume fields after ETYPE have been overwritten
(we reached the area pointed to by mbuf->data)

I didn't look closely enough at m_makewritable() to explain how we could end
up in this situation.
Anyway I don't think we can use mtod() on a mbuf whose len is 0, and it doens't
looks like m_makewritable() garantees that the first mbuf will contain the
whole arp header.

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--