Repository navigation
Conversation
tmleman
requested review from
serhiy-katsyuba-intel,
softwarecki and
wjablon1
and
a balanced review from Copilot
October 8, 2026 12:16
tmleman
requested review from
abonislawski,
dbaluta,
kv2019i,
lbetlej,
lgirdwood,
mmaka1,
pblaszko and
plbossart
as code owners
October 8, 2026 12:16
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Release handling can still overflow when the mailbox contains the documented invalid start-offset sentinel.
1 open finding
What changed in this PR
Fixes overflow and underflow in host-visible stream-position offset calculations.
Changes:
- Clamps negative pipeline latency to zero.
- Promotes latency multiplication to 64-bit.
- Guards resume-offset subtraction against underflow.
| File | Description |
|---|---|
src/audio/pipeline/pipeline-graph.c |
Prevents unsigned latency wraparound. |
src/audio/copier/copier.c |
Hardens stream offset arithmetic. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Comment on lines
+520
to
+521
| if (posn.dai_posn > pipe_reg.stream_end_offset) | ||
| pipe_reg.stream_start_offset += posn.dai_posn - pipe_reg.stream_end_offset; |
pipeline_get_dai_comp_latency() accumulated the buffered block count as *latency += input_data / ibs - output_data / obs; comp_get_total_data_processed() returns uint64_t and ibs/obs are uint32_t, so both divisions and the subtraction are evaluated as unsigned 64-bit with no ordering guarantee between the two terms. When the sink has processed more blocks than the source the expression wraps to a value close to 2^64 and is then truncated into the uint32_t *latency as a huge block count. The sink leading the source is not an error case: it is exactly the "dai is enabled before host since they are in different pipelines" scenario described by the copier code that consumes this value. The resulting latency is multiplied by the period size in copier_comp_trigger() and published to the host driver as stream_start_offset, so the host computes its stream position from a bogus offset. A buffered block count cannot be negative, so only accumulate when the source leads the sink. Assisted-by: Copilot:claude-opus-5 Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
latency and audio_stream_period_bytes() are both 32 bit, so the product was computed in 32-bit arithmetic before being added to the 64-bit stream_start_offset reported to the host driver. With well formed values the product stays around 10^6 bytes, far below the 32-bit limit: an overflow would require a latency of roughly 2.8 million blocks, i.e. tens of minutes of buffered audio. The only way to reach such a value was the unsigned underflow in pipeline_get_dai_comp_latency() fixed in the preceding commit, and a cast alone would not have helped there. This change is therefore hardening, not a fix for a reachable overflow. Cast latency to uint64_t on both the START and RELEASE paths so the outer multiplication is carried out at the width of the destination. Reported by CodeQL cpp/integer-multiplication-cast-to-long. Assisted-by: Copilot:claude-opus-5 codeql Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
stream_end_offset is sampled from the dai position when the pipeline is paused and is read back from the host visible mailbox on release. The release path subtracts it from the current dai position with an unsigned 64-bit subtraction and no ordering check, so a stale or out of order value wraps into a huge number and corrupts the stream start offset reported to the host driver. The host computes the stream position as position = DMA counter - stream_start_offset so a corrupted offset is directly visible as a bogus position. Accumulate only when the current position is actually ahead of the saved end offset, mirroring the clamp applied to the latency accumulation in pipeline_get_dai_comp_latency(). Assisted-by: Copilot:claude-opus-5 Signed-off-by: Tomasz Leman <tomasz.m.leman@intel.com>
PR 11287: test resultsRun date: 2026-10-08 16:46 UTC Tested commit: 17272cf8177acae65554a36204d2f03815d83679
|
lgirdwood
approved these changes
Oct 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.
Three related arithmetic defects on the path that computes the stream position offsets written to the host-visible mailbox.
Found while reviewing a CodeQL integer-overflow report in the copier.
The reported truncation turned out to be unreachable on its own: it only mattered because the latency value feeding it could underflow first (commit 1), which is the actual bug. Commit 2 is the hardening CodeQL asked for. Commit 3 is the same underflow pattern found by inspection in the release path, where the operand is read back from the mailbox and cannot be trusted. The sink leading the source is a documented, legitimate condition, so commit 1 is reachable on normal playback with the dai and host in separate pipelines.