From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anton Altaparmakov Subject: ntfs - was: Re: + fix-possible-page_cache_shift-overflows.patch added to -mm tree Date: Wed, 23 Nov 2005 22:01:56 +0000 (GMT) Message-ID: References: <200511232121.jANLLo7J024428@shell0.pdx.osdl.net> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Cc: blaisorblade@yahoo.it, dhowells@redhat.com, dwmw2@infradead.org, green@linuxhacker.ru, hch@lst.de, jdike@addtoit.com, linux-fsdevel@vger.kernel.org, miklos@szeredi.hu, neilb@cse.unsw.edu.au, reiserfs-dev@namesys.com, rmk@arm.linux.org.uk, trond.myklebust@fys.uio.no, zippel@linux-m68k.org, mm-commits@vger.kernel.org Return-path: Received: from ppsw-9.csi.cam.ac.uk ([131.111.8.139]:3500 "EHLO ppsw-9.csi.cam.ac.uk") by vger.kernel.org with ESMTP id S932562AbVKWWCM (ORCPT ); Wed, 23 Nov 2005 17:02:12 -0500 To: akpm@osdl.org In-Reply-To: <200511232121.jANLLo7J024428@shell0.pdx.osdl.net> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org On Wed, 23 Nov 2005, akpm@osdl.org wrote: > > The patch titled > > fix possible PAGE_CACHE_SHIFT overflows > > has been added to the -mm tree. Its filename is > > fix-possible-page_cache_shift-overflows.patch > > > From: Andrew Morton > > We've had two instances recently of overflows when doing > > 64_bit_value = (32_bit_value << PAGE_CACHE_SHIFT) > > I did a tree-wide grep of `<<.*PAGE_CACHE_SHIFT' and this is the result. [snip] > - ntfs_read_compressed_block is doing fishy things with cur_page and > cb_max_page. Those are both fine. They deal with relative offsets rather than absolute page->index and the number of pages is very low (on a 4k PAGE_SIZE architecture the maximum number of pages is 16, thus cannot overflow when you shiftleft by PAGE_CACHE_SHIFT (16 * 4096 = 65536) and the offsets added are also relative to the page and the compression block thus a very maximum value of 65536 + 4095 (PAGE_SIZE - 1) + 65536 (maximum compression block size) = 135167 which is well within a signed 32-bit variable... > - load_and_init_attrdef()'s handling of `index' needs to be checked. Ditto Towards the top of the function you see: i_size = i_size_read(ino); if (i_size <= 0 || i_size > 0x7fffffff) goto iput_failed; Thus the file size is limited to 31-bits thus index << PAGE_CACHE_SHIFT can never overflow 32-bits. > load_and_init_upcase(). Ditto As above file size is limited towards top of the function: i_size = i_size_read(ino); if (!i_size || i_size & (sizeof(ntfschar) - 1) || i_size > 64ULL * 1024 * sizeof(ntfschar)) goto iput_upcase_failed; Since sizeof(ntfschar) is always 2 bytes, the file size is limited to 128kiB thus index << PAGE_CACHE_SHIFT can never overflow. > ntfs_attr_extend_initialized():initialized_size Yep, I can see a bug there! I missed one (s64) cast when I had it in the four other places doing a << PAGE_CACHE_SHIFT around the same place. )-: - ni->initialized_size = (index + 1) << PAGE_CACHE_SHIFT; + ni->initialized_size = (s64)(index + 1) << PAGE_CACHE_SHIFT; Well caught! Thanks! Normally I am quite careful about this as I already have been hit in ntfs by such overflows (a long time ago when the new ntfs driver was in early stages of development)... I will fix it tomorrow and push to my ntfs-2.6-devel.git tree so you will hopefully pick it up for your next -mm... Best regards, Anton -- Anton Altaparmakov (replace at with @) Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK Linux NTFS maintainer / IRC: #ntfs on irc.freenode.net WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/