Re: [PATCH v17 net-next 6/8] octeontx2-af: npc: Support for custom KPU profile from filesystem

[Ratheesh Kannoth <rkannoth@marvell.com>]

On 2026-06-01 at 08:28:41, Ratheesh Kannoth (rkannoth@marvell.com) wrote:
> 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.

>> +	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);
>Is it safe to store devm-allocated pointers into this kpu structure?
>Looking at npc_prepare_default_kpu(), the profile->kpu pointer is
>assigned to npc_kpu_profiles, which is a global static array.
>When memory bounded to a specific device via devm_kcalloc is stored
>in this global array, won't multiple devices race to overwrite these
>pointers? Additionally, if the device unbinds, devres will free the memory,
>leaving dangling pointers in the global array that could cause a
>use-after-free for other devices on the system.

There is only one AF device per system.

>> +	struct npc_kpu_profile_cam *cam;
>> +	struct npc_kpu_fwdata *fw_kpu;
>> +	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) {
>This is a pre-existing issue, but does this bounds check guarantee enough
>space to safely read fw_kpu->entries below?
>The check ensures the offset reaches the start of the fw_kpu structure,
>but it doesn't appear to verify there are at least sizeof(*fw_kpu) bytes
>remaining in the buffer. If the firmware image is truncated exactly
>at this boundary, could dereferencing fw_kpu->entries read out of
>bounds?
Pre existing issue; a seperate bug fix patch will be posted to net tree once this series
is merged.

>> +		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);
>> +		action = (struct npc_kpu_profile_action *)(fw->data + offset);
>> +		offset += fw_kpu->entries * sizeof(*action);
>This is a pre-existing issue, but could the offset calculation overflow
>on 32-bit architectures?
Pre existing issue; a seperate bug fix patch will be posted to net tree once this series
is merged.

>The expression sizeof(*fw_kpu) + fw_kpu->entries * sizeof(*cam)
>evaluates as a 32-bit unsigned size_t. If a maliciously large value is
>provided for fw_kpu->entries, the multiplication could wrap around,
>producing a small offset that passes the bounds check below, leading to
>out-of-bounds memory reads when copying the entries.<F11>
>> +		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];
>This is a pre-existing issue, but does this permanently corrupt the default
>built-in profile?

Pre existing issue; a seperate bug fix patch will be posted to net tree once this series
is merged.

>> +	/* 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 tying the firmware binary layout directly to the size of an
>internal kernel compilation constant cause backward compatibility issues?
We can't change ikpu structure to add another field as it will break backward compatability.
So we will keep same number of items in ikpu.

>	mkex_extr = rvu->kpu.mcam_kex_prfl.mkex_extr;
>> +	mkex = rvu->kpu.mcam_kex_prfl.mkex;
>> +
>>  	if (is_cn20k(rvu->pdev)) {
>> -		keyx_cfg = mkex_extr->keyx_cfg;
>> +		keyx_cfg = (u64 *)mkex_extr->keyx_cfg;
>>  	} else {
>> -		keyx_cfg = mkex->keyx_cfg;
>> +		keyx_cfg = (u64 *)mkex->keyx_cfg;
>This is a pre-existing issue, but does this cast away the const
>qualifier and modify read-only data? Both mkex_extr and mkex are typed
>as const, and frequently point to the global static npc_mkex_default profile.

Pre existing issue; a seperate bug fix patch will be posted to net tree once this series
is merged.