| Message ID | 1334011379-24445-1-git-send-email-xi.wang@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 2012-04-09 at 18:42 -0400, Xi Wang wrote: > Replace the verbose `je32_to_cpu(latest_node->csize)' with a shorter > variable `csize'. > > Also check for a bogus `csize' value 0xffffffff, which would turn the > subsequent kmalloc(cisze + 1, ...) into kmalloc(0, ...). > > Signed-off-by: Xi Wang <xi.wang@gmail.com> Looks good. I wonder, does it fix a real-life issue or you spotted this place while reviewing/fixing warnings/other janitor type of work?
On Apr 21, 2012, at 10:12 AM, Artem Bityutskiy wrote: > Looks good. I wonder, does it fix a real-life issue or you spotted this > place while reviewing/fixing warnings/other janitor type of work? Thanks for reviewing. ;-) The patch tries to fix a warning issued by a homemade static analysis tool that looks for integer overflows. - xi
On Mon, 2012-04-09 at 18:42 -0400, Xi Wang wrote: > Replace the verbose `je32_to_cpu(latest_node->csize)' with a shorter > variable `csize'. > > Also check for a bogus `csize' value 0xffffffff, which would turn the > subsequent kmalloc(cisze + 1, ...) into kmalloc(0, ...). > > Signed-off-by: Xi Wang <xi.wang@gmail.com> Please, introduce the variable in a separate patch. WRT the csize check - you should compare it to something more sensible than INT_MAX - try to dig the code and find out what is maximum value JFFS2 expects.
On Apr 22, 2012, at 7:57 AM, Artem Bityutskiy wrote: > Please, introduce the variable in a separate patch. Okay. > WRT the csize check - you should compare it to something more sensible > than INT_MAX - try to dig the code and find out what is maximum value > JFFS2 expects. KMALLOC_MAX_SIZE - 1 is the smallest number I can find, since the code calls `kmalloc(csize + 1)'. Does that look good? - xi
On Mon, 2012-04-23 at 01:43 -0400, Xi Wang wrote: > On Apr 22, 2012, at 7:57 AM, Artem Bityutskiy wrote: > > > Please, introduce the variable in a separate patch. > > Okay. > > > WRT the csize check - you should compare it to something more sensible > > than INT_MAX - try to dig the code and find out what is maximum value > > JFFS2 expects. > > KMALLOC_MAX_SIZE - 1 is the smallest number I can find, since the code > calls `kmalloc(csize + 1)'. Does that look good? I think JFFS2 has its own limit on the maximum size of the symlink target. Probably it is PAGE_CACHE_SIZE, but not sure.
On Apr 23, 2012, at 2:00 AM, Artem Bityutskiy wrote: > > I think JFFS2 has its own limit on the maximum size of the symlink > target. Probably it is PAGE_CACHE_SIZE, but not sure. Is it this one? fs/jffs2/dir.c:297 static int jffs2_symlink (struct inode *dir_i, struct dentry *dentry, const char *target) { ... int targetlen = strlen(target); /* FIXME: If you care. We'd need to use frags for the target if it grows much more than this */ if (targetlen > 254) return -ENAMETOOLONG; ... } I guess the magic value 254 is JFFS2_MAX_NAME_LEN defined in include/linux/jffs2.h. - xi
diff --git a/fs/jffs2/readinode.c b/fs/jffs2/readinode.c index dc0437e..2be7a8e 100644 --- a/fs/jffs2/readinode.c +++ b/fs/jffs2/readinode.c @@ -1266,19 +1266,24 @@ 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. */ - f->target = kmalloc(je32_to_cpu(latest_node->csize) + 1, GFP_KERNEL); + uint32_t csize = je32_to_cpu(latest_node->csize); + /* Avoid overflowing csize + 1. */ + if (csize > INT_MAX) + f->target = 0; + else + 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; @@ -1287,7 +1292,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); }
Replace the verbose `je32_to_cpu(latest_node->csize)' with a shorter variable `csize'. Also check for a bogus `csize' value 0xffffffff, which would turn the subsequent kmalloc(cisze + 1, ...) into kmalloc(0, ...). Signed-off-by: Xi Wang <xi.wang@gmail.com> --- fs/jffs2/readinode.c | 17 +++++++++++------ 1 files changed, 11 insertions(+), 6 deletions(-)