diff mbox series

remoteproc: elf_loader: Update resource table name check

Message ID 1669897248-23052-1-git-send-email-quic_srivasam@quicinc.com
State New
Headers
Series remoteproc: elf_loader: Update resource table name check |

Commit Message

Srinivasa Rao Mandadapu Dec. 1, 2022, 12:20 p.m. UTC 
  Update resource table name check with sub string search instead of
complete string search.
In general Qualcomm binary contains, section header name
(e.g. .resource_table), amended with extra string to differentiate
with other sections.
So far Android adsp binaries are being authenticated using TZ,
hence this mismatch hasn't created any problem.
In recent developments, ADSP binary is being used in Chrome based
platforms, which doesn't have TZ path, hence resource table is
required for memory sandboxing.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
 drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stephen Boyd Dec. 7, 2022, 1:55 a.m. UTC | #1 
Quoting Srinivasa Rao Mandadapu (2022-12-01 04:20:48)
> Update resource table name check with sub string search instead of
> complete string search.
> In general Qualcomm binary contains, section header name
> (e.g. .resource_table), amended with extra string to differentiate
> with other sections.
> So far Android adsp binaries are being authenticated using TZ,
> hence this mismatch hasn't created any problem.
> In recent developments, ADSP binary is being used in Chrome based
> platforms, which doesn't have TZ path, hence resource table is
> required for memory sandboxing.
>

Does this need a Fixes tag?

> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> ---
>  drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index 5a412d7..0feb120 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
>                 u64 offset = elf_shdr_get_sh_offset(class, shdr);
>                 u32 name = elf_shdr_get_sh_name(class, shdr);
>
> -               if (strcmp(name_table + name, ".resource_table"))
> +               if (!strstr(name_table + name, ".resource_table"))

Was the strcmp not working before because the 'name_table' has something
else in it? It really looks like your elf file is malformed.
Srinivasa Rao Mandadapu Dec. 8, 2022, 1:40 p.m. UTC | #2 
On 12/7/2022 7:25 AM, Stephen Boyd wrote:
Thanks for Your Time Stephen!!!
> Quoting Srinivasa Rao Mandadapu (2022-12-01 04:20:48)
>> Update resource table name check with sub string search instead of
>> complete string search.
>> In general Qualcomm binary contains, section header name
>> (e.g. .resource_table), amended with extra string to differentiate
>> with other sections.
>> So far Android adsp binaries are being authenticated using TZ,
>> hence this mismatch hasn't created any problem.
>> In recent developments, ADSP binary is being used in Chrome based
>> platforms, which doesn't have TZ path, hence resource table is
>> required for memory sandboxing.
>>
> Does this need a Fixes tag?
I don't think so. I feel It's kind of enhancement.
>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> ---
>>   drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
>> index 5a412d7..0feb120 100644
>> --- a/drivers/remoteproc/remoteproc_elf_loader.c
>> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
>> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
>>                  u64 offset = elf_shdr_get_sh_offset(class, shdr);
>>                  u32 name = elf_shdr_get_sh_name(class, shdr);
>>
>> -               if (strcmp(name_table + name, ".resource_table"))
>> +               if (!strstr(name_table + name, ".resource_table"))
> Was the strcmp not working before because the 'name_table' has something
> else in it? It really looks like your elf file is malformed.

Actually, DSP binary is prepared by combining different elfs. So Section 
header names are appended with

something else to distinguish same section name of different elfs, by 
using some Qualcomm specific QURT scripts.
Hence final binary contains resource_table name appended with module 
specific name.

So this patch is required to handle such modified name.
Stephen Boyd Dec. 9, 2022, 8:52 p.m. UTC | #3 
Quoting Srinivasa Rao Mandadapu (2022-12-08 05:40:54)
>
> On 12/7/2022 7:25 AM, Stephen Boyd wrote:
> Thanks for Your Time Stephen!!!
> > Quoting Srinivasa Rao Mandadapu (2022-12-01 04:20:48)
> >> Update resource table name check with sub string search instead of
> >> complete string search.
> >> In general Qualcomm binary contains, section header name
> >> (e.g. .resource_table), amended with extra string to differentiate
> >> with other sections.
> >> So far Android adsp binaries are being authenticated using TZ,
> >> hence this mismatch hasn't created any problem.
> >> In recent developments, ADSP binary is being used in Chrome based
> >> platforms, which doesn't have TZ path, hence resource table is
> >> required for memory sandboxing.
> >>
> > Does this need a Fixes tag?
> I don't think so. I feel It's kind of enhancement.
> >
> >> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> >> ---
> >>   drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> >> index 5a412d7..0feb120 100644
> >> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> >> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> >> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
> >>                  u64 offset = elf_shdr_get_sh_offset(class, shdr);
> >>                  u32 name = elf_shdr_get_sh_name(class, shdr);
> >>
> >> -               if (strcmp(name_table + name, ".resource_table"))
> >> +               if (!strstr(name_table + name, ".resource_table"))
> > Was the strcmp not working before because the 'name_table' has something
> > else in it? It really looks like your elf file is malformed.
>
> Actually, DSP binary is prepared by combining different elfs. So Section
> header names are appended with
>
> something else to distinguish same section name of different elfs, by
> using some Qualcomm specific QURT scripts.
> Hence final binary contains resource_table name appended with module
> specific name.
>
> So this patch is required to handle such modified name.
>

Can you clarify how the section header name looks? Probably you can
objdump the section here and provide that information to help us
understand.

I think remoteproc_elf_loader.c assumes the ELF file is properly formed.
There should be a section named '.resource_table', so the strcmp will
find it by looking at the section header names.
Peng Fan (OSS) Dec. 10, 2022, 3:06 a.m. UTC | #4 
On 12/1/2022 8:20 PM, Srinivasa Rao Mandadapu wrote:
> Update resource table name check with sub string search instead of
> complete string search.
> In general Qualcomm binary contains, section header name
> (e.g. .resource_table), amended with extra string to differentiate
> with other sections.
> So far Android adsp binaries are being authenticated using TZ,
> hence this mismatch hasn't created any problem.
> In recent developments, ADSP binary is being used in Chrome based
> platforms, which doesn't have TZ path, hence resource table is
> required for memory sandboxing.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> ---
>   drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> index 5a412d7..0feb120 100644
> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
>   		u64 offset = elf_shdr_get_sh_offset(class, shdr);
>   		u32 name = elf_shdr_get_sh_name(class, shdr);
>   
> -		if (strcmp(name_table + name, ".resource_table"))
> +		if (!strstr(name_table + name, ".resource_table"))
>   			continue;

How about ?
	if (strcmp(name_table + name, ".resource_table")) {
		if (strstr(name_table + name, ".resource_table"))
   			break;
		continue;
	}

Regards,
Peng.
>   
>   		table = (struct resource_table *)(elf_data + offset);
Srinivasa Rao Mandadapu Dec. 12, 2022, 1:49 p.m. UTC | #5 
On 12/10/2022 2:22 AM, Stephen Boyd wrote:
Thanks for your time Stephen!!!
> Quoting Srinivasa Rao Mandadapu (2022-12-08 05:40:54)
>> On 12/7/2022 7:25 AM, Stephen Boyd wrote:
>> Thanks for Your Time Stephen!!!
>>> Quoting Srinivasa Rao Mandadapu (2022-12-01 04:20:48)
>>>> Update resource table name check with sub string search instead of
>>>> complete string search.
>>>> In general Qualcomm binary contains, section header name
>>>> (e.g. .resource_table), amended with extra string to differentiate
>>>> with other sections.
>>>> So far Android adsp binaries are being authenticated using TZ,
>>>> hence this mismatch hasn't created any problem.
>>>> In recent developments, ADSP binary is being used in Chrome based
>>>> platforms, which doesn't have TZ path, hence resource table is
>>>> required for memory sandboxing.
>>>>
>>> Does this need a Fixes tag?
>> I don't think so. I feel It's kind of enhancement.
>>>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>>>> ---
>>>>    drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
>>>> index 5a412d7..0feb120 100644
>>>> --- a/drivers/remoteproc/remoteproc_elf_loader.c
>>>> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
>>>> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
>>>>                   u64 offset = elf_shdr_get_sh_offset(class, shdr);
>>>>                   u32 name = elf_shdr_get_sh_name(class, shdr);
>>>>
>>>> -               if (strcmp(name_table + name, ".resource_table"))
>>>> +               if (!strstr(name_table + name, ".resource_table"))
>>> Was the strcmp not working before because the 'name_table' has something
>>> else in it? It really looks like your elf file is malformed.
>> Actually, DSP binary is prepared by combining different elfs. So Section
>> header names are appended with
>>
>> something else to distinguish same section name of different elfs, by
>> using some Qualcomm specific QURT scripts.
>> Hence final binary contains resource_table name appended with module
>> specific name.
>>
>> So this patch is required to handle such modified name.
>>
> Can you clarify how the section header name looks? Probably you can
> objdump the section here and provide that information to help us
> understand.

Here is the Section header info.

$ readelf -SW bootimage_relocflag_kodiak.adsp.prodQ.pbn
There are 65 section headers, starting at offset 0x434:
readelf: Error: File contains multiple dynamic symbol tables

Section Headers:
   [Nr] Name              Type
   [ 0]                   NULL
   [ 1] .shstrtab         STRTAB
   [ 2] .text.ukernel.island PROGBITS
   [ 3] .data.ukernel.island PROGBITS
   [ 4] .qskernel_exports.island PROGBITS
   [ 5] .start            PROGBITS
   [ 6] .qskernel_main    PROGBITS
   [ 7] .qskernel_rest    PROGBITS
   [ 8] .comment          PROGBITS
   [ 9] .data.common.island PROGBITS
   [10] .rodata.common.island PROGBITS
   [11] .data.va.island   NOBITS
   [12] .rodata.va.island PROGBITS
   [13] .text.common.island PROGBITS
   [14] .text.va.island   PROGBITS
   [15] .start            PROGBITS
   [16] .ukernel.main     PROGBITS
   [17] .init             PROGBITS
   [18] .text             PROGBITS
   [19] .fini             PROGBITS
   [20] .interp           PROGBITS
   [21] .hash             HASH
   [22] .dynsym           DYNSYM
   [23] .dynstr           STRTAB
   [24] .rodata           PROGBITS
   [25] .eh_frame         PROGBITS
   [26] .ctors            PROGBITS
   [27] .dtors            PROGBITS
   [28] .dynamic          DYNAMIC
   [29] .data             PROGBITS
   [30] .bss              NOBITS
   [31] .sdata            PROGBITS
   [32] QSR_STRING        PROGBITS
   [33] .comment          PROGBITS
   [34] .tcm_island.audio_process PROGBITS
   [35] .data.common.island.audio_process PROGBITS
   [36] .rodata.common.island.audio_process PROGBITS
   [37] .data.va.island.audio_process PROGBITS
   [38] .rodata.va.island.audio_process PROGBITS
   [39] .text.common.island.audio_process PROGBITS
   [40] .text.va.island.audio_process PROGBITS
   [41] .start.audio_process PROGBITS
   [42] .init.audio_process PROGBITS
   [43] .text.audio_process PROGBITS
   [44] .fini.audio_process PROGBITS
   [45] .interp.audio_process PROGBITS
   [46] .hash.audio_process HASH
   [47] .dynsym.audio_process DYNSYM
   [48] .dynstr.audio_process STRTAB
   [49] .rodata.audio_process PROGBITS
   [50] .eh_frame.audio_process PROGBITS
   [51] .ctors.audio_process PROGBITS
   [52] .dtors.audio_process PROGBITS
   [53] .data.rel.ro.audio_process PROGBITS
   [54] .dynamic.audio_process DYNAMIC
   [55] .data.audio_process PROGBITS
   [56] .bss.audio_process NOBITS
   [57] .sdata.audio_process PROGBITS
   [58] QSR_STRING.audio_process PROGBITS
   [59] .comment.audio_process PROGBITS
   [60] .start.ac_bin_process PROGBITS
   [61] .resource_table.ac_bin_process PROGBITS
   [62] .comment.ac_bin_process PROGBITS

>
> I think remoteproc_elf_loader.c assumes the ELF file is properly formed.
> There should be a section named '.resource_table', so the strcmp will
> find it by looking at the section header names.
Srinivasa Rao Mandadapu Dec. 12, 2022, 2:27 p.m. UTC | #6 
On 12/10/2022 8:36 AM, Peng Fan wrote:
Thanks for your time and valuable suggestion Pang!!!
>
> On 12/1/2022 8:20 PM, Srinivasa Rao Mandadapu wrote:
>> Update resource table name check with sub string search instead of
>> complete string search.
>> In general Qualcomm binary contains, section header name
>> (e.g. .resource_table), amended with extra string to differentiate
>> with other sections.
>> So far Android adsp binaries are being authenticated using TZ,
>> hence this mismatch hasn't created any problem.
>> In recent developments, ADSP binary is being used in Chrome based
>> platforms, which doesn't have TZ path, hence resource table is
>> required for memory sandboxing.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> ---
>>   drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c 
>> b/drivers/remoteproc/remoteproc_elf_loader.c
>> index 5a412d7..0feb120 100644
>> --- a/drivers/remoteproc/remoteproc_elf_loader.c
>> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
>> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct 
>> firmware *fw)
>>           u64 offset = elf_shdr_get_sh_offset(class, shdr);
>>           u32 name = elf_shdr_get_sh_name(class, shdr);
>>   -        if (strcmp(name_table + name, ".resource_table"))
>> +        if (!strstr(name_table + name, ".resource_table"))
>>               continue;
>
> How about ?
>     if (strcmp(name_table + name, ".resource_table")) {
>         if (strstr(name_table + name, ".resource_table"))
>               break;
>         continue;
>     }

It looks valid approach to me. But other reviewers may object on double 
check. Let's see Stephen Boyd comments also,

and handle it accordingly.

>
> Regards,
> Peng.
>>             table = (struct resource_table *)(elf_data + offset);
Stephen Boyd Dec. 12, 2022, 7:04 p.m. UTC | #7 
Quoting Srinivasa Rao Mandadapu (2022-12-12 05:49:29)
>
> On 12/10/2022 2:22 AM, Stephen Boyd wrote:
> Thanks for your time Stephen!!!
> > Quoting Srinivasa Rao Mandadapu (2022-12-08 05:40:54)
> >> On 12/7/2022 7:25 AM, Stephen Boyd wrote:
> >> Thanks for Your Time Stephen!!!
> >>> Quoting Srinivasa Rao Mandadapu (2022-12-01 04:20:48)
> >>>> Update resource table name check with sub string search instead of
> >>>> complete string search.
> >>>> In general Qualcomm binary contains, section header name
> >>>> (e.g. .resource_table), amended with extra string to differentiate
> >>>> with other sections.
> >>>> So far Android adsp binaries are being authenticated using TZ,
> >>>> hence this mismatch hasn't created any problem.
> >>>> In recent developments, ADSP binary is being used in Chrome based
> >>>> platforms, which doesn't have TZ path, hence resource table is
> >>>> required for memory sandboxing.
> >>>>
> >>> Does this need a Fixes tag?
> >> I don't think so. I feel It's kind of enhancement.
> >>>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> >>>> ---
> >>>>    drivers/remoteproc/remoteproc_elf_loader.c | 2 +-
> >>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
> >>>> index 5a412d7..0feb120 100644
> >>>> --- a/drivers/remoteproc/remoteproc_elf_loader.c
> >>>> +++ b/drivers/remoteproc/remoteproc_elf_loader.c
> >>>> @@ -272,7 +272,7 @@ find_table(struct device *dev, const struct firmware *fw)
> >>>>                   u64 offset = elf_shdr_get_sh_offset(class, shdr);
> >>>>                   u32 name = elf_shdr_get_sh_name(class, shdr);
> >>>>
> >>>> -               if (strcmp(name_table + name, ".resource_table"))
> >>>> +               if (!strstr(name_table + name, ".resource_table"))
> >>> Was the strcmp not working before because the 'name_table' has something
> >>> else in it? It really looks like your elf file is malformed.
> >> Actually, DSP binary is prepared by combining different elfs. So Section
> >> header names are appended with
> >>
> >> something else to distinguish same section name of different elfs, by
> >> using some Qualcomm specific QURT scripts.
> >> Hence final binary contains resource_table name appended with module
> >> specific name.
> >>
> >> So this patch is required to handle such modified name.
> >>
> > Can you clarify how the section header name looks? Probably you can
> > objdump the section here and provide that information to help us
> > understand.
>
> Here is the Section header info.
>
> $ readelf -SW bootimage_relocflag_kodiak.adsp.prodQ.pbn
> There are 65 section headers, starting at offset 0x434:
> readelf: Error: File contains multiple dynamic symbol tables
>
[...]
>    [60] .start.ac_bin_process PROGBITS
>    [61] .resource_table.ac_bin_process PROGBITS

Cool, the readelf output is helpful. Please rewrite the commit text to
include this detail. It wasn't obvious to me what 'amended' meant. You
probably mean "appended", which clarifies that it has a string added to
the end. I'm also not sure why TZ or not TZ matters for the resource
table section. It may be meaningful to you, but to others it doesn't
have any relation to this resource table appending scheme so it is not
helpful by itself.

Either way, this is not up to me as I'm not the maintainer of
remoteproc. I peeked at the documentation, but this section
name isn't clearly defined. It seems to just be how it has been for a
long time. Maybe you can also update the documentation
(Documentation/staging/remoteproc.rst) to indicate that this elf section
can have anything appended after it, but it must start with
".resource_table"? That would help everyone. And I don't know why that's
in the staging directory. Bjorn?

Finally, I'd prefer the use of strstarts() instead so it is clear what
you're trying to implement.
diff mbox series

Patch

diff --git a/drivers/remoteproc/remoteproc_elf_loader.c b/drivers/remoteproc/remoteproc_elf_loader.c
index 5a412d7..0feb120 100644
--- a/drivers/remoteproc/remoteproc_elf_loader.c
+++ b/drivers/remoteproc/remoteproc_elf_loader.c
@@ -272,7 +272,7 @@  find_table(struct device *dev, const struct firmware *fw)
 		u64 offset = elf_shdr_get_sh_offset(class, shdr);
 		u32 name = elf_shdr_get_sh_name(class, shdr);
 
-		if (strcmp(name_table + name, ".resource_table"))
+		if (!strstr(name_table + name, ".resource_table"))
 			continue;
 
 		table = (struct resource_table *)(elf_data + offset);