[PATCH 1/2] x86: separate out sanitize_e820_map return codes

[PATCH 1/2] x86: separate out sanitize_e820_map return codes

[Martin Kelly]

Previously, sanitize_e820_map returned -1 in all cases in which it did
nothing. However, sanitize_e820_map can do nothing either because the
input map has size 1 (this is ok) or because the input map passed in is
invalid (likely an issue). It is nice for the caller to be able to
distinguish the two cases and treat them separately.

Signed-off-by: Martin Kelly <martkell@amazon.com>
---
 arch/x86/include/asm/e820.h |  5 +++++
 arch/x86/kernel/e820.c      | 18 +++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/e820.h b/arch/x86/include/asm/e820.h
index 779c2ef..96377df 100644
--- a/arch/x86/include/asm/e820.h
+++ b/arch/x86/include/asm/e820.h
@@ -12,6 +12,11 @@
 /* see comment in arch/x86/kernel/e820.c */
 extern struct e820map e820;
 extern struct e820map e820_saved;
+/* sanitize_e820_map return codes */
+#define E820_RC_ONLY_ONE (-1)  /* return code when there's only one memory
+				  region in the map */
+#define E820_RC_BAD_MAP (-2)   /* return code when passed a map containing an
+				  invalid memory region */
 
 extern unsigned long pci_mem_start;
 extern int e820_any_mapped(u64 start, u64 end, unsigned type);
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 49f8864..3e1fd63 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -189,11 +189,15 @@ void __init e820_print_map(char *who)
  * (something no more than max_nr_map.)
  *
  * The return value from sanitize_e820_map() is zero if it
- * successfully 'sanitized' the map entries passed in, and is -1
- * if it did nothing, which can happen if either of (1) it was
- * only passed one map entry, or (2) any of the input map entries
- * were invalid (start + size < start, meaning that the size was
- * so big the described memory range wrapped around through zero.)
+ * successfully 'sanitized' the map entries passed in and negative if it did
+ * nothing. There are two cases in which sanitize_e820_map() does nothing:
+ * (1) it was passed only one map entry, so nothing needs to be done. In this
+ *     case, it returns E820_RC_ONLY_ONE.
+ * (2) any of the input map entries * were invalid (start + size < start)
+ *     meaning that the size was so big the described memory range wrapped
+ *     around through zero. In this case, it returns E820_RC_BAD_MAP.
+ * Since (1) is sometimes an expected case and (2) indicates an error, the
+ * distinct return codes allow callers to handle the two cases separately.
  *
  *	Visually we're performing the following
  *	(1,2,3,4 = memory types)...
@@ -269,7 +273,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
 
 	/* if there's only one memory region, don't bother */
 	if (*pnr_map < 2)
-		return -1;
+		return E820_RC_ONLY_ONE;
 
 	old_nr = *pnr_map;
 	BUG_ON(old_nr > max_nr_map);
@@ -277,7 +281,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
 	/* bail out if we find any unreasonable addresses in bios map */
 	for (i = 0; i < old_nr; i++)
 		if (biosmap[i].addr + biosmap[i].size < biosmap[i].addr)
-			return -1;
+			return E820_RC_BAD_MAP;
 
 	/* create pointers for initial change-point information (for sorting) */
 	for (i = 0; i < 2 * old_nr; i++)
-- 
2.1.1

[PATCH 2/2] xen/setup: warn on bad Xen-supplied memory map

[Martin Kelly]

Currently, we don't check the return code of sanitize_e820_map. However,
if the Xen-supplied memory map is invalid, then sanitize_e820_map will
return without doing anything, potentially leading to errors later on.
Add a WARN_ON in case sanitize_e820_map detects an invalid memory map.

Signed-off-by: Martin Kelly <martkell@amazon.com>
---
 arch/x86/xen/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index af72161..48c7072 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -608,7 +608,8 @@ char * __init xen_memory_setup(void)
 		xen_ignore_unusable(map, memmap.nr_entries);
 
 	/* Make sure the Xen-supplied memory map is well-ordered. */
-	sanitize_e820_map(map, memmap.nr_entries, &memmap.nr_entries);
+	rc = sanitize_e820_map(map, memmap.nr_entries, &memmap.nr_entries);
+	WARN_ON(rc == E820_RC_BAD_MAP);
 
 	max_pages = xen_get_max_pages();
 	if (max_pages > max_pfn)
-- 
2.1.1

[PATCH 2/2] xen/setup: warn on bad Xen-supplied memory map

[Martin Kelly]

Currently, we don't check the return code of sanitize_e820_map. However,
if the Xen-supplied memory map is invalid, then sanitize_e820_map will
return without doing anything, potentially leading to errors later on.
Add a WARN_ON in case sanitize_e820_map detects an invalid memory map.

Signed-off-by: Martin Kelly <martkell@amazon.com>
---
 arch/x86/xen/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
index af72161..48c7072 100644
--- a/arch/x86/xen/setup.c
+++ b/arch/x86/xen/setup.c
@@ -608,7 +608,8 @@ char * __init xen_memory_setup(void)
 		xen_ignore_unusable(map, memmap.nr_entries);
 
 	/* Make sure the Xen-supplied memory map is well-ordered. */
-	sanitize_e820_map(map, memmap.nr_entries, &memmap.nr_entries);
+	rc = sanitize_e820_map(map, memmap.nr_entries, &memmap.nr_entries);
+	WARN_ON(rc == E820_RC_BAD_MAP);
 
 	max_pages = xen_get_max_pages();
 	if (max_pages > max_pfn)
-- 
2.1.1

Re: [PATCH 1/2] x86: separate out sanitize_e820_map return codes

[David Vrabel]

On 14/10/14 03:30, Martin Kelly wrote:
> Previously, sanitize_e820_map returned -1 in all cases in which it did
> nothing. However, sanitize_e820_map can do nothing either because the
> input map has size 1 (this is ok) or because the input map passed in is
> invalid (likely an issue). It is nice for the caller to be able to
> distinguish the two cases and treat them separately.

Wouldn't it be more sensible to return 0 (success) in the case of a
single entry map?  IMO, a 1 entry map is by definition sanitized.

David

> --- a/arch/x86/include/asm/e820.h
> +++ b/arch/x86/include/asm/e820.h
> @@ -12,6 +12,11 @@
>  /* see comment in arch/x86/kernel/e820.c */
>  extern struct e820map e820;
>  extern struct e820map e820_saved;
> +/* sanitize_e820_map return codes */
> +#define E820_RC_ONLY_ONE (-1)  /* return code when there's only one memory
> +				  region in the map */
> +#define E820_RC_BAD_MAP (-2)   /* return code when passed a map containing an
> +				  invalid memory region */
>  
>  extern unsigned long pci_mem_start;
>  extern int e820_any_mapped(u64 start, u64 end, unsigned type);
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 49f8864..3e1fd63 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -189,11 +189,15 @@ void __init e820_print_map(char *who)
>   * (something no more than max_nr_map.)
>   *
>   * The return value from sanitize_e820_map() is zero if it
> - * successfully 'sanitized' the map entries passed in, and is -1
> - * if it did nothing, which can happen if either of (1) it was
> - * only passed one map entry, or (2) any of the input map entries
> - * were invalid (start + size < start, meaning that the size was
> - * so big the described memory range wrapped around through zero.)
> + * successfully 'sanitized' the map entries passed in and negative if it did
> + * nothing. There are two cases in which sanitize_e820_map() does nothing:
> + * (1) it was passed only one map entry, so nothing needs to be done. In this
> + *     case, it returns E820_RC_ONLY_ONE.
> + * (2) any of the input map entries * were invalid (start + size < start)
> + *     meaning that the size was so big the described memory range wrapped
> + *     around through zero. In this case, it returns E820_RC_BAD_MAP.
> + * Since (1) is sometimes an expected case and (2) indicates an error, the
> + * distinct return codes allow callers to handle the two cases separately.
>   *
>   *	Visually we're performing the following
>   *	(1,2,3,4 = memory types)...
> @@ -269,7 +273,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  
>  	/* if there's only one memory region, don't bother */
>  	if (*pnr_map < 2)
> -		return -1;
> +		return E820_RC_ONLY_ONE;
>  
>  	old_nr = *pnr_map;
>  	BUG_ON(old_nr > max_nr_map);
> @@ -277,7 +281,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  	/* bail out if we find any unreasonable addresses in bios map */
>  	for (i = 0; i < old_nr; i++)
>  		if (biosmap[i].addr + biosmap[i].size < biosmap[i].addr)
> -			return -1;
> +			return E820_RC_BAD_MAP;
>  
>  	/* create pointers for initial change-point information (for sorting) */
>  	for (i = 0; i < 2 * old_nr; i++)
> 

Re: [Xen-devel] [PATCH 1/2] x86: separate out sanitize_e820_map return codes

[David Vrabel]

On 14/10/14 03:30, Martin Kelly wrote:
> Previously, sanitize_e820_map returned -1 in all cases in which it did
> nothing. However, sanitize_e820_map can do nothing either because the
> input map has size 1 (this is ok) or because the input map passed in is
> invalid (likely an issue). It is nice for the caller to be able to
> distinguish the two cases and treat them separately.

Wouldn't it be more sensible to return 0 (success) in the case of a
single entry map?  IMO, a 1 entry map is by definition sanitized.

David

> --- a/arch/x86/include/asm/e820.h
> +++ b/arch/x86/include/asm/e820.h
> @@ -12,6 +12,11 @@
>  /* see comment in arch/x86/kernel/e820.c */
>  extern struct e820map e820;
>  extern struct e820map e820_saved;
> +/* sanitize_e820_map return codes */
> +#define E820_RC_ONLY_ONE (-1)  /* return code when there's only one memory
> +				  region in the map */
> +#define E820_RC_BAD_MAP (-2)   /* return code when passed a map containing an
> +				  invalid memory region */
>  
>  extern unsigned long pci_mem_start;
>  extern int e820_any_mapped(u64 start, u64 end, unsigned type);
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 49f8864..3e1fd63 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -189,11 +189,15 @@ void __init e820_print_map(char *who)
>   * (something no more than max_nr_map.)
>   *
>   * The return value from sanitize_e820_map() is zero if it
> - * successfully 'sanitized' the map entries passed in, and is -1
> - * if it did nothing, which can happen if either of (1) it was
> - * only passed one map entry, or (2) any of the input map entries
> - * were invalid (start + size < start, meaning that the size was
> - * so big the described memory range wrapped around through zero.)
> + * successfully 'sanitized' the map entries passed in and negative if it did
> + * nothing. There are two cases in which sanitize_e820_map() does nothing:
> + * (1) it was passed only one map entry, so nothing needs to be done. In this
> + *     case, it returns E820_RC_ONLY_ONE.
> + * (2) any of the input map entries * were invalid (start + size < start)
> + *     meaning that the size was so big the described memory range wrapped
> + *     around through zero. In this case, it returns E820_RC_BAD_MAP.
> + * Since (1) is sometimes an expected case and (2) indicates an error, the
> + * distinct return codes allow callers to handle the two cases separately.
>   *
>   *	Visually we're performing the following
>   *	(1,2,3,4 = memory types)...
> @@ -269,7 +273,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  
>  	/* if there's only one memory region, don't bother */
>  	if (*pnr_map < 2)
> -		return -1;
> +		return E820_RC_ONLY_ONE;
>  
>  	old_nr = *pnr_map;
>  	BUG_ON(old_nr > max_nr_map);
> @@ -277,7 +281,7 @@ int __init sanitize_e820_map(struct e820entry *biosmap, int max_nr_map,
>  	/* bail out if we find any unreasonable addresses in bios map */
>  	for (i = 0; i < old_nr; i++)
>  		if (biosmap[i].addr + biosmap[i].size < biosmap[i].addr)
> -			return -1;
> +			return E820_RC_BAD_MAP;
>  
>  	/* create pointers for initial change-point information (for sorting) */
>  	for (i = 0; i < 2 * old_nr; i++)
> 

Re: [Xen-devel] [PATCH 1/2] x86: separate out sanitize_e820_map return codes

[Martin Kelly]

On 10/14/2014 02:33 AM, David Vrabel wrote:
> On 14/10/14 03:30, Martin Kelly wrote:
>> Previously, sanitize_e820_map returned -1 in all cases in which it did
>> nothing. However, sanitize_e820_map can do nothing either because the
>> input map has size 1 (this is ok) or because the input map passed in is
>> invalid (likely an issue). It is nice for the caller to be able to
>> distinguish the two cases and treat them separately.
> 
> Wouldn't it be more sensible to return 0 (success) in the case of a
> single entry map?  IMO, a 1 entry map is by definition sanitized.
> 
> David
> 

I had that thought as I writing the patch, but I was worried about breaking callers. Luckily, it appears there are only 11 callers in the kernel, and all except one either:
(1) Don't check the return value of sanitize_e820_map or
(2) Check against 0 rather than < 0

One caller is checking for < 0: arch/x86/kernel/e820.c:finish_e820_parsing :
        if (userdef) {
                u32 nr = e820.nr_map;

                if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr) < 0)
                        early_panic("Invalid user supplied memory map");
                e820.nr_map = nr;

                printk(KERN_INFO "e820: user-defined physical RAM map:\n");
                e820_print_map("user");
        }

This seems like a bug, as if the user-defined memory map is size 1, there will be an erroneous panic.

I will issue a new revision to change the return values to 0 or -1, with 0 including the size 1 case. In addition, I will add a patch to either change all the callers to actually check this value or to panic in the error case of sanitize_e820_map itself. Which do you think is a cleaner approach?

Re: [PATCH 1/2] x86: separate out sanitize_e820_map return codes

[Martin Kelly]

On 10/14/2014 02:33 AM, David Vrabel wrote:
> On 14/10/14 03:30, Martin Kelly wrote:
>> Previously, sanitize_e820_map returned -1 in all cases in which it did
>> nothing. However, sanitize_e820_map can do nothing either because the
>> input map has size 1 (this is ok) or because the input map passed in is
>> invalid (likely an issue). It is nice for the caller to be able to
>> distinguish the two cases and treat them separately.
> 
> Wouldn't it be more sensible to return 0 (success) in the case of a
> single entry map?  IMO, a 1 entry map is by definition sanitized.
> 
> David
> 

I had that thought as I writing the patch, but I was worried about breaking callers. Luckily, it appears there are only 11 callers in the kernel, and all except one either:
(1) Don't check the return value of sanitize_e820_map or
(2) Check against 0 rather than < 0

One caller is checking for < 0: arch/x86/kernel/e820.c:finish_e820_parsing :
        if (userdef) {
                u32 nr = e820.nr_map;

                if (sanitize_e820_map(e820.map, ARRAY_SIZE(e820.map), &nr) < 0)
                        early_panic("Invalid user supplied memory map");
                e820.nr_map = nr;

                printk(KERN_INFO "e820: user-defined physical RAM map:\n");
                e820_print_map("user");
        }

This seems like a bug, as if the user-defined memory map is size 1, there will be an erroneous panic.

I will issue a new revision to change the return values to 0 or -1, with 0 including the size 1 case. In addition, I will add a patch to either change all the callers to actually check this value or to panic in the error case of sanitize_e820_map itself. Which do you think is a cleaner approach?