linux-kernel - Re: [Xen-devel] [PATCH 1/2] x86: separate out sanitize_e820

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <543CEDFD.6020501@citrix.com>
Date:	Tue, 14 Oct 2014 10:33:49 +0100
From:	David Vrabel <david.vrabel@...rix.com>
To:	Martin Kelly <martin@...tingkelly.com>,
	<linux-kernel@...r.kernel.org>, <xen-devel@...ts.xenproject.org>,
	<x86@...nel.org>
CC:	<mingo@...hat.com>, <david.vrabel@...rix.com>,
	Martin Kelly <martkell@...zon.com>, <hpa@...or.com>,
	<boris.ostrovsky@...cle.com>, <tglx@...utronix.de>
Subject: Re: [Xen-devel] [PATCH 1/2] x86: separate out sanitize_e820_map return
 codes

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/