Hello, 
we got this via vendor-sec. 
 
From: Marcelo Tosatti <marcelo.tosatti@cyclades.com> 
To: vendor-sec@lst.de 
Cc: torvalds@osdl.org, Alan Cox <alan@lxorguk.ukuu.org.uk>, 
        Chris Wright <chrisw@osdl.org>, 
        viro@parcelfarce.linux.theplanet.co.uk 
User-Agent: Mutt/1.5.5.1i 
Subject: [vendor-sec] Sign handling issues on v2.6 
Errors-To: vendor-sec-admin@lst.de 
Date: Sun, 30 Jan 2005 12:30:42 -0200 
 
Hi folks,rc.theaimsgroup.com/?l=linux-netdev&m=110608318200957&w=2 
 
Georgi Guninski has discovered three important sign comparison problems 
in v2.6 
kernels. 
- Ð ÐœÐµÐŒÐ°Ð»ÐŸÐ²Ð°Ð¶ÐœÐ°Ñ ÐŸÑПбеММПÑÑÑ - 
ПÑлОÑÐœÑй ЎОзайМ. 
The collection of vulnerabilities has led Linus to add appropriate range 
checking 
at higher VFS levels (ie sys_read{v}/sys_write{v}): 
 
        ChangeSet@1.1966.1.68, 2005-01-25 20:29:47-08:00, 
torvalds@ppc970.osdl.org 
          Add 'f_maxcount' to allow filesystems to set a per-file maximum IO 
size. 
 
        ChangeSet@1.1966.1.67, 2005-01-25 15:00:33-08:00, 
torvalds@ppc970.osdl.org 
          Rename "locks_verify_area()" to "rw_verify_area()" and clean up the 
          arguments. 
 
With such range checking in place those bugs will be protected (can't be 
be triggered) by the higher levels - they should be fixed nevertheless. 
 
The following fixes are going into Linus's tree Wednesday, so this date 
(February 2) can 
be though of as the release date. 
 
There are for sure more sign handling bugs lying in drivers/fs'es. 
Kudos again to Georgi for his bright auditing! 
 
1) drivers/char/n_tty.c::copy_from_read_buf() 
 
"nr" is user passed parameter, so if its value is higher than MAX_INT, the 
signed "min" will 
happily return "nr", causing unexpected results. 
 
Note that "n" is used afterwards as lenght parameter of copy_to_user: 
 
retval = copy_to_user(*b, &tty->read_buf[tty->read_tail], n); 
 
This fixes it as suggested by Linus: 
 
--- drivers/char/n_tty.c.orig   2005-01-20 13:33:30.000000000 -0200 
+++ drivers/char/n_tty.c        2005-01-30 13:56:05.038069640 -0200 
@@ -1143,13 +1143,13 @@ 
 
 { 
        int retval; 
-       ssize_t n; 
+       size_t n; 
        unsigned long flags; 
 
        retval = 0; 
        spin_lock_irqsave(&tty->read_lock, flags); 
        n = min(tty->read_cnt, N_TTY_BUF_SIZE - tty->read_tail); 
-       n = min((ssize_t)*nr, n); 
+       n = min(*nr, n); 
        spin_unlock_irqrestore(&tty->read_lock, flags); 
        if (n) { 
                mb(); 
 
2) fs/proc/generic.c::proc_file_read() 
 
"nbytes" is user passed parameter, so if its value is higher than MAX_INT, 
the signed 
"min" comparison returns "nbytes". 
 
Several proc handling functions do not expect huge values. 
get_locks_status() for example 
(handler for /proc/locks) can be exploited to read kernel memory. 
 
--- fs/proc/generic.c.orig      2005-01-20 12:39:42.000000000 -0200 
+++ fs/proc/generic.c   2005-01-30 13:58:00.420528840 -0200 
@@ -60,7 +60,7 @@ 
                return -ENOMEM; 
 
        while ((nbytes > 0) && !eof) { 
-               count = min_t(ssize_t, PROC_BLOCK_SIZE, nbytes); 
+               count = min_t(size_t, PROC_BLOCK_SIZE, nbytes); 
 
                start = NULL; 
                if (dp->get_info) { 
 
3) reiserfs: 
 
NOTE: The fix is incomplete, reiserfs_copy_from_user_to_file_region() also 
suffers 
from similar issue but is not fixed in the following patch. The reiserfs people 
should provide a more appropriate fix. 
 
reiserfs_file_write() casts its (size_t) count parameter to int, which can 
become 
a problem on 64-bit architectures because the following range check will fail 
if for eg count is "0x1ffffffff": 
 
       if ( unlikely((ssize_t) count < 0)) 
              return -EINVAL; 
 
Which will later be casted to "int" here: 
 
        int write_bytes; /* amount of bytes to write during this iteration */ 
 
        if ( !num_pages ) { /* If we do not have enough space even for */ 
            res = -ENOSPC;  /* single page, return -ENOSPC */ 
            if ( pos > (inode->i_size & (inode->i_sb->s_blocksize-1))) 
                break; // In case we are writing past the file end, break. 
            // Otherwise we are possibly overwriting the file, so 
            // let's set write size to be equal or less than blocksize. 
            // This way we get it correctly for file holes. 
            // But overwriting files on absolutelly full volumes would not 
            // be very efficient. Well, people are not supposed to fill 
            // 100% of disk space anyway. 
            write_bytes = min_t(int, count, inode->i_sb->s_blocksize - (pos & 
(inode->i_sb->s_blocksize - 1))); 
 
Making "write_bytes" negative, which potentially causes unexpected results. 
 
The following patch is an attempt to fix this by changing the variables 
dealing 
with count and offset and the "min_t" comparisons to unsigned "size_t". 
 
--- file.c.orig 2005-01-25 16:01:13.000000000 -0200 
+++ file.c      2005-01-26 13:28:11.819375848 -0200 
@@ -588,7 +588,7 @@ error_exit: 
 
 /* Unlock pages prepared by reiserfs_prepare_file_region_for_write */ 
 static void reiserfs_unprepare_pages(struct page **prepared_pages, /* list 
of locked pages */ 
-                             int num_pages /* amount of pages */) { 
+                             size_t num_pages /* amount of pages */) { 
     int i; // loop counter 
 
     for (i=0; i < num_pages ; i++) { 
@@ -619,7 +619,7 @@ static int reiserfs_copy_from_user_to_fi 
     int offset; // offset in page 
 
     for ( i = 0, offset = (pos & (PAGE_CACHE_SIZE-1)); i < num_pages ; 
i++,offset=0) { 
-       int count = min_t(int,PAGE_CACHE_SIZE-offset,write_bytes); // How 
much of bytes to write to this page 
+       size_t count = 
min_t(size_t,PAGE_CACHE_SIZE-offset,write_bytes); // How much of 
bytes to write to this page 
        struct page *page=prepared_pages[i]; // Current page we process. 
 
        fault_in_pages_readable( buf, count); 
@@ -718,8 +718,8 @@ static int reiserfs_submit_file_region_f 
                                struct reiserfs_transaction_handle *th, 
                                struct inode *inode, 
                                loff_t pos, /* Writing position offset */ 
-                               int num_pages, /* Number of pages to write */ 
-                               int write_bytes, /* number of bytes to write */ 
+                               size_t num_pages, /* Number of pages to write */ 
+                               size_t write_bytes, /* number of bytes to write */ 
                                struct page **prepared_pages /* list of pages */ 
                                ) 
 { 
@@ -854,9 +854,9 @@ static int reiserfs_check_for_tail_and_c 
 static int reiserfs_prepare_file_region_for_write( 
                                struct inode *inode /* Inode of the file */, 
                                loff_t pos, /* position in the file */ 
-                               int num_pages, /* number of pages to 
+                               size_t num_pages, /* number of pages to 
                                                  prepare */ 
-                               int write_bytes, /* Amount of bytes to be 
+                               size_t write_bytes, /* Amount of bytes to be 
                                                    overwritten from 
                                                    @pos */ 
                                struct page **prepared_pages /* pointer to array 
@@ -1252,10 +1252,9 @@ static ssize_t reiserfs_file_write( stru 
     while ( count > 0) { 
        /* This is the main loop in which we running until some error occures 
           or until we write all of the data. */ 
-       int num_pages;/* amount of pages we are going to write this iteration 
*/ 
-       int write_bytes; /* amount of bytes to write during this iteration */ 
-       int blocks_to_allocate; /* how much blocks we need to allocate for 
-                                  this iteration */ 
+       size_t num_pages;/* amount of pages we are going to write this 
iteration */ 
+       size_t write_bytes; /* amount of bytes to write during this iteration */ 
+       size_t blocks_to_allocate; /* how much blocks we need to allocate for 
this iteration */ 
 
         /*  (pos & (PAGE_CACHE_SIZE-1)) is an idiom for offset into a page 
of pos*/ 
        num_pages = !!((pos+count) & (PAGE_CACHE_SIZE - 1)) + /* round 
up partial 
@@ -1269,7 +1268,7 @@ static ssize_t reiserfs_file_write( stru 
            /* If we were asked to write more data than we want to or if there 
               is not that much space, then we shorten amount of data to write 
               for this iteration. */ 
-           num_pages = min_t(int, REISERFS_WRITE_PAGES_AT_A_TIME, 
reiserfs_can_fit_pages(inode->i_sb)); 
+           num_pages = min_t(size_t, 
REISERFS_WRITE_PAGES_AT_A_TIME, 
reiserfs_can_fit_pages(inode->i_sb)); 
            /* Also we should not forget to set size in bytes accordingly */ 
            write_bytes = (num_pages << PAGE_CACHE_SHIFT) - 
                            (pos & (PAGE_CACHE_SIZE-1)); 
@@ -1295,7 +1294,7 @@ static ssize_t reiserfs_file_write( stru 
            // But overwriting files on absolutelly full volumes would not 
            // be very efficient. Well, people are not supposed to fill 
            // 100% of disk space anyway. 
-           write_bytes = min_t(int, count, inode->i_sb->s_blocksize - (pos & 
(inode->i_sb->s_blocksize - 1))); 
+           write_bytes = min_t(size_t, count, inode->i_sb->s_blocksize - (pos 
& (inode->i_sb->s_blocksize - 1))); 
            num_pages = 1; 
            // No blocks were claimed before, so do it now. 
            reiserfs_claim_blocks_to_be_allocated(inode->i_sb, 1 << 
(PAGE_CACHE_SHIFT - inode->i_blkbits));