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

[Simon Horman <horms@kernel.org>]

On Mon, Sep 07, 2026 at 12:37:51AM +0100, Aamir Ahmed wrote:
> ncsi_validate_rsp_pkt() computes a pointer to the checksum field based
> on the payload length without first verifying that the skb actually
> contains enough data.  For response types whose expected payload is
> fixed (e.g., GCPS with 204 bytes), a short frame whose header falsely
> claims the expected length passes the "ntohs(h->common.length) !=
> payload" check, but the skb may be much smaller, leading to an
> out-of-bounds read when dereferencing the checksum pointer.
> 
> For response types with variable-length payloads (GP, OEM, PLDM,
> GMCMA), the payload value comes directly from ntohs(h->common.length),
> so the header check is tautological and provides no protection at all.
> 
> Add an skb length check in ncsi_validate_rsp_pkt() to ensure the
> packet has at least sizeof(ncsi_rsp_pkt_hdr) + payload bytes before
> any field access.
> 
> Additionally, validate address_count bounds in the GMCMA response
> handler.  The handler iterates over rsp->address_count entries from
> the flexible array member without verifying that the packet is large
> enough to contain them, leading to a heap out-of-bounds read if
> address_count exceeds what the packet actually carries.
> 
> Fixes: 0b49507fc090 ("net/ncsi: Resource management")
> Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
> ---
>  net/ncsi/ncsi-rsp.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a..1401528dbd6c 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -45,6 +45,13 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>  	 */
>  	h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
>  
> +	/* Ensure the packet is large enough for header + payload */
> +	if (nr->rsp->len < sizeof(*h) + payload) {

I am concerned that there is no guarantee that the skb is linear and thus
this check is not sufficient. If so, I think the check should be
implemented using pskb_may_pull().  And h should be set after it is called.

...

As noted collectively by Greg and Paolo elsewhere:
1. Please include an Assisted-by tag
2. Please keep the number of patches in-flight to 15 or less

-- 
pw-bot: changes-requested