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

[Simon Horman <horms@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.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
octeontx2-af: npc: Support for custom KPU profile from filesystem

Flashing updated firmware on deployed devices is cumbersome. Provide a
mechanism to load a custom KPU (Key Parse Unit) profile directly from
the filesystem at module load time.

When the rvu_af module is loaded with the kpu_profile parameter, the
specified profile is read from /lib/firmware/kpu and programmed into
the KPU registers. Add npc_kpu_profile_cam2 for the extended cam format
used by filesystem-loaded profiles and support ptype/ptype_mask in
npc_config_kpucam when profile->from_fs is set.

Link: https://lore.kernel.org/netdev/20260310172100.890161-1-horms@kernel.org/
Link: https://lore.kernel.org/netdev/20260317033214.646969-2-kuba@kernel.org/

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> index 69439ff76e10..a3015b12a20b 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/cn20k/npc.c

[ ... ]

> @@ -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.

> 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.