Re: [PATCH 1/1] firmware: fix use-after-free in _request_firmware()

[Luis Chamberlain]

On Tue, Jul 13, 2021 at 10:49:42AM +0800, Zhen Lei wrote:
> 		CPU0			CPU1
> __device_uncache_fw_images():		assign_fw():
> 					fw_cache_piggyback_on_request()
> 					<----- P0
> 	spin_lock(&fwc->name_lock);
> 	...
> 	list_del(&fce->list);
> 	spin_unlock(&fwc->name_lock);
> 
> 	uncache_firmware(fce->name);
> 					<----- P1
> 					kref_get(&fw_priv->ref);
> 
> If CPU1 is interrupted at position P0, the new 'fce' has been added to the
> list fwc->fw_names by the fw_cache_piggyback_on_request(). In this case,
> CPU0 executes __device_uncache_fw_images() and will be able to see it when
> it traverses list fwc->fw_names. Before CPU1 executes kref_get() at P1, if
> CPU0 further executes uncache_firmware(), the count of fw_priv->ref may
> decrease to 0, causing fw_priv to be released in advance.
> 
> Move kref_get() to the lock protection range of fwc->name_lock to fix it.
> 
> Fixes: ac39b3ea73aa ("firmware loader: let caching firmware piggyback on loading firmware")
> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>

Acked-by: Luis Chamberlain <mcgrof@xxxxxxxxxx>

Good catch! Can you resend a v2 patch describing how this race is rather
difficult to run into given it likely involves looping modprobe / rmod on a
driver while doing the suspend / resume cycle? I can't see how else to
trigger this. Additionally if you can describe in the patch how you
found this, (code inspection, a robot code system which looks for UAF?)
it would be good for the commit log.

Having a possible impact described in the commit log is useful for folks
who may want to put effort into backporting this for for older kernels
in case it does not apply as-is.

 Luis