Re: [PATCH] eCryptfs: infinite loop bug

[Tyler Hicks <tyhicks@canonical.com>]

[-- Attachment #1: Type: text/plain, Size: 3421 bytes --]

On 2012-01-18 23:26:52, Cong Wang wrote:
> On 01/18/2012 03:30 PM, Li Wang wrote:
> >Hi,
> >   There is an infinite loop bug in eCryptfs, to make it present,
> >just truncate to generate a huge file (>= 4G) on a 32-bit machine
> >under the plain text foleder mounted with eCryptfs, a simple command
> >'truncate -s 4G dummy' is enough. Note: 4GB is smaller than 4G,
> >therefore the following command 'truncate -s 4GB dummy' will not trigger this bug.
> >The bug comes from a data overflow, the patch below fixes it.
> >
> >
> 
> Hi,
> 
> Your patch is not correctly generated, you need to make the diff on
> top of the source tree.
> 
> Also, after reviewing the code, I think there are more places need
> to fix. Can you try my patch below?

Thanks for the review and additional fixes. See my comments below.

> 
> Thanks.
> 
> ---->
> 
> Signed-off-by: WANG Cong <xiyou.wangcong@gmail.com>
> 
> 

> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index a9f29b1..9ca9c17 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -653,7 +653,7 @@ int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
>  			 loff_t offset, size_t size);
>  int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
>  				      struct page *page_for_lower,
> -				      size_t offset_in_page, size_t size);
> +				      loff_t offset_in_page, size_t size);
>  int ecryptfs_write(struct inode *inode, char *data, loff_t offset, size_t size);
>  int ecryptfs_read_lower(char *data, loff_t offset, size_t size,
>  			struct inode *ecryptfs_inode);
> diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
> index 3745f7c..93d80c4 100644
> --- a/fs/ecryptfs/read_write.c
> +++ b/fs/ecryptfs/read_write.c
> @@ -72,7 +72,7 @@ int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
>   */
>  int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
>  				      struct page *page_for_lower,
> -				      size_t offset_in_page, size_t size)
> +				      loff_t offset_in_page, size_t size)

mm/filemap.c uses unsigned long for variables used to identify an offset
into a single page. That's what I'm tempted to use for offset_in_page,
rather than loff_t.

>  {
>  	char *virt;
>  	loff_t offset;
> @@ -128,15 +128,15 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset,
>  		pos = offset;
>  	while (pos < (offset + size)) {
>  		pgoff_t ecryptfs_page_idx = (pos >> PAGE_CACHE_SHIFT);
> -		size_t start_offset_in_page = (pos & ~PAGE_CACHE_MASK);
> -		size_t num_bytes = (PAGE_CACHE_SIZE - start_offset_in_page);
> -		size_t total_remaining_bytes = ((offset + size) - pos);
> +		loff_t start_offset_in_page = (pos & ~PAGE_CACHE_MASK);
> +		loff_t num_bytes = (PAGE_CACHE_SIZE - start_offset_in_page);

unsigned long should work for start_offset_in_page and num_bytes, too.

Tyler

> +		loff_t total_remaining_bytes = ((offset + size) - pos);
>  
>  		if (num_bytes > total_remaining_bytes)
>  			num_bytes = total_remaining_bytes;
>  		if (pos < offset) {
>  			/* remaining zeros to write, up to destination offset */
> -			size_t total_remaining_zeros = (offset - pos);
> +			loff_t total_remaining_zeros = (offset - pos);
>  
>  			if (num_bytes > total_remaining_zeros)
>  				num_bytes = total_remaining_zeros;


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]