Re: [PATCH 1/1] firmware_loading_store: fix firmware loading use-after-free oops

Re: [PATCH 1/1] firmware_loading_store: fix firmware loading use-after-free oops

[Greg KH]

On Thu, Dec 01, 2011 at 05:26:10PM +0800, DONG-DONG YANG wrote:
> 
> firmware_loading_store() should check for fw_priv->fw before fw_load_abort.
> Otherwise, the system has a chance to run into use-after-free oops,
> if the user-space handler sends "-1" to firmware controller(loading) in
> exception scenarios before sending "1" to start a load.
> See the attachment for the kernel log and the analysis
> 
> Tested:
>   ARM_ca9 + android init uevent + webtop udev
> 
> Patch:
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 06ed6b4..b333082 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -271,7 +271,8 @@ static ssize_t firmware_loading_store(struct device *dev,
>                 /* fallthrough */
>         case -1:
>         err:
> -               fw_load_abort(fw_priv);
> +               if (fw_priv->fw)
> +                       fw_load_abort(fw_priv);
>                 break;
>         }
> 

Can you resend this in the format described in
Documentation/SubmittingPatches so that I can accept it?  (hint, you
forgot the signed-off-by: line.)

Also, shouldn't we do the check for ->fw in the fw_load_abort() call?

And why is userspace doing wierd things like this?  What causes that?

thanks,

greg k-h

Re: [PATCH 1/1] firmware_loading_store: fix firmware loading use-after-free oops

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Tue, Dec 13, 2011 at 01:31:32PM +0800, DONG-DONG YANG wrote:
> If we do the check for fw in the fw_load_abort call, I concern whether it would
> limit FW_STATUS_ABORT scope, which would presume any fw_abort occur after fw
> instance generation.

Are you sure this really fixes anything?  What happens if the fw pointer
becomes null after the check happens but before the call?  What is
protecting that from happening here?

> The issue was introduced when the webtop components are integrated into the
> Linux Android platform.
> The webtop firmware loading performs udev configuration files (under /lib/udev/
> *) by default, however,
> ueventd is in charge of such loading task on android framework. The conflict
> causes the weird things happen.
>
> 50-firmware.rules, one of udev default rules, forks a sub-process, named
> "firmware", which sends "-1" to "loading" fw ctrl file on receiving "add"
> event. Such firmware loading failure should bring on APP processes exit or
> error report, but it causes use-after-free oops due to we have no check for fw
> in firmware_loading_store.

Sounds like this is a major userspace configuration error.  Not that
this means we shouldn't be applying the kernel patch, just that you need
to fix up your system first :)

Hm, so, you have two different processes trying to write to the firmware
file at the same time?  And when one finishes, the other one tries to
abort things?

Ok, I can understand that, it's a very messed up userspace, but I
understand.

But, your patch needs to properly hold the lock when this is being
checked, otherwise it will still race and could still oops.

And, your check needs to be done in a different place, again, what's to
keep that pointer from going away later, in the middle of that call you
just made?

Care to fix this up properly?

> The patch and log analysis are attached.

Please don't attach patches, it's not easy to apply them when they are
in base64 mode.

Also, your patch can't be applied as you made it against a very wierd
patch "rbase"?

thanks,

greg k-h