Parameter check for EINJ error injection - V2

Parameter check for EINJ error injection - V2

[Chen Gong]

[PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection
[PATCH 2/2 V2] ACPI/APEI: Update einj documentation for param1/param2

These two patches are employed to enhance the check for param1/param2
when einj is used. Param1/param2 are very subtle for the guy
who lacks of experience. These two patches add some extra checks to
avoid happening further errors to confuse users.

[PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection

[Chen Gong]

When param1 is enabled in EINJ but not assigned with a valid
value, sometimes it will cause the error like below:

APEI: Can not request [mem 0x7aaa7000-0x7aaa7007] for APEI EINJ Trigger
registers

It is because some firmware will access target address specified in
param1 to trigger the error when injecting memory error. This will
cause resource conflict with regular memory. So It must be removed
from trigger table resources, but incorrect param1/param2
combination will stop this action. Add extra check to avoid
this kind of error.

v2 -> v1: update redundant logic following the suggestion from Boris

Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
 drivers/acpi/apei/einj.c |   38 +++++++++++++++++++++++++++++++++++---
 kernel/resource.c        |    1 +
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 8d457b5..841765f 100644
--- a/drivers/acpi/apei/einj.c
+++ b/drivers/acpi/apei/einj.c
@@ -32,6 +32,7 @@
 #include <linux/seq_file.h>
 #include <linux/nmi.h>
 #include <linux/delay.h>
+#include <linux/mm.h>
 #include <acpi/acpi.h>
 
 #include "apei-internal.h"
@@ -41,6 +42,10 @@
 #define SPIN_UNIT		100			/* 100ns */
 /* Firmware should respond within 1 milliseconds */
 #define FIRMWARE_TIMEOUT	(1 * NSEC_PER_MSEC)
+#define ACPI5_VENDOR_BIT	BIT(31)
+#define MEM_ERROR_MASK		(ACPI_EINJ_MEMORY_CORRECTABLE | \
+				ACPI_EINJ_MEMORY_UNCORRECTABLE | \
+				ACPI_EINJ_MEMORY_FATAL)
 
 /*
  * ACPI version 5 provides a SET_ERROR_TYPE_WITH_ADDRESS action.
@@ -367,7 +372,7 @@ static int __einj_error_trigger(u64 trigger_paddr, u32 type,
 	 * This will cause resource conflict with regular memory.  So
 	 * remove it from trigger table resources.
 	 */
-	if ((param_extension || acpi5) && (type & 0x0038) && param2) {
+	if ((param_extension || acpi5) && (type & MEM_ERROR_MASK) && param2) {
 		struct apei_resources addr_resources;
 		apei_resources_init(&addr_resources);
 		trigger_param_region = einj_get_trigger_parameter_region(
@@ -427,7 +432,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
 		struct set_error_type_with_address *v5param = einj_param;
 
 		v5param->type = type;
-		if (type & 0x80000000) {
+		if (type & ACPI5_VENDOR_BIT) {
 			switch (vendor_flags) {
 			case SETWA_FLAGS_APICID:
 				v5param->apicid = param1;
@@ -512,7 +517,34 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
 static int einj_error_inject(u32 type, u64 param1, u64 param2)
 {
 	int rc;
+	unsigned long pfn;
 
+	/* ensure param1/param2 existed */
+	if (!(param_extension || acpi5))
+		goto inject;
+
+	/* ensure injection is memory related */
+	if (type & ACPI5_VENDOR_BIT) {
+		if (vendor_flags != SETWA_FLAGS_MEM)
+			goto inject;
+	} else if (!(type & MEM_ERROR_MASK))
+		goto inject;
+
+	/*
+	 * When error injection type is memory related, param2 is the address
+	 * mask of param1. This mask is used to ensure that the final address
+	 * (param1 & param2) is meaningful. If param2 has a *weird* style
+	 * like 0xf0f0f0f0f0f0f0f0, it means the injection address can be
+	 * anywhere around param1, and that must be forbidden. In that reason,
+	 * PAGE_MASK is employed to avoid injection address discontinuous.
+	 * If one finds a special case not to satisfy this requirement, please
+	 * fix it.
+	 */
+	pfn = PFN_DOWN(param1 & param2);
+	if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
+		return -EINVAL;
+
+inject:
 	mutex_lock(&einj_mutex);
 	rc = __einj_error_inject(type, param1, param2);
 	mutex_unlock(&einj_mutex);
@@ -590,7 +622,7 @@ static int error_type_set(void *data, u64 val)
 	 * Vendor defined types have 0x80000000 bit set, and
 	 * are not enumerated by ACPI_EINJ_GET_ERROR_TYPE
 	 */
-	vendor = val & 0x80000000;
+	vendor = val & ACPI5_VENDOR_BIT;
 	tval = val & 0x7fffffff;
 
 	/* Only one error type can be specified */
diff --git a/kernel/resource.c b/kernel/resource.c
index d738698..77bf11a 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -409,6 +409,7 @@ int __weak page_is_ram(unsigned long pfn)
 {
 	return walk_system_ram_range(pfn, 1, NULL, __is_ram) == 1;
 }
+EXPORT_SYMBOL_GPL(page_is_ram);
 
 void __weak arch_remove_reservations(struct resource *avail)
 {
-- 
1.7.10.4

[PATCH 2/2 V2] ACPI/APEI: Update einj documentation for param1/param2

[Chen Gong]

To ensure EINJ working well when injecting errors via EINJ
table, add some attentions for param1/param2.

Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
---
 Documentation/acpi/apei/einj.txt |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/acpi/apei/einj.txt b/Documentation/acpi/apei/einj.txt
index e20b6da..3b08e62 100644
--- a/Documentation/acpi/apei/einj.txt
+++ b/Documentation/acpi/apei/einj.txt
@@ -47,11 +47,15 @@ directory apei/einj. The following files are provided.
 
 - param1
   This file is used to set the first error parameter value. Effect of
-  parameter depends on error_type specified.
+  parameter depends on error_type specified. For example, if error
+  type is memory related type, the param1 should be a valid physical
+  memory address.
 
 - param2
   This file is used to set the second error parameter value. Effect of
-  parameter depends on error_type specified.
+  parameter depends on error_type specified. For example, if error
+  type is memory related type, the param2 should be a valid physical
+  memory address mask, say, 0xfffffffffffff000.
 
 - notrigger
   The EINJ mechanism is a two step process. First inject the error, then
-- 
1.7.10.4

RE: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection

[Luck, Tony]

+	/* ensure param1/param2 existed */
+	if (!(param_extension || acpi5))
+		goto inject;
+
+	/* ensure injection is memory related */
+	if (type & ACPI5_VENDOR_BIT) {
+		if (vendor_flags != SETWA_FLAGS_MEM)
+			goto inject;
+	} else if (!(type & MEM_ERROR_MASK))
+		goto inject;

Maybe a comment before all these three goto blocks saying why we are jumping
around.  Perhaps:

	/* We need extra sanity checks for memory errors. Other types leap directly to injection */

+
+	/*
+	 * When error injection type is memory related, param2 is the address
+	 * mask of param1. This mask is used to ensure that the final address
+	 * (param1 & param2) is meaningful. If param2 has a *weird* style
+	 * like 0xf0f0f0f0f0f0f0f0, it means the injection address can be
+	 * anywhere around param1, and that must be forbidden. In that reason,
+	 * PAGE_MASK is employed to avoid injection address discontinuous.
+	 * If one finds a special case not to satisfy this requirement, please
+	 * fix it.
+	 */
+	pfn = PFN_DOWN(param1 & param2);
+	if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
+		return -EINVAL;

This has too much comment (rare!) and is still too complicated. Split the tests apart?

	/*
	 * Disallow crazy address masks that give BIOS leeway to pick injection address
	 * almost anywhere. Insist on page or better granularity
	 */
	if ((param2 & PAGE_MASK) != PAGE_MASK)
		return -EINVAL;

	/* make sure target address is normal memory */
	pfn = PFN_DOWN(param1 & param2);
	if (!page_is_ram(pfn))
		return -EINVAL;

Re: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection

[Chen Gong]

[-- Attachment #1: Type: text/plain, Size: 2367 bytes --]

On Tue, May 21, 2013 at 08:19:16PM +0000, Luck, Tony wrote:
> Date: Tue, 21 May 2013 20:19:16 +0000
> From: "Luck, Tony" <tony.luck@intel.com>
> To: Chen Gong <gong.chen@linux.intel.com>, "bp@alien8.de" <bp@alien8.de>
> CC: "linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>
> Subject: RE: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error
>  injection
> 
> +	/* ensure param1/param2 existed */
> +	if (!(param_extension || acpi5))
> +		goto inject;
> +
> +	/* ensure injection is memory related */
> +	if (type & ACPI5_VENDOR_BIT) {
> +		if (vendor_flags != SETWA_FLAGS_MEM)
> +			goto inject;
> +	} else if (!(type & MEM_ERROR_MASK))
> +		goto inject;
> 
> Maybe a comment before all these three goto blocks saying why we are jumping
> around.  Perhaps:
> 
> 	/* We need extra sanity checks for memory errors. Other types leap directly to injection */
> 
Thanks, this comment is great.

> +
> +	/*
> +	 * When error injection type is memory related, param2 is the address
> +	 * mask of param1. This mask is used to ensure that the final address
> +	 * (param1 & param2) is meaningful. If param2 has a *weird* style
> +	 * like 0xf0f0f0f0f0f0f0f0, it means the injection address can be
> +	 * anywhere around param1, and that must be forbidden. In that reason,
> +	 * PAGE_MASK is employed to avoid injection address discontinuous.
> +	 * If one finds a special case not to satisfy this requirement, please
> +	 * fix it.
> +	 */
> +	pfn = PFN_DOWN(param1 & param2);
> +	if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
> +		return -EINVAL;
> 
> This has too much comment (rare!) and is still too complicated. Split the tests apart?

Your comment is great to me. Boris ever mentioned that
"(param2 & PAGE_MASK) != PAGE_MASK)" is not usual, most of situations are
like "(param2 & PAGE_MASK) != param2). So he wants here I can give a clear
explanation for it. Maybe I can move my explanation into patch description.

> 
> 	/*
> 	 * Disallow crazy address masks that give BIOS leeway to pick injection address
> 	 * almost anywhere. Insist on page or better granularity
> 	 */
> 	if ((param2 & PAGE_MASK) != PAGE_MASK)
> 		return -EINVAL;
> 
> 	/* make sure target address is normal memory */
> 	pfn = PFN_DOWN(param1 & param2);
> 	if (!page_is_ram(pfn))
> 		return -EINVAL;
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2 V2] ACPI/APEI: Add parameter check before error injection

[Borislav Petkov]

On Wed, May 22, 2013 at 02:10:29AM -0400, Chen Gong wrote:
> > +	 * When error injection type is memory related, param2 is the address
> > +	 * mask of param1. This mask is used to ensure that the final address
> > +	 * (param1 & param2) is meaningful. If param2 has a *weird* style
> > +	 * like 0xf0f0f0f0f0f0f0f0, it means the injection address can be
> > +	 * anywhere around param1, and that must be forbidden. In that reason,
> > +	 * PAGE_MASK is employed to avoid injection address discontinuous.
> > +	 * If one finds a special case not to satisfy this requirement, please
> > +	 * fix it.
> > +	 */
> > +	pfn = PFN_DOWN(param1 & param2);
> > +	if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != PAGE_MASK))
> > +		return -EINVAL;
> > 
> > This has too much comment (rare!) and is still too complicated. Split the tests apart?
> 
> Your comment is great to me. Boris ever mentioned that
> "(param2 & PAGE_MASK) != PAGE_MASK)" is not usual, most of situations are
> like "(param2 & PAGE_MASK) != param2). So he wants here I can give a clear
> explanation for it. Maybe I can move my explanation into patch description.

No, please not in the patch description as no one would read it there.
You could move it above the function definition. Also, you could
simplify it a bit.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--