[PWCI] "net/intel: check raw item spec/mask for null"#601
[PWCI] "net/intel: check raw item spec/mask for null"#601
Conversation
adds explicit NULL pointer checks for the RAW flow item’s spec and mask fields in both the IAVF and ICE PMDs. This prevents potential null pointer dereferences and provides clearer error reporting to the user. Signed-off-by: Shaiq Wani <shaiq.wani@intel.com> Signed-off-by: 0-day Robot <robot@bytheb.org>
Reviewer's GuideAdds NULL checks and proper error reporting for RAW rte_flow items in Intel iavf and ice drivers, and threads rte_flow_error into the RAW pattern parsing helpers that previously assumed non-NULL spec/mask pointers. Sequence diagram for RAW pattern parsing with NULL checks in Intel driverssequenceDiagram
actor App
participant PMD as iavf_hash_parse_pattern_action
participant RawParser as iavf_hash_parse_raw_pattern
participant Error as rte_flow_error
App->>PMD: rte_flow_create(pattern, actions, error)
PMD->>PMD: detect IAVF_PHINT_RAW
PMD->>RawParser: iavf_hash_parse_raw_pattern(item, error, meta)
RawParser->>RawParser: raw_spec = item->spec
RawParser->>RawParser: raw_mask = item->mask
alt RAW spec or mask is NULL
RawParser->>Error: rte_flow_error_set(error, EINVAL, RTE_FLOW_ERROR_TYPE_ITEM, item, NULL_RAW_spec_mask)
RawParser-->>PMD: return -rte_errno
PMD-->>App: fail rte_flow_create with error
else RAW spec and mask are non NULL
RawParser->>RawParser: validate lengths and content
RawParser-->>PMD: return 0
PMD-->>App: continue flow rule setup
end
Flow diagram for RAW rte_flow item NULL checking in Intel driversflowchart TD
Start[Start RAW item handling] --> GetItem[Load item spec and mask]
GetItem --> CheckNull{raw_spec or raw_mask is NULL?}
CheckNull -->|Yes| Log[Log NULL RAW spec/mask]
Log --> SetErr[Call rte_flow_error_set with EINVAL and RTE_FLOW_ERROR_TYPE_ITEM]
SetErr --> ReturnErr[Return -rte_errno to caller]
CheckNull -->|No| Validate[Validate RAW pattern lengths and content]
Validate --> Continue[Continue with RSS/FDIR rule configuration]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughAdds NULL validation checks for RAW pattern items across Intel IAVF and ICE network drivers. Files in hash modules (iavf_hash.c, ice_hash.c) additionally propagate a new error parameter to parsing functions, enabling consistent error reporting. Changes strengthen error handling for incomplete RAW pattern specifications. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- When detecting NULL RAW spec/mask, consider returning the result of
rte_flow_error_set(...)directly instead of calling it and thenreturn -rte_errno;, to avoid relying on external errno state and to match the common DPDK error-handling pattern. - In the hash parse paths,
iavf_hash_parse_raw_pattern/ice_hash_parse_raw_patternnow setrte_flow_errorthemselves and the caller also callsrte_flow_error_seton non-zero return; it may be clearer to centralize error reporting in one place to avoid double-setting or conflicting error details. - The new NULL RAW spec/mask checks log an error via
PMD_DRV_LOGfor iavf but not for the corresponding ice paths; consider making the logging behavior consistent across drivers for easier debugging.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- When detecting NULL RAW spec/mask, consider returning the result of `rte_flow_error_set(...)` directly instead of calling it and then `return -rte_errno;`, to avoid relying on external errno state and to match the common DPDK error-handling pattern.
- In the hash parse paths, `iavf_hash_parse_raw_pattern`/`ice_hash_parse_raw_pattern` now set `rte_flow_error` themselves and the caller also calls `rte_flow_error_set` on non-zero return; it may be clearer to centralize error reporting in one place to avoid double-setting or conflicting error details.
- The new NULL RAW spec/mask checks log an error via `PMD_DRV_LOG` for iavf but not for the corresponding ice paths; consider making the logging behavior consistent across drivers for easier debugging.
## Individual Comments
### Comment 1
<location> `drivers/net/intel/iavf/iavf_hash.c:899-907` </location>
<code_context>
raw_spec = item->spec;
raw_mask = item->mask;
+ if (!raw_spec || !raw_mask) {
+ PMD_DRV_LOG(ERR, "NULL RAW spec/mask");
+ rte_flow_error_set(error, EINVAL,
+ RTE_FLOW_ERROR_TYPE_ITEM,
+ item, "NULL RAW spec/mask");
+ return -rte_errno;
+ }
+
spec_len = strlen((char *)(uintptr_t)raw_spec->pattern);
if (strlen((char *)(uintptr_t)raw_mask->pattern) !=
spec_len)
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider using a bounded string length (like strnlen with raw_spec->length) rather than strlen on RAW pattern.
The NULL guard is helpful, but strlen on raw_spec->pattern is still unbounded. If the pattern is not NUL-terminated within raw_spec->length, this can read past the buffer. ice_hash_parse_raw_pattern already uses strnlen with raw_spec->length; please do the same here for consistency and safety:
```c
spec_len = strnlen((char *)(uintptr_t)raw_spec->pattern,
raw_spec->length + 1);
if (spec_len != raw_spec->length)
...
```
Suggested implementation:
```c
const struct rte_flow_item_raw *raw_spec, *raw_mask;
size_t spec_len, mask_len;
raw_spec = item->spec;
raw_mask = item->mask;
```
```c
spec_len = strnlen((char *)(uintptr_t)raw_spec->pattern,
raw_spec->length + 1);
if (spec_len != raw_spec->length) {
PMD_DRV_LOG(ERR,
"RAW spec pattern length %zu does not match length field %u",
spec_len, raw_spec->length);
rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM,
item,
"RAW spec pattern must be NUL-terminated within length");
return -rte_errno;
}
mask_len = strnlen((char *)(uintptr_t)raw_mask->pattern,
raw_mask->length + 1);
if (mask_len != raw_mask->length || mask_len != spec_len) {
PMD_DRV_LOG(ERR,
"RAW mask pattern length %zu does not match length field %u or spec length %zu",
mask_len, raw_mask->length, spec_len);
rte_flow_error_set(error, EINVAL,
RTE_FLOW_ERROR_TYPE_ITEM,
item,
"RAW mask pattern length must match spec length and be NUL-terminated within length");
return -rte_errno;
}
```
Because the provided snippet does not show the body of the original:
```c
if (strlen((char *)(uintptr_t)raw_mask->pattern) !=
spec_len)
/* ... */
```
you should remove or adjust any remaining block that handled this mismatch to avoid duplicate or now-unreachable checks. In other words, the existing error-handling `if` for `mask` vs `spec` length mismatch should be deleted or updated so that the new `strnlen`-based checks are the single source of truth for validating RAW pattern lengths, mirroring the approach used in `ice_hash_parse_raw_pattern`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/net/intel/iavf/iavf_hash.c (1)
885-920: Fix pkt_buf leak on mask allocation failure; RAW NULL handling itself is goodThe new
raw_spec/raw_maskNULL check iniavf_hash_parse_raw_pattern()correctly prevents dereferences and reports anITEMerror viarte_flow_error_set, and the newerrorparameter is wired through fromiavf_hash_parse_pattern_action()as intended.However, there is a pre‑existing resource‑management bug in the same function:
- On failure to allocate
msk_buf,pkt_bufis not freed, leakingpkt_lenbytes.You can fix this by freeing
pkt_bufin that branch:Proposed fix for `iavf_hash_parse_raw_pattern` leak
pkt_buf = rte_zmalloc(NULL, pkt_len, 0); if (!pkt_buf) return -ENOMEM; msk_buf = rte_zmalloc(NULL, pkt_len, 0); - if (!msk_buf) - return -ENOMEM; + if (!msk_buf) { + rte_free(pkt_buf); + return -ENOMEM; + }Note: similar to
ice_hash_parse_raw_pattern,iavf_hash_parse_pattern_action()unconditionally wraps any non‑zero return with a generic"Parse raw pattern failed"rte_flow_error_set, which overwrites the more specific"NULL RAW spec/mask"message set in the callee. That’s harmless but you may want to rely on the callee’s error text for this path for clearer diagnostics.Also applies to: 1555-1563
🧹 Nitpick comments (1)
drivers/net/intel/ice/ice_hash.c (1)
642-667: RAW spec/mask NULL validation is correct; consider avoiding redundant error overwriteThe new
raw_spec/raw_maskNULL check inice_hash_parse_raw_pattern()correctly prevents dereferences and reports anITEMerror through the newerrorparameter. The caller inice_hash_parse_pattern_action()then unconditionally wraps any failure with a secondrte_flow_error_set("Parse raw pattern failed"), which will overwrite the more specific message from the callee. If you care about preserving the detailed cause (e.g."NULL RAW spec/mask"), you could rely on the callee’srte_flow_error_setfor this path and only set a generic message here for other error codes.Also applies to: 1194-1203
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
drivers/net/intel/iavf/iavf_fdir.cdrivers/net/intel/iavf/iavf_fsub.cdrivers/net/intel/iavf/iavf_hash.cdrivers/net/intel/ice/ice_fdir_filter.cdrivers/net/intel/ice/ice_hash.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (3)
drivers/net/intel/iavf/iavf_fsub.c (1)
184-195: RAW NULL check iniavf_fsub_parse_patternlooks correctThe added
raw_spec/raw_maskNULL guard cleanly prevents dereferences and reports anITEMerror viarte_flow_error_set, matching existing error‑handling patterns in this function.drivers/net/intel/ice/ice_fdir_filter.c (1)
1850-1865: RAW NULL check inice_fdir_parse_patternis appropriateThe added
raw_spec/raw_maskNULL guard ensures RAW patterns are rejected with a clearITEM/EINVAL error instead of risking a NULL dereference when converting the patterns.drivers/net/intel/iavf/iavf_fdir.c (1)
772-783: RAW NULL guard iniavf_fdir_parse_patternis correct and consistentAdding the
raw_spec/raw_maskNULL check here safely prevents dereferences of missing RAW data and reports anITEM/EINVAL error with a clear"NULL RAW spec/mask"message, matching the style used elsewhere in this driver.
2 participants
NOTE: This is an auto submission for "net/intel: check raw item spec/mask for null".
See "http://patchwork.dpdk.org/project/dpdk/list/?series=36977" for details.
Summary by Sourcery
Validate RAW flow item spec/mask pointers in Intel iavf and ice drivers and propagate errors when they are missing.
Bug Fixes:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.