audio: kpb: Fixing issues reported by CodeQL during PTL code scanning#10858
Open
tmleman wants to merge 2 commits into
Open
audio: kpb: Fixing issues reported by CodeQL during PTL code scanning#10858tmleman wants to merge 2 commits into
tmleman wants to merge 2 commits into
Conversation
tmleman
requested review from
abonislawski,
kv2019i,
lgirdwood,
serhiy-katsyuba-intel,
softwarecki and
wjablon1
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses CodeQL-reported integer width/overflow concerns in the KPB (Key Phrase Buffer) audio component by widening intermediate types and forcing safer integer promotion during size calculations.
Changes:
- Widened
chloop variables fromuint16_ttouint32_tin mic selection copy helpers. - Added
(size_t)casts in several size/multiplication expressions to avoid unintended 32-bit overflow/truncation in arithmetic.
| struct audio_stream *istream = &source->stream; | ||
| struct audio_stream *ostream = &sink->stream; | ||
| uint16_t ch; | ||
| uint32_t ch; |
| struct audio_stream *istream = &source->stream; | ||
| struct audio_stream *ostream = &sink->stream; | ||
| uint16_t ch; | ||
| uint32_t ch; |
| buffer_stream_invalidate(source, size); | ||
| size_t out_samples; | ||
| uint16_t ch; | ||
| uint32_t ch; |
| buffer_stream_invalidate(source, size); | ||
| size_t out_samples; | ||
| uint16_t ch; | ||
| uint32_t ch; |
Six multiplications in the key-phrase buffer compute their product at 32-bit width and only then assign it to a wider size_t result. If the operands are large the overflow has already occurred before widening. The KPB sizing math is partly driven by externally-influenced values (cli->drain_req, the configured channel count, sampling frequency and container width), so this is a real overflow surface rather than a purely theoretical one. Cast the leading operand to size_t in each expression so the whole product is evaluated at the destination width: - kpb_micselect_copy16/32(): loop bound samples_per_chan * in_channels - kpb_init_draining(): drain_req and bytes_per_ms - adjust_drain_interval(): pipeline_period - validate_host_params(): bytes_per_ms No functional change on in-range inputs; only the intermediate arithmetic width changes. Found-by: CodeQL 2.24.2 (codeql/cpp-queries cpp-security-extended), rule cpp/integer-multiplication-cast-to-long. Run with database build-mode=none over sof/src (host clang cannot target the Xtensa production build), 867 files / 98 queries. Findings at kpb.c:1117,1148,1610,1619,1791,2397. AI-triaged: findings manually cross-referenced against clang-tidy bugprone-implicit-widening-of-multiplication-result and semgrep raptor-integer-truncation on the same surface, and confirmed the operand types (uint32_t / macro constants) against struct sof_kpb_config and struct kpb_client before fixing. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
The four kpb_micselect_copy16/32() variants declare the channel loop counter "ch" as uint16_t while iterating "for (ch = 0; ch < ... micsel_channels; ch++)", where micsel_channels is uint32_t. The loop condition promotes ch to a wider type for the comparison, so if micsel_channels ever exceeds UINT16_MAX the counter wraps at 65536 and the loop fails to terminate. The same narrow counter would also truncate channel indexing. Declare ch as uint32_t so its width matches the bound it is compared against and the channel count it indexes. KPB_MAX_MICSEL_CHANNELS keeps the real value small, so this is hardening rather than an observed runaway, but the type mismatch is removed at the source. CodeQL flagged the two non-HiFi variants (kpb.c:1112,1143); the two KPB_HIFI3 variants (kpb.c:1050,1084) carry the identical pattern and are fixed in the same change for consistency. Found-by: CodeQL 2.24.2 (codeql/cpp-queries cpp-security-extended), rule cpp/comparison-with-wider-type. Run with database build-mode=none over sof/src, 867 files / 98 queries. AI-triaged: traced ch and micsel_channels declarations across all four micselect copy functions and confirmed the bound is uint32_t before widening the counter; extended the fix to the HiFi3 variants the tool did not reach in this build configuration. Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
lgirdwood
reviewed
Jun 9, 2026
lgirdwood
left a comment
lgirdwood
left a comment
Member
There was a problem hiding this comment.
LGTM, but can you resolve the copilot comments. I suspect this pattern/fix will apply in other places too.
softwarecki
reviewed
Jun 9, 2026
|
|
||
| const int16_t *in_data; | ||
| int16_t *out_data; | ||
| const uint32_t samples_per_chan = size / (sizeof(uint16_t) * micsel_channels); |
Collaborator
There was a problem hiding this comment.
const size_t samples_per_chan
softwarecki
approved these changes
Jun 9, 2026
serhiy-katsyuba-intel
approved these changes
Jun 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These two commits are addressing issues reported by CodeQL and confirmed by other tools with use of AI for verification.