Repository navigation
math: iir: reject series count that does not divide num_sections - #11318
Conversation
|
Can one of the admins verify this patch?
|
There was a problem hiding this comment.
🟢 Approval recommended
The checks safely enforce the processing loops’ requirements, and no blocking issues remain.
No open issues 🎉
What changed in this PR
Extends IIR configuration validation to reject series counts that do not divide the section count, preventing out-of-bounds filter processing.
Changes:
- Adds divisibility checks to both DF1 and DF2T validators.
- Adds a DF1 regression test covering valid and invalid series counts.
| File | Description |
|---|---|
test/cmocka/src/audio/eq_iir/eq_iir_process.c |
Tests series-count validation. |
src/math/iir_df2t.c |
Rejects non-divisible DF2T configurations. |
src/math/iir_df1.c |
Rejects non-divisible DF1 configurations. |
Review effort: Balanced
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
thanks @Shrusti-pk . @singalsu please check before merge
| int i; | ||
|
|
||
| struct CMUnitTest tests[ARRAY_SIZE(parameters)]; | ||
| struct CMUnitTest tests[ARRAY_SIZE(parameters) + 1]; |
There was a problem hiding this comment.
Looks like we need to fix the array loop and not pad the array ?
There was a problem hiding this comment.
Agreed, padding it was the wrong shape. The parameter loop now fills exactly ARRAY_SIZE(parameters) entries again, and the validator case runs as its own cmocka group via cmocka_unit_test(), with main() adding up the failures from both groups. Checked that the new group still fails against the unpatched iir_delay_size_df1().
iir_delay_size_df1() is the only check an IIR response header gets before eq_iir, sound_dose and the tdfb direction emphasis filter size the delay lines (4 * num_sections words per channel) and point iir->coef at the blob biquads. It accepts any num_sections_in_series in [1, num_sections]. iir_df1() and its HiFi, ARM and RISC-V variants step the outer loop by num_sections_in_series until it reaches num_sections, so they run ceil(num_sections / nseries) * nseries biquads. When the series count does not divide num_sections the filter reads coefficients past the end of the blob and writes filter state past the delay line. Both values come from the bytes control blob. With num_sections = 11 and num_sections_in_series = 10, 20 biquads run: ASan reports a heap read past the 332 byte blob copy and a heap write past the 352 byte two channel delay allocation in iir_df1(). Require num_sections to be a multiple of num_sections_in_series. Do the same in iir_delay_size_df2t(), which has the identical check. The tuning scripts always set num_sections_in_series to num_sections, so generated blobs are not affected. Add a cmocka case for the validator. Signed-off-by: Shrushti P K <shrusthi@labs.digiscrypt.com>
c648176 to
ac9f06c
Compare
|
On the % question, the two added here aren't in a hot path. iir_delay_size_df1() and iir_delay_size_df2t() only run when the filter is configured: from eq_iir_init_coef(), sound_dose_filters_init() and tdfb_direction_init(), i.e. at init or when a new blob is applied. The per-sample iir_df1()/iir_df2t() loops are untouched, so there's no divide on the processing path. |
PR 11318: test resultsRun date: 2026-10-22 11:39 UTC Tested commit: ac9f06c37d679d0225a7e0c2995dd4d28e5632fa
|
iir_delay_size_df1() range-checks num_sections_in_series (added in #10892) but never requires it to divide num_sections:
The tuning scripts always emit num_sections_in_series == num_sections, so generated blobs still load. sof-testbench4 output for the eqiir16/24/32, gain32 and drc32 benchmark topologies is byte-identical before and after, test-all-defconfigs.sh is green, and the new cmocka case fails without the fix.