Summary: acpi: fix acpi_ec_probe to only check EC devices

[bwidawsk]

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.

[jhb]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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

[bwidawsk]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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]

sys/dev/acpica/acpi_ec.c

350 ↗

(On Diff #46432)

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]

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.

[jhb]

sys/dev/acpica/acpi_ec.c

337 ↗

(On Diff #47549)

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

338 ↗

(On Diff #47549)

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]

sys/dev/acpica/acpi_ec.c

338 ↗

(On Diff #47549)

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]

sys/dev/acpica/acpi_ec.c

338 ↗

(On Diff #47549)

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]

This comment was removed by bwidawsk.

[bwidawsk]

This comment has been deleted.

sys/dev/acpica/acpi_ec.c

338 ↗

(On Diff #47549)

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]

sys/dev/acpica/acpi_ec.c

338 ↗

(On Diff #47549)

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]

commit 5e5bf62ac3ec965fd2809b8c393d7b088b125cab (HEAD)
Author: Ben Widawsky <[e-mail]>
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@.

[jhb]

sys/kern/subr_bus.c

2789 ↗

(On Diff #48685)

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

[emaste]

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);