[PATCH] eCryptfs: infinite loop bug

[PATCH] eCryptfs: infinite loop bug

[Li Wang]

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.

Cheers,
Li Wang

---

signed-off-by: Li Wang <liwang@nudt.edu.cn >
               Yunchuan Wen (wenyunchuan@kylinos.com.cn )

--- read_write.c.orig 2012-01-18 10:39:26.000000000 +0800
+++ read_write.c 2012-01-18 19:48:41.484196221 +0800
@@ -130,7 +130,7 @@ int ecryptfs_write(struct inode *ecryptf
   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 total_remaining_bytes = ((offset + size) - pos);
 
   if (num_bytes > total_remaining_bytes)
    num_bytes = total_remaining_bytes;



--- read_write.c.orig	2012-01-18 10:39:26.000000000 +0800
+++ read_write.c	2012-01-18 19:48:41.484196221 +0800
@@ -130,7 +130,7 @@ int ecryptfs_write(struct inode *ecryptf
 		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 total_remaining_bytes = ((offset + size) - pos);
 
 		if (num_bytes > total_remaining_bytes)
 			num_bytes = total_remaining_bytes;

Re: [PATCH] eCryptfs: infinite loop bug

[Cong Wang]

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

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.

---->

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



[-- Attachment #2: ecryptfs-loff_t.diff --]
[-- Type: text/x-patch, Size: 2094 bytes --]

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)
 {
 	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);
+		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;

Re: [PATCH] eCryptfs: infinite loop bug

[Tyler Hicks]

[-- 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 --]

Re: [PATCH] eCryptfs: infinite loop bug

[Linus Torvalds]

On Wed, Jan 18, 2012 at 1:40 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>
> 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.

Indeed. The offset within a page will fit fine even in an "unsigned
int", and "loff_t" is crazy overkill - and usually generates horrible
code on 32-bit architectures.

                      Linus

Re:Re: [PATCH] eCryptfs: infinite loop bug

[Li Wang]

Hi,
  Thanks Cong Wang for the kind reminding regarding the patch format.
  We did notice that the total_remaining_zeroes need be revised as well, 
and the start_offset_in_page, num_bytes need not be revised (always smaller 
than PAGE_CACHE_SIZE, even the huge page size is supported, 
the 4G page size is not present in the current world?)
but we forget to include the revision for total_remaining_zeroes, so here comes the patch.

Cheers,
Li Wang

Signed-off-by: Li Wang <liwang@nudt.edu.cn>
               Yunchuan Wen <wenyunchuan@kylinos.com.cn>

---

--- a/fs/ecryptfs/read_write.c	2012-01-19 17:34:54.666940824 +0800
+++ b/fs/ecryptfs/read_write.c	2012-01-19 17:35:16.257940840 +0800
@@ -130,13 +130,13 @@ int ecryptfs_write(struct inode *ecryptf
 		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 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;





---------- Origin message ----------
>From："Tyler Hicks" <tyhicks@canonical.com>
>To："Cong Wang" <xiyou.wangcong@gmail.com>
>Subject：Re: [PATCH] eCryptfs: infinite loop bug
>Date：2012-01-19 05:40:51

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?

Re: [PATCH] eCryptfs: infinite loop bug

[Tyler Hicks]

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

On 2012-01-19 09:44:36, Li Wang wrote:
> 
> 
> Hi,
>   Thanks Cong Wang for the kind reminding regarding the patch format.

The commit message still needs some work. But there's no need to drag
out the process for a fix like this, so I'll rewrite the commit message
and reply to this email with the cleaned up version. Let me know if you
have any problems with that. You'll still be credited as the author.

For future kernel patches, please see "15) The canonical patch format"
of Documentation/SubmittingPatches and "5.4: PATCH FORMATTING AND
CHANGELOGS" of Documentation/development-process/5.Posting for more
information on how the commit message should be written.

Also, your email came across in base64 encoding. I'm not sure of the
reason for that or how to fix it.

>   We did notice that the total_remaining_zeroes need be revised as well, 
> and the start_offset_in_page, num_bytes need not be revised (always smaller 

Yes, size_t will work fine, as Linus confirmed.

> than PAGE_CACHE_SIZE, even the huge page size is supported, 
> the 4G page size is not present in the current world?)
> but we forget to include the revision for total_remaining_zeroes, so here comes the patch.

I really appreciate the patch - thanks!

Tyler

> 
> Cheers,
> Li Wang
> 
> Signed-off-by: Li Wang <liwang@nudt.edu.cn>
>                Yunchuan Wen <wenyunchuan@kylinos.com.cn>
> 
> ---
> 
> --- a/fs/ecryptfs/read_write.c	2012-01-19 17:34:54.666940824 +0800
> +++ b/fs/ecryptfs/read_write.c	2012-01-19 17:35:16.257940840 +0800
> @@ -130,13 +130,13 @@ int ecryptfs_write(struct inode *ecryptf
>  		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 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;
> 
> 
> 
> 
> 
> ---------- Origin message ----------
> >From："Tyler Hicks" <tyhicks@canonical.com>
> >To："Cong Wang" <xiyou.wangcong@gmail.com>
> >Subject：Re: [PATCH] eCryptfs: infinite loop bug
> >Date：2012-01-19 05:40:51
> 
> 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?

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

Re: [PATCH] eCryptfs: infinite loop bug

[Dustin Kirkland]

On Thu, Jan 19, 2012 at 2:48 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> On 2012-01-19 09:44:36, Li Wang wrote:
>>
>>
>> Hi,
>>   Thanks Cong Wang for the kind reminding regarding the patch format.
>
> The commit message still needs some work. But there's no need to drag
> out the process for a fix like this, so I'll rewrite the commit message
> and reply to this email with the cleaned up version. Let me know if you
> have any problems with that. You'll still be credited as the author.
>
> For future kernel patches, please see "15) The canonical patch format"
> of Documentation/SubmittingPatches and "5.4: PATCH FORMATTING AND
> CHANGELOGS" of Documentation/development-process/5.Posting for more
> information on how the commit message should be written.
>
> Also, your email came across in base64 encoding. I'm not sure of the
> reason for that or how to fix it.
>
>>   We did notice that the total_remaining_zeroes need be revised as well,
>> and the start_offset_in_page, num_bytes need not be revised (always smaller
>
> Yes, size_t will work fine, as Linus confirmed.
>
>> than PAGE_CACHE_SIZE, even the huge page size is supported,
>> the 4G page size is not present in the current world?)
>> but we forget to include the revision for total_remaining_zeroes, so here comes the patch.
>
> I really appreciate the patch - thanks!

Tyler,

Is this the problem behind our long standing
tor-download-on-ecryptfs-hangs-cpu bug?
 * https://bugs.launchpad.net/ecryptfs/+bug/431975

-- 
:-Dustin

Dustin Kirkland
Chief Architect
Gazzang, Inc.
www.gazzang.com

Re: [PATCH] eCryptfs: infinite loop bug

[Tyler Hicks]

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

On 2012-01-19 08:03:57, Dustin Kirkland wrote:
> On Thu, Jan 19, 2012 at 2:48 AM, Tyler Hicks <tyhicks@canonical.com> wrote:
> > On 2012-01-19 09:44:36, Li Wang wrote:
> >>
> >>
> >> Hi,
> >>   Thanks Cong Wang for the kind reminding regarding the patch format.
> >
> > The commit message still needs some work. But there's no need to drag
> > out the process for a fix like this, so I'll rewrite the commit message
> > and reply to this email with the cleaned up version. Let me know if you
> > have any problems with that. You'll still be credited as the author.
> >
> > For future kernel patches, please see "15) The canonical patch format"
> > of Documentation/SubmittingPatches and "5.4: PATCH FORMATTING AND
> > CHANGELOGS" of Documentation/development-process/5.Posting for more
> > information on how the commit message should be written.
> >
> > Also, your email came across in base64 encoding. I'm not sure of the
> > reason for that or how to fix it.
> >
> >>   We did notice that the total_remaining_zeroes need be revised as well,
> >> and the start_offset_in_page, num_bytes need not be revised (always smaller
> >
> > Yes, size_t will work fine, as Linus confirmed.
> >
> >> than PAGE_CACHE_SIZE, even the huge page size is supported,
> >> the 4G page size is not present in the current world?)
> >> but we forget to include the revision for total_remaining_zeroes, so here comes the patch.
> >
> > I really appreciate the patch - thanks!
> 
> Tyler,
> 
> Is this the problem behind our long standing
> tor-download-on-ecryptfs-hangs-cpu bug?
>  * https://bugs.launchpad.net/ecryptfs/+bug/431975

Yes. Unfortunately, I had missed the infinite loop on i386 part, as I
was testing/developing on x86_64. 

Tyler

> 
> -- 
> :-Dustin
> 
> Dustin Kirkland
> Chief Architect
> Gazzang, Inc.
> www.gazzang.com
> --
> To unsubscribe from this list: send the line "unsubscribe ecryptfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[PATCH] eCryptfs: infinite loop due to overflow in ecryptfs_write()

[Tyler Hicks]

From: Li Wang <liwang@nudt.edu.cn>

ecryptfs_write() can enter an infinite loop when truncating a file to a
size larger than 4G. This only happens on architectures where size_t is
represented by 32 bits.

This was caused by a size_t overflow due to it incorrectly being used to
store the result of a calculation which uses potentially large values of
type loff_t.

[tyhicks@canonical.com: rewrite subject and commit message]
Signed-off-by: Li Wang <liwang@nudt.edu.cn>
Signed-off-by: Yunchuan Wen <wenyunchuan@kylinos.com.cn>
Cc: Cong Wang <xiyou.wangcong@gmail.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 fs/ecryptfs/read_write.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/ecryptfs/read_write.c b/fs/ecryptfs/read_write.c
index 3745f7c..ec3d936 100644
--- a/fs/ecryptfs/read_write.c
+++ b/fs/ecryptfs/read_write.c
@@ -130,13 +130,13 @@ int ecryptfs_write(struct inode *ecryptfs_inode, char *data, loff_t offset,
 		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 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;
-- 
1.7.8.3

Re: [PATCH] eCryptfs: infinite loop due to overflow in ecryptfs_write()

[Cong Wang]

On 01/19/2012 05:13 PM, Tyler Hicks wrote:
> From: Li Wang<liwang@nudt.edu.cn>
>
> ecryptfs_write() can enter an infinite loop when truncating a file to a
> size larger than 4G. This only happens on architectures where size_t is
> represented by 32 bits.
>
> This was caused by a size_t overflow due to it incorrectly being used to
> store the result of a calculation which uses potentially large values of
> type loff_t.
>
> [tyhicks@canonical.com: rewrite subject and commit message]
> Signed-off-by: Li Wang<liwang@nudt.edu.cn>
> Signed-off-by: Yunchuan Wen<wenyunchuan@kylinos.com.cn>
> Cc: Cong Wang<xiyou.wangcong@gmail.com>
> Cc:<stable@vger.kernel.org>
> Signed-off-by: Tyler Hicks<tyhicks@canonical.com>


Tyler, thanks for cleaning this up! Looks pretty good now.