NetBSD Problem Report #56894

From www@netbsd.org  Mon Jun 20 01:25:09 2022
Return-Path: <www@netbsd.org>
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 358781A921F
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 20 Jun 2022 01:25:09 +0000 (UTC)
Message-Id: <20220620012508.3DA941A9239@mollari.NetBSD.org>
Date: Mon, 20 Jun 2022 01:25:08 +0000 (UTC)
From: tgl@sss.pgh.pa.us
Reply-To: tgl@sss.pgh.pa.us
To: gnats-bugs@NetBSD.org
Subject: if_lagg.c crashes on alignment-picky architectures
X-Send-Pr-Version: www-1.0

>Number:         56894
>Category:       kern
>Synopsis:       if_lagg.c crashes on alignment-picky architectures
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jun 20 01:30:00 +0000 2022
>Closed-Date:    Tue Jun 28 15:00:56 +0000 2022
>Last-Modified:  Tue Jun 28 15:20:01 +0000 2022
>Originator:     Tom Lane
>Release:        HEAD/202206150250Z
>Organization:
PostgreSQL Global Development Group
>Environment:
NetBSD sss2.sss.pgh.pa.us 9.99.97 NetBSD 9.99.97 (SD0) #0: Wed Jun 15 15:24:17 EDT 2022  tgl@nuc1.sss.pgh.pa.us:/home/tgl/netbsd-H-202206150250Z/obj.hppa/sys/arch/hppa/compile/SD0 hppa
>Description:
if_lagg.c:947 does this:

	flowlabel = ip6->ip6_flow & IPV6_FLOWLABEL_MASK;

ip6_flow refers to an int32 field, but the *ip6 struct is not necessarily aligned on a 4-byte boundary.  On machines that are picky about such things, that leads to crashes in the

net/if_lagg/t_lagg:lagg_lacp_vlan_ipv6
net/if_lagg/t_lagg:lagg_lacp_vlanl2tp_ipv6

tests.
>How-To-Repeat:
Run /usr/tests tests on HPPA, or another alignment-picky architecture.
>Fix:
memcpy'ing the field into a suitably-aligned value would do the trick here.  I do not know if there are more hazards elsewhere in the file, but both of the aforementioned tests crash exactly here.

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: gnats-bugs@NetBSD.org, Shoichi Yamaguchi <yamaguchi@NetBSD.org>
Subject: Re: kern/56894: if_lagg.c crashes on alignment-picky architectures
Date: Mon, 20 Jun 2022 02:02:21 +0000

 This is a multi-part message in MIME format.
 --=_9iRVuQui7SP0XI0my0i42KB+RbbJU+ZS

 Can you try the attached patch and see if it helps?

 --=_9iRVuQui7SP0XI0my0i42KB+RbbJU+ZS
 Content-Type: text/plain; charset="ISO-8859-1"; name="lagg-align"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="lagg-align.patch"

 From a4ca0a1f76a25d5121cfd5946320655cdf4c7c69 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh@NetBSD.org>
 Date: Mon, 20 Jun 2022 02:00:10 +0000
 Subject: [PATCH] lagg(4): Safely handle misaligned mbufs.

 Optimizing for non-strict-alignment architectures -- without falling
 afoul of alignment sanitizers or overeager compilers -- is left as an
 exercise for the reader.
 ---
  sys/net/lagg/if_lagg.c      | 14 ++++++++------
  sys/net/lagg/if_laggproto.h |  5 +++--
  2 files changed, 11 insertions(+), 8 deletions(-)

 diff --git a/sys/net/lagg/if_lagg.c b/sys/net/lagg/if_lagg.c
 index 68d2c43663d1..d98bd1c51777 100644
 --- a/sys/net/lagg/if_lagg.c
 +++ b/sys/net/lagg/if_lagg.c
 @@ -895,7 +895,7 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
  	*(hp) =3D hash32_buf(&(v), sizeof(v), *(hp));	\
  } while(0)
 =20
 -	eh =3D lagg_m_extract(m, 0, sizeof(*eh), &buf);
 +	eh =3D lagg_m_extract(m, 0, sizeof(*eh), __alignof(*eh), &buf);
  	if (eh =3D=3D NULL)
  		goto out;
 =20
 @@ -903,7 +903,8 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
  	etype =3D ntohs(eh->ether_type);
 =20
  	if (etype =3D=3D ETHERTYPE_VLAN) {
 -		evl =3D lagg_m_extract(m, 0, sizeof(*evl), &buf);
 +		evl =3D lagg_m_extract(m, 0, sizeof(*evl), __alignof(*evl),
 +		    &buf);
  		if (evl =3D=3D NULL)
  			goto out;
 =20
 @@ -924,7 +925,7 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
 =20
  	switch (etype) {
  	case ETHERTYPE_IP:
 -		ip =3D lagg_m_extract(m, off, sizeof(*ip), &buf);
 +		ip =3D lagg_m_extract(m, off, sizeof(*ip), __alignof(*ip), &buf);
  		if (ip =3D=3D NULL)
  			goto out;
 =20
 @@ -937,7 +938,8 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
  		proto =3D ip->ip_p;
  		break;
  	case ETHERTYPE_IPV6:
 -		ip6 =3D lagg_m_extract(m, off, sizeof(*ip6), &buf);
 +		ip6 =3D lagg_m_extract(m, off, sizeof(*ip6), __alignof(*ip6),
 +		    &buf);
  		if (ip6 =3D=3D NULL)
  			goto out;
 =20
 @@ -957,7 +959,7 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
 =20
  	switch (proto) {
  	case IPPROTO_TCP:
 -		th =3D lagg_m_extract(m, off, sizeof(*th), &buf);
 +		th =3D lagg_m_extract(m, off, sizeof(*th), __alignof(*th), &buf);
  		if (th =3D=3D NULL)
  			goto out;
 =20
 @@ -967,7 +969,7 @@ lagg_hashmbuf(struct lagg_softc *sc, struct mbuf *m)
  		}
  		break;
  	case IPPROTO_UDP:
 -		uh =3D lagg_m_extract(m, off, sizeof(*uh), &buf);
 +		uh =3D lagg_m_extract(m, off, sizeof(*uh), __alignof(*uh), &buf);
  		if (uh =3D=3D NULL)
  			goto out;
 =20
 diff --git a/sys/net/lagg/if_laggproto.h b/sys/net/lagg/if_laggproto.h
 index c9732ea52564..7c7497f134bf 100644
 --- a/sys/net/lagg/if_laggproto.h
 +++ b/sys/net/lagg/if_laggproto.h
 @@ -217,7 +217,8 @@ struct lagg_softc {
  	(_lp)->lp_ioctl((_lp)->lp_ifp, (_cmd), (_data))
 =20
  static inline const void *
 -lagg_m_extract(struct mbuf *m, size_t off, size_t reqlen, void *buf)
 +lagg_m_extract(struct mbuf *m, size_t off, size_t reqlen, size_t align,
 +    void *buf)
  {
  	ssize_t len;
  	const void *rv;
 @@ -229,7 +230,7 @@ lagg_m_extract(struct mbuf *m, size_t off, size_t reqle=
 n, void *buf)
  		return NULL;
  	}
 =20
 -	if (m->m_len >=3D len) {
 +	if (m->m_len >=3D len && ((uintptr_t)mtod(m, uint8_t *) % align) =3D=3D 0=
 ) {
  		rv =3D mtod(m, uint8_t *) + off;
  	} else {
  		m_copydata(m, off, reqlen, buf);

 --=_9iRVuQui7SP0XI0my0i42KB+RbbJU+ZS--

From: Tom Lane <tgl@sss.pgh.pa.us>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: gnats-bugs@NetBSD.org, Shoichi Yamaguchi <yamaguchi@NetBSD.org>
Subject: Re: kern/56894: if_lagg.c crashes on alignment-picky architectures
Date: Mon, 20 Jun 2022 15:29:50 -0400

 ------- =_aaaaaaaaaa0
 Content-Type: text/plain; charset="us-ascii"
 Content-ID: <2573332.1655753358.1@sss.pgh.pa.us>

 Taylor R Campbell <riastradh@NetBSD.org> writes:
 > Can you try the attached patch and see if it helps?

 As-is, nope: you've got a thinko about which value needs to be aligned.
 Adding the attached delta patch gets me a clean pass on all the
 net/if_lagg/t_lagg test cases.

 			regards, tom lane


 ------- =_aaaaaaaaaa0
 Content-Type: text/x-diff; name="lagg-fix.patch"; charset="us-ascii"
 Content-ID: <2573332.1655753358.2@sss.pgh.pa.us>
 Content-Description: lagg-fix.patch

 --- sys/net/lagg/if_laggproto.h~	2022-06-20 10:27:29.744899667 -0400
 +++ sys/net/lagg/if_laggproto.h	2022-06-20 13:26:45.025311487 -0400
 @@ -230,7 +230,8 @@
  		return NULL;
  	}

 -	if (m->m_len >= len && ((uintptr_t)mtod(m, uint8_t *) % align) == 0) {
 +	if (m->m_len >= len &&
 +	    ((uintptr_t)(mtod(m, uint8_t *) + off) % align) == 0) {
  		rv = mtod(m, uint8_t *) + off;
  	} else {
  		m_copydata(m, off, reqlen, buf);

 ------- =_aaaaaaaaaa0--

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/56894 CVS commit: src/sys/net/lagg
Date: Sun, 26 Jun 2022 17:55:24 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Sun Jun 26 17:55:24 UTC 2022

 Modified Files:
 	src/sys/net/lagg: if_lagg.c if_laggproto.h

 Log Message:
 lagg(4): Safely handle misaligned mbufs.

 Optimizing for non-strict-alignment architectures -- without falling
 afoul of alignment sanitizers or overeager compilers -- is left as an
 exercise for the reader.

 PR kern/56894


 To generate a diff of this commit:
 cvs rdiff -u -r1.47 -r1.48 src/sys/net/lagg/if_lagg.c
 cvs rdiff -u -r1.17 -r1.18 src/sys/net/lagg/if_laggproto.h

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

State-Changed-From-To: open->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 28 Jun 2022 15:00:56 +0000
State-Changed-Why:
fixed


From: Tom Lane <tgl@sss.pgh.pa.us>
To: gnats-bugs@NetBSD.org
Cc: Taylor R Campbell <riastradh@NetBSD.org>,
        Shoichi Yamaguchi <yamaguchi@NetBSD.org>
Subject: Re: kern/56894: if_lagg.c crashes on alignment-picky architectures
Date: Tue, 28 Jun 2022 10:55:32 -0400

 I confirm that the net/if_lagg tests pass on HPPA as of
 HEAD/202206271040Z.  Thanks!

 			regards, tom lane

>Unformatted:

Home	PR Database Search