[RFT,2/7] firmware: fix checking for return values for fw_add_devm_name()
diff mbox

Message ID 20180227232101.20786-3-mcgrof@kernel.org
State New
Headers show

Commit Message

Luis Chamberlain Feb. 27, 2018, 11:20 p.m. UTC
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@kernel.org>
---
 drivers/base/firmware_loader.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Kees Cook Feb. 27, 2018, 11:29 p.m. UTC | #1
On Tue, Feb 27, 2018 at 3:20 PM, Luis R. Rodriguez <mcgrof@kernel.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@kernel.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) ?

-Kees

> +                       mutex_unlock(&fw_lock);
> +                       return ret;
> +               }
> +       }
>
>         /*
>          * After caching firmware image is started, let it piggyback
> --
> 2.16.2
>
Luis Chamberlain Feb. 28, 2018, 1:19 a.m. UTC | #2
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@kernel.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@kernel.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
>

Patch
diff mbox

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) {
+			mutex_unlock(&fw_lock);
+			return ret;
+		}
+	}
 
 	/*
 	 * After caching firmware image is started, let it piggyback