linux-kernel - Re: [PATCH] platform/chrome: cros

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: <CABXOdTcZoGy3w_aVPju0DLr+Ab0C8XUsMs0+XsTW0QacR53iug@mail.gmail.com>
Date:   Wed, 16 Feb 2022 08:12:42 -0800
From:   Guenter Roeck <groeck@...gle.com>
To:     Tong Zhang <ztong0001@...il.com>
Cc:     Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/chrome: cros_ec: honor acpi=off

On Tue, Feb 15, 2022 at 10:26 PM Tong Zhang <ztong0001@...il.com> wrote:
>
> when acpi=off is provided in bootarg, kernel crash with
>
>  BUG: kernel NULL pointer dereference, address: 0000000000000018
>  RIP: 0010:acpi_ns_walk_namespace+0x57/0x280
>   <TASK>
>  ? acpi_get_devices+0x140/0x140
>  cros_ec_lpc_init+0x25/0x100
>
> Driver should check if ACPI is disabled before calling acpi_get_devices(),
> otherwise acpi_walk_namespace() will dereference null pointer since the
> acpi_gbl_root_node is not initialized.
> This is a common pattern and should be fixed in ACPI framework to prevent
> such crash in the future, but since many drivers are already doing explicit
> check(acpi_disable) we do the same thing here.
>
> Signed-off-by: Tong Zhang <ztong0001@...il.com>
> ---
>  drivers/platform/chrome/cros_ec_lpc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index d6306d2a096f..95412a55ed8d 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -550,6 +550,9 @@ static int __init cros_ec_lpc_init(void)
>         int ret;
>         acpi_status status;
>
> +       if (acpi_disable)

acpi_disabled ?

One does wonder why anyone would disable ACPI on any of the supported
systems, but in either case this is wrong. The driver should not abort
if acpi is disabled but just not call acpi_get_devices().

Guenter

> +               return -ENODEV;
> +
>         status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
>                                   &cros_ec_lpc_acpi_device_found, NULL);
>         if (ACPI_FAILURE(status))
> --
> 2.25.1
>