Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <60034623-a681-4398-fe19-660006245e3b@quicinc.com>
Date:   Mon, 12 Dec 2022 19:19:29 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] remoteproc: elf_loader: Update resource table name check
Content-Language: en-US
To:     Stephen Boyd <swboyd@chromium.org>, <agross@kernel.org>,
        <andersson@kernel.org>, <bgoswami@quicinc.com>,
        <broonie@kernel.org>, <devicetree@vger.kernel.org>,
        <judyhsiao@chromium.org>, <krzysztof.kozlowski@linaro.org>,
        <lgirdwood@gmail.com>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-remoteproc@vger.kernel.org>,
        <mathieu.poirier@linaro.org>, <perex@perex.cz>,
        <quic_plai@quicinc.com>, <quic_rohkumar@quicinc.com>,
        <robh+dt@kernel.org>, <srinivas.kandagatla@linaro.org>,
        <tiwai@suse.com>
References: <1669897248-23052-1-git-send-email-quic_srivasam@quicinc.com>
 <CAE-0n520=mjdc4H1m8au0iBo2qEeaL8OrF1HCP0bXORe2Wa_7w@mail.gmail.com>
 <ec403926-24ef-947d-2a1c-6cbf0e31ab89@quicinc.com>
 <CAE-0n50VYGNF_rGzyb_2Jd8dY8cFx3BeOw0s-ywELkRqEEig4g@mail.gmail.com>
From:   Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Organization: Qualcomm
In-Reply-To: <CAE-0n50VYGNF_rGzyb_2Jd8dY8cFx3BeOw0s-ywELkRqEEig4g@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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.