RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

Hi David and Josh,
 
Sorry if I am bothering you guys. I know you guys are busy or might
be on vacation (it was August).
 
Usually how long does it take for patches on the linux-mtd mailing
list for JFFS2 patches to get ACKed or NACKed. Or get an email
response from the MAINTAINER.
 
I submitted the following patch and need to monitor it until it gets applied.
 
http://lists.infradead.org/pipermail/linux-mtd/2009-August/027005.html
http://patchwork.ozlabs.org/patch/31854/

Best Regards,
 
Victor Gallardo

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Artem Bityutskiy]

On Thu, 2009-08-27 at 16:37 -0700, Victor Gallardo wrote:
> Hi David and Josh,
>  
> Sorry if I am bothering you guys. I know you guys are busy or might
> be on vacation (it was August).
>  
> Usually how long does it take for patches on the linux-mtd mailing
> list for JFFS2 patches to get ACKed or NACKed. Or get an email
> response from the MAINTAINER.
>  
> I submitted the following patch and need to monitor it until it gets applied.
>  
> http://lists.infradead.org/pipermail/linux-mtd/2009-August/027005.html
> http://patchwork.ozlabs.org/patch/31854/

Your patch is in my l2-mtd-2.6 tree:

http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commit/a33f02363d4f57c7bc6133e892ad99d7ce7e7c87

// the secretary.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

> >
> > I submitted the following patch and need to monitor it until it gets applied.
> >
> > http://lists.infradead.org/pipermail/linux-mtd/2009-August/027005.html
> > http://patchwork.ozlabs.org/patch/31854/
> 
> Your patch is in my l2-mtd-2.6 tree:
> 
> http://git.infradead.org/users/dedekind/l2-mtd-2.6.git/commit/a33f02363d4f57c7bc6133e892ad99d7ce7e7c87
> 

Thanks Artem, I did not know to look here.

Best Regards,

Victor Gallardo

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[David Woodhouse]

On Thu, 2009-08-27 at 16:37 -0700, Victor Gallardo wrote:
> 
> Hi David and Josh,
>  
> Sorry if I am bothering you guys. I know you guys are busy or might
> be on vacation (it was August).
>  
> Usually how long does it take for patches on the linux-mtd mailing
> list for JFFS2 patches to get ACKed or NACKed. Or get an email
> response from the MAINTAINER.
>  
> I submitted the following patch and need to monitor it until it gets
> applied.
>  
> http://lists.infradead.org/pipermail/linux-mtd/2009-August/027005.html
> http://patchwork.ozlabs.org/patch/31854/

Apologies for the delay. I'm slightly concerned that your patch
increases the size of the 'struct jffs2_tmp_dnode_info', and we've
previously taken care to _shrink_ that structure. We have _lots_ of
these.

Can you test this alternative approach instead, please?

diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 67f36c3..cc3aca9 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -231,9 +231,15 @@ struct jffs2_tmp_dnode_info
 	uint32_t version;
 	uint32_t data_crc;
 	uint32_t partial_crc;
-	uint32_t csize;
-	uint16_t overlapped;
+	uint32_t __csize; /* We abuse the top bit for the overlap flag */
 };
+#define TN_OVERLAP (1<<31)
+
+#define tn_set_overlapped(tn)	((tn)->__csize |= TN_OVERLAP)
+#define tn_clear_overlapped(tn)	((tn)->__csize &= ~TN_OVERLAP)
+
+#define tn_overlapped(tn)	(!!(((tn)->__csize) & TN_OVERLAP))
+#define tn_csize(tn)		((tn)->__csize & !TN_OVERLAP)
 
 /* Temporary data structure used during readinode. */
 struct jffs2_readinode_info
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 1a80301..a80cf02 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -35,20 +35,20 @@ static int check_node_data(struct jffs2_sb_info *c, struct jffs2_tmp_dnode_info
 	uint32_t crc, ofs, len;
 	size_t retlen;
 
-	BUG_ON(tn->csize == 0);
+	BUG_ON(tn_csize(tn) == 0);
 
 	/* Calculate how many bytes were already checked */
 	ofs = ref_offset(ref) + sizeof(struct jffs2_raw_inode);
-	len = tn->csize;
+	len = tn_csize(tn);
 
 	if (jffs2_is_writebuffered(c)) {
 		int adj = ofs % c->wbuf_pagesize;
 		if (likely(adj))
 			adj = c->wbuf_pagesize - adj;
 
-		if (adj >= tn->csize) {
+		if (adj >= tn_csize(tn)) {
 			dbg_readinode("no need to check node at %#08x, data length %u, data starts at %#08x - it has already been checked.\n",
-				      ref_offset(ref), tn->csize, ofs);
+				      ref_offset(ref), tn_csize(tn), ofs);
 			goto adj_acc;
 		}
 
@@ -57,7 +57,7 @@ static int check_node_data(struct jffs2_sb_info *c, struct jffs2_tmp_dnode_info
 	}
 
 	dbg_readinode("check node at %#08x, data length %u, partial CRC %#08x, correct CRC %#08x, data starts at %#08x, start checking from %#08x - %u bytes.\n",
-		ref_offset(ref), tn->csize, tn->partial_crc, tn->data_crc, ofs - len, ofs, len);
+		ref_offset(ref), tn_csize(tn), tn->partial_crc, tn->data_crc, ofs - len, ofs, len);
 
 #ifndef __ECOS
 	/* TODO: instead, incapsulate point() stuff to jffs2_flash_read(),
@@ -66,7 +66,7 @@ static int check_node_data(struct jffs2_sb_info *c, struct jffs2_tmp_dnode_info
 		err = c->mtd->point(c->mtd, ofs, len, &retlen,
 				    (void **)&buffer, NULL);
 		if (!err && retlen < len) {
-			JFFS2_WARNING("MTD point returned len too short: %zu instead of %u.\n", retlen, tn->csize);
+			JFFS2_WARNING("MTD point returned len too short: %zu instead of %u.\n", retlen, tn_csize(tn));
 			c->mtd->unpoint(c->mtd, ofs, retlen);
 		} else if (err)
 			JFFS2_WARNING("MTD point failed: error code %d.\n", err);
@@ -251,14 +251,14 @@ static int jffs2_add_tn_to_tree(struct jffs2_sb_info *c,
 	if (this) {
 		/* If the node is coincident with another at a lower address,
 		   back up until the other node is found. It may be relevant */
-		while (this->overlapped) {
+		while (tn_overlapped(this)) {
 			ptn = tn_prev(this);
 			if (!ptn) {
 				/*
 				 * We killed a node which set the overlapped
 				 * flags during the scan. Fix it up.
 				 */
-				this->overlapped = 0;
+				tn_clear_overlapped(this);
 				break;
 			}
 			this = ptn;
@@ -362,10 +362,10 @@ static int jffs2_add_tn_to_tree(struct jffs2_sb_info *c,
 				dbg_readinode("Node is overlapped by %p (v %d, 0x%x-0x%x)\n",
 					      this, this->version, this->fn->ofs,
 					      this->fn->ofs+this->fn->size);
-				tn->overlapped = 1;
+				tn_set_overlapped(tn);
 				break;
 			}
-			if (!this->overlapped)
+			if (!tn_overlapped(this))
 				break;
 
 			ptn = tn_prev(this);
@@ -374,7 +374,7 @@ static int jffs2_add_tn_to_tree(struct jffs2_sb_info *c,
 				 * We killed a node which set the overlapped
 				 * flags during the scan. Fix it up.
 				 */
-				this->overlapped = 0;
+				tn_clear_overlapped(this);
 				break;
 			}
 			this = ptn;
@@ -384,7 +384,7 @@ static int jffs2_add_tn_to_tree(struct jffs2_sb_info *c,
 	/* If the new node overlaps anything ahead, note it */
 	this = tn_next(tn);
 	while (this && this->fn->ofs < fn_end) {
-		this->overlapped = 1;
+		tn_set_overlapped(this);
 		dbg_readinode("Node ver %d, 0x%x-0x%x is overlapped\n",
 			      this->version, this->fn->ofs,
 			      this->fn->ofs+this->fn->size);
@@ -462,7 +462,7 @@ static int jffs2_build_inode_fragtree(struct jffs2_sb_info *c,
 	this = tn_last(&rii->tn_root);
 	while (this) {
 		dbg_readinode("tn %p ver %d range 0x%x-0x%x ov %d\n", this, this->version, this->fn->ofs,
-			      this->fn->ofs+this->fn->size, this->overlapped);
+			      this->fn->ofs+this->fn->size, tn_overlapped(this));
 		this = tn_prev(this);
 	}
 #endif
@@ -473,14 +473,14 @@ static int jffs2_build_inode_fragtree(struct jffs2_sb_info *c,
 		eat_last(&rii->tn_root, &last->rb);
 		ver_insert(&ver_root, last);
 
-		if (unlikely(last->overlapped)) {
+		if (unlikely(tn_overlapped(last))) {
 			if (pen)
 				continue;
 			/*
 			 * We killed a node which set the overlapped
 			 * flags during the scan. Fix it up.
 			 */
-			last->overlapped = 0;
+			tn_clear_overlapped(last);
 		}
 
 		/* Now we have a bunch of nodes in reverse version
@@ -510,7 +510,7 @@ static int jffs2_build_inode_fragtree(struct jffs2_sb_info *c,
 				}
 				dbg_readinode("Add %p (v %d, 0x%x-0x%x, ov %d) to fragtree\n",
 					     this, this->version, this->fn->ofs,
-					     this->fn->ofs+this->fn->size, this->overlapped);
+					     this->fn->ofs+this->fn->size, tn_overlapped(this));
 
 				ret = jffs2_add_full_dnode_to_inode(c, f, this->fn);
 				if (ret) {
@@ -836,9 +836,8 @@ static inline int read_dnode(struct jffs2_sb_info *c, struct jffs2_raw_node_ref
 	tn->version = je32_to_cpu(rd->version);
 	tn->fn->ofs = je32_to_cpu(rd->offset);
 	tn->data_crc = je32_to_cpu(rd->data_crc);
-	tn->csize = csize;
+	tn->__csize = csize;
 	tn->fn->raw = ref;
-	tn->overlapped = 0;
 
 	if (tn->version > rii->highest_version)
 		rii->highest_version = tn->version;
@@ -868,7 +867,7 @@ static inline int read_dnode(struct jffs2_sb_info *c, struct jffs2_raw_node_ref
 	while (tn) {
 		dbg_readinode2("%p: v %d r 0x%x-0x%x ov %d\n",
 			       tn, tn->version, tn->fn->ofs,
-			       tn->fn->ofs+tn->fn->size, tn->overlapped);
+			       tn->fn->ofs+tn->fn->size, tn_overlapped(tn));
 		tn = tn_next(tn);
 	}
 #endif


-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[David Woodhouse]

On Sat, 2009-09-19 at 13:40 -0700, David Woodhouse wrote:
> 
> +#define tn_csize(tn)           ((tn)->__csize & !TN_OVERLAP)

You'll want to fix that when you test, of course...

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

Hi David,

I don't have access to a system till Monday. I'll give it a try then.

Overall the patch looks good to me, except this line.

> +#define tn_overlapped(tn)	(!!(((tn)->__csize) & TN_OVERLAP))

Should just be this.

#define tn_overlapped(tn)	((tn)->__csize) & TN_OVERLAP)

Best Regards,

Victor Gallardo

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[David Woodhouse]

On Sat, 2009-09-19 at 22:57 -0700, Victor Gallardo wrote:
> Hi David,
> 
> I don't have access to a system till Monday. I'll give it a try then.

Thanks.

> Overall the patch looks good to me, except this line.
> 
> > +#define tn_overlapped(tn)	(!!(((tn)->__csize) & TN_OVERLAP))
> 
> Should just be this.
> 
> #define tn_overlapped(tn)	((tn)->__csize) & TN_OVERLAP)

You object to the !! ? 

That was intentional. Sometimes we print the value of tn_overlapped(),
and I wanted it to be 1, not a huge number.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

Hi David,

> > Overall the patch looks good to me, except this line.
> >
> > > +#define tn_overlapped(tn)	(!!(((tn)->__csize) & TN_OVERLAP))
> >
> > Should just be this.
> >
> > #define tn_overlapped(tn)	((tn)->__csize) & TN_OVERLAP)
> 
> You object to the !! ?
> 
> That was intentional. Sometimes we print the value of tn_overlapped(),
> and I wanted it to be 1, not a huge number.

Sorry about that. Yes, you are 100% correct.

I'll try to patch on Monday.

Best Regards,

Victor Gallardo

RE: [PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

Hi David,

> 
> Apologies for the delay. I'm slightly concerned that your patch
> increases the size of the 'struct jffs2_tmp_dnode_info', and we've
> previously taken care to _shrink_ that structure. We have _lots_ of
> these.
> 
> Can you test this alternative approach instead, please?
> 

Tested it on my system. Your patch works.

-Victor Gallardo

[PATCH] [JFFS2] Fix csize integer overflow issue due to truncation

[Victor Gallardo]

This fixes a kernel BUG_ON(tn->size == 0) panic in check_node_data
due to integer overflow in read_dnone().

The code incorrectly assigns a uin32_t local variable (csize) to
uint16_t structure member in jffs2_tmp_dnode_info. This results
in an overflow when the local variable csize is greater than
65536 (0x10000)

This issue is seen when kernel PAGE_SIZE is 64K.

The following example illustrates the issue:

fs/jffs2/nodelist.h
  struct jffs2_tmp_dnode_info
  {
    ...
    uint16_t csize;
    ...
  };

fs/jffs2/readinode.c
  static inline int read_dnode(...)
  {
    struct jffs2_tmp_dnode_info *tn;
    uint32_t len, csize;
    ...
    csize = je32_to_cpu(rd->csize);
    ...
    tn->csize = csize;  // <=== result truncated if > 0x10000
    ...
  }

  static int check_node_data(...)
  {
    ...
    BUG_ON(tn->csize == 0);
    ...
  }

Signed-off-by: Victor Gallardo <vgallardo@amcc.com>
Acked-by: Prodyut Hazarika <phazarika@amcc.com>
Acked-by: Feng Kan <fkan@amcc.com>
---
 fs/jffs2/nodelist.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/jffs2/nodelist.h b/fs/jffs2/nodelist.h
index 507ed6e..67f36c3 100644
--- a/fs/jffs2/nodelist.h
+++ b/fs/jffs2/nodelist.h
@@ -231,7 +231,7 @@ struct jffs2_tmp_dnode_info
 	uint32_t version;
 	uint32_t data_crc;
 	uint32_t partial_crc;
-	uint16_t csize;
+	uint32_t csize;
 	uint16_t overlapped;
 };
 
-- 
1.5.5