linux-kernel - Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Jan 2020 08:30:42 -0800
From:   Rob Clark <robdclark@...il.com>
To:     Tom Rix <trix@...hat.com>
Cc:     freedreno <freedreno@...ts.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Jordan Crouse <jcrouse@...eaurora.org>,
        Sharat Masetty <smasetty@...eaurora.org>,
        Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Douglas Anderson <dianders@...omium.org>,
        Brian Masney <masneyb@...tation.org>,
        Fabio Estevam <festevam@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] drm/msm: support firmware-name for zap fw

On Wed, Jan 8, 2020 at 7:38 AM Tom Rix <trix@...hat.com> wrote:
>
>
> On 1/7/20 5:38 PM, Rob Clark wrote:
> > From: Rob Clark <robdclark@...omium.org>
> >
> > Since zap firmware can be device specific, allow for a firmware-name
> > property in the zap node to specify which firmware to load, similarly to
> > the scheme used for dsp/wifi/etc.
> >
> > Signed-off-by: Rob Clark <robdclark@...omium.org>
> > ---
> >  drivers/gpu/drm/msm/adreno/adreno_gpu.c | 32 ++++++++++++++++++++++---
> >  1 file changed, 29 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > index 112e8b8a261e..aa8737bd58db 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> > @@ -26,6 +26,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> >  {
> >       struct device *dev = &gpu->pdev->dev;
> >       const struct firmware *fw;
> > +     const char *signed_fwname = NULL;
> >       struct device_node *np, *mem_np;
> >       struct resource r;
> >       phys_addr_t mem_phys;
> > @@ -58,8 +59,33 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> >
> >       mem_phys = r.start;
> >
> > -     /* Request the MDT file for the firmware */
> > -     fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
> > +     /*
> > +      * Check for a firmware-name property.  This is the new scheme
> > +      * to handle firmware that may be signed with device specific
> > +      * keys, allowing us to have a different zap fw path for different
> > +      * devices.
> > +      *
> > +      * If the firmware-name property is found, we bypass the
> > +      * adreno_request_fw() mechanism, because we don't need to handle
> > +      * the /lib/firmware/qcom/* vs /lib/firmware/* case.
> > +      *
> > +      * If the firmware-name property is not found, for backwards
> > +      * compatibility we fall back to the fwname from the gpulist
> > +      * table.
> > +      */
> > +     of_property_read_string_index(np, "firmware-name", 0, &signed_fwname);
> > +     if (signed_fwname) {
> > +             fwname = signed_fwname;
> > +             ret = request_firmware_direct(&fw, signed_fwname, gpu->dev->dev);
> > +             if (ret) {
> > +                     DRM_DEV_ERROR(dev, "could not load signed zap firmware: %d\n", ret);
> Could adreno_request_fw be called with fwname if request_firmware_direct fails ?


*possibly*.. initially I avoided this because the failure mode for
incorrectly signed firmware was silent and catestrophic.  But Bjorn
tells me this has been fixed.. in which case we could try and detect
if it is the incorrect fw.  I need to try some experiments to confirm
we can detect this case properly.

BR,
-R

> > +                     fw = ERR_PTR(ret);
> > +             }
> > +     } else {
> > +             /* Request the MDT file for the firmware */
> > +             fw = adreno_request_fw(to_adreno_gpu(gpu), fwname);
> > +     }
> > +
> >       if (IS_ERR(fw)) {
> >               DRM_DEV_ERROR(dev, "Unable to load %s\n", fwname);
> >               return PTR_ERR(fw);
> > @@ -95,7 +121,7 @@ static int zap_shader_load_mdt(struct msm_gpu *gpu, const char *fwname,
> >        * not.  But since we've already gotten through adreno_request_fw()
> >        * we know which of the two cases it is:
> >        */
> > -     if (to_adreno_gpu(gpu)->fwloc == FW_LOCATION_LEGACY) {
> > +     if (signed_fwname || (to_adreno_gpu(gpu)->fwloc == FW_LOCATION_LEGACY)) {
> >               ret = qcom_mdt_load(dev, fw, fwname, pasid,
> >                               mem_region, mem_phys, mem_size, NULL);
> >       } else {
>