Messages in this thread | | | Date | Fri, 9 Jan 2004 04:46:46 -0500 (EST) | From | Ingo Molnar <> | Subject | Re: [PATCH][RFC] variable size and signedness issues in ldt.c - potential problem? |
| |
On Thu, 8 Jan 2004, Jesper Juhl wrote:
> 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). [...]
but the user does not control 'newsize'. Can you outline a scenario in where the value could overflow?
> The second thing is that in the body of the 'for' loop there is this > comparison : > > if (bytes > PAGE_SIZE)
no, the value of bytes is really limited. Again, can you suggest a scenario in where this could overflow?
> 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? [...]
LDT size is limited by LDT_ENTRY_SIZE*LDT_ENTRIES. We explicitly truncate bytecount to this range so unsigned vs. signed makes no difference.
> [...] 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?
that's quite common for the interface definitions. Since we are on x86 unsigned long == unsigned int.
> 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;
yep, you are right, this is the wrong style.
Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |