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

["Egger, Christoph" <chegger@amazon.de>]

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)
> 
>