linux-kernel - Re: [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while iterating entries

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b82cd435-437d-e384-c95e-a7e031559c7e@huawei.com>
Date:   Mon, 14 Sep 2020 11:24:55 +0800
From:   Zhihao Cheng <chengzhihao1@...wei.com>
To:     Richard Weinberger <richard.weinberger@...il.com>
CC:     <linux-mtd@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Richard Weinberger <richard@....at>,
        "zhangyi (F)" <yi.zhang@...wei.com>
Subject: Re: [PATCH 1/2] ubifs: xattr: Fix some potential memory leaks while
 iterating entries

在 2020/9/14 3:08, Richard Weinberger 写道:
> On Mon, Jun 1, 2020 at 11:11 AM Zhihao Cheng <chengzhihao1@...wei.com> wrote:
>>

> I agree that this needs fixing. Did you also look into getting rid of pxent?
> UBIFS uses the pxent pattern over and over and the same error got copy pasted
> a lot. :-(
> 
I thought about it. I'm not sure whether it is good to drop 'pxent'. 
Maybe you mean doing changes looks like following(Takes 
ubifs_jnl_write_inode() for example):

diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
index 4a5b06f8d812..fcd5ac047b34 100644
--- a/fs/ubifs/journal.c
+++ b/fs/ubifs/journal.c
@@ -879,13 +879,19 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
  		union ubifs_key key;
  		struct fscrypt_name nm = {0};
  		struct inode *xino;
-		struct ubifs_dent_node *xent, *pxent = NULL;
+		struct ubifs_dent_node *xent;

  		if (ui->xattr_cnt >= ubifs_xattr_max_cnt(c)) {
  			ubifs_err(c, "Cannot delete inode, it has too much xattrs!");
  			goto out_release;
  		}

+		fname_name(&nm) = kmalloc(UBIFS_MAX_NLEN, GFP_NOFS);
+		if (!fname_name(&nm)) {
+			err = -ENOMEM;
+			goto out_release;
+		}
+
  		lowest_xent_key(c, &key, inode->i_ino);
  		while (1) {
  			xent = ubifs_tnc_next_ent(c, &key, &nm);
@@ -894,11 +900,12 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
  				if (err == -ENOENT)
  					break;

+				kfree(fname_name(&nm));
  				goto out_release;
  			}

-			fname_name(&nm) = xent->name;
  			fname_len(&nm) = le16_to_cpu(xent->nlen);
+			strncpy(fname_name(&nm), xent->name, fname_len(&nm));

  			xino = ubifs_iget(c->vfs_sb, le64_to_cpu(xent->inum));
  			if (IS_ERR(xino)) {
@@ -907,6 +914,7 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
  					  xent->name, err);
  				ubifs_ro_mode(c, err);
  				kfree(xent);
+				kfree(fname_name(&nm));
  				goto out_release;
  			}
  			ubifs_assert(c, ubifs_inode(xino)->xattr);
@@ -916,11 +924,10 @@ int ubifs_jnl_write_inode(struct ubifs_info *c, 
const struct inode *inode)
  			ino = (void *)ino + UBIFS_INO_NODE_SZ;
  			iput(xino);

-			kfree(pxent);
-			pxent = xent;
  			key_read(c, &xent->key, &key);
+			kfree(xent);
  		}
-		kfree(pxent);
+		kfree(fname_name(&nm));
  	}

  	pack_inode(c, ino, inode, 1);

The kill_xattrs process is more intuitive without the pxent. However, 
the release process for the memory (stores xent->name) is similar to 
'pxent'. If you think it's better than v1, I will send v2.