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 <[email protected]>
---
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)
+ 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
Hi Tong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on v5.17-rc4 next-20220216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tong-Zhang/platform-chrome-cros_ec-honor-acpi-off/20220216-142709
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
config: i386-randconfig-a001 (https://download.01.org/0day-ci/archive/20220216/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/66345a4aecd6e4acba257476c6e44559fccca143
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tong-Zhang/platform-chrome-cros_ec-honor-acpi-off/20220216-142709
git checkout 66345a4aecd6e4acba257476c6e44559fccca143
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/chrome/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/platform/chrome/cros_ec_lpc.c: In function 'cros_ec_lpc_init':
>> drivers/platform/chrome/cros_ec_lpc.c:553:6: warning: the address of 'acpi_disable' will always evaluate as 'true' [-Waddress]
553 | if (acpi_disable)
| ^~~~~~~~~~~~
vim +553 drivers/platform/chrome/cros_ec_lpc.c
547
548 static int __init cros_ec_lpc_init(void)
549 {
550 int ret;
551 acpi_status status;
552
> 553 if (acpi_disable)
554 return -ENODEV;
555
556 status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
557 &cros_ec_lpc_acpi_device_found, NULL);
558 if (ACPI_FAILURE(status))
559 pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
560
561 if (!cros_ec_lpc_acpi_device_found &&
562 !dmi_check_system(cros_ec_lpc_dmi_table)) {
563 pr_err(DRV_NAME ": unsupported system.\n");
564 return -ENODEV;
565 }
566
567 cros_ec_lpc_mec_init(EC_HOST_CMD_REGION0,
568 EC_LPC_ADDR_MEMMAP + EC_MEMMAP_SIZE);
569
570 /* Register the driver */
571 ret = platform_driver_register(&cros_ec_lpc_driver);
572 if (ret) {
573 pr_err(DRV_NAME ": can't register driver: %d\n", ret);
574 cros_ec_lpc_mec_destroy();
575 return ret;
576 }
577
578 if (!cros_ec_lpc_acpi_device_found) {
579 /* Register the device, and it'll get hooked up automatically */
580 ret = platform_device_register(&cros_ec_lpc_device);
581 if (ret) {
582 pr_err(DRV_NAME ": can't register device: %d\n", ret);
583 platform_driver_unregister(&cros_ec_lpc_driver);
584 cros_ec_lpc_mec_destroy();
585 }
586 }
587
588 return ret;
589 }
590
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Tong,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on chrome-platform/for-next]
[also build test WARNING on v5.17-rc4 next-20220216]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Tong-Zhang/platform-chrome-cros_ec-honor-acpi-off/20220216-142709
base: https://git.kernel.org/pub/scm/linux/kernel/git/chrome-platform/linux.git for-next
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220216/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 0e628a783b935c70c80815db6c061ec84f884af5)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/66345a4aecd6e4acba257476c6e44559fccca143
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Tong-Zhang/platform-chrome-cros_ec-honor-acpi-off/20220216-142709
git checkout 66345a4aecd6e4acba257476c6e44559fccca143
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/chrome/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/platform/chrome/cros_ec_lpc.c:553:6: warning: address of function 'acpi_disable' will always evaluate to 'true' [-Wpointer-bool-conversion]
if (acpi_disable)
~~ ^~~~~~~~~~~~
drivers/platform/chrome/cros_ec_lpc.c:553:6: note: prefix with the address-of operator to silence this warning
if (acpi_disable)
^
&
1 warning generated.
vim +553 drivers/platform/chrome/cros_ec_lpc.c
547
548 static int __init cros_ec_lpc_init(void)
549 {
550 int ret;
551 acpi_status status;
552
> 553 if (acpi_disable)
554 return -ENODEV;
555
556 status = acpi_get_devices(ACPI_DRV_NAME, cros_ec_lpc_parse_device,
557 &cros_ec_lpc_acpi_device_found, NULL);
558 if (ACPI_FAILURE(status))
559 pr_warn(DRV_NAME ": Looking for %s failed\n", ACPI_DRV_NAME);
560
561 if (!cros_ec_lpc_acpi_device_found &&
562 !dmi_check_system(cros_ec_lpc_dmi_table)) {
563 pr_err(DRV_NAME ": unsupported system.\n");
564 return -ENODEV;
565 }
566
567 cros_ec_lpc_mec_init(EC_HOST_CMD_REGION0,
568 EC_LPC_ADDR_MEMMAP + EC_MEMMAP_SIZE);
569
570 /* Register the driver */
571 ret = platform_driver_register(&cros_ec_lpc_driver);
572 if (ret) {
573 pr_err(DRV_NAME ": can't register driver: %d\n", ret);
574 cros_ec_lpc_mec_destroy();
575 return ret;
576 }
577
578 if (!cros_ec_lpc_acpi_device_found) {
579 /* Register the device, and it'll get hooked up automatically */
580 ret = platform_device_register(&cros_ec_lpc_device);
581 if (ret) {
582 pr_err(DRV_NAME ": can't register device: %d\n", ret);
583 platform_driver_unregister(&cros_ec_lpc_driver);
584 cros_ec_lpc_mec_destroy();
585 }
586 }
587
588 return ret;
589 }
590
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On Wed, Feb 16, 2022 at 8:12 AM Guenter Roeck <[email protected]> wrote:
>
> On Tue, Feb 15, 2022 at 10:26 PM Tong Zhang <[email protected]> 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.
> >
Thanks for the comment. This is no longer an issue since the ACPI
patch I mentioned is staged in ACPICA tree.
https://github.com/acpica/acpica/commit/b1c3656ef4950098e530be68d4b589584f06cddc
On Tue, Feb 15, 2022 at 10:26 PM Tong Zhang <[email protected]> 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 <[email protected]>
> ---
> 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
>