George Washington Dunlap III
2005-Jun-06 15:35 UTC
[Xen-devel] [PATCH] Off-by-one in cpu_gdt_init
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) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
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> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel