Re: [PATCH v6 net-next 5/5] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth]

On 2026-03-20 at 22:24:32, Simon Horman (horms@xxxxxxxxxx) wrote:
>
> > @@ -665,9 +675,15 @@ void npc_cn20k_parser_profile_init(struct rvu *rvu, int blkaddr)
> >  	num_pkinds = rvu->kpu.pkinds;
> >  	num_pkinds = min_t(int, hw->npc_pkinds, num_pkinds);
> >
> > -	for (idx = 0; idx < num_pkinds; idx++)
> > -		npc_config_kpmaction(rvu, blkaddr, &rvu->kpu.ikpu[idx],
> > +	/* Cn20k does not support Custom profile from filesystem */
> > +	for (idx = 0; idx < num_pkinds; idx++) {
> > +		act = npc_get_ikpu_nth_entry(rvu, idx);
> > +		if (!act)
> > +			continue;
> > +
> > +		npc_config_kpmaction(rvu, blkaddr, act,
> >  				     0, idx, true);
> > +	}
>
> When a filesystem-loaded profile is used on cn20k (from_fs=true),
> npc_get_ikpu_nth_entry() returns &profile->ikpu2[idx]:
>
> drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c:npc_get_ikpu_nth_entry() {
>     struct npc_kpu_profile_adapter *profile = &rvu->kpu;
>
>     if (profile->from_fs)
>         return &profile->ikpu2[idx];
>
>     return &profile->ikpu[n];
> }
>
> However, ikpu2 is never allocated for the cn20k path. The allocation
> happens inside npc_apply_custom_kpu() in the from_fs block:
>
> drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c:npc_apply_custom_kpu() {
>     if (is_cn20k(rvu->pdev))
>         return npc_cn20k_apply_custom_kpu(rvu, profile);
>     ...
>     if (from_fs) {
>         profile->ikpu2 = devm_kcalloc(rvu->dev, 1,
>                                       sizeof(ikpu_action_entries),
>                                       GFP_KERNEL);
>         ...
>     }
> }
>
> But this from_fs block is never reached for cn20k because the function
> returns early via npc_cn20k_apply_custom_kpu().
>
> Does this code dereference a NULL-offset pointer causing a kernel crash?
>
> The same issue appears to affect cam2 and action2 pointers which are
> allocated in npc_alloc_kpu_cam2_n_action2() but never called for cn20k.
>
> This was raised by Jakub Kicinski during v4 review on 2026-03-16.
This is false positive. from_fs is never set to "true" for cn20k. In npc_load_kpu_profile(), if soc is
cn20k, it skips "loading from filesystem" method. So npc_get_ikpu_nth_entry()
will never return ikpu2.

>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > index 8d260bcfbf38..8d8feb7742e8 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
>
> [ ... ]
>
> > @@ -1730,32 +1835,89 @@ static int npc_apply_custom_kpu(struct rvu *rvu,
> >  		return -EINVAL;
> >  	}
> >
> > +	*fw_kpus = fw->kpus;
> > +
> > +	sfw = devm_kcalloc(rvu->dev, 1, sizeof(*sfw), GFP_KERNEL);
> > +	if (!sfw)
> > +		return -ENOMEM;
> > +
> > +	memcpy(sfw, fw, sizeof(*sfw));
> > +
> >  	profile->custom = 1;
> > -	profile->name = fw->name;
> > +	profile->name = sfw->name;
> >  	profile->version = le64_to_cpu(fw->version);
> > -	profile->mcam_kex_prfl.mkex = &fw->mkex;
> > -	profile->lt_def = &fw->lt_def;
> > +	profile->mcam_kex_prfl.mkex = &sfw->mkex;
> > +	profile->lt_def = &sfw->lt_def;
> > +
> > +	/* Binary blob contains ikpu actions entries at start of data[0] */
> > +	if (from_fs) {
> > +		profile->ikpu2 = devm_kcalloc(rvu->dev, 1,
> > +					      sizeof(ikpu_action_entries),
> > +					      GFP_KERNEL);
> > +		if (!profile->ikpu2)
> > +			return -ENOMEM;
> > +
> > +		action = (struct npc_kpu_profile_action *)(fw->data + offset);
> > +
> > +		if (rvu->kpu_fwdata_sz < hdr_sz + sizeof(ikpu_action_entries))
> > +			return -ENOMEM;
> > +
> > +		memcpy((void *)profile->ikpu2, action, sizeof(ikpu_action_entries));
> > +		offset += sizeof(ikpu_action_entries);
> > +	}
> > +
> >  	for (kpu = 0; kpu < fw->kpus; kpu++) {
> >  		fw_kpu = (struct npc_kpu_fwdata *)(fw->data + offset);
> > -		if (fw_kpu->entries > KPU_MAX_CST_ENT)
> > -			dev_warn(rvu->dev,
> > -				 "Too many custom entries on KPU%d: %d > %d\n",
> > -				 kpu, fw_kpu->entries, KPU_MAX_CST_ENT);
> > -		entries = min(fw_kpu->entries, KPU_MAX_CST_ENT);
> > -		cam = (struct npc_kpu_profile_cam *)fw_kpu->data;
> > -		offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam);
> > +		if (!from_fs) {
> > +			if (fw_kpu->entries > KPU_MAX_CST_ENT)
> > +				dev_warn(rvu->dev,
> > +					 "Too many custom entries on KPU%d: %d > %d\n",
> > +					 kpu, fw_kpu->entries, KPU_MAX_CST_ENT);
> > +			entries = min(fw_kpu->entries, KPU_MAX_CST_ENT);
> > +			cam = (struct npc_kpu_profile_cam *)fw_kpu->data;
> > +			offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam);
> > +			action = (struct npc_kpu_profile_action *)(fw->data + offset);
> > +			offset += fw_kpu->entries * sizeof(*action);
> > +			if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
> > +				dev_warn(rvu->dev,
> > +					 "Profile size mismatch on KPU%i parsing.\n",
> > +					 kpu + 1);
> > +				return -EINVAL;
> > +			}
> > +			for (entry = 0; entry < entries; entry++) {
> > +				profile->kpu[kpu].cam[entry] = cam[entry];
> > +				profile->kpu[kpu].action[entry] = action[entry];
> > +			}
> > +			continue;
> > +		}
> > +		entries = fw_kpu->entries;
> > +		dev_info(rvu->dev,
> > +			 "Loading %u entries on KPU%d\n", entries, kpu);
> > +
> > +		cam2 = (struct npc_kpu_profile_cam2 *)fw_kpu->data;
> > +		offset += sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam2);
> >  		action = (struct npc_kpu_profile_action *)(fw->data + offset);
> >  		offset += fw_kpu->entries * sizeof(*action);
> >  		if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
> >  			dev_warn(rvu->dev,
> > -				 "Profile size mismatch on KPU%i parsing.\n",
> > +				 "profile size mismatch on kpu%i parsing.\n",
> >  				 kpu + 1);
> >  			return -EINVAL;
> >  		}
> > +
> > +		profile->kpu[kpu].cam_entries2 = entries;
> > +		profile->kpu[kpu].action_entries2 = entries;
> > +		ret = npc_alloc_kpu_cam2_n_action2(rvu, kpu, entries);
> > +		if (ret) {
> > +			dev_warn(rvu->dev,
> > +				 "profile entry allocation failed for kpu=%d for %d entries\n",
> > +				 kpu, entries);
> > +			return -EINVAL;
> > +		}
> > +
> >  		for (entry = 0; entry < entries; entry++) {
> > -			profile->kpu[kpu].cam[entry] = cam[entry];
> > -			profile->kpu[kpu].action[entry] = action[entry];
> > +			profile->kpu[kpu].cam2[entry] = cam2[entry];
> > +			profile->kpu[kpu].action2[entry] = action[entry];
> >  		}
> >  	}
>
> Could this loop overflow and hang the kernel if firmware specifies more
> than 65535 entries?
>
> The loop variable 'entry' is declared as int at the top of the function,
> but in the from_fs path, 'entries' is read directly from the firmware
> blob:
>
>     entries = fw_kpu->entries;
>
> In the non-from_fs path, entries is capped:
>
>     entries = min(fw_kpu->entries, KPU_MAX_CST_ENT);
>
> But in the from_fs path there is no cap on entries. If a corrupt or
> malicious firmware file specifies fw_kpu->entries with a very large
> value, the loop could run for an extremely long time or cause other
> issues.
>
> The kernel should not rely on filesystem data being well-formed.
> Validation is needed for the from_fs case.
>
> This was raised by Simon Horman during v3 review on 2026-03-10. The
> author acknowledged this in v3 saying they would fix it in v5, but the
> issue persists in the current version.
ACK. Last AI review suggest is - either to use "int" as loop variable or do a capping
on the value. I did modify it to "int".