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 <[email protected]> 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/[email protected]?part=9
