linux-kernel - fs/debugfs/inode.c: thoughts about "CID 101681 Dereference after null check"

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>] [day] [month] [year] [list]

Date:	Sat, 12 Apr 2014 20:47:51 +0200
From:	Toralf Förster <toralf.foerster@....de>
To:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	Linux Kernel <linux-kernel@...r.kernel.org>
Subject: fs/debugfs/inode.c: thoughts about "CID 101681 Dereference after
 null check"

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256


Just for fun (and to learn a little bit) I picked up an arbitrary entry from coverity [1] :

	CID 101681 Dereference after null check
	Either the check against null is unnecessary, or there may be a null pointer dereference.
	In debugfs_rename: Pointer is checked against null but then dereferenced anyway 

IMO it might be a real issue. At line 606 of fs/debugfs/inode.c we have:

		trap = lock_rename(new_dir, old_dir);
		/* Source or destination directories don't exist? */
		if (!old_dir->d_inode || !new_dir->d_inode)
			goto exit;

and later :
	exit:
		if (dentry && !IS_ERR(dentry))
			dput(dentry);
		unlock_rename(new_dir, old_dir);
		return NULL;

And in the file fs/namei.c starting with line 2481 we do have :

	void unlock_rename(struct dentry *p1, struct dentry *p2)
	{
		mutex_unlock(&p1->d_inode->i_mutex);
		if (p1 != p2) {
			mutex_unlock(&p2->d_inode->i_mutex);
			mutex_unlock(&p1->d_inode->i_sb->s_vfs_rename_mutex);
		}
	}


I'm wondering if the NULL check should be made before lock_rename()  and probably just returns NULL, eg.:

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 8c41b52..7ead0f6 100644
- --- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -603,10 +603,10 @@ struct dentry *debugfs_rename(struct dentry *old_dir, struct dentry *old_dentry,
        struct dentry *dentry = NULL, *trap;
        const char *old_name;

- -       trap = lock_rename(new_dir, old_dir);
        /* Source or destination directories don't exist? */
        if (!old_dir->d_inode || !new_dir->d_inode)
- -               goto exit;
+               return NULL;
+       trap = lock_rename(new_dir, old_dir);
        /* Source does not exist, cyclic rename, or mountpoint? */
        if (!old_dentry->d_inode || old_dentry == trap ||
            d_mountpoint(old_dentry))



[1] https://scan5.coverity.com:8443/reports.htm#v32611/p10063/fileInstanceId=55651059&defectInstanceId=17130709&mergedDefectId=101681&eventId=17130709-7

- -- 
MfG/Sincerely
Toralf Förster
pgp finger print:1A37 6F99 4A9D 026F 13E2 4DCF C4EA CDDE 0076 E94E
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlNJilcACgkQxOrN3gB26U5Y6AD7BYgw4eNSRZQsQj2B8dx5rr+S
TWXKJbTdU/YbBYMRU0wA/Anqpr/i9HoSLN20L57F6U+1uJir4WTXWCIenjV/jpkY
=MuEz
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/