Page MenuHomeFreeBSD

Summary: acpi: fix acpi_ec_probe to only check EC devices
ClosedPublic

Authored by bwidawsk on Aug 8 2018, 8:00 PM.

Details

Summary

The existing code assumes that acpi_ec_probe is only ever called with a
dereferencable acpi param. Aside from being incorrect because other
devices of ACPI_TYPE_DEVICE may be probed here which aren't ec devices,
(and they may have set acpi private data), it is even more nefarious if
another ACPI driver uses private data which is not dereferancable. This
will result in a pointer deref during boot and therefore boot failure.

On X86, as it stands today, no other devices actually do this (acpi_cpu
checks for PROCESSOR type devices) and so there is no issue. I ran into
this because I am adding such a device which gets probed before
acpi_ec_probe and sets private data. If ARM ever has an EC, I think
they'd run into this issue as well.

Test Plan

My platform which has an ECDT, and a single EC Device(). I ran the code as is, as well as with an early return in ecdt_probe and verified the EC device (and only one) was present in both cases.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 19334
Build 18939: arc lint + arc unit

Event Timeline

bwidawsk created this revision.Aug 8 2018, 8:00 PM
jhb added inline comments.Aug 16 2018, 12:21 PM
sys/dev/acpica/acpi_ec.c
360

This breaks the case that a device was added via ECDT. On systems that only enumerate the EC via ECDT without a corresponding Device() node, the device_t is added above in acpi_ec_ecdt_probe() and it won't have a valid _HID or _CID so would always fail here. A better way is to check to see if the device has a "fixed" device class and using that below instead of the 'params != NULL' check. The DF_FIXEDCLASS flag is what you want, but we don't currently expose that out of sys/kern/subr_bus.c. We could maybe add a 'device_is_fixedclass()' accessor.

bwidawsk added inline comments.Aug 16 2018, 1:28 PM
sys/dev/acpica/acpi_ec.c
360

You're right. I will respin to add the accessor and use that flag.

bwidawsk added inline comments.Aug 16 2018, 1:45 PM
sys/dev/acpica/acpi_ec.c
360

On looking a bit closer, won't we pretty much always get a FIXEDCLASS device? The only case we do not is when there is no ECDT, or what am I missing here?

jhb added inline comments.Aug 17 2018, 8:35 AM
sys/dev/acpica/acpi_ec.c
360

Yes, if there is no ECDT but the EC is still enumerated as a Device() with a EC _HID. That is the 'params == NULL' case.

bwidawsk added inline comments.Aug 17 2018, 9:09 AM
sys/dev/acpica/acpi_ec.c
360

Okay, just seemed a bit weird that the most common case is probably there is an ECDT and EC Device(), which we're calling "FIXEDCLASS"... I'm fine with that, just making sure I understood first.

jhb added inline comments.Aug 20 2018, 10:55 AM
sys/dev/acpica/acpi_ec.c
360

Yes, it is odd. :( I think there were older systems that only used ECDT or only used Device(). I guess we have something in place to not create two device_t objects for the EC if you have both, but I'd have to look some more to see how that works.

Actually, my ivy bridge desktop doesn't have an ECDT at all, only a Device(). Checking the 6.2 spec it seems that ECDT is optional, and it wasn't present in ACPI 1.0. I actually don't know what we do on a system with the ECDT present currently. It seems like we might create two acpi_ec devices. I wonder if the second one (via Device()) fails to attach because it's resources are already allocated?

bwidawsk updated this revision to Diff 47549.Sep 1 2018, 1:07 AM

Update the revision to hopefully work properly. The patch this time attempts to
separate sharing acpi_ec_probe, and only use acpi_ec_probe for the Device() as
opposed to the fixed ECDT enumerated one.

bwidawsk marked 6 inline comments as done.Sep 1 2018, 1:08 AM
bwidawsk edited the test plan for this revision. (Show Details)Sep 1 2018, 1:13 AM
jhb added inline comments.Sep 13 2018, 6:50 PM
sys/dev/acpica/acpi_ec.c
337

You don't need this line because adding a device with a set name (the BUS_ADD_CHILD) already sets the devclass.

338

So I don't know how I quite feel about bypassing device_probe() in this case. The other suggestion I had made was to add a 'device_has_fixed_devclass' or some such and using that to make acpi_ec_probe fail unless either it had a valid ID or the fixed devclass. That would be a much smaller patch overall. There might be some other places in the future where 'device_has_fixed_devclass' might also be useful.

bwidawsk added inline comments.Sep 13 2018, 7:08 PM
sys/dev/acpica/acpi_ec.c
338

I intentionally ignored your advice earlier because when I tried to implement it, it seemed impossible to ever support more than a single EC that way.

Perhaps I mangled what you meant though. In my interpretation, you meant to add a check in the probe to find a fixedclass EC device, if found, skip the probe. If not found, continue on with probe. This solution I think always works when there is a single EC. If you ever want to support multiple ECs, it no longer works.

Perhaps that case never will exist, or we should only bother when it does? I'm fine with changing it if you prefer, I just wanted to make my intention clear.

Please advise.

jhb added inline comments.Sep 13 2018, 11:54 PM
sys/dev/acpica/acpi_ec.c
338

The change as described won't prevent multiple ECs, though the current code already tries to avoid duplicate ECs. I'd be surprised if there was a system with more than one EC. If we encounter one we can revise the current code that tries to handle duplicates.

My suggestion was to use the check to avoid rejecting ECDT devices for which the ACPI_ID_PROBE check would fail. That is, if we had 'device_has_fixed_devclass()' then the entire change in acpi_ec.c would be to make the first few lines in acpi_ec_probe() this:

/* Check that this is a device and that EC is not disabled. */
if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec") ||
    (!device_has_fixed_devclass(dev) && ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids) != 0))
    return (ENXIO);

Those lines are a bit long though so I might structure it as something like:

if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec"))
    return (ENXIO);

if (device_has_fixed_devclass(dev)) {
    ecdt = 1;
    params = acpi_get_private(dev);
    if (params != NULL)
        ret = 0;
    else
        ret = ENXIO;
} else {
    ret = ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids);
    if (ret > 0)
        return (ret);

    params = malloc(...);
    /* Existing non-ECDT code */
}

This also ensures that the invariant about parameters only existing for ECDT devices still holds.

bwidawsk planned changes to this revision.Sep 14 2018, 5:35 PM
This comment was removed by bwidawsk.
bwidawsk marked an inline comment as done.EditedSep 26 2018, 5:36 PM
This comment has been deleted.
sys/dev/acpica/acpi_ec.c
338

Won't this fail if another ACPI device has a fixed class type, and private data? It seems to just be kicking the can down the road unless I don't understand quite right.

jhb added inline comments.Sep 27 2018, 8:45 PM
sys/dev/acpica/acpi_ec.c
338

No, because if it has a fixed device class then it can only be probed by drivers whose name matches the fixed devclass. Thus, in this case, only drivers that have the "acpi_ec" name (of which there is only this driver). We do have some device classes (names) that have multiple drivers (e.g. "pci" and "pcib"), but "acpi_ec" only has a single driver, so this test is sufficient.

bwidawsk updated this revision to Diff 48685.Oct 3 2018, 6:15 PM

commit 5e5bf62ac3ec965fd2809b8c393d7b088b125cab (HEAD)
Author: Ben Widawsky <ben.widawsky@intel.com>
Date: Tue Aug 7 18:40:37 2018 -0700

acpi: fix acpi_ec_probe to only check EC devices

This patch utilizes the fixed_devclass attribute in order to make sure
other acpi devices with params don't get confused for an EC device.

The existing code assumes that acpi_ec_probe is only ever called with a
dereferencable acpi param. Aside from being incorrect because other
devices of ACPI_TYPE_DEVICE may be probed here which aren't ec devices,
(and they may have set acpi private data), it is even more nefarious if
another ACPI driver uses private data which is not dereferancable. This
will result in a pointer deref during boot and therefore boot failure.

On X86, as it stands today, no other devices actually do this (acpi_cpu
checks for PROCESSOR type devices) and so there is no issue. I ran into
this because I am adding such a device which gets probed before
acpi_ec_probe and sets private data. If ARM ever has an EC, I think
they'd run into this issue as well.

There have been several iterations of this patch. Earlier
iterations had ECDT enumerated ECs not call into the probe/attach
functions of this driver. This change was Suggested by: jhb@.
bwidawsk marked 4 inline comments as done.Oct 3 2018, 6:35 PM
bwidawsk marked an inline comment as done.
jhb accepted this revision.Nov 15 2018, 6:51 PM
jhb added inline comments.
sys/kern/subr_bus.c
2789 ↗(On Diff #48685)

Minor style nit would be to put ()'s around the entire expression passed to return.

This revision is now accepted and ready to land.Nov 15 2018, 6:51 PM

Approved based on @jhb review.

sys/kern/subr_bus.c
2789 ↗(On Diff #48685)

Most style-compliant is probably return (dev->flags & DF_FIXEDCLASS != 0);

bwidawsk marked 2 inline comments as done.Nov 19 2018, 6:29 PM
This revision was automatically updated to reflect the committed changes.