potential memory corruption in check_leaf()

potential memory corruption in check_leaf()

[Dan Carpenter]

Hello Artem Bityutskiy,

The patch 1e51764a3c2a: "UBIFS: add new flash file system" from Jul
14, 2008, leads to the following static checker warning:

	fs/ubifs/debug.c:2039 check_leaf()
	warn: is 'node' large enough for 'struct ubifs_data_node'?

fs/ubifs/debug.c
  1978  static int check_leaf(struct ubifs_info *c, struct ubifs_zbranch *zbr,
  1979                        void *priv)
  1980  {
  1981          ino_t inum;
  1982          void *node;
  1983          struct ubifs_ch *ch;
  1984          int err, type = key_type(c, &zbr->key);
  1985          struct fsck_inode *fscki;
  1986  
  1987          if (zbr->len < UBIFS_CH_SZ) {
                    ^^^^^^^^^^^^^^^^^^^^^^^
We check that ->len is at least 24 bytes.

  1988                  ubifs_err("bad leaf length %d (LEB %d:%d)",
  1989                            zbr->len, zbr->lnum, zbr->offs);
  1990                  return -EINVAL;
  1991          }
  1992  
  1993          node = kmalloc(zbr->len, GFP_NOFS);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Allocate node.

  1994          if (!node)
  1995                  return -ENOMEM;
  1996  
  1997          err = ubifs_tnc_read_node(c, zbr, node);
  1998          if (err) {
  1999                  ubifs_err("cannot read leaf node at LEB %d:%d, error %d",
  2000                            zbr->lnum, zbr->offs, err);
  2001                  goto out_free;
  2002          }
  2003  
  2004          /* If this is an inode node, add it to RB-tree of inodes */
  2005          if (type == UBIFS_INO_KEY) {
  2006                  fscki = add_inode(c, priv, node);
  2007                  if (IS_ERR(fscki)) {
  2008                          err = PTR_ERR(fscki);
  2009                          ubifs_err("error %d while adding inode node", err);
  2010                          goto out_dump;
  2011                  }
  2012                  goto out;
  2013          }
  2014  
  2015          if (type != UBIFS_DENT_KEY && type != UBIFS_XENT_KEY &&
  2016              type != UBIFS_DATA_KEY) {
  2017                  ubifs_err("unexpected node type %d at LEB %d:%d",
  2018                            type, zbr->lnum, zbr->offs);
  2019                  err = -EINVAL;
  2020                  goto out_free;
  2021          }
  2022  
  2023          ch = node;
                ^^^^^^^^^^
24 bytes is large enough for "ch".

  2024          if (le64_to_cpu(ch->sqnum) > c->max_sqnum) {
  2025                  ubifs_err("too high sequence number, max. is %llu",
  2026                            c->max_sqnum);
  2027                  err = -EINVAL;
  2028                  goto out_dump;
  2029          }
  2030  
  2031          if (type == UBIFS_DATA_KEY) {
  2032                  long long blk_offs;
  2033                  struct ubifs_data_node *dn = node;
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
But it's not large enough for "dn".

  2034  
  2035                  /*
  2036                   * Search the inode node this data node belongs to and insert
  2037                   * it to the RB-tree of inodes.
  2038                   */
  2039                  inum = key_inum_flash(c, &dn->key);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The check waits until we use "dn" before complaining, in case there is
another size check after the assignment.

Also on the other side of the if statement:
fs/ubifs/debug.c:2071 check_leaf() warn: is 'node' large enough for 'struct ubifs_dent_node'?

regards,
dan carpenter

Re: potential memory corruption in check_leaf()

[Artem Bityutskiy]

On Thu, 2014-11-06 at 13:09 +0300, Dan Carpenter wrote:
>   1988                  ubifs_err("bad leaf length %d (LEB %d:%d)",
>   1989                            zbr->len, zbr->lnum, zbr->offs);
>   1990                  return -EINVAL;
>   1991          }

Yes, this code is a small sanity check. zbr->len is supposed to be the
length of whatever node type is referred by this znode branch.


>   1993          node = kmalloc(zbr->len, GFP_NOFS);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Allocate node.

Supposedly we allocate enough to store the node we refer to.

>   2031          if (type == UBIFS_DATA_KEY) {
>   2032                  long long blk_offs;
>   2033                  struct ubifs_data_node *dn = node;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> But it's not large enough for "dn".

Well, this should not happen, but let's add an assert here to check that
we have enough space for 'dn'.

Something like this.


>From 6785baa1697c15a51408e7317709cbf078604695 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Tue, 25 Nov 2014 16:41:26 +0200
Subject: [PATCH] UBIFS: add a couple of extra asserts

... to catch possible memory corruptions.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/ubifs/debug.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/ubifs/debug.c b/fs/ubifs/debug.c
index 7ed13e1..4cfb3e8 100644
--- a/fs/ubifs/debug.c
+++ b/fs/ubifs/debug.c
@@ -2032,6 +2032,8 @@ static int check_leaf(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 		long long blk_offs;
 		struct ubifs_data_node *dn = node;
 
+		ubifs_assert(zbr->len >= UBIFS_DATA_NODE_SZ);
+
 		/*
 		 * Search the inode node this data node belongs to and insert
 		 * it to the RB-tree of inodes.
@@ -2060,6 +2062,8 @@ static int check_leaf(struct ubifs_info *c, struct ubifs_zbranch *zbr,
 		struct ubifs_dent_node *dent = node;
 		struct fsck_inode *fscki1;
 
+		ubifs_assert(zbr->len >= UBIFS_DENT_NODE_SZ);
+
 		err = ubifs_validate_entry(c, dent);
 		if (err)
 			goto out_dump;
-- 
1.9.3