Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ipc-helper.c: reject invalid SOF_MEM_CAPS_* bits #8850

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

marc-hb
Copy link
Collaborator

@marc-hb marc-hb commented Feb 10, 2024

Fixes lack of SOF_MEM_CAPS_* input validation found in #8832

@marc-hb

This comment was marked as outdated.

@marc-hb

This comment was marked as outdated.

src/ipc/ipc-helper.c Show resolved Hide resolved
src/include/ipc/topology.h Outdated Show resolved Hide resolved
@marc-hb
Copy link
Collaborator Author

marc-hb commented Feb 27, 2024

Unrelated "signal overflow!" failure in https://sof-ci.01.org/sofpr/PR8850/build2957/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=check-alsabat-headset-playback-997 on sh-mtlp-rvp-nocodec-06. Everything else green.

1 unrelated suspend/resume failure in https://sof-ci.01.org/sofpr/PR8850/build2958/devicetest/index.html, everything else green.

1 unrelated failure in https://sof-ci.01.org/sof-pr-viewer/#/build/PR8850/build13640869, every other test is green.

12:53:09,629 INFO  - tests/avs/fw_01_base_fw_modules/test_13_updwmix.py::TestUpdwmix::test_01_13_updownmixer_bat_scope[48000Hz_32in32bit_4ch-48000Hz_32in32bit_2ch1]
12:53:09,649 INFO  - [gw0] [100%] FAILED tests/avs/fw_01_base_fw_modules/test_13_updwmix.py::TestUpdwmix::test_01_13_updownmixer_bat_scope[48000Hz_32in32bit_4ch-48000Hz_32in32bit_2ch1]
12:53:09,650 INFO  -
12:53:09,650 INFO  - =================================== FAILURES ===================================
12:53:09,650 INFO  - _ TestUpdwmix.test_01_13_updownmixer_bat_scope[48000Hz_32in32bit_4ch-48000Hz_32in32bit_2ch1] _

@@ -87,6 +87,8 @@ struct sof_ipc_comp {
#define SOF_MEM_CAPS_EXEC BIT(7) /**< executable */
#define SOF_MEM_CAPS_L3 BIT(8) /**< L3 memory */

#define SOF_MEM_CAPS_MAX_BIT BIT(8) /**< Used for input validation */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is ABI, its better we make it obvious via an inline comment that MAX_BIT needs to be updated for any new MEM_CAPS addition. I would also define MAX_BIT as CAPS_L3 rather than BIT(8) as this makes it even clearer to anyone reading the code.

Copy link
Collaborator Author

@marc-hb marc-hb Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we make it obvious via an inline comment that MAX_BIT needs to be updated for any new MEM_CAPS addition.

Comment added.

I would also define MAX_BIT as CAPS_L3 rather than BIT(8) as this makes it even clearer to anyone reading the code.

I can't do that because I realized v2 12aff94 was buggy, apologies. In v2 I made _MAX equal to the last last valid bit but that is wrong, it would incorrectly reject _MAX + some other BIT. The tests passed anyway because we don't seem to test L3_HEAP.

The comparison must be with the first invalid bit. I confused myself with the name "_MAX", it is a bad name. I just replaced it with _LOWEST_INVALID = max << 1

@marc-hb marc-hb marked this pull request as draft February 28, 2024 18:10
Fixes lack of SOF_MEM_CAPS_* input validation found in thesofproject#8832

Signed-off-by: Marc Herbert <[email protected]>
@marc-hb marc-hb marked this pull request as ready for review February 28, 2024 19:29
@marc-hb
Copy link
Collaborator Author

marc-hb commented Feb 28, 2024

@lgirdwood lgirdwood added this to the v2.9 milestone Feb 29, 2024
@kv2019i kv2019i merged commit 4f59ee8 into thesofproject:main Mar 5, 2024
38 of 44 checks passed
@marc-hb marc-hb deleted the mem-caps-max branch March 7, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants