[PATCH net v2 1/2] net: ncsi: validate response packet length before accessing fields

[Aamir Ahmed]

ncsi_validate_rsp_pkt() takes a pointer to the response header and then
reads the checksum at the end of the padded payload, without checking
that the skb holds either. ncsi_rcv_rsp() reads the common header the
same way before that.

For response types with a fixed payload the header length check is not
enough: a short frame whose header claims the expected length passes it.
For the variable-length types (GP, OEM, PLDM, GMCMA) the payload comes
from the header itself, so the check is tautological.

The response skb is not guaranteed to be linear, so use pskb_may_pull()
rather than testing skb->len, and take the header pointers afterwards -
pskb_may_pull() may move the data. The payload is padded to four bytes
and the checksum occupies the last four, so the validator pulls
ALIGN(payload, 4) rather than payload. ncsi_rcv_rsp() keeps a copy of
the packet type for its error paths, as its own header pointer does not
survive the validator.

Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@xxxxxxxxxxxxx>
---
v2:
  - use pskb_may_pull() instead of testing skb->len, and take the header
    pointer after the call (Simon)
  - pull ALIGN(payload, 4), not payload: the checksum sits in the last
    four bytes of the padded payload, so the v1 bound did not cover it
  - guard the common-header read in ncsi_rcv_rsp() too, and keep a copy
    of the packet type, since its header pointer does not survive the
    validator's pull
  - correct the Fixes: tag; v1 quoted a hash that does not resolve, and
    the blame for this file is the commit that added it
  - drop the GMCMA hunk; it belongs with its own handler
  - add the Assisted-by: LLM tag (Simon, Greg)
  - name the target tree in the subject
v1: https://lore.kernel.org/netdev/AS8P251MB0001E6ABBE0B6809D3E21B9DC8B32@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

 net/ncsi/ncsi-rsp.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index fbd84bc8026a..e4264a028acb 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -42,7 +42,14 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 	/* Check NCSI packet header. We don't need validate
 	 * the packet type, which should have been checked
 	 * before calling this function.
+	 *
+	 * The response is not guaranteed to be linear, so make the
+	 * header and the padded payload - the checksum sits in its last
+	 * four bytes - available before taking a pointer into the skb.
 	 */
+	if (!pskb_may_pull(nr->rsp, sizeof(*h) + ALIGN(payload, 4)))
+		return -EINVAL;
+
 	h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
 
 	if (h->common.revision != NCSI_PKT_REVISION) {
@@ -1172,6 +1179,7 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	struct ncsi_pkt_hdr *hdr;
 	unsigned long flags;
 	int payload, i, ret;
+	unsigned char type;
 
 	/* Find the NCSI device */
 	nd = ncsi_find_dev(orig_dev);
@@ -1181,9 +1189,15 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 		goto err_free_skb;
 	}
 
+	if (!pskb_may_pull(skb, sizeof(*hdr))) {
+		ret = -EINVAL;
+		goto err_free_skb;
+	}
+
 	/* Check if it is AEN packet */
 	hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
-	if (hdr->type == NCSI_PKT_AEN)
+	type = hdr->type;
+	if (type == NCSI_PKT_AEN)
 		return ncsi_aen_handler(ndp, skb);
 
 	/* Find the handler */
@@ -1230,7 +1244,7 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	if (ret) {
 		netdev_warn(ndp->ndev.dev,
 			    "NCSI: 'bad' packet ignored for type 0x%x\n",
-			    hdr->type);
+			    type);
 
 		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
 			if (ret == -EPERM)
@@ -1250,7 +1264,7 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	if (ret)
 		netdev_err(ndp->ndev.dev,
 			   "NCSI: Handler for packet type 0x%x returned %d\n",
-			   hdr->type, ret);
+			   type, ret);
 
 out_netlink:
 	if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) {
@@ -1258,7 +1272,7 @@ out_netlink:
 		if (ret) {
 			netdev_err(ndp->ndev.dev,
 				   "NCSI: Netlink handler for packet type 0x%x returned %d\n",
-				   hdr->type, ret);
+				   type, ret);
 		}
 	}
 

base-commit: df2908090cda368b01ff43709f51890076c56157
-- 
2.55.0