svn commit: r340644 - in head/sys: dev/acpica kern sys
Ben Widawsky
bwidawsk at FreeBSD.org
Mon Nov 19 18:29:04 UTC 2018
Author: bwidawsk
Date: Mon Nov 19 18:29:03 2018
New Revision: 340644
URL: https://svnweb.freebsd.org/changeset/base/340644
Log:
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 at .
Reviewed by: jhb
Approved by: emaste (mentor)
Differential Revision: https://reviews.freebsd.org/D16635
Modified:
head/sys/dev/acpica/acpi_ec.c
head/sys/kern/subr_bus.c
head/sys/sys/bus.h
Modified: head/sys/dev/acpica/acpi_ec.c
==============================================================================
--- head/sys/dev/acpica/acpi_ec.c Mon Nov 19 18:26:11 2018 (r340643)
+++ head/sys/dev/acpica/acpi_ec.c Mon Nov 19 18:29:03 2018 (r340644)
@@ -345,92 +345,95 @@ acpi_ec_probe(device_t dev)
struct acpi_ec_params *params;
static char *ec_ids[] = { "PNP0C09", NULL };
+ ret = ENXIO;
+
/* Check that this is a device and that EC is not disabled. */
if (acpi_get_type(dev) != ACPI_TYPE_DEVICE || acpi_disabled("ec"))
- return (ENXIO);
+ return (ret);
- /*
- * If probed via ECDT, set description and continue. Otherwise,
- * we can access the namespace and make sure this is not a
- * duplicate probe.
- */
- ret = ENXIO;
- ecdt = 0;
+ if (device_is_devclass_fixed(dev)) {
+ /*
+ * If probed via ECDT, set description and continue. Otherwise, we can
+ * access the namespace and make sure this is not a duplicate probe.
+ */
+ ecdt = 1;
+ params = acpi_get_private(dev);
+ if (params != NULL)
+ ret = 0;
+
+ goto out;
+ }
+
+ ret = ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids, NULL);
+ if (ret > 0)
+ return (ret);
+
+ params = malloc(sizeof(struct acpi_ec_params), M_TEMP, M_WAITOK | M_ZERO);
+
buf.Pointer = NULL;
buf.Length = ACPI_ALLOCATE_BUFFER;
- params = acpi_get_private(dev);
- if (params != NULL) {
- ecdt = 1;
- ret = 0;
- } else {
- ret = ACPI_ID_PROBE(device_get_parent(dev), dev, ec_ids, NULL);
- if (ret > 0)
- goto out;
- params = malloc(sizeof(struct acpi_ec_params), M_TEMP,
- M_WAITOK | M_ZERO);
- h = acpi_get_handle(dev);
+ h = acpi_get_handle(dev);
- /*
- * Read the unit ID to check for duplicate attach and the
- * global lock value to see if we should acquire it when
- * accessing the EC.
- */
- status = acpi_GetInteger(h, "_UID", ¶ms->uid);
- if (ACPI_FAILURE(status))
- params->uid = 0;
- status = acpi_GetInteger(h, "_GLK", ¶ms->glk);
- if (ACPI_FAILURE(status))
- params->glk = 0;
+ /*
+ * Read the unit ID to check for duplicate attach and the global lock value
+ * to see if we should acquire it when accessing the EC.
+ */
+ status = acpi_GetInteger(h, "_UID", ¶ms->uid);
+ if (ACPI_FAILURE(status))
+ params->uid = 0;
- /*
- * Evaluate the _GPE method to find the GPE bit used by the EC to
- * signal status (SCI). If it's a package, it contains a reference
- * and GPE bit, similar to _PRW.
- */
- status = AcpiEvaluateObject(h, "_GPE", NULL, &buf);
- if (ACPI_FAILURE(status)) {
- device_printf(dev, "can't evaluate _GPE - %s\n",
- AcpiFormatException(status));
- goto out;
- }
- obj = (ACPI_OBJECT *)buf.Pointer;
- if (obj == NULL)
- goto out;
+ status = acpi_GetInteger(h, "_GLK", ¶ms->glk);
+ if (ACPI_FAILURE(status))
+ params->glk = 0;
- switch (obj->Type) {
- case ACPI_TYPE_INTEGER:
- params->gpe_handle = NULL;
- params->gpe_bit = obj->Integer.Value;
- break;
- case ACPI_TYPE_PACKAGE:
- if (!ACPI_PKG_VALID(obj, 2))
- goto out;
- params->gpe_handle =
- acpi_GetReference(NULL, &obj->Package.Elements[0]);
- if (params->gpe_handle == NULL ||
- acpi_PkgInt32(obj, 1, ¶ms->gpe_bit) != 0)
- goto out;
- break;
- default:
- device_printf(dev, "_GPE has invalid type %d\n", obj->Type);
- goto out;
- }
+ /*
+ * Evaluate the _GPE method to find the GPE bit used by the EC to signal
+ * status (SCI). If it's a package, it contains a reference and GPE bit,
+ * similar to _PRW.
+ */
+ status = AcpiEvaluateObject(h, "_GPE", NULL, &buf);
+ if (ACPI_FAILURE(status)) {
+ device_printf(dev, "can't evaluate _GPE - %s\n", AcpiFormatException(status));
+ goto out;
+ }
- /* Store the values we got from the namespace for attach. */
- acpi_set_private(dev, params);
+ obj = (ACPI_OBJECT *)buf.Pointer;
+ if (obj == NULL)
+ goto out;
- /*
- * Check for a duplicate probe. This can happen when a probe
- * via ECDT succeeded already. If this is a duplicate, disable
- * this device.
- */
- peer = devclass_get_device(acpi_ec_devclass, params->uid);
- if (peer != NULL && device_is_alive(peer)){
- ret = ENXIO;
- device_disable(dev);
- }
+ switch (obj->Type) {
+ case ACPI_TYPE_INTEGER:
+ params->gpe_handle = NULL;
+ params->gpe_bit = obj->Integer.Value;
+ break;
+ case ACPI_TYPE_PACKAGE:
+ if (!ACPI_PKG_VALID(obj, 2))
+ goto out;
+ params->gpe_handle = acpi_GetReference(NULL, &obj->Package.Elements[0]);
+ if (params->gpe_handle == NULL ||
+ acpi_PkgInt32(obj, 1, ¶ms->gpe_bit) != 0)
+ goto out;
+ break;
+ default:
+ device_printf(dev, "_GPE has invalid type %d\n", obj->Type);
+ goto out;
}
+ /* Store the values we got from the namespace for attach. */
+ acpi_set_private(dev, params);
+
+ /*
+ * Check for a duplicate probe. This can happen when a probe via ECDT
+ * succeeded already. If this is a duplicate, disable this device.
+ */
+ peer = devclass_get_device(acpi_ec_devclass, params->uid);
+ if (peer == NULL || !device_is_alive(peer))
+ ret = 0;
+ else
+ device_disable(dev);
+
+ if (buf.Pointer)
+ AcpiOsFree(buf.Pointer);
out:
if (ret <= 0) {
snprintf(desc, sizeof(desc), "Embedded Controller: GPE %#x%s%s",
@@ -439,8 +442,7 @@ out:
device_set_desc_copy(dev, desc);
} else
free(params, M_TEMP);
- if (buf.Pointer)
- AcpiOsFree(buf.Pointer);
+
return (ret);
}
Modified: head/sys/kern/subr_bus.c
==============================================================================
--- head/sys/kern/subr_bus.c Mon Nov 19 18:26:11 2018 (r340643)
+++ head/sys/kern/subr_bus.c Mon Nov 19 18:29:03 2018 (r340644)
@@ -2780,6 +2780,16 @@ device_set_devclass_fixed(device_t dev, const char *cl
}
/**
+ * @brief Query the device to determine if it's of a fixed devclass
+ * @see device_set_devclass_fixed()
+ */
+bool
+device_is_devclass_fixed(device_t dev)
+{
+ return ((dev->flags & DF_FIXEDCLASS) != 0);
+}
+
+/**
* @brief Set the driver of a device
*
* @retval 0 success
Modified: head/sys/sys/bus.h
==============================================================================
--- head/sys/sys/bus.h Mon Nov 19 18:26:11 2018 (r340643)
+++ head/sys/sys/bus.h Mon Nov 19 18:29:03 2018 (r340644)
@@ -612,6 +612,7 @@ void device_set_desc(device_t dev, const char* desc);
void device_set_desc_copy(device_t dev, const char* desc);
int device_set_devclass(device_t dev, const char *classname);
int device_set_devclass_fixed(device_t dev, const char *classname);
+bool device_is_devclass_fixed(device_t dev);
int device_set_driver(device_t dev, driver_t *driver);
void device_set_flags(device_t dev, u_int32_t flags);
void device_set_softc(device_t dev, void *softc);
More information about the svn-src-head
mailing list