From nobody Wed Sep 17 10:49:10 2025 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 222C7C4167B for ; Wed, 21 Dec 2022 05:23:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234400AbiLUFXF (ORCPT ); Wed, 21 Dec 2022 00:23:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46756 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234040AbiLUFW5 (ORCPT ); Wed, 21 Dec 2022 00:22:57 -0500 Received: from wout5-smtp.messagingengine.com (wout5-smtp.messagingengine.com [64.147.123.21]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1E530201A1; Tue, 20 Dec 2022 21:22:56 -0800 (PST) Received: from compute6.internal (compute6.nyi.internal [10.202.2.47]) by mailout.west.internal (Postfix) with ESMTP id F0C85320077A; Wed, 21 Dec 2022 00:22:54 -0500 (EST) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Wed, 21 Dec 2022 00:22:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pjd.dev; h=cc:cc :content-transfer-encoding:date:date:from:from:in-reply-to :in-reply-to:message-id:mime-version:references:reply-to:sender :subject:subject:to:to; s=fm3; t=1671600174; x=1671686574; bh=zN 958PcJz/PV3DsW6WEQWLRBp7oQ21G6ASIhZz2zjz8=; b=gTkvPPKBsVQewxvhoO 8i/LZtepYyhu1NIwV3uA35RL1ygeNwI4BuoROiciJiUr5QeM397aGaR0XIvCYY3Z PSRPtSqC2msRmu+7jt430lys3xY6usvzFfUoC+XhRTxoDmzU3jkTVcF8zGdaU7y2 hXzwQEzpD7ViGKvUqi56tNUX1Pe8LmGci2/74/pVpWWvnM0SQuxZz7rTnluTDHN4 RKqVPdO8SjfsG1bnqB7tXkX0vkl7X0Gp+3lqrzIAhnWbysI9hESvI8sOJyvW0Cb6 xpf25MEiMCgoQLIH2v/mApVCSJOKU7DkbwU4g1/6v7FRAdJ7gut/5KnmTN+vBFBH UiRw== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding:date:date :feedback-id:feedback-id:from:from:in-reply-to:in-reply-to :message-id:mime-version:references:reply-to:sender:subject :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; t=1671600174; x=1671686574; bh=zN958PcJz/PV3 DsW6WEQWLRBp7oQ21G6ASIhZz2zjz8=; b=goS9Fz9lczB92xHHmz3DfJJgyc1NZ +MD9j/Ma9yFMfZqEv1aqFvaRUogfZzuY74GfIE9pWH8WFqzsJ4syUSk7029OMPNH Qapr8FmxwHM5XURupeDribwstO7s6b6+j1Ey66ByzU2K0N/NZPKimj12UMn0RqbD 3oPVyoqFYhaigQOKWqbKkwoU3QDgLPkVi3cZfAIMHecoiSOvVLmn/oukwKHXOK3I yvabyUN2ehuFJv2/nGiAZVfgESwaOsQsz9NJLp7cjpATfEu3I4t5xNulB9AmA9FU 3jtVMhW74wETm+eF1FN1FKVfLYnkW/Q4ByE/wy9pq+4aV/yuashsxsJdg== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvhedrgeejgdekfecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecumhhishhsihhnghcuvffquchfihgvlhguucdlfedtmd enogetfedtuddqtdduucdludehmdenucfjughrpefhvfevufffkffojghfggfgsedtkeer tdertddtnecuhfhrohhmpefrvghtvghrucffvghlvghvohhrhigrshcuoehpvghtvghrse hpjhgurdguvghvqeenucggtffrrghtthgvrhhnpeeliedvteeuieekuefhieeghfehueff heeutdduueejiedvfedtudevgeevhfffhfenucffohhmrghinhepughmthhfrdhorhhgpd ifihhkihhpvgguihgrrdhorhhgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghm pehmrghilhhfrhhomhepphgvthgvrhesphhjugdruggvvh X-ME-Proxy: Feedback-ID: i9e814621:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 21 Dec 2022 00:22:54 -0500 (EST) From: Peter Delevoryas Cc: peter@pjd.dev, sam@mendozajonas.com, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, joel@jms.id.au, gwshan@linux.vnet.ibm.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH 2/3] net/ncsi: Fix netlink major/minor version numbers Date: Tue, 20 Dec 2022 21:22:45 -0800 Message-Id: <20221221052246.519674-3-peter@pjd.dev> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20221221052246.519674-1-peter@pjd.dev> References: <20221221052246.519674-1-peter@pjd.dev> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable To: unlisted-recipients:; (no To-header on input) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Type: text/plain; charset="utf-8" The netlink interface for major and minor version numbers doesn't actually return the major and minor version numbers. It reports a u32 that contains the (major, minor, update, alpha1) components as the major version number, and then alpha2 as the minor version number. For whatever reason, the u32 byte order was reversed (ntohl): maybe it was assumed that the encoded value was a single big-endian u32, and alpha2 was the minor version. The correct way to get the supported NC-SI version from the network controller is to parse the Get Version ID response as described in 8.4.44 of the NC-SI spec[1]. Get Version ID Response Packet Format Bits +--------+--------+--------+--------+ Bytes | 31..24 | 23..16 | 15..8 | 7..0 | +-------+--------+--------+--------+--------+ | 0..15 | NC-SI Header | +-------+--------+--------+--------+--------+ | 16..19| Response code | Reason code | +-------+--------+--------+--------+--------+ |20..23 | Major | Minor | Update | Alpha1 | +-------+--------+--------+--------+--------+ |24..27 | reserved | Alpha2 | +-------+--------+--------+--------+--------+ | .... other stuff .... | The major, minor, and update fields are all binary-coded decimal (BCD) encoded [2]. The spec provides examples below the Get Version ID response format in section 8.4.44.1, but for practical purposes, this is an example from a live network card: root@bmc:~# ncsi-util 0x15 NC-SI Command Response: cmd: GET_VERSION_ID(0x15) Response: COMMAND_COMPLETED(0x0000) Reason: NO_ERROR(0x0000) Payload length =3D 40 20: 0xf1 0xf1 0xf0 0x00 <<<<<<<<< (major, minor, update, alpha1) 24: 0x00 0x00 0x00 0x00 <<<<<<<<< (_, _, _, alpha2) 28: 0x6d 0x6c 0x78 0x30 32: 0x2e 0x31 0x00 0x00 36: 0x00 0x00 0x00 0x00 40: 0x16 0x1d 0x07 0xd2 44: 0x10 0x1d 0x15 0xb3 48: 0x00 0x17 0x15 0xb3 52: 0x00 0x00 0x81 0x19 This should be parsed as "1.1.0". "f" in the upper-nibble means to ignore it, contributing zero. If both nibbles are "f", I think the whole field is supposed to be ignored. Major and minor are "required", meaning they're not supposed to be "ff", but the update field is "optional" so I think it can be ff. I think the simplest thing to do is just set the major and minor to zero instead of juggling some conditional logic or something. bcd2bin() from "include/linux/bcd.h" seems to assume both nibbles are 0-9, so I've provided a custom BCD decoding function. Alpha1 and alpha2 are ISO/IEC 8859-1 encoded, which just means ASCII characters as far as I can tell, although the full encoding table for non-alphabetic characters is slightly different (I think). I imagine the alpha fields are just supposed to be alphabetic characters, but I haven't seen any network cards actually report a non-zero value for either. If people wrote software against this netlink behavior, and were parsing the major and minor versions themselves from the u32, then this would definitely break their code. [1] https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.= 0.0.pdf [2] https://en.wikipedia.org/wiki/Binary-coded_decimal [2] https://en.wikipedia.org/wiki/ISO/IEC_8859-1 Signed-off-by: Peter Delevoryas Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler") --- net/ncsi/internal.h | 7 +++++-- net/ncsi/ncsi-netlink.c | 4 ++-- net/ncsi/ncsi-pkt.h | 7 +++++-- net/ncsi/ncsi-rsp.c | 26 ++++++++++++++++++++++++-- 4 files changed, 36 insertions(+), 8 deletions(-) diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h index 03757e76bb6b..374412ed780b 100644 --- a/net/ncsi/internal.h +++ b/net/ncsi/internal.h @@ -105,8 +105,11 @@ enum { =20 =20 struct ncsi_channel_version { - u32 version; /* Supported BCD encoded NCSI version */ - u32 alpha2; /* Supported BCD encoded NCSI version */ + u8 major; /* NCSI version major */ + u8 minor; /* NCSI version minor */ + u8 update; /* NCSI version update */ + char alpha1; /* NCSI version alpha1 */ + char alpha2; /* NCSI version alpha2 */ u8 fw_name[12]; /* Firmware name string */ u32 fw_version; /* Firmware version */ u16 pci_ids[4]; /* PCI identification */ diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c index c189b4c8a182..db350b8f5d88 100644 --- a/net/ncsi/ncsi-netlink.c +++ b/net/ncsi/ncsi-netlink.c @@ -71,8 +71,8 @@ static int ncsi_write_channel_info(struct sk_buff *skb, if (nc =3D=3D nc->package->preferred_channel) nla_put_flag(skb, NCSI_CHANNEL_ATTR_FORCED); =20 - nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.version); - nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MINOR, nc->version.alpha2); + nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.major); + nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MINOR, nc->version.minor); nla_put_string(skb, NCSI_CHANNEL_ATTR_VERSION_STR, nc->version.fw_name); =20 vid_nest =3D nla_nest_start_noflag(skb, NCSI_CHANNEL_ATTR_VLAN_LIST); diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h index ba66c7dc3a21..c9d1da34dc4d 100644 --- a/net/ncsi/ncsi-pkt.h +++ b/net/ncsi/ncsi-pkt.h @@ -197,9 +197,12 @@ struct ncsi_rsp_gls_pkt { /* Get Version ID */ struct ncsi_rsp_gvi_pkt { struct ncsi_rsp_pkt_hdr rsp; /* Response header */ - __be32 ncsi_version; /* NCSI version */ + unsigned char major; /* NCSI version major */ + unsigned char minor; /* NCSI version minor */ + unsigned char update; /* NCSI version update */ + unsigned char alpha1; /* NCSI version alpha1 */ unsigned char reserved[3]; /* Reserved */ - unsigned char alpha2; /* NCSI version */ + unsigned char alpha2; /* NCSI version alpha2 */ unsigned char fw_name[12]; /* f/w name string */ __be32 fw_version; /* f/w version */ __be16 pci_ids[4]; /* PCI IDs */ diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c index 6447a09932f5..7a805b86a12d 100644 --- a/net/ncsi/ncsi-rsp.c +++ b/net/ncsi/ncsi-rsp.c @@ -19,6 +19,19 @@ #include "ncsi-pkt.h" #include "ncsi-netlink.h" =20 +/* Nibbles within [0xA, 0xF] add zero "0" to the returned value. + * Optional fields (encoded as 0xFF) will default to zero. + */ +static u8 decode_bcd_u8(u8 x) +{ + int lo =3D x & 0xF; + int hi =3D x >> 4; + + lo =3D lo < 0xA ? lo : 0; + hi =3D hi < 0xA ? hi : 0; + return lo + hi * 10; +} + static int ncsi_validate_rsp_pkt(struct ncsi_request *nr, unsigned short payload) { @@ -804,9 +817,18 @@ static int ncsi_rsp_handler_gvi(struct ncsi_request *n= r) if (!nc) return -ENODEV; =20 - /* Update to channel's version info */ + /* Update channel's version info + * + * Major, minor, and update fields are supposed to be + * unsigned integers encoded as packed BCD. + * + * Alpha1 and alpha2 are ISO/IEC 8859-1 characters. + */ ncv =3D &nc->version; - ncv->version =3D ntohl(rsp->ncsi_version); + ncv->major =3D decode_bcd_u8(rsp->major); + ncv->minor =3D decode_bcd_u8(rsp->minor); + ncv->update =3D decode_bcd_u8(rsp->update); + ncv->alpha1 =3D rsp->alpha1; ncv->alpha2 =3D rsp->alpha2; memcpy(ncv->fw_name, rsp->fw_name, 12); ncv->fw_version =3D ntohl(rsp->fw_version); --=20 2.30.2