Re: [PATCH net 1/6] net/ncsi: validate response packet lengths against the skb

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/ncsi: validate response packet lengths against the skb

This commit adds packet length validation for NC-SI responses. It pulls the
common header first, then pulls the full response body before validating the
packet to prevent reading past the received packet body.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a3..1fe061ede26dd 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -38,11 +38,18 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>  	struct ncsi_rsp_pkt_hdr *h;
>  	u32 checksum;
>  	__be32 *pchecksum;
> +	unsigned int len;
>  
>  	/* Check NCSI packet header. We don't need validate
>  	 * the packet type, which should have been checked
>  	 * before calling this function.
>  	 */
> +	len = skb_network_offset(nr->rsp) + sizeof(*h) + ALIGN(payload, 4);
> +	if (!pskb_may_pull(nr->rsp, len)) {

Does this pskb_may_pull() call potentially invalidate the hdr pointer cached
earlier in ncsi_rcv_rsp()?

Looking at ncsi_rcv_rsp(), it caches the header pointer before validating:

ncsi_rcv_rsp() {
    ...
    hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
    ...
    ret = ncsi_validate_rsp_pkt(nr, payload);

If the SKB is fragmented and pskb_may_pull() needs to pull data into the
linear area, it can reallocate the SKB's data buffer and free the old buffer.

When ncsi_validate_rsp_pkt() returns, the local hdr pointer in ncsi_rcv_rsp()
would still point to the freed memory.

This pointer is then dereferenced in multiple error paths:

    if (ret) {
        netdev_warn(ndp->ndev.dev,
                    "NCSI: 'bad' packet ignored for type 0x%x\n",
                    hdr->type);
...
    ret = nrh->handler(nr);
    if (ret)
        netdev_err(ndp->ndev.dev,
                   "NCSI: Handler for packet type 0x%x returned %d\n",
                   hdr->type, ret);

Could this cause a use-after-free when reading hdr->type?
-- 
pw-bot: cr