Skip to contentSkip to conversation

xen/acpi: off by one in read_acpi_id()

PATCH Superseded Dan Carpenter 4 participants 6 messages Mar 28, 2018 – Mar 31, 2018

Subsystems (2)
Kernel janitors Xen hypervisor interface
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 xen/acpi: off by one in read_acpi_id()

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_cst_present);

Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>
drivers/xen/xen-acpi-processor.c +1 -1
index c80195e8fbd1..d23c9c150199 100644
@@ -364,7 +364,7 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 	/* There are more ACPI Processor objects than in x2APIC or MADT.
 	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id > nr_acpi_bits) {
+	if (acpi_id >= nr_acpi_bits) {
 		pr_debug("We only have %u, trying to set %u\n",
 			 nr_acpi_bits, acpi_id);
 		return AE_OK;
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 12 lines · Dan Carpenter

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_cst_present);

Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>
drivers/xen/xen-acpi-processor.c +0 -0
index c80195e8fbd1..d23c9c150199 100644

drivers/xen/xen-acpi-processor.c read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv) · line 369

Quoted 7 lines
drivers/xen/xen-acpi-processor.c +1 -1
@@ -364,7 +364,7 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 	/* There are more ACPI Processor objects than in x2APIC or MADT.
 	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id > nr_acpi_bits) {
+	if (acpi_id >= nr_acpi_bits) {
 		pr_debug("We only have %u, trying to set %u\n",
 			 nr_acpi_bits, acpi_id);

Can you please modify this message, too? E.g. something like:

pr_debug("max acpi id %u, trying to set %u\n",

         nr_acpi_bits - 1, acpi_id);

With that:

Reviewed-by Juergen Gross <[email protected]>

Juergen

author
#2 lore raw
Quoted 23 lines · Juergen Gross
Quoted 12 lines · Dan Carpenter

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_cst_present);

Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>
drivers/xen/xen-acpi-processor.c +0 -0
index c80195e8fbd1..d23c9c150199 100644

drivers/xen/xen-acpi-processor.c read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv) · line 369

Quoted 7 lines
drivers/xen/xen-acpi-processor.c +1 -1
@@ -364,7 +364,7 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 	/* There are more ACPI Processor objects than in x2APIC or MADT.
 	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id > nr_acpi_bits) {
+	if (acpi_id >= nr_acpi_bits) {
 		pr_debug("We only have %u, trying to set %u\n",
 			 nr_acpi_bits, acpi_id);

Can you please modify this message, too? E.g. something like:

Quoted 7 lines

pr_debug("max acpi id %u, trying to set %u\n",

         nr_acpi_bits - 1, acpi_id);

With that:

Reviewed-by Juergen Gross <[email protected]>

Sure, let me resend.

regards,
dan carpenter

Reviewed-by on v1 1/1
#3 lore raw
In reply to Dan Carpenter
Quoted 4 lines · Dan Carpenter

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_cst_present);

... or even acpi_id_present (which comes right after the condition you're fixing).

Quoted 3 lines
Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>

FWIW,

Reviewed-by Joao Martins <[email protected]>
Quoted 14 lines
drivers/xen/xen-acpi-processor.c +1 -1
index c80195e8fbd1..d23c9c150199 100644
@@ -364,7 +364,7 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 	/* There are more ACPI Processor objects than in x2APIC or MADT.
 	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id > nr_acpi_bits) {
+	if (acpi_id >= nr_acpi_bits) {
 		pr_debug("We only have %u, trying to set %u\n",
 			 nr_acpi_bits, acpi_id);
 		return AE_OK;
author
#4 lore raw

PATCH v2 1/1 xen/acpi: off by one in read_acpi_id()

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_present);

Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Joao Martins <[email protected]>
Reviewed-by Juergen Gross <[email protected]>
drivers/xen/xen-acpi-processor.c +3 -3
index c80195e8fbd1..b29f4e40851f 100644
@@ -364,9 +364,9 @@ read_acpi_id(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 	/* There are more ACPI Processor objects than in x2APIC or MADT.
 	 * This can happen with incorrect ACPI SSDT declerations. */
-	if (acpi_id > nr_acpi_bits) {
-		pr_debug("We only have %u, trying to set %u\n",
-			 nr_acpi_bits, acpi_id);
+	if (acpi_id >= nr_acpi_bits) {
+		pr_debug("max acpi id %u, trying to set %u\n",
+			 nr_acpi_bits - 1, acpi_id);
 		return AE_OK;
 	}
 	/* OK, There is a ACPI Processor object */
Applied
#5 lore raw
Quoted 8 lines · Dan Carpenter

If acpi_id is = nr_acpi_bits, then we access one element beyond the end of the acpi_psd[] array or we set one bit beyond the end of the bit map when we do __set_bit(acpi_id, acpi_id_present);

Fixes 59a568029181 ("xen/acpi-processor: C and P-state driver that uploads said data to hypervisor.")
Signed-off-by Dan Carpenter <[email protected]>
Reviewed-by Joao Martins <[email protected]>
Reviewed-by Juergen Gross <[email protected]>

Applied to for-linus-4.17

-boris