| Summary: | Sign handling issues on v2.6 (Vendor-Sec) | ||
|---|---|---|---|
| Product: | Gentoo Security | Reporter: | Sune Kloppenborg Jeppesen (RETIRED) <jaervosz> |
| Component: | Kernel | Assignee: | Gentoo Security <security> |
| Status: | RESOLVED DUPLICATE | ||
| Severity: | normal | ||
| Priority: | High | ||
| Version: | unspecified | ||
| Hardware: | All | ||
| OS: | All | ||
| Whiteboard: | /CLASSIFIED 20050202 | ||
| Package list: | Runtime testing required: | --- | |
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));