Re: Null pointer dereference in cros-ec-typec

From: Alyssa Ross
Date: Wed Jan 19 2022 - 15:32:59 EST

Next message: Bjorn Helgaas: "Re: [PATCH] PCI: endpoint: Add prefetch bar support"
Previous message: Bhupesh Sharma: "[PATCH 4/4] arm64: dts: qcom: sm8150: Add PDC as the interrupt parent for tlmm"
In reply to: Mr. Chromebox: "Re: Null pointer dereference in cros-ec-typec"
Next in thread: Prashant Malani: "Re: Null pointer dereference in cros-ec-typec"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Jan 19, 2022 at 12:44:06PM -0600, Mr. Chromebox wrote:
> On Wed, Jan 19, 2022 at 12:24 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> >
> > Thanks Alyssa,
> >
> > It looks like the right fix here should go in coreboot.
> >
> > I'll wait for a response from Matt regarding whether those EC commands
> > are supported on the eve EC firmware image Mr.Chromebox releases.
>
> looking at Chrome-EC branch firmware-eve-9584.B,
>
> EC_CMD_GET_PD_PORT_CAPS is *not* supported
> EC_CMD_USB_PD_PORTS is supported
>
> no difference in this regard between my updated EC firmware and the
> latest stock EC firmware provided by Google for EVE
>
> > I have a fix but I can't test it since :
> > - I am not sure how the Mr.Chromebox eve BIOS is compiled.
>
> same build system as upstream coreboot
>
> > - I don't have an eve.
>
> me neither :)

I'd be more than happy to test Coreboot patches on my eve and report
back on anything that needs to be checked, if that helps? I've built
and installed it from source before.

> >
> > On Tue, Jan 18, 2022 at 6:37 PM Alyssa Ross <hi@xxxxxxxxx> wrote:
> > >
> > > On Tue, Jan 18, 2022 at 04:35:45PM -0800, Prashant Malani wrote:
> > > > There seem to be some differences.
> > > > The Mr.Chromebox GOOG0004 device doesn't have any connectors listed.
> > > > See here for a BIOS snippet from voxel:
> > > > Scope (\_SB.PCI0.LPCB.EC0.CREC)
> > > > {
> > > > Device (USBC)
> > > > {
> > > > Name (_HID, "GOOG0014") // _HID: Hardware ID
> > > > Name (_DDN, "ChromeOS EC Embedded Controller USB Type-C
> > > > Control") // _DDN: DOS Device Name
> > > > Device (CON0)
> > > > {
> > > > ...
> > > > }
> > > >
> > > > Device (CON1)
> > > > {
> > > > ...
> > > > }
> > > > }
> > > > }
> > > >
> > > > Does the EC on this device support EC_CMD_GET_PD_PORT_CAPS , or
> > > > EC_CMD_USB_PD_PORTS ?
> > > > It doesn't look like the EC is returning anything for these commands,
> > > > which are used to populate GOOG0014.
> > > >
> > > > The CREC device looks about right:
> > > >
> > > > Device (CREC)
> > > > {
> > > > Name (_HID, "GOOG0004") // _HID: Hardware ID
> > > > Name (_UID, One) // _UID: Unique ID
> > > > Name (_DDN, "EC Command Device") // _DDN: DOS Device Name
> > > > Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake
> > > > {
> > > > ....
> > > > })
> > > > }
> > > >
> > > > 2 observations:
> > > > - We probably shouldn't be generating a GOOG0014 device at all if the
> > > > EC_CMD_GET_PD_PORT_CAPS and EC_CMD_USB_PD_PORTS commands aren't
> > > > supported by the EC. I can work with coreboot to make that change
> > > > - Is the order of probing for some reason causing the GOOG0014 child
> > > > device to not be linked to the GOOG0004 device? Alyssa, does the
> > > > following diff help:
> > >
> > > With the diff applied, I no longer see the Oops from cros_ec_typec, and
> > > I can reboot normally. But, it looks like the driver is deferred
> > > indefinitely — I added a debug print after the check, and it was never
> > > triggered.
> > >
> > > > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > > > b/drivers/platform/chrome/cros_ec_typec.c
> > > > index 5de0bfb0bc4d..7059912b75c1 100644
> > > > --- a/drivers/platform/chrome/cros_ec_typec.c
> > > > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > > > @@ -1076,6 +1076,10 @@ static int cros_typec_probe(struct platform_device *pdev)
> > > >
> > > > typec->dev = dev;
> > > > typec->ec = dev_get_drvdata(pdev->dev.parent);
> > > > +
> > > > + if (!typec->ec)
> > > > + return -EPROBE_DEFER;
> > > > +
> > > > platform_set_drvdata(pdev, typec);
> > > >
> > > > ret = cros_typec_get_cmd_version(typec);
> > > >
> > > >
> > > > On Tue, Jan 18, 2022 at 2:34 PM Mr. Chromebox <mrchromebox@xxxxxxxxx> wrote:
> > > > >
> > > > > On Tue, Jan 18, 2022 at 4:16 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> > > > > >
> > > > > > Hi Matt,
> > > > > >
> > > > > > On Tue, Jan 18, 2022 at 2:04 PM Mr. Chromebox <mrchromebox@xxxxxxxxx> wrote:
> > > > > > >
> > > > > > > hi Prashant,
> > > > > > >
> > > > > > > my releases track upstream coreboot; my most recent release was based
> > > > > > > on coreboot 4.14 (I'm behind on getting a 4.15-based release out).
> > > > > > >
> > > > > > > A quick perusal of the source for src/ec/google/chromeec/ doesn't show
> > > > > > > any recent changes to the location of the GOOG0014 ACPI device. The
> > > > > > > most recent change was 2 years ago (so, landed in the 4.12 release),
> > > > > > > which moved the USB-C child device to its present location: under
> > > > > > > \_SB.PCI0.LPCB.EC0.CREC
> > > > > > >
> > > > > > > ref: https://github.com/coreboot/coreboot/commit/eec30f7beae074c3f80a182cc2950ed8e4f0a640
> > > > > > >
> > > > > > > prior to that, it was located under \_SB.PCI0.LPCB.EC0.
> > > > > > >
> > > > > > > I also dumped/disassembled the ACPI from a recent build to confirm the above.
> > > > > >
> > > > > > Is it possible to share the disassembled ACPI tables? We can then
> > > > > > compare it to the ones on shipping Chromebooks to identify a
> > > > > > discrepancy.
> > > > > > If the GOOG0014 device is correctly listed as a child of the EC device
> > > > > > (GOOG0004), then the kernel ACPI framework should be setting
> > > > > > GOOG0004 as a parent (and dev_get_drvdata(pdev->dev.parent) shouldn't
> > > > > > return NULL).
> > > > >
> > > > > as the GOOG0014 device is runtime-generated, it's located in the SSDT:
> > > > >
> > > > > External (_SB_.PCI0.LPCB.EC0_.CREC, DeviceObj)
> > > > > ...
> > > > > Scope (\_SB.PCI0.LPCB.EC0.CREC)
> > > > > {
> > > > > Device (USBC)
> > > > > {
> > > > > Name (_HID, "GOOG0014") // _HID: Hardware ID
> > > > > Name (_DDN, "ChromeOS EC Embedded Controller USB Type-C
> > > > > Control") // _DDN: DOS Device Name
> > > > > }
> > > > > }
> > > > >
> > > > > GOOG0004 is defined in the DSDT, under EC0:
> > > > >
> > > > > Device (CREC)
> > > > > {
> > > > > Name (_HID, "GOOG0004") // _HID: Hardware ID
> > > > > Name (_UID, One) // _UID: Unique ID
> > > > > Name (_DDN, "EC Command Device") // _DDN: DOS Device Name
> > > > > Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake
> > > > > {
> > > > > 0x70,
> > > > > 0x05
> > > > > })
> > > > > ...
> > > > > }
> > > > >
> > > > > -Matt
> > > > >
> > > > > > > regards,
> > > > > > > Matt / MrChromebox
> > > > > > >
> > > > > > > On Tue, Jan 18, 2022 at 2:12 PM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > (+Mr.Chromebox team; using the address listed in
> > > > > > > > https://mrchromebox.tech/#support )
> > > > > > > >
> > > > > > > > Hi Team Mr.Chromebox,
> > > > > > > >
> > > > > > > > Could you kindly provide some more detail regarding how the GOOG0014
> > > > > > > > Type C ACPI device is set up in the Mr Chromebox BIOS for Chromebooks
> > > > > > > > (the driver expects it to be embedded in the GOOG0004 EC device)?
> > > > > > > > We want to enable Alyssa and other developers using the Mr.Chromebox
> > > > > > > > BIOS to have a functional cros-ec-typec driver, so would like to help
> > > > > > > > ensure that the device is set up correctly in ACPI.
> > > > > > > >
> > > > > > > > Thanks!
> > > > > > > >
> > > > > > > > -Prashant
> > > > > > > >
> > > > > > > > On Tue, Jan 18, 2022 at 11:49 AM Prashant Malani <pmalani@xxxxxxxxxxxx> wrote:
> > > > > > > > >
> > > > > > > > > Hi Benson and Alyssa,
> > > > > > > > >
> > > > > > > > > On Tue, Jan 18, 2022 at 11:33 AM Benson Leung <bleung@xxxxxxxxxx> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Alyssa,
> > > > > > > > > >
> > > > > > > > > > Thanks for reaching out.
> > > > > > > > > >
> > > > > > > > > > On Tue, Jan 18, 2022 at 04:37:54PM +0000, Alyssa Ross wrote:
> > > > > > > > > > > My distribution recently enabled the Chrome OS EC Type C control driver
> > > > > > > > > > > in its kernel builds. On my Google Pixelbook i7 (eve), the driver reports
> > > > > > > > > > > a null pointer dereference at boot. From what I can tell, this happens
> > > > > > > > > > > because typec->ec is set to NULL in cros_typec_probe. Other drivers,
> > > > > > > > > > > like cros-usbpd-notify, appear to be set up to handle this case. As a
> > > > > > > > > > > result of this bug, I'm no longer able to reboot my computer, because
> > > > > > > > > > > udevd hangs while trying to do something with the device whose driver
> > > > > > > > > > > isn't working.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I've copied Prashant, who's the author of the typec driver as well as
> > > > > > > > > > cros-usbpd-notify.
> > > > > > > > > >
> > > > > > > > > > Prashant, any thoughts on a more graceful failure out of the typec driver's
> > > > > > > > > > probe in case there's no ec object?
> > > > > > > > >
> > > > > > > > > We can add a NULL check and just abort the driver probe if the pointer is
> > > > > > > > > not valid (the driver is useless without that pointer anyway).
> > > > > > > > >
> > > > > > > > > A note: The NULL check makes sense on older drivers like cros-usbpd-notify since
> > > > > > > > > they can exist in ACPI configurations where they are *not* embedded
> > > > > > > > > inside the GOOG0004
> > > > > > > > > EC device (on older Chromebooks). That is not the case for the EC Type C device.
> > > > > > > > >
> > > > > > > > > This raises another issue: the custom BIOS from Mr. Chromebox is
> > > > > > > > > likely not setting
> > > > > > > > > up the EC Type C ACPI (GOOG0014) device correctly; it *must* be
> > > > > > > > > embedded inside the overall
> > > > > > > > > EC device (GOOG0004). If this is not being done, then the GOOG0014
> > > > > > > > > device should not
> > > > > > > > > be added to the ACPI tables at all.
> > > > > > > > >
> > > > > > > > > I would like to understand whether the above was intentional from the
> > > > > > > > > Mr. Chromebox BIOS developers;
> > > > > > > > > otherwise we are letting an incorrect ACPI configuration just fail
> > > > > > > > > with a probe error.
> > > > > > > > >
> > > > > > > > > Thanks,
> > > > > > > > >
> > > > > > > > > -Prashant
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Here's the full Oops. I was able to reproduce the issue with every
> > > > > > > > > > > kernel I tried, from 5.10 to mainline.
> > > > > > > > > > >
> > > > > > > > > > > cros-usbpd-notify-acpi GOOG0003:00: Couldn't get Chrome EC device pointer.
> > > > > > > > > > > input: Intel Virtual Buttons as /devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input14
> > > > > > > > > > > BUG: kernel NULL pointer dereference, address: 00000000000000d8
> > > > > > > > > > > #PF: supervisor read access in kernel mode
> > > > > > > > > > > #PF: error_code(0x0000) - not-present page
> > > > > > > > > > > PGD 0 P4D 0
> > > > > > > > > > > Oops: 0000 [#1] SMP PTI
> > > > > > > > > > > CPU: 1 PID: 561 Comm: systemd-udevd Not tainted 5.15.12 #4
> > > > > > > > > > > Hardware name: Google Eve/Eve, BIOS MrChromebox-4.14 08/06/2021
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Ah, here's the problem. It looks like this is a custom bios from Mr Chromebox,
> > > > > > > > > > so this is not a bios combination we validate at Google.
> > > > > > > > > >
> > > > > > > > > > Thank you for the report. We'll look into fixing this and marking the fix
> > > > > > > > > > for stable kernels so that it goes back to 5.10.
> > > > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > >
> > > > > > > > > > Benson
> > > > > > > > > >
> > > > > > > > > > > RIP: 0010:__mutex_lock+0x59/0x8c0
> > > > > > > > > > > Code: 53 48 89 cb 48 83 ec 70 89 75 9c be 3d 02 00 00 4c 89 45 90 e8 18 47 33 ff e8 e3 e2 ff ff 44 8b 35 a4 85 e8 02 45 85 f6 75 0a <4d> 3b 6d 68 0f 85 bf 07 00 00 65 ff 05 b6 5b 23 75 ff 75 90 4d 8d
> > > > > > > > > > > RSP: 0018:ffffb44580a4bb50 EFLAGS: 00010246
> > > > > > > > > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000001
> > > > > > > > > > > RDX: 0000000000000000 RSI: ffffffff8bf91320 RDI: ffff922cbba50e20
> > > > > > > > > > > RBP: ffffb44580a4bbf0 R08: 0000000000000000 R09: ffff922c5bac8140
> > > > > > > > > > > R10: ffffb44580a4bc10 R11: 0000000000000000 R12: 0000000000000000
> > > > > > > > > > > R13: 0000000000000070 R14: 0000000000000000 R15: 0000000000000001
> > > > > > > > > > > FS: 00007f55338d6b40(0000) GS:ffff922fae200000(0000) knlGS:0000000000000000
> > > > > > > > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > > > > > > > CR2: 00000000000000d8 CR3: 000000011bbb2006 CR4: 00000000003706e0
> > > > > > > > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > > > > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > > > > > > > Call Trace:
> > > > > > > > > > > <TASK>
> > > > > > > > > > > ? fs_reclaim_acquire+0x4d/0xd0
> > > > > > > > > > > ? lock_is_held_type+0xaa/0x120
> > > > > > > > > > > ? cros_ec_cmd_xfer_status+0x1f/0x110
> > > > > > > > > > > ? lock_is_held_type+0xaa/0x120
> > > > > > > > > > > ? cros_ec_cmd_xfer_status+0x1f/0x110
> > > > > > > > > > > cros_ec_cmd_xfer_status+0x1f/0x110
> > > > > > > > > > > cros_typec_ec_command+0x91/0x1c0 [cros_ec_typec]
> > > > > > > > > > > cros_typec_probe+0x7f/0x5a8 [cros_ec_typec]
> > > > > > > > > > > platform_probe+0x3f/0x90
> > > > > > > > > > > really_probe+0x1f5/0x3f0
> > > > > > > > > > > __driver_probe_device+0xfe/0x180
> > > > > > > > > > > driver_probe_device+0x1e/0x90
> > > > > > > > > > > __driver_attach+0xc4/0x1d0
> > > > > > > > > > > ? __device_attach_driver+0xe0/0xe0
> > > > > > > > > > > ? __device_attach_driver+0xe0/0xe0
> > > > > > > > > > > bus_for_each_dev+0x67/0x90
> > > > > > > > > > > bus_add_driver+0x12e/0x1f0
> > > > > > > > > > > driver_register+0x8f/0xe0
> > > > > > > > > > > ? 0xffffffffc04ec000
> > > > > > > > > > > do_one_initcall+0x67/0x320
> > > > > > > > > > > ? rcu_read_lock_sched_held+0x3f/0x80
> > > > > > > > > > > ? trace_kmalloc+0x38/0xe0
> > > > > > > > > > > ? kmem_cache_alloc_trace+0x17c/0x2b0
> > > > > > > > > > > do_init_module+0x5c/0x270
> > > > > > > > > > > __do_sys_finit_module+0x95/0xe0
> > > > > > > > > > > do_syscall_64+0x3b/0x90
> > > > > > > > > > > entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > > > > > > > > > RIP: 0033:0x7f55344b1f3d
> > > > > > > > > > > Code: 5b 41 5c c3 66 0f 1f 84 00 00 00 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d bb ee 0e 00 f7 d8 64 89 01 48
> > > > > > > > > > > RSP: 002b:00007fff187f1388 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
> > > > > > > > > > > RAX: ffffffffffffffda RBX: 000055a53acbe6e0 RCX: 00007f55344b1f3d
> > > > > > > > > > > RDX: 0000000000000000 RSI: 00007f553461732c RDI: 000000000000000e
> > > > > > > > > > > RBP: 0000000000020000 R08: 0000000000000000 R09: 0000000000000002
> > > > > > > > > > > R10: 000000000000000e R11: 0000000000000246 R12: 00007f553461732c
> > > > > > > > > > > R13: 000055a53ad94010 R14: 0000000000000007 R15: 000055a53ad95690
> > > > > > > > > > > </TASK>
> > > > > > > > > > > Modules linked in: fjes(+) cros_ec_typec(+) typec intel_vbtn(+) cros_usbpd_notify sparse_keymap soc_button_array int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel cros_kbd_led_backlight zram ip_tables i915 hid_multitouch i2c_algo_bit ttm crct10dif_pclmul crc32_pclmul crc32c_intel drm_kms_helper nvme ghash_clmulni_intel sdhci_pci cqhci cec nvme_core sdhci serio_raw drm mmc_core i2c_hid_acpi i2c_hid video pinctrl_sunrisepoint fuse
> > > > > > > > > > > CR2: 00000000000000d8
> > > > > > > > > > > ---[ end trace 4a12c4896d70352b ]---
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Benson Leung
> > > > > > > > > > Staff Software Engineer
> > > > > > > > > > Chrome OS Kernel
> > > > > > > > > > Google Inc.
> > > > > > > > > > bleung@xxxxxxxxxx
> > > > > > > > > > Chromium OS Project
> > > > > > > > > > bleung@xxxxxxxxxxxx

Attachment: signature.asc
Description: PGP signature

Next message: Bjorn Helgaas: "Re: [PATCH] PCI: endpoint: Add prefetch bar support"
Previous message: Bhupesh Sharma: "[PATCH 4/4] arm64: dts: qcom: sm8150: Add PDC as the interrupt parent for tlmm"
In reply to: Mr. Chromebox: "Re: Null pointer dereference in cros-ec-typec"
Next in thread: Prashant Malani: "Re: Null pointer dereference in cros-ec-typec"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]