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

login
register
mail settings
Submitter Xi Wang
Date April 25, 2012, 6:45 p.m.
Message ID <1335379523-31415-1-git-send-email-xi.wang@gmail.com>
Download mbox | patch
Permalink /patch/155057/
State New
Headers show

Comments

Xi Wang - April 25, 2012, 6:45 p.m.
`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(-)
Artem Bityutskiy - April 29, 2012, 3:44 p.m.
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."
Xi Wang - April 29, 2012, 9:45 p.m.
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

Patch

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