[PATCH 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT

[PATCH 0/3] mailbox: ACPI: Remove incorrect error message about parsing PCCT

[Al Stone]

This set of patches provide some cleanup in ACPI for minor issues
found while correcting a bogus error message (the first two patches),
and the correction for the error message itself (patch 3/3).  Note
that patches 1/3 and 2/3 are not required for 3/3 to work: 1/3 only
changes a comment and 2/3 makes an ACPI table parsing loop a wee bit
more robust.

For patch 3/3, many systems on boot have been reporting "Error parsing
PCC subspaces from PCCT" which turns out to not be an error at all.
The issue is that the probe for ACPI mailboxes defined in the PCCT
(Platform Communications Channel Table) makes a faulty assumption about
the content of the PCCT.  What's more, when the error is reported, no
further PCC mailboxes are set up, even when they have been defined
in the PCCT.  So, in the reported cases, there was no error and the
data in the PCCT was being ignored.  This is described in more detail
in patch 3/3.

Since these patches primarily involve ACPI usages, it may make
sense for all of them to go through the linux-acpi tree; clearly,
this is up to the maintainers, though.


Al Stone (3):
  ACPI: improve function documentation for acpi_parse_entries_array()
  ACPI: ensure acpi_parse_entries_array() does not access non-existent
    table data
  mailbox: ACPI: erroneous error message when parsing the ACPI PCCT

 drivers/acpi/tables.c |  9 ++---
 drivers/mailbox/pcc.c | 99 +++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 78 insertions(+), 30 deletions(-)

-- 
2.14.3

[PATCH 1/3] ACPI: improve function documentation for acpi_parse_entries_array()

[Al Stone]

I found the description of the table_size argument to the function
acpi_parse_entries_array() unclear and ambiguous.  This is a minor
documentation change to improve that description so I don't misuse
the argument again in the future, and it is hopefully clearer to
other future users.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index 6c5ee7e66842..b6ac8162a2c0 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -222,7 +222,9 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header)
  * acpi_parse_entries_array - for each proc_num find a suitable subtable
  *
  * @id: table id (for debugging purposes)
- * @table_size: single entry size
+ * @table_size: size of the root table; i.e., the offset from the very
+ *              first byte of the complete ACPI table, to the first byte
+ *              of the first subtable
  * @table_header: where does the table start?
  * @proc: array of acpi_subtable_proc struct containing entry id
  *        and associated handler with it
@@ -400,7 +402,7 @@ int __init acpi_table_parse(char *id, acpi_tbl_table_handler handler)
 		return -ENODEV;
 }
 
-/* 
+/*
  * The BIOS is supposed to supply a single APIC/MADT,
  * but some report two.  Provide a knob to use either.
  * (don't you wish instance 0 and 1 were not the same?)
-- 
2.14.3

[PATCH 2/3] ACPI: ensure acpi_parse_entries_array() does not access non-existent table data

[Al Stone]

For ACPI tables that have subtables, acpi_parse_entries_array() gets used
to step through each of the subtables in memory.  The primary loop for this
was checking that the beginning location of the subtable being examined
plus the length of struct acpi_subtable_header was not beyond the end of
the complete ACPI table; if it wasn't, the subtable could be examined, but
if it was the loop would terminate as it should.

In the middle of this subtable loop, a callback is used to examine the
subtable in detail.

Should the callback function try to examine elements of the subtable that
are located past the subtable header, and the ACPI table containing this
subtable has an incorrect length, it is possible to access either invalid
or protected memory and cause a fault.  And, the length of struct
acpi_subtable_header will always be smaller than the length of the actual
subtable.

To fix this, we make the main loop check that the beginning of the
subtable being examined plus the actual length of the subtable does
not go past the end of the enclosing ACPI table.  While this cannot
protect us from malicious callback functions, it can prevent us from
failing because of some poorly constructed ACPI tables.

Found by inspection.  There is no functional change to existing code
that is known to work when calling acpi_parse_entries_array().

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/acpi/tables.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c
index b6ac8162a2c0..672a1f22c3a5 100644
--- a/drivers/acpi/tables.c
+++ b/drivers/acpi/tables.c
@@ -271,8 +271,7 @@ acpi_parse_entries_array(char *id, unsigned long table_size,
 	entry = (struct acpi_subtable_header *)
 	    ((unsigned long)table_header + table_size);
 
-	while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
-	       table_end) {
+	while (((unsigned long)entry + entry->length) <= table_end) {
 		if (max_entries && count >= max_entries)
 			break;
 
-- 
2.14.3

[PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT

[Al Stone]

There have been multiple reports of the following error message:

[    0.068293] Error parsing PCC subspaces from PCCT

This error message is not correct.  In multiple cases examined, the PCCT
(Platform Communications Channel Table) concerned is actually properly
constructed; the problem is that acpi_pcc_probe() which reads the PCCT
is making the assumption that the only valid PCCT is one that contains
subtables of one of two types: ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE or
ACPI_PCCT_TYPE_HW_REDUCED_TYPE2.  The number of subtables of these
types are counted and as long as there is at least one of the desired
types, the acpi_pcc_probe() succeeds.  When no subtables of these types
are found, regardless of whether or not any other subtable types are
present, the error mentioned above is reported.

In the cases reported to me personally, the PCCT contains exactly one
subtable of type ACPI_PCCT_TYPE_GENERIC_SUBSPACE.  The function
acpi_pcc_probe() does not count it as a valid subtable, so believes
there to be no valid subtables, and hence outputs the error message.

An example of the PCCT being reported as erroneous yet perfectly fine
is the following:

                    Signature : "PCCT"
                 Table Length : 0000006E
                     Revision : 05
                     Checksum : A9
                       Oem ID : "XXXXXX"
                 Oem Table ID : "XXXXX   "
                 Oem Revision : 00002280
              Asl Compiler ID : "XXXX"
        Asl Compiler Revision : 00000002

        Flags (decoded below) : 00000001
                     Platform : 1
                     Reserved : 0000000000000000

                Subtable Type : 00 [Generic Communications Subspace]
                       Length : 3E

                     Reserved : 000000000000
                 Base Address : 00000000DCE43018
               Address Length : 0000000000001000

            Doorbell Register : [Generic Address Structure]
                     Space ID : 01 [SystemIO]
                    Bit Width : 08
                   Bit Offset : 00
         Encoded Access Width : 01 [Byte Access:8]
                      Address : 0000000000001842

                Preserve Mask : 00000000000000FD
                   Write Mask : 0000000000000002
              Command Latency : 00001388
          Maximum Access Rate : 00000000
      Minimum Turnaround Time : 0000

To fix this, we count up all of the possible subtable types for the
PCCT, and only report an error when there are none (which could mean
either no subtables, or no valid subtables), or there are too many.
We also change the logic so that if there is a valid subtable, we
do try to initialize it per the PCCT subtable contents.  This is a
change in functionality; previously, the probe would have returned
right after the error message and would not have tried to use any
other subtable definition.

Tested on my personal laptop which showed the error previously; the
error message no longer appears and the laptop appears to operate
normally.

Signed-off-by: Al Stone <ahs3@redhat.com>
Cc: Jassi Brar <jassisinghbrar@gmail.com>
Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Len Brown <lenb@kernel.org>
---
 drivers/mailbox/pcc.c | 99 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 26 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 3ef7f036ceea..c6294476acc1 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -372,6 +372,36 @@ static const struct mbox_chan_ops pcc_chan_ops = {
 	.send_data = pcc_send_data,
 };
 
+/*+
+ *
+ * count_pcc_subspaces -- Count the PCC subspaces that are not used in
+ *		reduced hardware systems.
+ * @header: Pointer to the ACPI subtable header under the PCCT.
+ * @end: End of subtable entry.
+ *
+ * Return: 0 for Success, else errno.
+ *
+ * This gets called for each entry in the PCC table.
+ */
+static int count_pcc_subspaces(struct acpi_subtable_header *header,
+		const unsigned long end)
+{
+	struct acpi_pcct_subspace *pcct_ss =
+				(struct acpi_pcct_subspace *) header;
+
+	if ((pcct_ss->header.type >= ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
+	    (pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
+	    (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
+	    (pcct_ss->header.type !=
+				ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
+		pr_warn("PCCT count: useful subtype = %d\n",
+			pcct_ss->header.type);
+		return 0;
+	}
+	pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
+	return -EINVAL;
+}
+
 /**
  * parse_pcc_subspace - Parse the PCC table and verify PCC subspace
  *		entries. There should be one entry per PCC client.
@@ -395,10 +424,14 @@ static int parse_pcc_subspace(struct acpi_subtable_header *header,
 		    && (pcct_ss->header.type !=
 				ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
 			pr_err("Incorrect PCC Subspace type detected\n");
+			pr_warn("PCCT parse: unwanted subtype = %d\n",
+				pcct_ss->header.type);
 			return -EINVAL;
 		}
 	}
 
+	pcct_ss = (struct acpi_pcct_hw_reduced *) header;
+	pr_warn("PCCT parse: useful subtype = %d\n", pcct_ss->header.type);
 	return 0;
 }
 
@@ -449,8 +482,8 @@ static int __init acpi_pcc_probe(void)
 	struct acpi_table_header *pcct_tbl;
 	struct acpi_subtable_header *pcct_entry;
 	struct acpi_table_pcct *acpi_pcct_tbl;
+	struct acpi_subtable_proc proc[ACPI_PCCT_TYPE_RESERVED];
 	int count, i, rc;
-	int sum = 0;
 	acpi_status status = AE_OK;
 
 	/* Search for PCCT */
@@ -459,43 +492,50 @@ static int __init acpi_pcc_probe(void)
 	if (ACPI_FAILURE(status) || !pcct_tbl)
 		return -ENODEV;
 
-	count = acpi_table_parse_entries(ACPI_SIG_PCCT,
-			sizeof(struct acpi_table_pcct),
-			ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE,
-			parse_pcc_subspace, MAX_PCC_SUBSPACES);
-	sum += (count > 0) ? count : 0;
-
-	count = acpi_table_parse_entries(ACPI_SIG_PCCT,
-			sizeof(struct acpi_table_pcct),
-			ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2,
-			parse_pcc_subspace, MAX_PCC_SUBSPACES);
-	sum += (count > 0) ? count : 0;
+	/* Set up the subtable handlers */
+	for (i = ACPI_PCCT_TYPE_GENERIC_SUBSPACE;
+	     i < ACPI_PCCT_TYPE_RESERVED;
+	     i++) {
+		proc[i].id = i;
+		if (i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+		    i == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)
+			proc[i].handler = parse_pcc_subspace;
+		else
+			proc[i].handler = count_pcc_subspaces;
+		proc[i].count = 0;
+	}
 
-	if (sum == 0 || sum >= MAX_PCC_SUBSPACES) {
-		pr_err("Error parsing PCC subspaces from PCCT\n");
+	count = acpi_table_parse_entries_array(ACPI_SIG_PCCT,
+			sizeof(struct acpi_table_pcct), proc,
+			ACPI_PCCT_TYPE_RESERVED, MAX_PCC_SUBSPACES);
+	if (count == 0) {
+		pr_warn("PCCT provided but has no subspaces defined\n");
+		return -EINVAL;
+	} else if (count > MAX_PCC_SUBSPACES) {
+		pr_warn("Too many PCC subspaces defined in PCCT\n");
 		return -EINVAL;
 	}
 
 	pcc_mbox_channels = kzalloc(sizeof(struct mbox_chan) *
-			sum, GFP_KERNEL);
+			count, GFP_KERNEL);
 	if (!pcc_mbox_channels) {
 		pr_err("Could not allocate space for PCC mbox channels\n");
 		return -ENOMEM;
 	}
 
-	pcc_doorbell_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+	pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
 	if (!pcc_doorbell_vaddr) {
 		rc = -ENOMEM;
 		goto err_free_mbox;
 	}
 
-	pcc_doorbell_ack_vaddr = kcalloc(sum, sizeof(void *), GFP_KERNEL);
+	pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
 	if (!pcc_doorbell_ack_vaddr) {
 		rc = -ENOMEM;
 		goto err_free_db_vaddr;
 	}
 
-	pcc_doorbell_irq = kcalloc(sum, sizeof(int), GFP_KERNEL);
+	pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL);
 	if (!pcc_doorbell_irq) {
 		rc = -ENOMEM;
 		goto err_free_db_ack_vaddr;
@@ -509,18 +549,25 @@ static int __init acpi_pcc_probe(void)
 	if (acpi_pcct_tbl->flags & ACPI_PCCT_DOORBELL)
 		pcc_mbox_ctrl.txdone_irq = true;
 
-	for (i = 0; i < sum; i++) {
+	for (i = 0; i < count; i++) {
 		struct acpi_generic_address *db_reg;
-		struct acpi_pcct_hw_reduced *pcct_ss;
+		struct acpi_pcct_subspace *pcct_ss;
 		pcc_mbox_channels[i].con_priv = pcct_entry;
 
-		pcct_ss = (struct acpi_pcct_hw_reduced *) pcct_entry;
+		if (pcct_entry->type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
+		    pcct_entry->type ==
+				ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
+			struct acpi_pcct_hw_reduced *pcct_hrss;
+
+			pcct_hrss = (struct acpi_pcct_hw_reduced *) pcct_entry;
 
-		if (pcc_mbox_ctrl.txdone_irq) {
-			rc = pcc_parse_subspace_irq(i, pcct_ss);
-			if (rc < 0)
-				goto err;
+			if (pcc_mbox_ctrl.txdone_irq) {
+				rc = pcc_parse_subspace_irq(i, pcct_hrss);
+				if (rc < 0)
+					goto err;
+			}
 		}
+		pcct_ss = (struct acpi_pcct_subspace *) pcct_entry;
 
 		/* If doorbell is in system memory cache the virt address */
 		db_reg = &pcct_ss->doorbell_register;
@@ -531,7 +578,7 @@ static int __init acpi_pcc_probe(void)
 			((unsigned long) pcct_entry + pcct_entry->length);
 	}
 
-	pcc_mbox_ctrl.num_chans = sum;
+	pcc_mbox_ctrl.num_chans = count;
 
 	pr_info("Detected %d PCC Subspaces\n", pcc_mbox_ctrl.num_chans);
 
-- 
2.14.3

Re: [PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT

[kbuild test robot]

Hi Al,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.16 next-20180404]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Al-Stone/ACPI-improve-function-documentation-for-acpi_parse_entries_array/20180404-151910
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next

smatch warnings:
drivers/mailbox/pcc.c:392 count_pcc_subspaces() warn: always true condition '(pcct_ss->header.type >= 0) => (0-255 >= 0)'

vim +392 drivers/mailbox/pcc.c

   374	
   375	/*+
   376	 *
   377	 * count_pcc_subspaces -- Count the PCC subspaces that are not used in
   378	 *		reduced hardware systems.
   379	 * @header: Pointer to the ACPI subtable header under the PCCT.
   380	 * @end: End of subtable entry.
   381	 *
   382	 * Return: 0 for Success, else errno.
   383	 *
   384	 * This gets called for each entry in the PCC table.
   385	 */
   386	static int count_pcc_subspaces(struct acpi_subtable_header *header,
   387			const unsigned long end)
   388	{
   389		struct acpi_pcct_subspace *pcct_ss =
   390					(struct acpi_pcct_subspace *) header;
   391	
 > 392		if ((pcct_ss->header.type >= ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
   393		    (pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
   394		    (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
   395		    (pcct_ss->header.type !=
   396					ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {
   397			pr_warn("PCCT count: useful subtype = %d\n",
   398				pcct_ss->header.type);
   399			return 0;
   400		}
   401		pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
   402		return -EINVAL;
   403	}
   404	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 3/3] mailbox: ACPI: erroneous error message when parsing the ACPI PCCT

[Al Stone]

On 04/04/2018 02:39 PM, kbuild test robot wrote:
> Hi Al,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on pm/linux-next]
> [also build test WARNING on v4.16 next-20180404]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Al-Stone/ACPI-improve-function-documentation-for-acpi_parse_entries_array/20180404-151910
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
> 
> smatch warnings:
> drivers/mailbox/pcc.c:392 count_pcc_subspaces() warn: always true condition '(pcct_ss->header.type >= 0) => (0-255 >= 0)'
> 
> vim +392 drivers/mailbox/pcc.c
> 
>    374	
>    375	/*+
>    376	 *
>    377	 * count_pcc_subspaces -- Count the PCC subspaces that are not used in
>    378	 *		reduced hardware systems.
>    379	 * @header: Pointer to the ACPI subtable header under the PCCT.
>    380	 * @end: End of subtable entry.
>    381	 *
>    382	 * Return: 0 for Success, else errno.
>    383	 *
>    384	 * This gets called for each entry in the PCC table.
>    385	 */
>    386	static int count_pcc_subspaces(struct acpi_subtable_header *header,
>    387			const unsigned long end)
>    388	{
>    389		struct acpi_pcct_subspace *pcct_ss =
>    390					(struct acpi_pcct_subspace *) header;
>    391	
>  > 392		if ((pcct_ss->header.type >= ACPI_PCCT_TYPE_GENERIC_SUBSPACE) &&
>    393		    (pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
>    394		    (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
>    395		    (pcct_ss->header.type !=
>    396					ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {

Aha.  Yup, can be rewritten as:

	if ((pcct_ss->header.type <= ACPI_PCCT_TYPE_RESERVED) &&
	    (pcct_ss->header.type != ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE) &&
	    (pcct_ss->header.type !=
				ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2)) {

I will let this patch sit for a little while to see if there are any further
comments before sending out a v2.

>    397			pr_warn("PCCT count: useful subtype = %d\n",
>    398				pcct_ss->header.type);
>    399			return 0;
>    400		}
>    401		pr_warn("PCCT count: unwanted subtype = %d\n", pcct_ss->header.type);
>    402		return -EINVAL;
>    403	}
>    404	
> 
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
> 


-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------