linux-kernel - Re: [PATCH] ARM: 9220/1: amba: Add sanity check for dev->periphid in amba

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Aug 2022 14:17:22 -0700
From:   Saravana Kannan <saravanak@...gle.com>
To:     Zhen Lei <thunder.leizhen@...wei.com>
Cc:     Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        patches@...linux.org.uk, Kefeng Wang <wangkefeng.wang@...wei.com>,
        Isaac Manjarres <isaacmanjarres@...gle.com>
Subject: Re: [PATCH] ARM: 9220/1: amba: Add sanity check for dev->periphid in amba_probe()

On Sat, Aug 27, 2022 at 3:21 AM Zhen Lei <thunder.leizhen@...wei.com> wrote:
>
> Commit f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> forcibly invokes device_add() even if dev->periphid is not ready. Although
> it will be remedied in amba_match(): dev->periphid will be initialized
> if everything is in place; Otherwise, return -EPROBE_DEFER to block
> __driver_attach() from further execution. But not all drivers have .match
> hook, such as pl031, the dev->bus->probe will be called directly in
> __driver_attach(). Unfortunately, if dev->periphid is still not
> initialized, the following exception will be triggered.
>
> 8<--- cut here ---
> Unable to handle kernel NULL pointer dereference at virtual address 00000008
> [00000008] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.0.0-rc2+ #7
> Hardware name: ARM-Versatile Express
> PC is at pl031_probe+0x8/0x208
> LR is at amba_probe+0xf0/0x160
> pc : 80698df8  lr : 8050eb54  psr: 80000013
> sp : c0825df8  ip : 00000000  fp : 811fda38
> r10: 00000000  r9 : 80d72470  r8 : fffffdfb
> r7 : 811fd800  r6 : be7eb330  r5 : 00000000  r4 : 811fd900
> r3 : 80698df0  r2 : 37000000  r1 : 00000000  r0 : 811fd800
> Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
> Control: 10c5387d  Table: 6000406a  DAC: 00000051
> ... ...
>  pl031_probe from amba_probe+0xf0/0x160
>  amba_probe from really_probe+0x118/0x290
>  really_probe from __driver_probe_device+0x84/0xe4
>  __driver_probe_device from driver_probe_device+0x30/0xd0
>  driver_probe_device from __driver_attach+0x8c/0xfc
>  __driver_attach from bus_for_each_dev+0x70/0xb0
>  bus_for_each_dev from bus_add_driver+0x168/0x1f4
>  bus_add_driver from driver_register+0x7c/0x118
>  driver_register from do_one_initcall+0x44/0x1ec
>  do_one_initcall from kernel_init_freeable+0x238/0x288
>  kernel_init_freeable from kernel_init+0x18/0x12c
>  kernel_init from ret_from_fork+0x14/0x2c
> ... ...
> ---[ end trace 0000000000000000 ]---
>
> Therefore, take the same action as in amba_match(): return -EPROBE_DEFER
> if dev->periphid is not ready in amba_probe().
>
> Fixes: f2d3b9a46e0e ("ARM: 9220/1: amba: Remove deferred device addition")
> Signed-off-by: Zhen Lei <thunder.leizhen@...wei.com>
> ---
> KernelVersion: v6.0-rc2
>  drivers/amba/bus.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 32b0e0b930c1068..2e5572f60692a33 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -274,9 +274,17 @@ static int amba_probe(struct device *dev)
>  {
>         struct amba_device *pcdev = to_amba_device(dev);
>         struct amba_driver *pcdrv = to_amba_driver(dev->driver);
> -       const struct amba_id *id = amba_lookup(pcdrv->id_table, pcdev);
> +       const struct amba_id *id;
>         int ret;
>
> +       if (!pcdev->periphid) {
> +               ret = amba_read_periphid(pcdev);
> +               if (ret)
> +                       return -EPROBE_DEFER;
> +       }
> +
> +       id = amba_lookup(pcdrv->id_table, pcdev);
> +
>         do {
>                 ret = of_amba_device_decode_irq(pcdev);
>                 if (ret)

Cc: Isaac

Hi Zhen,

Thanks for the report and trying to fix it. I think your change is in
the right direction but there are a couple of issues that need to be
addressed.

Firstly, please make your change on top of [1] that fixes a race in my
patch. So, you'll need to use some locks here. Also, you'll need to do
the uevent update thing I do in amba_match() when you read the
periphid successfully for the first time.

Thanks,
Saravana

[1] - https://lore.kernel.org/lkml/20220818172852.3548-1-isaacmanjarres@google.com/