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]

Message-ID: <43291037-4348-1d68-cecc-57e520a309db@huawei.com>
Date:   Tue, 30 Aug 2022 10:12:47 +0800
From:   "Leizhen (ThunderTown)" <thunder.leizhen@...wei.com>
To:     Saravana Kannan <saravanak@...gle.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 2022/8/30 5:17, Saravana Kannan wrote:
> 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.

OK, I will do it in v2. It seems necessary to add a helper function to
avoid duplication of code.

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

-- 
Regards,
  Zhen Lei