Re: [PATCH v12 net-next 7/9] octeontx2-af: npc: Support for custom KPU profile from filesystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Global Data Corruption and Data Race via shared static array
- [High] Integer underflow and missing bounds checks in legacy firmware parsers
- [High] Fragile firmware ABI heavily coupled to internal kernel structure size
- [High] Filesystem profile loading truncates KPU programming, breaking default parsing rules
- [Medium] Missing endianness conversion for u16 fields
--

commit c9233fb2ae7b0ce301cfbbff2b1cd306d7c325ba
Author: Ratheesh Kannoth <rkannoth@marvell.com>

octeontx2-af: npc: Support for custom KPU profile from filesystem

This commit provides a mechanism to load a custom KPU (Key Parse Unit)
profile directly from the filesystem at module load time.

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/npc.h b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> index cefc5d70f3e49..c8c0cb68535c7 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/npc.h
> @@ -265,6 +265,19 @@ struct npc_kpu_profile_cam {
>  	u16 dp2_mask;
>  } __packed;
>  
> +struct npc_kpu_profile_cam2 {
> +	u8 state;
> +	u8 state_mask;
> +	u16 dp0;
> +	u16 dp0_mask;
> +	u16 dp1;
> +	u16 dp1_mask;
> +	u16 dp2;
> +	u16 dp2_mask;

Since the firmware image is little-endian, should these fields be defined as
__le16?

When they are accessed as host-order integers without le16_to_cpu() conversion,
will this lead to incorrect packet parsing rules and silent data corruption on
big-endian architectures?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> index 5fa9e1c7ae9fa..a0f97e683145f 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_npc.c
[ ... ]
> @@ -1788,26 +1872,168 @@ static void npc_prepare_default_kpu(struct rvu *rvu,
>  	npc_cn20k_update_action_entries_n_flags(rvu, profile);
>  }
>  
> -static int npc_apply_custom_kpu(struct rvu *rvu,
> -				struct npc_kpu_profile_adapter *profile)
> +static int npc_alloc_kpu_cam2_n_action2(struct rvu *rvu, int kpu_num,
> +					int num_entries)
> +{
> +	struct npc_kpu_profile_adapter *adapter = &rvu->kpu;
> +	struct npc_kpu_profile *kpu;
> +
> +	kpu = &adapter->kpu[kpu_num];
> +
> +	kpu->cam2 = devm_kcalloc(rvu->dev, num_entries,
> +				 sizeof(*kpu->cam2), GFP_KERNEL);
> +	if (!kpu->cam2)
> +		return -ENOMEM;
> +
> +	kpu->action2 = devm_kcalloc(rvu->dev, num_entries,
> +				    sizeof(*kpu->action2), GFP_KERNEL);

Will this cause data corruption and a use-after-free across devices?

In npc_prepare_default_kpu(), the device's profile->kpu pointer is initialized
to point to the statically defined npc_kpu_profiles global array.

Here, we are assigning device-scoped devm_kcalloc memory directly into this
globally shared array. If multiple RVU devices probe concurrently, they might
data-race on this shared state. Additionally, upon device unbind, devres will
free the memory, leaving dangling pointers in the global array.

[ ... ]
> +static int npc_apply_custom_kpu_from_fw(struct rvu *rvu,
> +					struct npc_kpu_profile_adapter *profile)
>  {
>  	size_t hdr_sz = sizeof(struct npc_kpu_profile_fwdata), offset = 0;
> +	const struct npc_kpu_profile_fwdata *fw;
>  	struct npc_kpu_profile_action *action;
> -	struct npc_kpu_profile_fwdata *fw;
>  	struct npc_kpu_profile_cam *cam;
>  	struct npc_kpu_fwdata *fw_kpu;
> -	int entries;
> -	u16 kpu, entry;
> +	int entries, entry, kpu;
>  
> -	if (is_cn20k(rvu->pdev))
> -		return npc_cn20k_apply_custom_kpu(rvu, profile);
> +	fw = rvu->kpu_fwdata;
> +
> +	for (kpu = 0; kpu < fw->kpus; kpu++) {
> +		if (rvu->kpu_fwdata_sz < hdr_sz + offset) {
> +			dev_warn(rvu->dev,
> +				 "Profile size mismatch on KPU%i parsing\n",
> +				 kpu + 1);
> +			return -EINVAL;
> +		}
> +
> +		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_t(int, 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);

Is there a missing bounds check here for negative values?

Since fw_kpu->entries is read as a signed int, a malicious firmware blob
providing a negative value (e.g., -1) could cause the offset calculation to
underflow the unsigned size_t offset.

This would wrap the offset backwards, allowing the boundary check
(rvu->kpu_fwdata_sz < hdr_sz + offset) to incorrectly pass in the next
iteration, resulting in an out-of-bounds access.

[ ... ]
> +static int npc_apply_custom_kpu_from_fs(struct rvu *rvu,
> +					struct npc_kpu_profile_adapter *profile)
> +{
> +	size_t hdr_sz = sizeof(struct npc_kpu_profile_fwdata), offset = 0;
> +	const struct npc_kpu_profile_fwdata *fw;
> +	struct npc_kpu_profile_action *action;
> +	struct npc_kpu_profile_cam2 *cam2;
> +	struct npc_kpu_fwdata *fw_kpu;
> +	int entries, ret, entry, kpu;
>  
>  	fw = rvu->kpu_fwdata;
>  
> +	/* Binary blob contains ikpu actions entries at start of data[0] */
> +	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 -EINVAL;
> +
> +	/* The firmware layout does dependent on the internal size of
> +	 * ikpu_action_entries.
> +	 */
> +	memcpy((void *)profile->ikpu2, action, sizeof(ikpu_action_entries));
> +	offset += sizeof(ikpu_action_entries);

Will this create a fragile firmware ABI coupled to the internal kernel
structure size?

Because ikpu_action_entries is a kernel-internal static array, its size
depends on the number of port kinds (NPC_MAX_PKIND) defined in the specific
kernel version compiling it.

If future kernel versions add elements to this array, the parser will expect a
larger initial block than the firmware file actually provides. This would
misalign all subsequent read offsets, causing the driver to interpret garbage
memory.

[ ... ]
> +static int npc_load_kpu_profile_from_fs(struct rvu *rvu)
> +{
> +	struct npc_kpu_profile_adapter *profile = &rvu->kpu;
> +	const char *kpu_profile = rvu->kpu_pfl_name;
> +	const struct firmware *fw = NULL;
> +	int ret, fw_kpus = 0;
> +	char path[512] = "kpu/";
> +
> +	if (strlen(kpu_profile) > sizeof(path) - strlen("kpu/") - 1) {
> +		dev_err(rvu->dev, "kpu profile name is too big\n");
> +		return -ENOSPC;
> +	}
> +
> +	strcat(path, kpu_profile);
> +
> +	if (request_firmware_direct(&fw, path, rvu->dev))
> +		return -ENOENT;
> +
> +	dev_info(rvu->dev, "Loading KPU profile from filesystem: %s\n",
> +		 path);
> +
> +	rvu->kpu_fwdata = fw->data;
> +	rvu->kpu_fwdata_sz = fw->size;
> +
> +	ret = npc_apply_custom_kpu(rvu, profile, true, &fw_kpus);
> +	release_firmware(fw);
> +	rvu->kpu_fwdata = NULL;
> +
> +	if (ret) {
> +		rvu->kpu_fwdata_sz = 0;
> +		dev_err(rvu->dev,
> +			"Loading KPU profile from filesystem failed\n");
> +		return ret;
> +	}
> +
> +	rvu->kpu.kpus = fw_kpus;
> +	profile->kpus = fw_kpus;

Does this truncate the hardware programming loop in npc_parser_profile_init()?

By explicitly updating profile->kpus to fw_kpus, the hardware programming loop
will only configure up to fw_kpus. Since all KPUs are disabled at the beginning
of npc_parser_profile_init(), the remaining hardware KPUs will be left
completely disabled, breaking network parsing for traffic reliant on default
KPU logic.

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