NetBSD Problem Report #58870

From www@netbsd.org  Tue Dec  3 17:38:28 2024
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)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits)
	 client-signature RSA-PSS (2048 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 71D1D1A9238
	for <gnats-bugs@gnats.NetBSD.org>; Tue,  3 Dec 2024 17:38:28 +0000 (UTC)
Message-Id: <20241203173827.3C6661A9246@mollari.NetBSD.org>
Date: Tue,  3 Dec 2024 17:38:27 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: ipmi(4): misaligned buffer in get_sdr_partial
X-Send-Pr-Version: www-1.0

>Number:         58870
>Category:       kern
>Synopsis:       ipmi(4): misaligned buffer in get_sdr_partial
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Dec 03 17:40:00 +0000 2024
>Last-Modified:  Tue Dec 03 22:15:00 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current, 10, 9, ...
>Organization:
TheI pmiBSDF oundation
>Environment:
>Description:
   1105 static int
   1106 get_sdr_partial(struct ipmi_softc *sc, uint16_t recordId, uint16_t reserveId,
   1107     uint8_t offset, uint8_t length, void *buffer, uint16_t *nxtRecordId)
   1108 {
   1109 	uint8_t	cmd[256 + 8];
   1110 	int		len;
   1111 
   1112 	((uint16_t *) cmd)[0] = reserveId;
   1113 	((uint16_t *) cmd)[1] = recordId;
...
   1129 	if (nxtRecordId)
   1130 		*nxtRecordId = *(uint16_t *) cmd;

https://nxr.netbsd.org/xref/src/sys/dev/ipmi.c?r=1.10#1105

The buffer cmd is allocated with inadequate alignment for access as uint16_t, so this is undefined behaviour.
>How-To-Repeat:
code inspection, or run this on a strict-alignment architecture (if any exist with ipmi -- doesn't seem to be included in any non-x86 kernels), or run this on a compiler that assumes undefined behaviour like unaligned access is impossible and therefore infers it is safe to compile this code into nasal demons
>Fix:
Use a union/struct/&c.

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org
Cc: Michael van Elst <mlelstv@NetBSD.org>,
	Edgar =?iso-8859-1?B?RnXf?= <ef@math.uni-bonn.de>,
	Hauke Fath <hf@spg.tu-darmstadt.de>,
	Manuel Bouyer <bouyer@antioche.eu.org>
Subject: Re: kern/58870: ipmi(4): misaligned buffer in get_sdr_partial
Date: Tue, 3 Dec 2024 17:55:04 +0000

 This is a multi-part message in MIME format.
 --=_UuvISPrFv/VhYA5fbXFjrXMUYMCvsH8a

 The attached patch tries to address this as proposed.

 (I don't have time to test this myself so I don't plan to commit it
 at the moment unless someone else wants to test it.)

 This could conceivably affect Edgar's get_sdr_partial error at boot
 but it seems unlikely (nor do I have any better theories about what
 has caused that to happen).

 --=_UuvISPrFv/VhYA5fbXFjrXMUYMCvsH8a
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr58870-ipmialign"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr58870-ipmialign.patch"

 # HG changeset patch
 # User Taylor R Campbell <riastradh@NetBSD.org>
 # Date 1733247801 0
 #      Tue Dec 03 17:43:21 2024 +0000
 # Branch trunk
 # Node ID 42c04f18f85a06e870f0a3cf1ad1e2acaf576bc3
 # Parent  702bc3da2819f628a6d103b38eaa0e3cbfa40593
 # EXP-Topic riastradh-pr56568-ipmidelay
 ipmi(4): Avoid misaligned buffer access in get_sdr_partial.

 PR kern/58870: ipmi(4): misaligned buffer in get_sdr_partial

 diff --git a/sys/dev/ipmi.c b/sys/dev/ipmi.c
 --- a/sys/dev/ipmi.c
 +++ b/sys/dev/ipmi.c
 @@ -1109,29 +1109,44 @@ static int
  get_sdr_partial(struct ipmi_softc *sc, uint16_t recordId, uint16_t reserve=
 Id,
      uint8_t offset, uint8_t length, void *buffer, uint16_t *nxtRecordId)
  {
 -	uint8_t	cmd[256 + 8];
 +	union {
 +		struct {
 +			uint16_t	reserveId;
 +			uint16_t	recordId;
 +			uint8_t		offset;
 +			uint8_t		length;
 +		} __packed	cmd;
 +		struct {
 +			uint16_t	nxtRecordId;
 +			uint8_t		data[262];
 +		} __packed	msg;
 +	}		u;
  	int		len;
 =20
 -	((uint16_t *) cmd)[0] =3D reserveId;
 -	((uint16_t *) cmd)[1] =3D recordId;
 -	cmd[4] =3D offset;
 -	cmd[5] =3D length;
 +	__CTASSERT(sizeof(u) =3D=3D 256 + 8);
 +	__CTASSERT(sizeof(u.cmd) =3D=3D 6);
 +	__CTASSERT(offsetof(typeof(u.msg), data) =3D=3D 2);
 +
 +	u.cmd.reserveId =3D reserveId;
 +	u.cmd.recordId =3D recordId;
 +	u.cmd.offset =3D offset;
 +	u.cmd.length =3D length;
  	mutex_enter(&sc->sc_cmd_mtx);
 -	if (ipmi_sendcmd(sc, BMC_SA, 0, STORAGE_NETFN, STORAGE_GET_SDR, 6,
 -	    cmd)) {
 +	if (ipmi_sendcmd(sc, BMC_SA, 0, STORAGE_NETFN, STORAGE_GET_SDR,
 +		sizeof(u.cmd), &u.cmd)) {
  		mutex_exit(&sc->sc_cmd_mtx);
  		aprint_error_dev(sc->sc_dev, "%s: sendcmd fails\n", __func__);
  		return -1;
  	}
 -	if (ipmi_recvcmd(sc, 8 + length, &len, cmd)) {
 +	if (ipmi_recvcmd(sc, 8 + length, &len, &u.msg)) {
  		mutex_exit(&sc->sc_cmd_mtx);
  		aprint_error_dev(sc->sc_dev, "%s: recvcmd fails\n", __func__);
  		return -1;
  	}
  	mutex_exit(&sc->sc_cmd_mtx);
  	if (nxtRecordId)
 -		*nxtRecordId =3D *(uint16_t *) cmd;
 -	memcpy(buffer, cmd + 2, len - 2);
 +		*nxtRecordId =3D u.msg.nxtRecordId;
 +	memcpy(buffer, u.msg.data, len - offsetof(typeof(u.msg), data));
 =20
  	return 0;
  }

 --=_UuvISPrFv/VhYA5fbXFjrXMUYMCvsH8a--

From: Edgar =?iso-8859-1?B?RnXf?= <ef@math.uni-bonn.de>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: gnats-bugs@NetBSD.org, netbsd-bugs@NetBSD.org,
	Michael van Elst <mlelstv@NetBSD.org>,
	Hauke Fath <hf@spg.tu-darmstadt.de>,
	Manuel Bouyer <bouyer@antioche.eu.org>
Subject: Re: kern/58870: ipmi(4): misaligned buffer in get_sdr_partial
Date: Tue, 3 Dec 2024 21:02:14 +0100

 > This could conceivably affect Edgar's get_sdr_partial error at boot
 > but it seems unlikely (nor do I have any better theories about what
 > has caused that to happen).
 It does fix the problem, thanks.

 [     1.017037] ipmi_acpi0 at acpi0 (NIPM, IPI0001-IPMI Device): io 0xca8,0xcac irq 10
 [     1.017037] ipmi0 at ipmi_acpi0
 [...]
 [    52.455705] ipmi0: version 32.0 interface KCS iobase 0xca8/0x8 spacing 4
 [    52.455705] ipmi0: ID 32.1 IPMI 2.0 Available +SDRs
 [    52.455705] ipmi0: Additional Chassis Bridge IPMBRcv FRU SEL SDR Sensor
 [    52.455705] ipmi0: Manufacturer 002a2 Product 01a2
 [    52.455705] ipmi0: Firmware 7.0

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/58870 CVS commit: src/sys/dev
Date: Tue, 3 Dec 2024 22:11:38 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue Dec  3 22:11:38 UTC 2024

 Modified Files:
 	src/sys/dev: ipmi.c

 Log Message:
 ipmi(4): Avoid misaligned buffer access in get_sdr_partial.

 PR kern/58870: ipmi(4): misaligned buffer in get_sdr_partial


 To generate a diff of this commit:
 cvs rdiff -u -r1.11 -r1.12 src/sys/dev/ipmi.c

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

Home	PR Database Search