[lkml]   [2004]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH][RFC] variable size and signedness issues in ldt.c - potential problem?

    Replying to myself here since I just noticed one other bit in the same

    In read_ldt() wouldn't it make sense to move the declaration of 'bytes'
    outside the for() loop? It's initialized with "size - i" every time
    through the loop just before it's used, so there really is no reason to
    create the variable from scratch each time..
    That's just a minor thing, the main issues are below.

    /Jesper Juhl

    On Thu, 8 Jan 2004, Jesper Juhl wrote:

    > Hi,
    > I'm hunting the kernel source for any potential problem I can find (and
    > hopefully fix), and I've come across something that looks like a possible
    > problem in arch/i386/kernel/ldt.c
    > First thing that looks suspicious is this bit in read_ldt() :
    > for (i = 0; i < size; i += PAGE_SIZE) {
    > ...
    > }
    > 'i' is a plain int while 'size' is an 'unsigned long' leaving the
    > possibility that if size contains a value greater than what a signed int
    > can hold then this code won't do the right thing, either 'i' will wrap
    > around to zero and the loop will never exit or something "unknown" will
    > happen (as far as I know, what happens when an int overflows is
    > implementation defined).
    > The easy fix for this is to simply make 'i' an 'unsigned long' which
    > prevents the posibility that 'i' will be too small and also prevents a
    > signed vs unsigned comparison.
    > The second thing is that in the body of the 'for' loop there is this
    > comparison :
    > if (bytes > PAGE_SIZE)
    > 'bytes' is 'int', and from looking at include/asm-i386/page.h I see that
    > #define PAGE_SHIFT 12
    > #define PAGE_SIZE (1UL << PAGE_SHIFT)
    > so PAGE_SIZE is an 'unsigned long' utilizing 13 bits.
    > This looks like another instance of the signed int in the comparison
    > potentially being at risk of overflowing. Yes, I'm aware that the default
    > sizeof(int) on most i386 platforms is 32bits and that C requires it to
    > be at least 16bits and thus that there should never be a real issue, but
    > Changing it to be an 'unsigned long' type just like what it's compared
    > against guarantees that there definately is no issue, and also again
    > avoids a comparison between signed and unsigned values.
    > This also harmonizes well with the fact that bytes is initialized by
    > bytes = size - i; and both 'size' and 'i' being 'unsigned long' values
    > (assuming the change suggested above is made).
    > Assuming the above analysis and conclusion makes sense, here's a patch to
    > implement the conclusion (against 2.6.1-rc1-mm2) - at the very least it
    > kills some warnings from gcc about signed vs unsigned comparisons when
    > compiling with "-W -Wall" but I'd like to know if it also fixes a real
    > potential problem :
    > --- linux-2.6.1-rc1-mm2-orig/arch/i386/kernel/ldt.c 2004-01-06 01:33:04.000000000 +0100
    > +++ linux-2.6.1-rc1-mm2/arch/i386/kernel/ldt.c 2004-01-07 23:28:38.000000000 +0100
    > @@ -120,7 +120,8 @@ void destroy_context(struct mm_struct *m
    > static int read_ldt(void __user * ptr, unsigned long bytecount)
    > {
    > - int err, i;
    > + int err;
    > + unsigned long i;
    > unsigned long size;
    > struct mm_struct * mm = current->mm;
    > @@ -144,7 +145,8 @@ static int read_ldt(void __user * ptr, u
    > __flush_tlb_global();
    > for (i = 0; i < size; i += PAGE_SIZE) {
    > - int nr = i / PAGE_SIZE, bytes;
    > + int nr = i / PAGE_SIZE;
    > + unsigned long bytes;
    > char *kaddr = kmap(mm->context.ldt_pages[nr]);
    > bytes = size - i;
    > In order to "take my own medicine" and give the above patch a minimum
    > amount of testing, I applied it to my own 2.6.1-rc1-mm2 tree, build the
    > kernel (nothing blew up), installed the kernel and I'm currently running
    > that kernel (and nothing has blown up yet).
    > I realize that to test it properly I should ofcourse create a small
    > program that calls modify_ldt() and exercises it a bit, but I haven't done
    > so yet since I'd like some feedback first to find out if I'm off on a
    > wild goose chase here...?
    > there are a few other things in arch/i386/kernel/ldt.c that puzzle me.
    > I know that the only user of read_ldt() and write_ldt() is
    > sys_modify_ldt() , and the arguments for read_ldt and write_ldt thus have
    > to match sys_modify_ldt, but why is the 'bytecount' argument for
    > sys_modify_ldt an 'unsigned long' and the return type an 'int' ?
    > The signedness of the return type makes sense given that it't supposed to
    > return -1 on error. But on success, in the case where it calls read_ldt,
    > it's supposed to return the actual number of bytes read. But if the
    > number of bytes to read is given as an unsigned long, and the number
    > actually read exceeds the size of a signed int then the return value will
    > get truncated upon return - how can that be right? And if the return
    > value can never exceed what a signed int can hold, then why is it possible
    > to request an unsigned long amount of bytes to read in the first place?
    > and finally a purely style related thing (sure, call me pedantic); in both
    > read_ldt() and write_ldt() 'mm' is declared as
    > struct mm_struct * mm = current->mm;
    > looking at the rest of ldt.c (and the kernel source in general) it seems
    > to me that
    > struct mm_struct *mm = current->mm;
    > would be more in line with the general style... If this seems resonable
    > I'll create a patch to change it - let me know.
    > As you can probably deduce from the above I don't completely understand
    > what's going on here, so I'd really appreciate being enlightened a bit.
    > Kind regards,
    > Jesper Juhl
    > -
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to
    > More majordomo info at
    > Please read the FAQ at
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:59    [W:0.029 / U:75.692 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site