ARM: 9229/1: amba: Fix use-after-free in amba_read_periphid()

After commit a73324c05655 ("ARM: 9220/1: amba: Remove deferred device
addition"), it became possible for amba_read_periphid() to be invoked
concurrently from two threads for a particular AMBA device.

Consider the case where a thread (T0) is registering an AMBA driver, and
searching for all of the devices it can match with on the AMBA bus.
Suppose that another thread (T1) is executing the deferred probe work,
and is searching through all of the AMBA drivers on the bus for a driver
that matches a particular AMBA device. Assume that both threads begin
operating on the same AMBA device and the device's peripheral ID is
still unknown.

In this scenario, the amba_match() function will be invoked for the
same AMBA device by both threads, which means amba_read_periphid()
can also be invoked by both threads, and both threads will be able
to manipulate the AMBA device's pclk pointer without any synchronization.
It's possible that one thread will initialize the pclk pointer, then the
other thread will re-initialize it, overwriting the previous value, and
both will race to free the same pclk, resulting in a use-after-free for
whichever thread frees the pclk last.

Add a lock per AMBA device to synchronize the handling with detecting the
peripheral ID to avoid the use-after-free scenario.

The following KFENCE bug report helped detect this problem:
==================================================================
BUG: KFENCE: use-after-free read in clk_disable+0x14/0x34

Use-after-free read at 0x(ptrval) (in kfence-#19):
 clk_disable+0x14/0x34
 amba_read_periphid+0xdc/0x134
 amba_match+0x3c/0x84
 __driver_attach+0x20/0x158
 bus_for_each_dev+0x74/0xc0
 bus_add_driver+0x154/0x1e8
 driver_register+0x88/0x11c
 do_one_initcall+0x8c/0x2fc
 kernel_init_freeable+0x190/0x220
 kernel_init+0x10/0x108
 ret_from_fork+0x14/0x3c
 0x0

kfence-#19: 0x(ptrval)-0x(ptrval), size=36, cache=kmalloc-64

allocated by task 8 on cpu 0 at 11.629931s:
 clk_hw_create_clk+0x38/0x134
 amba_get_enable_pclk+0x10/0x68
 amba_read_periphid+0x28/0x134
 amba_match+0x3c/0x84
 __device_attach_driver+0x2c/0xc4
 bus_for_each_drv+0x80/0xd0
 __device_attach+0xb0/0x1f0
 bus_probe_device+0x88/0x90
 deferred_probe_work_func+0x8c/0xc0
 process_one_work+0x23c/0x690
 worker_thread+0x34/0x488
 kthread+0xd4/0xfc
 ret_from_fork+0x14/0x3c
 0x0

freed by task 8 on cpu 0 at 11.630095s:
 amba_read_periphid+0xec/0x134
 amba_match+0x3c/0x84
 __device_attach_driver+0x2c/0xc4
 bus_for_each_drv+0x80/0xd0
 __device_attach+0xb0/0x1f0
 bus_probe_device+0x88/0x90
 deferred_probe_work_func+0x8c/0xc0
 process_one_work+0x23c/0x690
 worker_thread+0x34/0x488
 kthread+0xd4/0xfc
 ret_from_fork+0x14/0x3c
 0x0

Cc: Saravana Kannan <saravanak@google.com>
Cc: patches@armlinux.org.uk
Fixes: a73324c05655 ("ARM: 9220/1: amba: Remove deferred device addition")
Reported-by: Guenter Roeck <linux@roeck-us.net>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Isaac J. Manjarres <isaacmanjarres@google.com>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 32b0e0b930c1068e6e8c867cf2fdbfea3d08556c..110a535648d2e1fa9ace1878b207f862b41dfca0 100644 (file)
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -209,6 +209,7 @@ static int amba_match(struct device *dev, struct device_driver *drv)
        struct amba_device *pcdev = to_amba_device(dev);
        struct amba_driver *pcdrv = to_amba_driver(drv);
 
+       mutex_lock(&pcdev->periphid_lock);
        if (!pcdev->periphid) {
                int ret = amba_read_periphid(pcdev);
 
@@ -218,11 +219,14 @@ static int amba_match(struct device *dev, struct device_driver *drv)
                 * permanent failure in reading pid and cid, simply map it to
                 * -EPROBE_DEFER.
                 */
-               if (ret)
+               if (ret) {
+                       mutex_unlock(&pcdev->periphid_lock);
                        return -EPROBE_DEFER;
+               }
                dev_set_uevent_suppress(dev, false);
                kobject_uevent(&dev->kobj, KOBJ_ADD);
        }
+       mutex_unlock(&pcdev->periphid_lock);
 
        /* When driver_override is set, only bind to the matching driver */
        if (pcdev->driver_override)
@@ -532,6 +536,7 @@ static void amba_device_release(struct device *dev)
 
        if (d->res.parent)
                release_resource(&d->res);
+       mutex_destroy(&d->periphid_lock);
        kfree(d);
 }
 
@@ -584,6 +589,7 @@ static void amba_device_initialize(struct amba_device *dev, const char *name)
        dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
        dev->dev.dma_parms = &dev->dma_parms;
        dev->res.name = dev_name(&dev->dev);
+       mutex_init(&dev->periphid_lock);
 }
 
 /**

diff --git a/include/linux/amba/bus.h b/include/linux/amba/bus.h
index e94cdf235f1db986164a2f9e3b16b4c9077cb196..5001e14c5c06ddac13d6cb64d0d50d8e8b4c7b17 100644 (file)
--- a/include/linux/amba/bus.h
+++ b/include/linux/amba/bus.h
@@ -67,6 +67,7 @@ struct amba_device {
        struct clk              *pclk;
        struct device_dma_parameters dma_parms;
        unsigned int            periphid;
+       struct mutex            periphid_lock;
        unsigned int            cid;
        struct amba_cs_uci_id   uci;
        unsigned int            irq[AMBA_NR_IRQS];