linux-kernel - Re: [PATCH v2] firmware: fix NULL pointer dereference in __fw_load

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]

Message-ID: <20170125154725.GB21106@kroah.com>
Date:   Wed, 25 Jan 2017 16:47:25 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>
Cc:     ming.lei@...onical.com, keescook@...omium.org,
        linux-kernel-dev@...khoff.com, jakub.kicinski@...ronome.com,
        chris@...is-wilson.co.uk, oss-drivers@...ronome.com,
        johannes@...solutions.net, j@...fi, teg@...m.no, kay@...y.org,
        jwboyer@...oraproject.org, dmitry.torokhov@...il.com,
        seth.forshee@...onical.com, bjorn.andersson@...aro.org,
        linux-kernel@...r.kernel.org, wagi@...om.org,
        stephen.boyd@...aro.org, zohar@...ux.vnet.ibm.com, tiwai@...e.de,
        dwmw2@...radead.org, fengguang.wu@...el.com, dhowells@...hat.com,
        arend.vanspriel@...adcom.com, kvalo@...eaurora.org,
        kimran@...eaurora.org, "[3.10+]" <stable@...r.kernel.org>
Subject: Re: [PATCH v2] firmware: fix NULL pointer dereference in
 __fw_load_abort()

On Wed, Jan 25, 2017 at 07:21:18AM -0800, Luis R. Rodriguez wrote:
> Since commit 5d47ec02c37ea632398cb251c884e3a488dff794
> ("firmware: Correct handling of fw_state_wait() return value")
> fw_load_abort(fw_priv) could be called twice and lead us to a
> kernel crash. This happens only when the firmware fallback mechanism
> (regular or custom) is used. The fallback mechanism exposes a sysfs
> interface for userspace to upload a file and notify the kernel when
> the file is loaded and ready, or to cancel an upload by echo'ing -1
> into on the loading file:
> 
> echo -n "-1" > /sys/$DEVPATH/loading
> 
> This will call fw_load_abort(). Some distributions actually have
> a udev rule in place to *always* immediately cancel all firmware
> fallback mechanism requests (Debian), they have:
> 
>   $ cat /lib/udev/rules.d/50-firmware.rules
>   # stub for immediately telling the kernel that userspace firmware loading
>   # failed; necessary to avoid long timeouts with CONFIG_FW_LOADER_USER_HELPER=y
>   SUBSYSTEM=="firmware", ACTION=="add", ATTR{loading}="-1
> 
> This was done since udev removed the firmware fallback mechanism a while ago
> and a long standing misunderstood issues with the timeout (but now corrected).
> Distributions with this udev rule would run into this crash only if the
> fallback mechanism is used. Since most distributions disable by default
> using the fallback mechanism (CONFIG_FW_LOADER_USER_HELPER_FALLBACK), this
> would typicaly mean only 2 drivers which *require* the fallback mechanism
> could typically incur a crash: drivers/firmware/dell_rbu.c and the
> drivers/leds/leds-lp55xx-common.c driver.
> 
> The crash happens because after commit 5b029624948d ("firmware: do not
> use fw_lock for fw_state protection") and subsequent fix commit
> 5d47ec02c37ea6 ("firmware: Correct handling of fw_state_wait() return
> value") a race can happen between this cancelation and the firmware
> fw_state_wait_timeout() being woken up after a state change with which
> fw_load_abort() as that calls swake_up(). Upon error fw_state_wait_timeout()
> will also again call fw_load_abort() and trigger a null reference.
> 
> At first glance we could just fix this with a !buf check on
> fw_load_abort() before accessing buf->fw_st, however there is
> a logical issue in having a state machine used for the fallback
> mechanism and preventing access from it once we abort as its inside
> the buf (buf->fw_st).
> 
> The firmware_class.c code is setting the buf to NULL to annotate an
> abort has occurred. Replace this mechanism by simply using the state check
> instead. All the other code in place already uses similar checks
> for aborting as well so no further changes are needed.
> 
> An oops can be reproduced with the new fw_fallback.sh fallback
> mechanism cancellation test. Either cancelling the fallback mechanism
> or the custom fallback mechanism triggers a crash.

You are still writing books here.

With crazy margins, pick one line width (72 columns), and stick with it
please.

Can you reformat this and resend please?

thanks,

greg k-h-