Skip to content

ext_manifest: Add UUID dictionary#17

Closed
ktrzcinx wants to merge 4 commits intothesofproject:masterfrom
ktrzcinx:ext-man-uuid
Closed

ext_manifest: Add UUID dictionary#17
ktrzcinx wants to merge 4 commits intothesofproject:masterfrom
ktrzcinx:ext-man-uuid

Conversation

@ktrzcinx
Copy link
Member

@ktrzcinx ktrzcinx commented May 13, 2020

Such a dictionary will be used in kernel code to translate UUID value
to UUID dictionary key used in DSP in runtime.
Translation will be used for the process component creation and
runtime trace filtering.
Because there is no way to put right size to this extended element
header, list of uuid entries are located in separate section to
allow convenient size calculation from rimage level.
Extract common header with simple, generic preprocessor functions.

This PR is related with thesofproject/sof#2914.

@ktrzcinx ktrzcinx force-pushed the ext-man-uuid branch 2 times, most recently from 3f2301c to 8238721 Compare May 14, 2020 06:53
sec_buffer allocated in elf_read_section() should be free
before function quit

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx ktrzcinx changed the title [WIP] [RFC] ext_manifest: Add UUID dictionary ext_manifest: Add UUID dictionary May 14, 2020
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

@plbossart @kv2019i can you confirm if kernel is doing the UUID translations

const char *section_name)
{
struct ext_man_elem_header* head = NULL;
uint8_t *meta_buff = *metadata_buff;
Copy link
Member

Choose a reason for hiding this comment

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

Can you confirm we are converting a UUID to a memory address (as stated in commit message) ? This is a security hole, we cannot pass in addresses from outside of FW, we can however pass in data we can validate like an index in an array. @lbetlej @mmaka1 any comments here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lgirdwood The code is not merged yet to kernel, but my understanding what is passed is a key to the dictionary, not an actual address. @keyonjie @ktrzcinx can you clarify?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it is only a index-liked "key", not the real memory address, those real 16 Bytes UUIDs are not stored in DSP SRAM actually. @mmaka1 please correct me if I am wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is address of UUID entry, from invalid memory region (0x1FFFA000 is already used as start address, but it's fully configurable in linker script) - so dereference leads to firmware crash. This address is used as UUID element identifier/representation from FW side because full UUID value is inaccessible from there. So this address is UUID dictionary key value.

Copy link
Member

Choose a reason for hiding this comment

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

ok, can you update the commit messages, inline comments and PR description to use key/index rather than address.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

ktrzcinx added 3 commits May 18, 2020 12:55
After change there will be possibility to pass pointer
to sec_buffer_size as rw pointer to another function.
Moreover remove offset variable, because of usage in single place.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Such a split will allow to add dictionary elements with list
of elements placed in particular elf section - because of possibility
to calculate sizeof of this section.
Moreover with such an approach, it is possible to define list of
mandatory extended manifest elements, which will be checked after
each build for each supported platform.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
Such a dictionary will be used in kernel code to translate UUID value
to UUID dictionary key used in DSP in runtime.
Translation will be used for the process component creation and
runtime trace filtering.
Because there is no way to put right size to this extended element
header, list of uuid entries are located in separate section to
allow convenient size calculation from rimage level.
Extract common header with simple, generic preprocessor functions.

Signed-off-by: Karol Trzcinski <karolx.trzcinski@linux.intel.com>
@ktrzcinx
Copy link
Member Author

This solution was rejected, full UUID will be passed during building topology.

@ktrzcinx ktrzcinx closed this May 26, 2020
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