ntfs - was: Re: + fix-possible-page_cache_shift-overflows.patch added to -mm tree

[Anton Altaparmakov <aia21@cam.ac.uk>]

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 <akpm@osdl.org>
> 
> 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 <aia21 at cam.ac.uk> (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/