Re: [PATCH v12 net-next 9/9] octeontx2-af: npc: cn20k: Allocate npc_priv and dstats dynamically.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Global file-scope pointers (`npc_priv` and `dstats`) are overwritten by per-device dynamic allocations, leading to memory leaks and Use-After-Free crashes in multi-device systems.
- [High] Unconditional memory leak of `npc_priv` during the `npc_priv_init()` error path.
- [High] Unconditional memory leak of `npc_priv` and all its allocations if `npc_setup_mcam_section()` fails.
- [High] Missing NULL pointer checks for `npc_priv` in debugfs and asynchronous handlers leads to NULL pointer dereferences.
- [High] Out-of-bounds array access due to unvalidated hardware geometry exceeding static array capacities.
- [High] Pre-existing memory leak of `defrag_lh` nodes during `npc_cn20k_deinit()`.
- [High] Pre-existing out-of-bounds write in `npc_create_srch_order()` when `num_subbanks` is 1.
--

commit dfbf4e512f9599787a56a7a0ecf88e54713d70e8
Author: Ratheesh Kannoth <rkannoth@marvell.com>
Subject: octeontx2-af: npc: cn20k: Allocate npc_priv and dstats dynamically.

This commit modifies the npc_priv and dstats structures to be dynamically
allocated instead of file-scope statics. It handles initialization and
teardown of these allocations and updates usages across the code paths.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> index 9a4c2ea5b19e4..15af7b0ccea64 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/debugfs.c
[ ... ]
> -static u64 dstats[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS] = {};
> +static u64 (*dstats)[MAX_NUM_BANKS][MAX_SUBBANK_DEPTH * MAX_NUM_SUB_BANKS];
[ ... ]
> @@ -396,6 +396,10 @@ int npc_cn20k_debugfs_init(struct rvu *rvu)
>  	debugfs_create_file("vidx2idx", 0444, rvu->rvu_dbg.npc,
>  			    npc_priv, &npc_vidx2idx_map_fops);
>  
> +	dstats = devm_kzalloc(rvu->dev, sizeof(*dstats), GFP_KERNEL);
> +	if (!dstats)
> +		return -ENOMEM;
> +

Since dstats is a file-scoped static pointer, will it be overwritten if
multiple RVU PCI devices are probed on the same system?

It looks like devm_kzalloc ties the allocation to a specific device. If that
device is unbound, the devres framework will automatically free the memory.
Could this leave the global pointer dangling and cause a use-after-free for
any remaining active devices that try to access the debugfs file?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index 15b3f29d60ee9..ea8fc43df0902 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> @@ -16,9 +16,7 @@
>  #include "cn20k/reg.h"
>  #include "rvu_npc_fs.h"
>  
> -static struct npc_priv_t npc_priv = {
> -	.num_banks = MAX_NUM_BANKS,
> -};
> +static struct npc_priv_t *npc_priv;

Similarly, since npc_priv is also a file-scoped global pointer, could probing
multiple devices cause previous allocations to be overwritten and leaked?
Should this per-device state be stored within a device-specific context
structure instead of a global pointer?

[ ... ]
> @@ -4185,8 +4183,8 @@ int rvu_mbox_handler_npc_get_pfl_info(struct rvu *rvu, struct msg_req *req,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	rsp->kw_type = npc_priv.kw;
> -	rsp->x4_slots = npc_priv.bank_depth;
> +	rsp->kw_type = npc_priv->kw;
> +	rsp->x4_slots = npc_priv->bank_depth;
>  	return 0;
>  }

If a Virtual Function sends a mailbox message before initialization is
complete or after teardown, could npc_priv be NULL here? Does this handler
need a NULL pointer check to prevent a crash?

[ ... ]
> @@ -4599,34 +4597,40 @@ static int npc_priv_init(struct rvu *rvu)
>  		return -EINVAL;
>  	}
>  
> -	npc_priv.num_subbanks = num_subbanks;
> +	npc_priv = kcalloc(1, sizeof(*npc_priv), GFP_KERNEL);
> +	if (!npc_priv)
> +		return -ENOMEM;
> +
> +	npc_priv->num_banks = num_banks;
> +
> +	npc_priv->num_subbanks = num_subbanks;
>  
>  	subbank_depth =	bank_depth / num_subbanks;
>  
> -	npc_priv.bank_depth = bank_depth;
> -	npc_priv.subbank_depth = subbank_depth;
> +	npc_priv->bank_depth = bank_depth;
> +	npc_priv->subbank_depth = subbank_depth;
>  
>  	/* Get kex configured key size */
>  	cfg = rvu_read64(rvu, blkaddr, NPC_AF_INTFX_KEX_CFG(0));
> -	npc_priv.kw = FIELD_GET(GENMASK_ULL(34, 32), cfg);
> +	npc_priv->kw = FIELD_GET(GENMASK_ULL(34, 32), cfg);
>  
>  	dev_info(rvu->dev,
>  		 "banks=%u depth=%u, subbanks=%u depth=%u, key type=%s\n",
>  		 num_banks, bank_depth, num_subbanks, subbank_depth,
> -		 npc_kw_name[npc_priv.kw]);
> +		 npc_kw_name[npc_priv->kw]);
>  
> -	npc_priv.sb = kcalloc(num_subbanks, sizeof(struct npc_subbank),
> -			      GFP_KERNEL);
> -	if (!npc_priv.sb)
> +	npc_priv->sb = kcalloc(num_subbanks, sizeof(struct npc_subbank),
> +			       GFP_KERNEL);
> +	if (!npc_priv->sb)
>  		return -ENOMEM;

If the allocation for npc_priv->sb fails and returns -ENOMEM, does this leak
the newly allocated npc_priv structure? Should there be a cleanup path here to
free npc_priv?

This isn't a bug introduced by your patch, but in this same file under
npc_create_srch_order():

static int npc_create_srch_order(int cnt)
{
        ...
        for (int i = 0; i < cnt; i += 2) {
                subbank_srch_order[i] = cnt / 2 - val - 1;
                subbank_srch_order[i + 1] = cnt / 2 + 1 + val;
                val++;
        }
        ...
}

If the hardware reports num_subbanks as 1, cnt is 1, and the loop executes
once. Could this cause an out-of-bounds write to subbank_srch_order[1] since
the array size is only 1?

[ ... ]
> @@ -4674,25 +4680,30 @@ void npc_cn20k_deinit(struct rvu *rvu)
>  {
>  	int i;
>  
> -	xa_destroy(&npc_priv.xa_sb_used);
> -	xa_destroy(&npc_priv.xa_sb_free);
> -	xa_destroy(&npc_priv.xa_idx2pf_map);
> -	xa_destroy(&npc_priv.xa_pf_map);
> -	xa_destroy(&npc_priv.xa_pf2dfl_rmap);
> -	xa_destroy(&npc_priv.xa_idx2vidx_map);
> -	xa_destroy(&npc_priv.xa_vidx2idx_map);
> +	if (!npc_priv)
> +		return;
> +
> +	xa_destroy(&npc_priv->xa_sb_used);
[ ... ]
> +	kfree(npc_priv);
> +	npc_priv = NULL;
>  }

This might be a pre-existing issue, but during teardown in npc_cn20k_deinit(),
does the defrag_lh list need to be cleared? The list appears to contain
dynamically allocated npc_defrag_show_node structures, and they might be
leaked if they are still present when npc_priv is freed.

[ ... ]
> @@ -4730,7 +4741,5 @@ int npc_cn20k_init(struct rvu *rvu)
>  		return err;
>  	}
>  
> -	npc_priv.init_done = true;
> -
>  	return 0;
>  }

Looking at the earlier part of npc_cn20k_init():

int npc_cn20k_init(struct rvu *rvu)
{
        err = npc_priv_init(rvu);
        if (err) { ... }

        err = npc_setup_mcam_section(rvu, NPC_MCAM_KEY_X2);
        if (err) {
                dev_err(rvu->dev, "%s: mcam section configuration failure\n",
                        __func__);
                return err;
        }
        ...
}

If npc_setup_mcam_section() fails, it returns the error without calling
npc_cn20k_deinit(). Does this leak the npc_priv structure and all of its
internal allocations?

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h
> index bf030e40fbf96..ee0bbeba7f259 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.h
> @@ -191,7 +191,7 @@ struct npc_defrag_show_node {
>   */
>  struct npc_priv_t {
>  	int bank_depth;
> -	const int num_banks;
> +	int num_banks;
>  	int num_subbanks;
>  	int subbank_depth;
>  	DECLARE_BITMAP(en_map, MAX_NUM_BANKS *

Since num_banks and bank_depth are now dynamically read from hardware rather
than using strict compile-time constants, is it possible for a device to
advertise geometry larger than the static sizes used for en_map and dstats
(MAX_NUM_BANKS, etc.)?

Could this cause out-of-bounds reads and writes when accessing those
structures?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508034912.4082520-1-rkannoth@marvell.com?part=9