Fix csize integer overflow issue due to truncation
A message that is not in the archive (sent off-list or not yet received): [email protected]
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
Quoted 5 lines · 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).
Quoted 8 lines
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:
// the secretary.
Signature · 2 lines
Best Regards, Artem Bityutskiy (Артём Битюцкий)
Quoted 10 lines
Quoted 5 lines
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:
Thanks Artem, I did not know to look here.
Best Regards,
Victor Gallardo
David Woodhouse
maintainerQuoted 10 lines · 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.
Quoted 5 lines
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?
fs/jffs2/nodelist.h +8 -2
@@ -231,9 +231,15 @@ struct jffs2_tmp_dnode_infouint32_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
fs/jffs2/readinode.c +18 -19
@@ -35,20 +35,20 @@ static int check_node_data(struct jffs2_sb_info *c, struct jffs2_tmp_dnode_infouint32_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_refwhile (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
Signature · 2 lines
David Woodhouse Open Source Technology Centre [email protected] Intel Corporation
David Woodhouse
maintainerQuoted 2 lines · David Woodhouse
+#define
tn_csize(tn)((tn)->__csize & !TN_OVERLAP)
You'll want to fix that when you test, of course...
Signature · 2 lines
David Woodhouse Open Source Technology Centre [email protected] Intel Corporation
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.
Quoted 1 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
David Woodhouse
maintainerQuoted 3 lines · Victor Gallardo
Hi David,
I don't have access to a system till Monday. I'll give it a try then.
Thanks.
Quoted 7 lines
Overall the patch looks good to me, except this line.
Quoted 1 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.
Signature · 2 lines
David Woodhouse Open Source Technology Centre [email protected] Intel Corporation
Hi David,
Quoted 4 lines
Quoted 4 lines
Overall the patch looks good to me, except this line.
Quoted 1 line
+#define
tn_overlapped(tn)(!!(((tn)->__csize) &TN_OVERLAP))
Quoted 8 lines
Quoted 3 lines
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
Hi David,
Quoted 8 lines
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