[PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Andi Kleen]

The 32bit version of e820_all_mapped() needs to use u64 to avoid
overflows on PAE systems.  Pointed out by Jan Beulich

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/i386/kernel/setup.c |    2 +-
 include/asm-i386/e820.h  |    3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

Index: linux/arch/i386/kernel/setup.c
===================================================================
--- linux.orig/arch/i386/kernel/setup.c
+++ linux/arch/i386/kernel/setup.c
@@ -970,7 +970,7 @@ efi_memory_present_wrapper(unsigned long
   * not-overlapping, which is the case
   */
 int __init
-e820_all_mapped(unsigned long start, unsigned long end, unsigned type)
+e820_all_mapped(u64 start, u64 end, unsigned type)
 {
 	int i;
 	for (i = 0; i < e820.nr_map; i++) {
Index: linux/include/asm-i386/e820.h
===================================================================
--- linux.orig/include/asm-i386/e820.h
+++ linux/include/asm-i386/e820.h
@@ -36,8 +36,7 @@ struct e820map {
 
 extern struct e820map e820;
 
-extern int e820_all_mapped(unsigned long start, unsigned long end,
-			   unsigned type);
+extern int e820_all_mapped(u64 start, u64 end, unsigned type);
 
 #endif/*!__ASSEMBLY__*/
 

Re: [PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Linus Torvalds]

On Fri, 28 Apr 2006, Andi Kleen wrote:
> 
> The 32bit version of e820_all_mapped() needs to use u64 to avoid
> overflows on PAE systems.  Pointed out by Jan Beulich

I don't think that's true.

It can't be called with 64-bit arguments anyway. If the base address 
doesn't fit in 32-bit, we'd be screwed in other places, afaik.

		Linus

Re: [PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Andi Kleen]

On Friday 28 April 2006 07:39, Linus Torvalds wrote:
> 
> On Fri, 28 Apr 2006, Andi Kleen wrote:
> > 
> > The 32bit version of e820_all_mapped() needs to use u64 to avoid
> > overflows on PAE systems.  Pointed out by Jan Beulich
> 
> I don't think that's true.
> 
> It can't be called with 64-bit arguments anyway. If the base address 
> doesn't fit in 32-bit, we'd be screwed in other places, afaik.

To quote Jan's original description (should have put that in)
I think it's needed.

-Andi

>>>

>> It would seem to me that using 'unsigned long' for start and end is inappropriate on 32bits; at least start should
be
>> 'unsigned long long' as it gets updated from ei->addr + ei->size, which may (truncated to 32 bits) happen to be
zero.
>
>the current user has it 32 bit for sure; once another user appears we certainly can fix this...

The effect is not on the current user's parameter passing, but in the result the function may produce. If, say, for the
PCI mmconfig and BIOS space, there is a (reserved) entry starting at E0000000 and being 20000000 in size, then as far as
I can tell the function will return zero (rather than one).

<<<

Re: [PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Linus Torvalds]

On Fri, 28 Apr 2006, Andi Kleen wrote:
> 
> Quoting jbeulich:
>
> The effect is not on the current user's parameter passing, but in the 
> result the function may produce. If, say, for the PCI mmconfig and BIOS 
> space, there is a (reserved) entry starting at E0000000 and being 
> 20000000 in size, then as far as I can tell the function will return 
> zero (rather than one).

Well, but that has nothing to do with the _calling_ convention.

That looks to be a totally internal bug to within e820_all_mapped().

I agree that the calling convention change would avoid the overflow 
internally too, I just don't much like the explanation (and not 
necessarily that it affects the caller, who doesn't much care).

		Linus

[discuss] Re: [PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Jan Beulich]

>>> Linus Torvalds <torvalds@osdl.org> 28.04.06 07:39 >>>
>
>
>On Fri, 28 Apr 2006, Andi Kleen wrote:
>> 
>> The 32bit version of e820_all_mapped() needs to use u64 to avoid
>> overflows on PAE systems.  Pointed out by Jan Beulich
>
>I don't think that's true.
>
>It can't be called with 64-bit arguments anyway. If the base address 
>doesn't fit in 32-bit, we'd be screwed in other places, afaik.

The arguments don't necessarily need to be u64, but (at least some of)
the calculations inside the function do. Otherwise, a region starting
below 4G and extending to or beyond 4G would not be considered
correctly.

Jan

Re: [discuss] Re: [PATCH] [3/4] i386: Fix overflow in e820_all_mapped

[Arjan van de Ven]

On Fri, 2006-04-28 at 09:07 +0200, Jan Beulich wrote:
> >>> Linus Torvalds <torvalds@osdl.org> 28.04.06 07:39 >>>
> >
> >
> >On Fri, 28 Apr 2006, Andi Kleen wrote:
> >> 
> >> The 32bit version of e820_all_mapped() needs to use u64 to avoid
> >> overflows on PAE systems.  Pointed out by Jan Beulich
> >
> >I don't think that's true.
> >
> >It can't be called with 64-bit arguments anyway. If the base address 
> >doesn't fit in 32-bit, we'd be screwed in other places, afaik.
> 
> The arguments don't necessarily need to be u64, but (at least some of)
> the calculations inside the function do. Otherwise, a region starting
> below 4G and extending to or beyond 4G would not be considered
> correctly.

since this is use-once __init code I rather keep it simple and safe (eg
use u64) than do complex tricks to make it safe in another way.. and the
4G thing is a real potential problem that's easily fixed with the u64
thing.