2022-08-18 18:00:39

by Isaac J. Manjarres

[permalink] [raw]
Subject: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

After commit f2d3b9a46e0e ("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 <[email protected]>
Cc: [email protected]
Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
Reported-by: Guenter Roeck <[email protected]>
Signed-off-by: Isaac J. Manjarres <[email protected]>
---
KernelVersion: rmk/for-next

drivers/amba/bus.c | 8 +++++++-
include/linux/amba/bus.h | 1 +
2 files changed, 8 insertions(+), 1 deletion(-)

v1 -> v2:
- Applied on rmk/for-next

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 32b0e0b930c1..110a535648d2 100644
--- 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 e94cdf235f1d..5001e14c5c06 100644
--- 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];
--
2.37.1.595.g718a3a8f04-goog


2022-08-25 23:07:52

by Saravana Kannan

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

On Thu, Aug 18, 2022 at 10:29 AM Isaac J. Manjarres
<[email protected]> wrote:
>
> After commit f2d3b9a46e0e ("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 <[email protected]>
> Cc: [email protected]
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Isaac J. Manjarres <[email protected]>
> ---
> KernelVersion: rmk/for-next

Russell,

Can you pull this in for 6.0-rcX please? It fixes crashes in a bunch
of devices. Please let us know if you need this rebased on top of
6.0-rc2

Thanks,
Saravana

>
> drivers/amba/bus.c | 8 +++++++-
> include/linux/amba/bus.h | 1 +
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> v1 -> v2:
> - Applied on rmk/for-next
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 32b0e0b930c1..110a535648d2 100644
> --- 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 e94cdf235f1d..5001e14c5c06 100644
> --- 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];
> --
> 2.37.1.595.g718a3a8f04-goog
>

2022-08-30 05:41:57

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

On Thu, Aug 18, 2022 at 10:28:51AM -0700, Isaac J. Manjarres wrote:
> After commit f2d3b9a46e0e ("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 <[email protected]>
> Cc: [email protected]
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Isaac J. Manjarres <[email protected]>

Tested-by: Guenter Roeck <[email protected]>

> ---
> KernelVersion: rmk/for-next
>
> drivers/amba/bus.c | 8 +++++++-
> include/linux/amba/bus.h | 1 +
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> v1 -> v2:
> - Applied on rmk/for-next
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 32b0e0b930c1..110a535648d2 100644
> --- 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 e94cdf235f1d..5001e14c5c06 100644
> --- 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];
> --
> 2.37.1.595.g718a3a8f04-goog
>

2022-09-04 11:47:43

by Gabriel Francisco

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

This patch alone on top of v6.0-rc2 still gives me the null pointer.

But combining it with Zhen Lei's patch (from
https://lkml.org/lkml/2022/8/27/164) my device boots successfully.

Thank you!

Tested-by: Gabriel Francisco <[email protected]>

On 18/08/2022 19:28, Isaac J. Manjarres wrote:
> After commit f2d3b9a46e0e ("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 <[email protected]>
> Cc: [email protected]
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Reported-by: Guenter Roeck <[email protected]>
> Signed-off-by: Isaac J. Manjarres <[email protected]>
> ---
> KernelVersion: rmk/for-next
>
> drivers/amba/bus.c | 8 +++++++-
> include/linux/amba/bus.h | 1 +
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> v1 -> v2:
> - Applied on rmk/for-next
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 32b0e0b930c1..110a535648d2 100644
> --- 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 e94cdf235f1d..5001e14c5c06 100644
> --- 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];

2022-09-06 18:23:20

by Isaac J. Manjarres

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

On Sun, Sep 4, 2022 at 4:15 AM Gabriel Francisco <[email protected]> wrote:
>
> This patch alone on top of v6.0-rc2 still gives me the null pointer.
>
> But combining it with Zhen Lei's patch (from
> https://lkml.org/lkml/2022/8/27/164) my device boots successfully.

Hi Gabriel,

Thanks for your e-mail. I think my AMBA bus patch and
https://lore.kernel.org/all/[email protected]/
(merged in 6.0-rc4), instead of Zhen Lei's patch, should fix the
problem entirely.

Thanks,
Isaac

2022-09-06 21:06:19

by Gabriel Francisco

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

On 06/09/2022 19:54, Isaac Manjarres wrote:
> On Sun, Sep 4, 2022 at 4:15 AM Gabriel Francisco <[email protected]> wrote:
>> This patch alone on top of v6.0-rc2 still gives me the null pointer.
>>
>> But combining it with Zhen Lei's patch (from
>> https://lkml.org/lkml/2022/8/27/164) my device boots successfully.
> Hi Gabriel,
>
> Thanks for your e-mail. I think my AMBA bus patch and
> https://lore.kernel.org/all/[email protected]/
> (merged in 6.0-rc4), instead of Zhen Lei's patch, should fix the
> problem entirely.
>
> Thanks,
> Isaac

I'm afraid the issue is still showing up (looks similar when I tested
with 6.0-rc2 + your patch and it went away when combining with Zhen
Lei's one).

I added the dmesg log at
https://bugzilla.kernel.org/attachment.cgi?id=301756

Thank you,

Gabriel Francisco


2022-09-06 21:53:36

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH v2] amba: Fix use-after-free in amba_read_periphid()

On Tue, Sep 06, 2022 at 10:58:55PM +0200, Gabriel Francisco wrote:
> I'm afraid the issue is still showing up (looks similar when I tested with
> 6.0-rc2 + your patch and it went away when combining with Zhen Lei's one).
>
> I added the dmesg log at
> https://bugzilla.kernel.org/attachment.cgi?id=301756

Bugzilla's all great and all, but not with firefox. Firefox wants me
to open that attachment in Libreoffice Writer... so wget -O - ... |less
to the rescue.

So you have an oops in __clk_put(), which suggests you don't have
"amba: Fix use-after-free in amba_read_periphid()" from Isaac Manjarres
applied. I haven't sent it upstream yet, but even if I had, 6.0-rc2
would be too old. Please try with Isaac's patch applied, thanks.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!