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

[Chen Gong <gong.chen@linux.intel.com>]

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

On Thu, May 16, 2013 at 01:05:03PM +0200, Borislav Petkov wrote:
> Date: Thu, 16 May 2013 13:05:03 +0200
> From: Borislav Petkov <bp@alien8.de>
> To: Chen Gong <gong.chen@linux.intel.com>
> Cc: tony.luck@intel.com, linux-acpi@vger.kernel.org
> Subject: Re: [PATCH 1/2] ACPI/APEI: Add parameter check before error
>  injection
> User-Agent: Mutt/1.5.21 (2010-09-15)
> 
> On Wed, May 15, 2013 at 10:29:06PM -0400, Chen Gong wrote:
> > 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 uncorrected param1/param2
> > combination will stop this action. Add extra check to avoid
> > happening this error.
> > 
> > Signed-off-by: Chen Gong <gong.chen@linux.intel.com>
> > ---
> >  drivers/acpi/apei/einj.c |   29 +++++++++++++++++++++++++----
> >  kernel/resource.c        |    1 +
> >  2 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
> > index 8d457b5..015546e 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"
> > @@ -511,11 +512,31 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
> >  /* Inject the specified hardware error */
> >  static int einj_error_inject(u32 type, u64 param1, u64 param2)
> >  {
> > -	int rc;
> > +	int rc = 0;
> > +	unsigned long pfn;
> > +	bool checkparam = false;
> > +	u64 param2_mask = 0xfffffffffffff000;
> >  
> > -	mutex_lock(&einj_mutex);
> > -	rc = __einj_error_inject(type, param1, param2);
> > -	mutex_unlock(&einj_mutex);
> > +	/* ensure param1/param2 existed & injection is memory related */
> > +	if (param_extension || acpi5) {
> > +		if (type & 0x80000000) {
> 
> 		if (type & BIT(31)) {
> 
> better readable.
> 
> even better if you define what bit 31 is
> 
> #define INJ_TYPE BIT(31)
> 
> > +			if (vendor_flags == SETWA_FLAGS_MEM)
> > +				checkparam = true;
> > +		} else if (type & 0x38)
> 
> what is 0x38? Also a macro define with readable name pls.
> 
> > +			checkparam = true;
> > +	}
> > +	if (checkparam) {
> > +		pfn = PFN_DOWN(param1 & param2);
> > +		if (!page_is_ram(pfn) ||
> > +			((param2 & param2_mask) != param2_mask))
> 
> Hmm, shouldn't this be:
> 
> 			(param2 & param2_mask) != param2
> 
> ?
> 
> > +			rc = -EINVAL;
> > +	}
> 
> This checkparam variable looks unneeded to me:
> 
> 		if (type & INJ_TYPE) {
> 			if (vendor_flags == SETWA_FLAGS_MEM || type & 0x38) {
> 				...
> 
> Ok, I went and rewrote the function to show you what I mean. I've
> also reversed the logic to flatten the indentation level (completely
> untested, of course):
> 
> static int einj_error_inject(u32 type, u64 param1, u64 param2)
> {
>        int rc;
>        unsigned long pfn;
> 
>        /* ensure param1/param2 exist and we inject to real memory */
>        if (!(param_extension || acpi5))
> 	       goto inject;
> 
> 	if (type & INJ_TYPE)
> 		if (vendor_flags != SETWA_FLAGS_MEM)
> 			goto inject;
> 	else if (!(type & THREE_BIT_MASK))
> 		goto inject;
> 
> 	pfn = PFN_DOWN(param1 & param2);
> 	if (!page_is_ram(pfn) || ((param2 & PAGE_MASK) != param2))
> 		return -EINVAL;
> 
>  inject:
> 	mutex_lock(&einj_mutex);
> 	rc = __einj_error_inject(type, param1, param2);
> 	mutex_unlock(&einj_mutex);
> 
> 	return rc;
> }
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> Sent from a fat crate under my desk. Formatting is fine.
> --

Hi, Boris

Thanks for your review. I agree with you about readability enhancement.
IMHO, I don't think your attached patch has obvious improvment. I paste
my updated patch here and please continue to review. I will send updated
patch based on your further review.

In this patch, I update previous confused definition at the same time.
They are very little and no ill-efect so I don't write a new patch
for it.

diff --git a/drivers/acpi/apei/einj.c b/drivers/acpi/apei/einj.c
index 8d457b5..dba59be 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,11 @@
 #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 PAGE_MASK              0xfffffffffffff000
+#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 +373,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 +433,7 @@ static int __einj_error_inject(u32 type, u64 param1, u64 param2)
                struct set_error_type_with_address *v5param = einj_param;
tototototoram->type = type;
-               if (type & 0x80000000) {
+               if (type & ACPI5_VENDOR_BIT) {
                        switch (vendor_flags) {
                        case SETWA_FLAGS_APICID:
                                v5param->apicid = param1;
@@ -512,6 +518,25 @@ 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;
+       bool checkparam;
+
+       /* ensure param1/param2 existed & injection is memory related */
+       if (param_extension || acpi5) {
+               checkparam = false;
+               if (type & ACPI5_VENDOR_BIT) {
+                       if (vendor_flags == SETWA_FLAGS_MEM)
+                               checkparam = true;
+               } else if (type & MEM_ERROR_MASK)
+                       checkparam = true;
+
+               if (checkparam) {
+                       pfn = PFN_DOWN(param1 & param2);
+                       if (!page_is_ram(pfn) ||
+                               ((param2 & PAGE_MASK) != PAGE_MASK))
+                               return -EINVAL;
+               }
+       }

        mutex_lock(&einj_mutex);
        rc = __einj_error_inject(type, param1, param2);
@@ -590,7 +615,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)
 {

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