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

[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@hotmail.co.uk>
---
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@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/

 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

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

[Simon Horman]

On Sat, Sep 12, 2026 at 07:09:37PM +0100, Aamir Ahmed wrote:
> 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@hotmail.co.uk>
> ---
> 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@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/

Reviewed-by: Simon Horman <horms@kernel.org>

For future reference:

This patch-set has two patches.  But they seem to have been sent as two
separate email threads.  Which confuses some tooling, such as Sashiko.
Using tools such as b4 or git send-email (in conjunction with git
format-patch) should avoid this problem.

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

[Jakub Kicinski]

On Sat, 12 Sep 2026 19:09:37 +0100 Aamir Ahmed wrote:
> 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.

Patches need to be posted in one thread, which is the default for git
send-email. Patch 2 should be in reply to patch 1. Please repost.