[PATCH] ACPI/ERST: fix signed/unsigned type conflicts

[PATCH] ACPI/ERST: fix signed/unsigned type conflicts

[Jan Beulich]

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

Error checks exist in the respective code path that expect negative
values to indicate errors, yet negative values can't be communicated
through size_t. Thus ssize_t needs to be introduced (also on ARM for
consistency, even if the code in question isn't currently being used
on there).

The bug is theoretical only in so far as all the involved code is
effectively dead. Reflect this by excluding that code from non-debug
builds.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -89,10 +89,12 @@ int apei_write_mce(struct mce *m)
 	return erst_write(&rcd.hdr);
 }
 
-size_t apei_read_mce(struct mce *m, u64 *record_id)
+#ifndef NDEBUG /* currently dead code */
+
+ssize_t apei_read_mce(struct mce *m, u64 *record_id)
 {
 	struct cper_mce_record rcd;
-	size_t len;
+	ssize_t len;
 
 	if (!m || !record_id)
 		return -EINVAL;
@@ -115,12 +117,14 @@ size_t apei_read_mce(struct mce *m, u64 
 }
 
 /* Check whether there is record in ERST */
-int apei_check_mce(void)
+bool_t apei_check_mce(void)
 {
-	return erst_get_record_count();
+	return erst_get_record_count() > 0;
 }
 
 int apei_clear_mce(u64 record_id)
 {
 	return erst_clear(record_id);
 }
+
+#endif /* currently dead code */
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -487,6 +487,9 @@ void __init init_constructors(void)
     const ctor_func_t *f;
     for ( f = __ctors_start; f < __ctors_end; ++f )
         (*f)();
+
+    /* Putting this here seems as good (or bad) as any other place. */
+    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
 }
 
 /*
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -383,21 +383,24 @@ static int erst_get_erange(struct erst_e
 	return 0;
 }
 
-static size_t __erst_get_record_count(void)
+static ssize_t __erst_get_record_count(void)
 {
 	struct apei_exec_context ctx;
 	int rc;
+	u64 output;
+	ssize_t count;
 
 	erst_exec_ctx_init(&ctx);
 	rc = apei_exec_run(&ctx, ACPI_ERST_GET_RECORD_COUNT);
 	if (rc)
 		return rc;
-	return apei_exec_ctx_get_output(&ctx);
+	count = output = apei_exec_ctx_get_output(&ctx);
+	return count >= 0 && count == output ? count : -ERANGE;
 }
 
-size_t erst_get_record_count(void)
+ssize_t erst_get_record_count(void)
 {
-	size_t count;
+	ssize_t count;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -483,6 +486,8 @@ static int __erst_write_to_storage(u64 o
 	return erst_errno(val);
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_from_storage(u64 record_id, u64 offset)
 {
 	struct apei_exec_context ctx;
@@ -565,6 +570,8 @@ static int __erst_clear_from_storage(u64
 	return erst_errno(val);
 }
 
+#endif /* currently dead code */
+
 /* NVRAM ERST Error Log Address Range is not supported yet */
 static int __erst_write_to_nvram(const struct cper_record_header *record)
 {
@@ -572,6 +579,8 @@ static int __erst_write_to_nvram(const s
 	return -ENOSYS;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)
 {
 	printk(KERN_WARNING
@@ -586,6 +595,8 @@ static int __erst_clear_from_nvram(u64 r
 	return -ENOSYS;
 }
 
+#endif /* currently dead code */
+
 int erst_write(const struct cper_record_header *record)
 {
 	int rc;
@@ -625,6 +636,8 @@ int erst_write(const struct cper_record_
 	return rc;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange(u64 record_id, u64 *offset)
 {
 	int rc;
@@ -641,22 +654,24 @@ static int __erst_read_to_erange(u64 rec
 	return 0;
 }
 
-static size_t __erst_read(u64 record_id, struct cper_record_header *record,
+static ssize_t __erst_read(u64 record_id, struct cper_record_header *record,
 			   size_t buflen)
 {
 	int rc;
-	u64 offset, len = 0;
+	u64 offset;
+	ssize_t len;
 	struct cper_record_header *rcd_tmp;
 
 	rc = __erst_read_to_erange(record_id, &offset);
 	if (rc)
 		return rc;
 	rcd_tmp = erst_erange.vaddr + offset;
+	if (rcd_tmp->record_length > buflen)
+		return -ENOBUFS;
 	len = rcd_tmp->record_length;
-	if (len <= buflen)
-		memcpy(record, rcd_tmp, len);
+	memcpy(record, rcd_tmp, len);
 
-	return len;
+	return len >= 0 ? len : -ERANGE;
 }
 
 /*
@@ -664,10 +679,10 @@ static size_t __erst_read(u64 record_id,
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen)
 {
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -685,10 +700,10 @@ size_t erst_read(u64 record_id, struct c
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read_next(struct cper_record_header *record, size_t buflen)
+ssize_t erst_read_next(struct cper_record_header *record, size_t buflen)
 {
 	int rc;
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 	u64 record_id;
 
@@ -731,6 +746,8 @@ int erst_clear(u64 record_id)
 	return rc;
 }
 
+#endif /* currently dead code */
+
 static int __init erst_check_table(struct acpi_table_erst *erst_tab)
 {
 	if (erst_tab->header.length < sizeof(*erst_tab))
--- a/xen/include/acpi/apei.h
+++ b/xen/include/acpi/apei.h
@@ -13,11 +13,11 @@
 #define FIX_APEI_RANGE_MAX 64
 
 int erst_write(const struct cper_record_header *record);
-size_t erst_get_record_count(void);
+ssize_t erst_get_record_count(void);
 int erst_get_next_record_id(u64 *record_id);
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen);
-size_t erst_read_next(struct cper_record_header *record, size_t buflen);
+ssize_t erst_read_next(struct cper_record_header *record, size_t buflen);
 int erst_clear(u64 record_id);
 
 void __iomem *apei_pre_map(paddr_t paddr, unsigned long size);
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -56,6 +56,7 @@ typedef u64 register_t;
 #endif
 
 typedef unsigned long size_t;
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)
--- a/xen/include/asm-x86/types.h
+++ b/xen/include/asm-x86/types.h
@@ -39,6 +39,7 @@ typedef __SIZE_TYPE__ size_t;
 #else
 typedef unsigned long size_t;
 #endif
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)



[-- Attachment #2: ACPI-ERST-types.patch --]
[-- Type: text/plain, Size: 6721 bytes --]

ACPI/ERST: fix signed/unsigned type conflicts

Error checks exist in the respective code path that expect negative
values to indicate errors, yet negative values can't be communicated
through size_t. Thus ssize_t needs to be introduced (also on ARM for
consistency, even if the code in question isn't currently being used
on there).

The bug is theoretical only in so far as all the involved code is
effectively dead. Reflect this by excluding that code from non-debug
builds.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -89,10 +89,12 @@ int apei_write_mce(struct mce *m)
 	return erst_write(&rcd.hdr);
 }
 
-size_t apei_read_mce(struct mce *m, u64 *record_id)
+#ifndef NDEBUG /* currently dead code */
+
+ssize_t apei_read_mce(struct mce *m, u64 *record_id)
 {
 	struct cper_mce_record rcd;
-	size_t len;
+	ssize_t len;
 
 	if (!m || !record_id)
 		return -EINVAL;
@@ -115,12 +117,14 @@ size_t apei_read_mce(struct mce *m, u64 
 }
 
 /* Check whether there is record in ERST */
-int apei_check_mce(void)
+bool_t apei_check_mce(void)
 {
-	return erst_get_record_count();
+	return erst_get_record_count() > 0;
 }
 
 int apei_clear_mce(u64 record_id)
 {
 	return erst_clear(record_id);
 }
+
+#endif /* currently dead code */
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -487,6 +487,9 @@ void __init init_constructors(void)
     const ctor_func_t *f;
     for ( f = __ctors_start; f < __ctors_end; ++f )
         (*f)();
+
+    /* Putting this here seems as good (or bad) as any other place. */
+    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
 }
 
 /*
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -383,21 +383,24 @@ static int erst_get_erange(struct erst_e
 	return 0;
 }
 
-static size_t __erst_get_record_count(void)
+static ssize_t __erst_get_record_count(void)
 {
 	struct apei_exec_context ctx;
 	int rc;
+	u64 output;
+	ssize_t count;
 
 	erst_exec_ctx_init(&ctx);
 	rc = apei_exec_run(&ctx, ACPI_ERST_GET_RECORD_COUNT);
 	if (rc)
 		return rc;
-	return apei_exec_ctx_get_output(&ctx);
+	count = output = apei_exec_ctx_get_output(&ctx);
+	return count >= 0 && count == output ? count : -ERANGE;
 }
 
-size_t erst_get_record_count(void)
+ssize_t erst_get_record_count(void)
 {
-	size_t count;
+	ssize_t count;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -483,6 +486,8 @@ static int __erst_write_to_storage(u64 o
 	return erst_errno(val);
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_from_storage(u64 record_id, u64 offset)
 {
 	struct apei_exec_context ctx;
@@ -565,6 +570,8 @@ static int __erst_clear_from_storage(u64
 	return erst_errno(val);
 }
 
+#endif /* currently dead code */
+
 /* NVRAM ERST Error Log Address Range is not supported yet */
 static int __erst_write_to_nvram(const struct cper_record_header *record)
 {
@@ -572,6 +579,8 @@ static int __erst_write_to_nvram(const s
 	return -ENOSYS;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)
 {
 	printk(KERN_WARNING
@@ -586,6 +595,8 @@ static int __erst_clear_from_nvram(u64 r
 	return -ENOSYS;
 }
 
+#endif /* currently dead code */
+
 int erst_write(const struct cper_record_header *record)
 {
 	int rc;
@@ -625,6 +636,8 @@ int erst_write(const struct cper_record_
 	return rc;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange(u64 record_id, u64 *offset)
 {
 	int rc;
@@ -641,22 +654,24 @@ static int __erst_read_to_erange(u64 rec
 	return 0;
 }
 
-static size_t __erst_read(u64 record_id, struct cper_record_header *record,
+static ssize_t __erst_read(u64 record_id, struct cper_record_header *record,
 			   size_t buflen)
 {
 	int rc;
-	u64 offset, len = 0;
+	u64 offset;
+	ssize_t len;
 	struct cper_record_header *rcd_tmp;
 
 	rc = __erst_read_to_erange(record_id, &offset);
 	if (rc)
 		return rc;
 	rcd_tmp = erst_erange.vaddr + offset;
+	if (rcd_tmp->record_length > buflen)
+		return -ENOBUFS;
 	len = rcd_tmp->record_length;
-	if (len <= buflen)
-		memcpy(record, rcd_tmp, len);
+	memcpy(record, rcd_tmp, len);
 
-	return len;
+	return len >= 0 ? len : -ERANGE;
 }
 
 /*
@@ -664,10 +679,10 @@ static size_t __erst_read(u64 record_id,
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen)
 {
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -685,10 +700,10 @@ size_t erst_read(u64 record_id, struct c
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read_next(struct cper_record_header *record, size_t buflen)
+ssize_t erst_read_next(struct cper_record_header *record, size_t buflen)
 {
 	int rc;
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 	u64 record_id;
 
@@ -731,6 +746,8 @@ int erst_clear(u64 record_id)
 	return rc;
 }
 
+#endif /* currently dead code */
+
 static int __init erst_check_table(struct acpi_table_erst *erst_tab)
 {
 	if (erst_tab->header.length < sizeof(*erst_tab))
--- a/xen/include/acpi/apei.h
+++ b/xen/include/acpi/apei.h
@@ -13,11 +13,11 @@
 #define FIX_APEI_RANGE_MAX 64
 
 int erst_write(const struct cper_record_header *record);
-size_t erst_get_record_count(void);
+ssize_t erst_get_record_count(void);
 int erst_get_next_record_id(u64 *record_id);
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen);
-size_t erst_read_next(struct cper_record_header *record, size_t buflen);
+ssize_t erst_read_next(struct cper_record_header *record, size_t buflen);
 int erst_clear(u64 record_id);
 
 void __iomem *apei_pre_map(paddr_t paddr, unsigned long size);
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -56,6 +56,7 @@ typedef u64 register_t;
 #endif
 
 typedef unsigned long size_t;
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)
--- a/xen/include/asm-x86/types.h
+++ b/xen/include/asm-x86/types.h
@@ -39,6 +39,7 @@ typedef __SIZE_TYPE__ size_t;
 #else
 typedef unsigned long size_t;
 #endif
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ACPI/ERST: fix signed/unsigned type conflicts

[Egger, Christoph]

On 14.04.14 12:19, Jan Beulich wrote:
> Error checks exist in the respective code path that expect negative
> values to indicate errors, yet negative values can't be communicated
> through size_t. Thus ssize_t needs to be introduced (also on ARM for
> consistency, even if the code in question isn't currently being used
> on there).
> 
> The bug is theoretical only in so far as all the involved code is
> effectively dead. Reflect this by excluding that code from non-debug
> builds.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Full ok with me.

Reviewed-by: Christoph Egger <chegger@amazon.de>

> 
> --- a/xen/arch/x86/cpu/mcheck/mce-apei.c
> +++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
> @@ -89,10 +89,12 @@ int apei_write_mce(struct mce *m)
>  	return erst_write(&rcd.hdr);
>  }
>  
> -size_t apei_read_mce(struct mce *m, u64 *record_id)
> +#ifndef NDEBUG /* currently dead code */
> +
> +ssize_t apei_read_mce(struct mce *m, u64 *record_id)
>  {
>  	struct cper_mce_record rcd;
> -	size_t len;
> +	ssize_t len;
>  
>  	if (!m || !record_id)
>  		return -EINVAL;
> @@ -115,12 +117,14 @@ size_t apei_read_mce(struct mce *m, u64 
>  }
>  
>  /* Check whether there is record in ERST */
> -int apei_check_mce(void)
> +bool_t apei_check_mce(void)
>  {
> -	return erst_get_record_count();
> +	return erst_get_record_count() > 0;
>  }
>  
>  int apei_clear_mce(u64 record_id)
>  {
>  	return erst_clear(record_id);
>  }
> +
> +#endif /* currently dead code */
> --- a/xen/common/lib.c
> +++ b/xen/common/lib.c
> @@ -487,6 +487,9 @@ void __init init_constructors(void)
>      const ctor_func_t *f;
>      for ( f = __ctors_start; f < __ctors_end; ++f )
>          (*f)();
> +
> +    /* Putting this here seems as good (or bad) as any other place. */
> +    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
>  }
>  
>  /*
> --- a/xen/drivers/acpi/apei/erst.c
> +++ b/xen/drivers/acpi/apei/erst.c
> @@ -383,21 +383,24 @@ static int erst_get_erange(struct erst_e
>  	return 0;
>  }
>  
> -static size_t __erst_get_record_count(void)
> +static ssize_t __erst_get_record_count(void)
>  {
>  	struct apei_exec_context ctx;
>  	int rc;
> +	u64 output;
> +	ssize_t count;
>  
>  	erst_exec_ctx_init(&ctx);
>  	rc = apei_exec_run(&ctx, ACPI_ERST_GET_RECORD_COUNT);
>  	if (rc)
>  		return rc;
> -	return apei_exec_ctx_get_output(&ctx);
> +	count = output = apei_exec_ctx_get_output(&ctx);
> +	return count >= 0 && count == output ? count : -ERANGE;
>  }
>  
> -size_t erst_get_record_count(void)
> +ssize_t erst_get_record_count(void)
>  {
> -	size_t count;
> +	ssize_t count;
>  	unsigned long flags;
>  
>  	if (!erst_enabled)
> @@ -483,6 +486,8 @@ static int __erst_write_to_storage(u64 o
>  	return erst_errno(val);
>  }
>  
> +#ifndef NDEBUG /* currently dead code */
> +
>  static int __erst_read_from_storage(u64 record_id, u64 offset)
>  {
>  	struct apei_exec_context ctx;
> @@ -565,6 +570,8 @@ static int __erst_clear_from_storage(u64
>  	return erst_errno(val);
>  }
>  
> +#endif /* currently dead code */
> +
>  /* NVRAM ERST Error Log Address Range is not supported yet */
>  static int __erst_write_to_nvram(const struct cper_record_header *record)
>  {
> @@ -572,6 +579,8 @@ static int __erst_write_to_nvram(const s
>  	return -ENOSYS;
>  }
>  
> +#ifndef NDEBUG /* currently dead code */
> +
>  static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)
>  {
>  	printk(KERN_WARNING
> @@ -586,6 +595,8 @@ static int __erst_clear_from_nvram(u64 r
>  	return -ENOSYS;
>  }
>  
> +#endif /* currently dead code */
> +
>  int erst_write(const struct cper_record_header *record)
>  {
>  	int rc;
> @@ -625,6 +636,8 @@ int erst_write(const struct cper_record_
>  	return rc;
>  }
>  
> +#ifndef NDEBUG /* currently dead code */
> +
>  static int __erst_read_to_erange(u64 record_id, u64 *offset)
>  {
>  	int rc;
> @@ -641,22 +654,24 @@ static int __erst_read_to_erange(u64 rec
>  	return 0;
>  }
>  
> -static size_t __erst_read(u64 record_id, struct cper_record_header *record,
> +static ssize_t __erst_read(u64 record_id, struct cper_record_header *record,
>  			   size_t buflen)
>  {
>  	int rc;
> -	u64 offset, len = 0;
> +	u64 offset;
> +	ssize_t len;
>  	struct cper_record_header *rcd_tmp;
>  
>  	rc = __erst_read_to_erange(record_id, &offset);
>  	if (rc)
>  		return rc;
>  	rcd_tmp = erst_erange.vaddr + offset;
> +	if (rcd_tmp->record_length > buflen)
> +		return -ENOBUFS;
>  	len = rcd_tmp->record_length;
> -	if (len <= buflen)
> -		memcpy(record, rcd_tmp, len);
> +	memcpy(record, rcd_tmp, len);
>  
> -	return len;
> +	return len >= 0 ? len : -ERANGE;
>  }
>  
>  /*
> @@ -664,10 +679,10 @@ static size_t __erst_read(u64 record_id,
>   * else if return value < 0, something goes wrong,
>   * else everything is OK, and return value is record length
>   */
> -size_t erst_read(u64 record_id, struct cper_record_header *record,
> +ssize_t erst_read(u64 record_id, struct cper_record_header *record,
>  		  size_t buflen)
>  {
> -	size_t len;
> +	ssize_t len;
>  	unsigned long flags;
>  
>  	if (!erst_enabled)
> @@ -685,10 +700,10 @@ size_t erst_read(u64 record_id, struct c
>   * else if return value < 0, something goes wrong,
>   * else everything is OK, and return value is record length
>   */
> -size_t erst_read_next(struct cper_record_header *record, size_t buflen)
> +ssize_t erst_read_next(struct cper_record_header *record, size_t buflen)
>  {
>  	int rc;
> -	size_t len;
> +	ssize_t len;
>  	unsigned long flags;
>  	u64 record_id;
>  
> @@ -731,6 +746,8 @@ int erst_clear(u64 record_id)
>  	return rc;
>  }
>  
> +#endif /* currently dead code */
> +
>  static int __init erst_check_table(struct acpi_table_erst *erst_tab)
>  {
>  	if (erst_tab->header.length < sizeof(*erst_tab))
> --- a/xen/include/acpi/apei.h
> +++ b/xen/include/acpi/apei.h
> @@ -13,11 +13,11 @@
>  #define FIX_APEI_RANGE_MAX 64
>  
>  int erst_write(const struct cper_record_header *record);
> -size_t erst_get_record_count(void);
> +ssize_t erst_get_record_count(void);
>  int erst_get_next_record_id(u64 *record_id);
> -size_t erst_read(u64 record_id, struct cper_record_header *record,
> +ssize_t erst_read(u64 record_id, struct cper_record_header *record,
>  		  size_t buflen);
> -size_t erst_read_next(struct cper_record_header *record, size_t buflen);
> +ssize_t erst_read_next(struct cper_record_header *record, size_t buflen);
>  int erst_clear(u64 record_id);
>  
>  void __iomem *apei_pre_map(paddr_t paddr, unsigned long size);
> --- a/xen/include/asm-arm/types.h
> +++ b/xen/include/asm-arm/types.h
> @@ -56,6 +56,7 @@ typedef u64 register_t;
>  #endif
>  
>  typedef unsigned long size_t;
> +typedef signed long ssize_t;
>  
>  typedef char bool_t;
>  #define test_and_set_bool(b)   xchg(&(b), 1)
> --- a/xen/include/asm-x86/types.h
> +++ b/xen/include/asm-x86/types.h
> @@ -39,6 +39,7 @@ typedef __SIZE_TYPE__ size_t;
>  #else
>  typedef unsigned long size_t;
>  #endif
> +typedef signed long ssize_t;
>  
>  typedef char bool_t;
>  #define test_and_set_bool(b)   xchg(&(b), 1)
> 
> 

答复: [PATCH] ACPI/ERST: fix signed/unsigned type conflicts

[刘劲松(凯耳)]

Fine to me, except I'm not sure the BUILD_BUG_ON at init_constructors() of
lib.c

Thanks,
Jinsong

-----邮件原件-----
发件人: Jan Beulich [mailto:JBeulich@suse.com] 
发送时间: 2014年4月14日 18:19
收件人: xen-devel
抄送: 刘劲松(凯耳); Christoph Egger; Ian Campbell; Stefano Stabellini; Keir
Fraser; Tim Deegan
主题: [PATCH] ACPI/ERST: fix signed/unsigned type conflicts

Error checks exist in the respective code path that expect negative values
to indicate errors, yet negative values can't be communicated through
size_t. Thus ssize_t needs to be introduced (also on ARM for consistency,
even if the code in question isn't currently being used on there).

The bug is theoretical only in so far as all the involved code is
effectively dead. Reflect this by excluding that code from non-debug builds.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/cpu/mcheck/mce-apei.c
+++ b/xen/arch/x86/cpu/mcheck/mce-apei.c
@@ -89,10 +89,12 @@ int apei_write_mce(struct mce *m)
 	return erst_write(&rcd.hdr);
 }
 
-size_t apei_read_mce(struct mce *m, u64 *record_id)
+#ifndef NDEBUG /* currently dead code */
+
+ssize_t apei_read_mce(struct mce *m, u64 *record_id)
 {
 	struct cper_mce_record rcd;
-	size_t len;
+	ssize_t len;
 
 	if (!m || !record_id)
 		return -EINVAL;
@@ -115,12 +117,14 @@ size_t apei_read_mce(struct mce *m, u64  }
 
 /* Check whether there is record in ERST */ -int apei_check_mce(void)
+bool_t apei_check_mce(void)
 {
-	return erst_get_record_count();
+	return erst_get_record_count() > 0;
 }
 
 int apei_clear_mce(u64 record_id)
 {
 	return erst_clear(record_id);
 }
+
+#endif /* currently dead code */
--- a/xen/common/lib.c
+++ b/xen/common/lib.c
@@ -487,6 +487,9 @@ void __init init_constructors(void)
     const ctor_func_t *f;
     for ( f = __ctors_start; f < __ctors_end; ++f )
         (*f)();
+
+    /* Putting this here seems as good (or bad) as any other place. */
+    BUILD_BUG_ON(sizeof(size_t) != sizeof(ssize_t));
 }
 
 /*
--- a/xen/drivers/acpi/apei/erst.c
+++ b/xen/drivers/acpi/apei/erst.c
@@ -383,21 +383,24 @@ static int erst_get_erange(struct erst_e
 	return 0;
 }
 
-static size_t __erst_get_record_count(void)
+static ssize_t __erst_get_record_count(void)
 {
 	struct apei_exec_context ctx;
 	int rc;
+	u64 output;
+	ssize_t count;
 
 	erst_exec_ctx_init(&ctx);
 	rc = apei_exec_run(&ctx, ACPI_ERST_GET_RECORD_COUNT);
 	if (rc)
 		return rc;
-	return apei_exec_ctx_get_output(&ctx);
+	count = output = apei_exec_ctx_get_output(&ctx);
+	return count >= 0 && count == output ? count : -ERANGE;
 }
 
-size_t erst_get_record_count(void)
+ssize_t erst_get_record_count(void)
 {
-	size_t count;
+	ssize_t count;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -483,6 +486,8 @@ static int __erst_write_to_storage(u64 o
 	return erst_errno(val);
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_from_storage(u64 record_id, u64 offset)  {
 	struct apei_exec_context ctx;
@@ -565,6 +570,8 @@ static int __erst_clear_from_storage(u64
 	return erst_errno(val);
 }
 
+#endif /* currently dead code */
+
 /* NVRAM ERST Error Log Address Range is not supported yet */  static int
__erst_write_to_nvram(const struct cper_record_header *record)  { @@ -572,6
+579,8 @@ static int __erst_write_to_nvram(const s
 	return -ENOSYS;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange_from_nvram(u64 record_id, u64 *offset)  {
 	printk(KERN_WARNING
@@ -586,6 +595,8 @@ static int __erst_clear_from_nvram(u64 r
 	return -ENOSYS;
 }
 
+#endif /* currently dead code */
+
 int erst_write(const struct cper_record_header *record)  {
 	int rc;
@@ -625,6 +636,8 @@ int erst_write(const struct cper_record_
 	return rc;
 }
 
+#ifndef NDEBUG /* currently dead code */
+
 static int __erst_read_to_erange(u64 record_id, u64 *offset)  {
 	int rc;
@@ -641,22 +654,24 @@ static int __erst_read_to_erange(u64 rec
 	return 0;
 }
 
-static size_t __erst_read(u64 record_id, struct cper_record_header *record,
+static ssize_t __erst_read(u64 record_id, struct cper_record_header 
+*record,
 			   size_t buflen)
 {
 	int rc;
-	u64 offset, len = 0;
+	u64 offset;
+	ssize_t len;
 	struct cper_record_header *rcd_tmp;
 
 	rc = __erst_read_to_erange(record_id, &offset);
 	if (rc)
 		return rc;
 	rcd_tmp = erst_erange.vaddr + offset;
+	if (rcd_tmp->record_length > buflen)
+		return -ENOBUFS;
 	len = rcd_tmp->record_length;
-	if (len <= buflen)
-		memcpy(record, rcd_tmp, len);
+	memcpy(record, rcd_tmp, len);
 
-	return len;
+	return len >= 0 ? len : -ERANGE;
 }
 
 /*
@@ -664,10 +679,10 @@ static size_t __erst_read(u64 record_id,
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read(u64 record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen)
 {
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 
 	if (!erst_enabled)
@@ -685,10 +700,10 @@ size_t erst_read(u64 record_id, struct c
  * else if return value < 0, something goes wrong,
  * else everything is OK, and return value is record length
  */
-size_t erst_read_next(struct cper_record_header *record, size_t buflen)
+ssize_t erst_read_next(struct cper_record_header *record, size_t 
+buflen)
 {
 	int rc;
-	size_t len;
+	ssize_t len;
 	unsigned long flags;
 	u64 record_id;
 
@@ -731,6 +746,8 @@ int erst_clear(u64 record_id)
 	return rc;
 }
 
+#endif /* currently dead code */
+
 static int __init erst_check_table(struct acpi_table_erst *erst_tab)  {
 	if (erst_tab->header.length < sizeof(*erst_tab))
--- a/xen/include/acpi/apei.h
+++ b/xen/include/acpi/apei.h
@@ -13,11 +13,11 @@
 #define FIX_APEI_RANGE_MAX 64
 
 int erst_write(const struct cper_record_header *record); -size_t
erst_get_record_count(void);
+ssize_t erst_get_record_count(void);
 int erst_get_next_record_id(u64 *record_id); -size_t erst_read(u64
record_id, struct cper_record_header *record,
+ssize_t erst_read(u64 record_id, struct cper_record_header *record,
 		  size_t buflen);
-size_t erst_read_next(struct cper_record_header *record, size_t buflen);
+ssize_t erst_read_next(struct cper_record_header *record, size_t 
+buflen);
 int erst_clear(u64 record_id);
 
 void __iomem *apei_pre_map(paddr_t paddr, unsigned long size);
--- a/xen/include/asm-arm/types.h
+++ b/xen/include/asm-arm/types.h
@@ -56,6 +56,7 @@ typedef u64 register_t;  #endif
 
 typedef unsigned long size_t;
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)
--- a/xen/include/asm-x86/types.h
+++ b/xen/include/asm-x86/types.h
@@ -39,6 +39,7 @@ typedef __SIZE_TYPE__ size_t;  #else  typedef unsigned
long size_t;  #endif
+typedef signed long ssize_t;
 
 typedef char bool_t;
 #define test_and_set_bool(b)   xchg(&(b), 1)




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel