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