Skip to contentSkip to conversation

ocfs2: Fix memory overflow in cow_by_page.

PATCH New Tao Ma 3 participants 5 messages Jan 21, 2010 – Jan 26, 2010 OCFS2

lore
Share & tools
lore

Tao Ma

author
#0 lore raw

PATCH 1/1 ocfs2: Fix memory overflow in cow_by_page.

In ocfs2_duplicate_clusters_by_page, we calculate map_end by shifting page_index. But actually in case we meet with a large offset(say in a i686 box, poff_t is only 32 bits and page_index=2056240), we will overflow. So change it by adding PAGE_CACHE_SIZE to offset.

---
 fs/ocfs2/refcounttree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +1 -1
index 74db2be..6db863d 100644
@@ -2945,7 +2945,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 
 	while (offset < end) {
 		page_index = offset >> PAGE_CACHE_SHIFT;
-		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+		map_end = offset + PAGE_CACHE_SIZE;
 		if (map_end > end)
 			map_end = end;
 
Signature · 1 line
1.6.3.3
#1 lore raw

PATCH 1/1 ocfs2: Fix memory overflow in cow_by_page.

ack

Quoted 25 lines · Tao Ma

In ocfs2_duplicate_clusters_by_page, we calculate map_end by shifting page_index. But actually in case we meet with a large offset(say in a i686 box, poff_t is only 32 bits and page_index=2056240), we will overflow. So change it by adding PAGE_CACHE_SIZE to offset.

---
 fs/ocfs2/refcounttree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +1 -1
index 74db2be..6db863d 100644
@@ -2945,7 +2945,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 
 	while (offset < end) {
 		page_index = offset >> PAGE_CACHE_SHIFT;
-		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+		map_end = offset + PAGE_CACHE_SIZE;
 		if (map_end > end)
 			map_end = end;
 
#2 lore raw

PATCH 1/1 ocfs2: Fix memory overflow in cow_by_page.

In reply to Tao Ma
Quoted 16 lines · Tao Ma

In ocfs2_duplicate_clusters_by_page, we calculate map_end by shifting page_index. But actually in case we meet with a large offset(say in a i686 box, poff_t is only 32 bits and page_index=2056240), we will overflow. So change it by adding PAGE_CACHE_SIZE to offset.

---
 fs/ocfs2/refcounttree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +0 -0
index 74db2be..6db863d 100644

fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2948

Quoted 5 lines
fs/ocfs2/refcounttree.c +1 -1
@@ -2945,7 +2945,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 
 	while (offset < end) {
 		page_index = offset >> PAGE_CACHE_SHIFT;
-		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+		map_end = offset + PAGE_CACHE_SIZE;

First, we can't be computing by offset, because map_end is
supposed to be page bounded, right? Also, what if we have an offset that is the last page possible? Won't that wrap as well, setting map_end to 0?
Why aren't we computing map_end like we compute end, as a loff_t
value?

page_index = offset >> PAGE_CACHE_SHIFT;
- map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+ map_end = ((loff_t)page_index + 1) << PAGE_CACHE_SHIFT;

  		if (map_end > end)
  			map_end = end;

The map_end>end check will catch anything too big.

Joel

Signature · 8 lines
"You don't make the poor richer by making the rich poorer."
	- Sir Winston Churchill

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
author
#3 lore raw

PATCH 1/1 ocfs2: Fix memory overflow in cow_by_page.

Quoted 37 lines · Joel Becker
Quoted 16 lines · Tao Ma

In ocfs2_duplicate_clusters_by_page, we calculate map_end by shifting page_index. But actually in case we meet with a large offset(say in a i686 box, poff_t is only 32 bits and page_index=2056240), we will overflow. So change it by adding PAGE_CACHE_SIZE to offset.

---
 fs/ocfs2/refcounttree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +0 -0
index 74db2be..6db863d 100644

fs/ocfs2/refcounttree.c static int ocfs2_duplicate_clusters_by_page(handle_t *handle, · line 2948

Quoted 5 lines
fs/ocfs2/refcounttree.c +1 -1
@@ -2945,7 +2945,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 
 	while (offset < end) {
 		page_index = offset >> PAGE_CACHE_SHIFT;
-		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+		map_end = offset + PAGE_CACHE_SIZE;

First, we can't be computing by offset, because map_end is
supposed to be page bounded, right? Also, what if we have an offset that is the last page possible? Won't that wrap as well, setting map_end to 0?
Why aren't we computing map_end like we compute end, as a loff_t
value?

  		page_index = offset >> PAGE_CACHE_SHIFT;
fs/ocfs2/refcounttree.c +1 -1
- 		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+ 		map_end = ((loff_t)page_index + 1) << PAGE_CACHE_SHIFT;
 
  		if (map_end > end)
  			map_end = end;
Quoted 1 line

The map_end>end check will catch anything too big.

oh, you are right.
I only considered the problem of overflow, but forget the original usage. Sorry.

Regards,
Tao

author
#4 lore raw

PATCH 1/1 ocfs2: Fix memory overflow in cow_by_page.

In reply to Joel Becker

In ocfs2_duplicate_clusters_by_page, we calculate map_end by shifting page_index. But actually in case we meet with a large offset(say in a i686 box, poff_t is only 32 bits and page_index=2056240), we will overflow. So change the type of page_index to loff_t.

---
 fs/ocfs2/refcounttree.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
fs/ocfs2/refcounttree.c +1 -1
index 74db2be..7687bf1 100644
@@ -2945,7 +2945,7 @@ static int ocfs2_duplicate_clusters_by_page(handle_t *handle,
 
 	while (offset < end) {
 		page_index = offset >> PAGE_CACHE_SHIFT;
-		map_end = (page_index + 1) << PAGE_CACHE_SHIFT;
+		map_end = ((loff_t)page_index + 1) << PAGE_CACHE_SHIFT;
 		if (map_end > end)
 			map_end = end;
 
Signature · 1 line
1.6.3.3