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

["Taylor R Campbell via gnats" <gnats-admin%NetBSD.org@localhost>]

The following reply was made to PR kern/58870; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Cc: Michael van Elst <mlelstv%NetBSD.org@localhost>,
	Edgar =?iso-8859-1?B?RnXf?= <ef%math.uni-bonn.de@localhost>,
	Hauke Fath <hf%spg.tu-darmstadt.de@localhost>,
	Manuel Bouyer <bouyer%antioche.eu.org@localhost>
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@localhost>
 # 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--