[PATCH] Off-by-one in cpu_gdt_init

[PATCH] Off-by-one in cpu_gdt_init

[George Washington Dunlap III]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 760 bytes --]

I forget what triggered this bug (it was a long time ago), but 
cpu_gdt_init() is trying to allocate an array, one per frame, based on 
gdt_descr->size.  However, the math currently rounds down instead of up! 
(I'm pretty sure that when I triggered it, (gdt_descr->size>>PAGE_SHIFT) 
was 0.)

  -George

+-------------------+----------------------------------------
| dunlapg@umich.edu | http://www-personal.umich.edu/~dunlapg 
+-------------------+----------------------------------------
|  Who could move a mountain, who could love their enemy?
|  Who could rejoice in pain, and turn the other cheek?
|	- Rich Mullins, "Surely God is With Us"
+------------------------------------------------------------
| Outlaw Junk Email! Support HR 1748 (www.cauce.org)

[-- Attachment #2: Type: TEXT/PLAIN, Size: 656 bytes --]

diff -urN --exclude=SCCS --exclude=BitKeeper xen-unstable.latest/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c xeno-ft/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c
--- xen-unstable.latest/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c	2005-05-16 13:05:03.000000000 -0400
+++ xeno-ft/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c	2005-05-16 13:55:06.000000000 -0400
@@ -554,7 +554,7 @@
 
 void __init cpu_gdt_init(struct Xgt_desc_struct *gdt_descr)
 {
-	unsigned long frames[gdt_descr->size >> PAGE_SHIFT];
+	unsigned long frames[(gdt_descr->size >> PAGE_SHIFT)+1];
 	unsigned long va;
 	int f;
 

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

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

Re: [PATCH] Off-by-one in cpu_gdt_init

[David Hopwood]

George Washington Dunlap III wrote:
> I forget what triggered this bug (it was a long time ago), but 
> cpu_gdt_init() is trying to allocate an array, one per frame, based on 
> gdt_descr->size.  However, the math currently rounds down instead of up! 
> (I'm pretty sure that when I triggered it, (gdt_descr->size>>PAGE_SHIFT) 
> was 0.)
> 
> diff -urN --exclude=SCCS --exclude=BitKeeper xen-unstable.latest/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c xeno-ft/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c
> --- xen-unstable.latest/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c	2005-05-16 13:05:03.000000000 -0400
> +++ xeno-ft/linux-2.6.11-xen-sparse/arch/xen/i386/kernel/cpu/common.c	2005-05-16 13:55:06.000000000 -0400
> @@ -554,7 +554,7 @@
>  
>  void __init cpu_gdt_init(struct Xgt_desc_struct *gdt_descr)
>  {
> -	unsigned long frames[gdt_descr->size >> PAGE_SHIFT];
> +	unsigned long frames[(gdt_descr->size >> PAGE_SHIFT)+1];

Variable-length arrays? Never use variable-length arrays in code that needs
to be robust: you can't guarantee that the stack won't overflow. If it does,
there is no way to detect that situtation (unlike malloc et al where you can
check for NULL), you just get undefined behaviour.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>

Re: [PATCH] Off-by-one in cpu_gdt_init

[Rusty Russell]

On Mon, 2005-06-06 at 17:14 +0100, David Hopwood wrote:
> George Washington Dunlap III wrote:
> >  void __init cpu_gdt_init(struct Xgt_desc_struct *gdt_descr)
> >  {
> > -	unsigned long frames[gdt_descr->size >> PAGE_SHIFT];
> > +	unsigned long frames[(gdt_descr->size >> PAGE_SHIFT)+1];
> 
> Variable-length arrays? Never use variable-length arrays in code that needs
> to be robust: you can't guarantee that the stack won't overflow. If it does,
> there is no way to detect that situtation (unlike malloc et al where you can
> check for NULL), you just get undefined behaviour.

Yes, and no.

It's pretty normal not to check malloc returns in init code: if it fails
what could be more informative than an OOPS?  You're in deep trouble
already.

The real reason for not putting variable length things on the stack is
that stack space is limited.  If you know there's a reasonable upper
bound, just use that in the array size.  If not, don't use the stack.

Cheers,
Rusty.
-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: [PATCH] Off-by-one in cpu_gdt_init

[David Hopwood]

Rusty Russell wrote:
> On Mon, 2005-06-06 at 17:14 +0100, David Hopwood wrote:
>>George Washington Dunlap III wrote:
>>
>>> void __init cpu_gdt_init(struct Xgt_desc_struct *gdt_descr)
>>> {
>>>-	unsigned long frames[gdt_descr->size >> PAGE_SHIFT];
>>>+	unsigned long frames[(gdt_descr->size >> PAGE_SHIFT)+1];
>>
>>Variable-length arrays? Never use variable-length arrays in code that needs
>>to be robust: you can't guarantee that the stack won't overflow. If it does,
>>there is no way to detect that situtation (unlike malloc et al where you can
>>check for NULL), you just get undefined behaviour.
> 
> Yes, and no.
> 
> It's pretty normal not to check malloc returns in init code: if it fails
> what could be more informative than an OOPS?

If a NULL return is in fact guaranteed to cause an oops (which depends on
how and when the pointer is used), possibly. But even then I prefer an error
message that explicitly says what has failed.

> You're in deep trouble already.

Well, I'm used to embedded systems without memory protection where a
NULL pointer dereference or stack overflow generally does not cause an
oops. But I think it's bad practice to rely on oopses rather than
explicit checking, even on systems that guarantee an oops will occur.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>