linux-kernel - Re: [RFT 2/7] firmware: fix checking for return values for fw_add_devm

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180228011900.GD14069@wotan.suse.de>
Date:   Wed, 28 Feb 2018 01:19:01 +0000
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>, cantabile.desu@...il.com,
        Jakub Kicinski <kubakici@...pl>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-wireless <linux-wireless@...r.kernel.org>,
        Shuah Khan <shuah@...nel.org>,
        Martin Fuzzey <mfuzzey@...keon.com>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        David Howells <dhowells@...hat.com>, pali.rohar@...il.com,
        Takashi Iwai <tiwai@...e.de>, arend.vanspriel@...adcom.com,
        Rafał Miłecki <zajec5@...il.com>,
        nbroeking@...com, Vikram Mulukutla <markivx@...eaurora.org>,
        stephen.boyd@...aro.org, Mark Brown <broonie@...nel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        David Woodhouse <dwmw2@...radead.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Abhay_Salunke@...l.com, bjorn.andersson@...aro.org,
        jewalt@...innovations.com, oneukum@...e.com,
        LKML <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>
Subject: Re: [RFT 2/7] firmware: fix checking for return values for
 fw_add_devm_name()

On Tue, Feb 27, 2018 at 03:29:53PM -0800, Kees Cook wrote:
> On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@...nel.org> wrote:
> > fw_add_devm_name() adds device's name onto the devres for the
> > device so that prior to suspend we cache the firmware onto memory,
> > so that on resume the firmware is reliably available. We never
> > were checking for success for this call though, meaning in some
> > really rare cases we my have never setup the firmware cache for
> > a device, which could in turn make resume fail.
> >
> > This is all theoretical, no known issues have been reported.
> > This small issue has been present way since the addition of the
> > devres firmware cache names on v3.7.
> >
> > Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
> > Signed-off-by: Luis R. Rodriguez <mcgrof@...nel.org>
> > ---
> >  drivers/base/firmware_loader.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader.c b/drivers/base/firmware_loader.c
> > index 21dd31ef08ae..48932581c70c 100644
> > --- a/drivers/base/firmware_loader.c
> > +++ b/drivers/base/firmware_loader.c
> > @@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
> >               unsigned int opt_flags)
> >  {
> >         struct fw_priv *fw_priv = fw->priv;
> > +       int ret;
> >
> >         mutex_lock(&fw_lock);
> >         if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
> > @@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
> >          */
> >         /* don't cache firmware handled without uevent */
> >         if (device && (opt_flags & FW_OPT_UEVENT) &&
> > -           !(opt_flags & FW_OPT_NOCACHE))
> > -               fw_add_devm_name(device, fw_priv->fw_name);
> > +           !(opt_flags & FW_OPT_NOCACHE)) {
> > +               ret = fw_add_devm_name(device, fw_priv->fw_name);
> > +               if (ret && ret != 1) {
> 
> Why not if (ret < 0) ?

Could be, either way I remove this later for just (ret) after I adjust
fw_add_devm_name() to not return 1 anymore.

So its just a temporal check, I added this patch first so that this can
be merged into kernels who want the fix.

  Luis

> 
> -Kees
> 
> > +                       mutex_unlock(&fw_lock);
> > +                       return ret;
> > +               }
> > +       }
> >
> >         /*
> >          * After caching firmware image is started, let it piggyback
> > --
> > 2.16.2
> >
> 
> 
> 
> -- 
> Kees Cook
> Pixel Security
> 

-- 
Luis Rodriguez, SUSE LINUX GmbH
Maxfeldstrasse 5; D-90409 Nuernberg