Skip site navigation (1)Skip section navigation (2) 
Date:      Mon, 19 Nov 2018 18:29:03 +0000 (UTC)
From:      Ben Widawsky <bwidawsk@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r340644 - in head/sys: dev/acpica kern sys
Message-ID:  <201811191829.wAJIT3WS094599@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help 
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@.
  
  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", &params->uid);
-	if (ACPI_FAILURE(status))
-	    params->uid = 0;
-	status = acpi_GetInteger(h, "_GLK", &params->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", &params->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", &params->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, &params->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, &params->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);

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201811191829.wAJIT3WS094599>