Re: [PATCH][RFC] variable size and signedness issues in ldt.c -potential problem?
From: Ingo Molnar
Date: Fri Jan 09 2004 - 04:49:41 EST
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/