Messages in this thread Patch in this message | | | Date | Thu, 8 Jan 2004 00:20:44 +0100 (CET) | From | Jesper Juhl <> | Subject | [PATCH][RFC] variable size and signedness issues in ldt.c - potential problem? |
| |
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 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/
| |