[PATCH] firmware: Prevent oops on delayed abort

[PATCH] firmware: Prevent oops on delayed abort

[Chris Wilson]

If the firmware load is slow and cancelled, we may call fw_load_abort()
twice and promptly oops on the second with a NULL dereference.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ming Lei <ming.lei@canonical.com>
Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4497d263209f..d03e21c1d2f3 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -557,6 +557,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
 {
 	struct firmware_buf *buf = fw_priv->buf;
 
+	if (!buf)
+		return;
+
 	__fw_load_abort(buf);
 
 	/* avoid user action after loading abort */
-- 
2.11.0

Re: [PATCH] firmware: Prevent oops on delayed abort

[Ming Lei]

On Wed, Jan 4, 2017 at 5:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> If the firmware load is slow and cancelled, we may call fw_load_abort()
> twice and promptly oops on the second with a NULL dereference.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ming Lei <ming.lei@canonical.com>
> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4497d263209f..d03e21c1d2f3 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -557,6 +557,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
>  {
>         struct firmware_buf *buf = fw_priv->buf;
>
> +       if (!buf)
> +               return;
> +
>         __fw_load_abort(buf);
>
>         /* avoid user action after loading abort */
> --
> 2.11.0
>

Looks fine,

Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
Ming

Re: [PATCH] firmware: Prevent oops on delayed abort

[Luis R. Rodriguez]

On Wed, Jan 04, 2017 at 07:12:11PM +0800, Ming Lei wrote:
> On Wed, Jan 4, 2017 at 5:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > If the firmware load is slow and cancelled, we may call fw_load_abort()
> > twice and promptly oops on the second with a NULL dereference.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ming Lei <ming.lei@canonical.com>
> > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/base/firmware_class.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4497d263209f..d03e21c1d2f3 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -557,6 +557,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
> >  {
> >         struct firmware_buf *buf = fw_priv->buf;
> >
> > +       if (!buf)
> > +               return;
> > +
> >         __fw_load_abort(buf);
> >
> >         /* avoid user action after loading abort */
> > --
> > 2.11.0
> >
> 
> Looks fine,
> 
> Acked-by: Ming Lei <ming.lei@canonical.com>

But does this fix an actual oops or just theoretical? If it does can you
include the trace, and amend the commit log to Cc stable so this gets
properly propagated into the stable kernels?

  Luis

Re: [PATCH] firmware: Prevent oops on delayed abort

[Chris Wilson]

On Wed, Jan 04, 2017 at 04:40:49PM +0100, Luis R. Rodriguez wrote:
> On Wed, Jan 04, 2017 at 07:12:11PM +0800, Ming Lei wrote:
> > On Wed, Jan 4, 2017 at 5:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > If the firmware load is slow and cancelled, we may call fw_load_abort()
> > > twice and promptly oops on the second with a NULL dereference.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ming Lei <ming.lei@canonical.com>
> > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > ---
> > >  drivers/base/firmware_class.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 4497d263209f..d03e21c1d2f3 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -557,6 +557,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
> > >  {
> > >         struct firmware_buf *buf = fw_priv->buf;
> > >
> > > +       if (!buf)
> > > +               return;
> > > +
> > >         __fw_load_abort(buf);
> > >
> > >         /* avoid user action after loading abort */
> > > --
> > > 2.11.0
> > >
> > 
> > Looks fine,
> > 
> > Acked-by: Ming Lei <ming.lei@canonical.com>
> 
> But does this fix an actual oops or just theoretical? If it does can you
> include the trace, and amend the commit log to Cc stable so this gets
> properly propagated into the stable kernels?

Actual oops, I was going to include the oops from dmesg but that was
lost on a the forced reboot. It's fairly clear the cause once you read
the callers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] firmware: Prevent oops on delayed abort

[Luis R. Rodriguez]

On Wed, Jan 04, 2017 at 03:47:49PM +0000, Chris Wilson wrote:
> On Wed, Jan 04, 2017 at 04:40:49PM +0100, Luis R. Rodriguez wrote:
> > On Wed, Jan 04, 2017 at 07:12:11PM +0800, Ming Lei wrote:
> > > On Wed, Jan 4, 2017 at 5:31 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > If the firmware load is slow and cancelled, we may call fw_load_abort()
> > > > twice and promptly oops on the second with a NULL dereference.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Ming Lei <ming.lei@canonical.com>
> > > > Cc: "Luis R. Rodriguez" <mcgrof@kernel.org>
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > ---
> > > >  drivers/base/firmware_class.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > > index 4497d263209f..d03e21c1d2f3 100644
> > > > --- a/drivers/base/firmware_class.c
> > > > +++ b/drivers/base/firmware_class.c
> > > > @@ -557,6 +557,9 @@ static void fw_load_abort(struct firmware_priv *fw_priv)
> > > >  {
> > > >         struct firmware_buf *buf = fw_priv->buf;
> > > >
> > > > +       if (!buf)
> > > > +               return;
> > > > +
> > > >         __fw_load_abort(buf);
> > > >
> > > >         /* avoid user action after loading abort */
> > > > --
> > > > 2.11.0
> > > >
> > > 
> > > Looks fine,
> > > 
> > > Acked-by: Ming Lei <ming.lei@canonical.com>
> > 
> > But does this fix an actual oops or just theoretical? If it does can you
> > include the trace, and amend the commit log to Cc stable so this gets
> > properly propagated into the stable kernels?
> 
> Actual oops, I was going to include the oops from dmesg but that was
> lost on a the forced reboot. It's fairly clear the cause once you read
> the callers.

I see, if you can reproduce and include it that would be greatly appreciated,
and please re-submit and Cc stable on the commit log, if you can indicate
as of what kernel this is needed even better (Fixes: tag might help).

  Luis