linux-kernel - Re: file locking fix for 3.2

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: <alpine.LFD.2.02.1112261035240.27600@i5.linux-foundation.org>
Date:	Mon, 26 Dec 2011 10:37:56 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	"J. Bruce Fields" <bfields@...ldses.org>
cc:	Al Viro <viro@...iv.linux.org.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-nfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: file locking fix for 3.2


I'm committing the appended version instead. It removes all the games with 
IS_ERR(), and just does the checking at the allocation point, the way the 
other lease_alloc() user does too.

Holler if you see something odd in there..

                   Linus
---
commit 6d4b9e38d3980826abccfbd90e95bf4bd41b8dd2
Author: Linus Torvalds <torvalds@...ux-foundation.org>
Date:   Mon Dec 26 10:25:26 2011 -0800

    vfs: fix handling of lock allocation failure in lease-break case
    
    Bruce Fields notes that commit 778fc546f749 ("locks: fix tracking of
    inprogress lease breaks") introduced a possible error pointer
    dereference on failure to allocate memory.  locks_conflict() will
    dereference the passed-in new lease lock structure that may be an error pointer.
    
    This means an open (without O_NONBLOCK set) on a file with a lease
    applied (generally only done when Samba or nfsd (with v4) is running)
    could crash if a kmalloc() fails.
    
    So instead of playing games with IS_ERROR() all over the place, just
    check the allocation failure early.  That makes the code more
    straightforward, and avoids this possible bad pointer dereference.
    
    Based-on-patch-by: J. Bruce Fields <bfields@...hat.com>
    Cc: Al Viro <viro@...iv.linux.org.uk>
    Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
---
 fs/locks.c |   11 +++--------
 1 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 3b0d05dcd7c1..637694bf3a03 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1205,6 +1205,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	int want_write = (mode & O_ACCMODE) != O_RDONLY;
 
 	new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
+	if (IS_ERR(new_fl))
+		return PTR_ERR(new_fl);
 
 	lock_flocks();
 
@@ -1221,12 +1223,6 @@ int __break_lease(struct inode *inode, unsigned int mode)
 		if (fl->fl_owner == current->files)
 			i_have_this_lease = 1;
 
-	if (IS_ERR(new_fl) && !i_have_this_lease
-			&& ((mode & O_NONBLOCK) == 0)) {
-		error = PTR_ERR(new_fl);
-		goto out;
-	}
-
 	break_time = 0;
 	if (lease_break_time > 0) {
 		break_time = jiffies + lease_break_time * HZ;
@@ -1284,8 +1280,7 @@ restart:
 
 out:
 	unlock_flocks();
-	if (!IS_ERR(new_fl))
-		locks_free_lock(new_fl);
+	locks_free_lock(new_fl);
 	return error;
 }
 
--
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/