[PATCH v2 1/2] jffs2: validate symlink size in jffs2_do_read_inode_internal()

[PATCH v2 1/2] jffs2: validate symlink size in jffs2_do_read_inode_internal()

[Xi Wang]

`csize' is read from disk and thus needs validation.  Otherwise a bogus
value 0xffffffff would turn the subsequent kmalloc(csize + 1, ...) into
kmalloc(0, ...), leading to out-of-bounds write.

This patch limits `csize' to JFFS2_MAX_NAME_LEN, which is also used
in jffs2_symlink().

Signed-off-by: Xi Wang <xi.wang@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
---
 fs/jffs2/readinode.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index dc0437e..9897f38 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -1266,6 +1266,12 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 			/* Symlink's inode data is the target path. Read it and
 			 * keep in RAM to facilitate quick follow symlink
 			 * operation. */
+			uint32_t csize = je32_to_cpu(latest_node->csize);
+			if (csize > JFFS2_MAX_NAME_LEN) {
+				mutex_unlock(&f->sem);
+				jffs2_do_clear_inode(c, f);
+				return -ENAMETOOLONG;
+			}
 			f->target = kmalloc(je32_to_cpu(latest_node->csize) + 1, GFP_KERNEL);
 			if (!f->target) {
 				JFFS2_ERROR("can't allocate %d bytes of memory for the symlink target path cache\n", je32_to_cpu(latest_node->csize));
-- 
1.7.5.4

[PATCH v2 2/2] jffs2: refactor csize usage in jffs2_do_read_inode_internal()

[Xi Wang]

Replace the verbose `je32_to_cpu(latest_node->csize)' with a shorter
`csize'.

Signed-off-by: Xi Wang <xi.wang@gmail.com>
Cc: Artem Bityutskiy <dedekind1@gmail.com>
---
 fs/jffs2/readinode.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c
index 9897f38..3833d74 100644
--- a/fs/jffs2/readinode.c
+++ b/fs/jffs2/readinode.c
@@ -1272,19 +1272,19 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 				jffs2_do_clear_inode(c, f);
 				return -ENAMETOOLONG;
 			}
-			f->target = kmalloc(je32_to_cpu(latest_node->csize) + 1, GFP_KERNEL);
+			f->target = kmalloc(csize + 1, GFP_KERNEL);
 			if (!f->target) {
-				JFFS2_ERROR("can't allocate %d bytes of memory for the symlink target path cache\n", je32_to_cpu(latest_node->csize));
+				JFFS2_ERROR("can't allocate %u bytes of memory for the symlink target path cache\n", csize);
 				mutex_unlock(&f->sem);
 				jffs2_do_clear_inode(c, f);
 				return -ENOMEM;
 			}
 
 			ret = jffs2_flash_read(c, ref_offset(rii.latest_ref) + sizeof(*latest_node),
-						je32_to_cpu(latest_node->csize), &retlen, (char *)f->target);
+					       csize, &retlen, (char *)f->target);
 
-			if (ret  || retlen != je32_to_cpu(latest_node->csize)) {
-				if (retlen != je32_to_cpu(latest_node->csize))
+			if (ret || retlen != csize) {
+				if (retlen != csize)
 					ret = -EIO;
 				kfree(f->target);
 				f->target = NULL;
@@ -1293,7 +1293,7 @@ static int jffs2_do_read_inode_internal(struct jffs2_sb_info *c,
 				return ret;
 			}
 
-			f->target[je32_to_cpu(latest_node->csize)] = '\0';
+			f->target[csize] = '\0';
 			dbg_readinode("symlink's target '%s' cached\n", f->target);
 		}
 
-- 
1.7.5.4

Re: [PATCH v2 1/2] jffs2: validate symlink size in jffs2_do_read_inode_internal()

[Artem Bityutskiy]

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

On Wed, 2012-04-25 at 14:45 -0400, Xi Wang wrote:
> `csize' is read from disk and thus needs validation.  Otherwise a bogus
> value 0xffffffff would turn the subsequent kmalloc(csize + 1, ...) into
> kmalloc(0, ...), leading to out-of-bounds write.
> 
> This patch limits `csize' to JFFS2_MAX_NAME_LEN, which is also used
> in jffs2_symlink().

I think your commit message is a not general enough because it talks
about 0xFFFFFFFF value, but there may be any other large value as well.
I've added the following cause to the commit message and pushed both
patches to l2-mtd.git, thanks! Please, verify.

The clause:

"Artem: we actually validate csize by checking CRC, so this 0xFFs cannot
come from empty flash region. But I guess an attacker could feed JFFS2
an image with random csize value, including 0xFFs."

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 1/2] jffs2: validate symlink size in jffs2_do_read_inode_internal()

[Xi Wang]

On Apr 29, 2012, at 11:44 AM, Artem Bityutskiy wrote:
> 
> I think your commit message is a not general enough because it talks
> about 0xFFFFFFFF value, but there may be any other large value as well.
> I've added the following cause to the commit message and pushed both
> patches to l2-mtd.git, thanks! Please, verify.
> 
> The clause:
> 
> "Artem: we actually validate csize by checking CRC, so this 0xFFs cannot
> come from empty flash region. But I guess an attacker could feed JFFS2
> an image with random csize value, including 0xFFs."

Looks good to me.  Thanks!

- xi