All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Al Viro <viro@zeniv.linux.org.uk>
Cc: Ben Woodard <woodard@redhat.com>,
	James Bottomley <James.Bottomley@HansenPartnership.com>,
	Jim Foraker <foraker1@llnl.gov>,
	Kees Cook <keescook@chromium.org>,
	Travis Gummels <tgummels@redhat.com>,
	linux-kernel@vger.kernel.org
Subject: [PATCH 2/5] exec: binfmt_misc: shift filp_close(interp_file) from kill_node() to bm_evict_inode()
Date: Fri, 22 Sep 2017 16:36:44 +0200	[thread overview]
Message-ID: <20170922143644.GA17216@redhat.com> (raw)
In-Reply-To: <20170922143619.GA17179@redhat.com>

to ensure that load_misc_binary() can't use the partially destroyed
Node, see also the next patch.

The current logic looks wrong in any case, once we close interp_file
it doesn't make any sense to delay kfree(inode->i_private), this Node
is no longer valid. Even if the MISC_FMT_OPEN_FILE/interp_file checks
were not racy (they are), load_misc_binary() should not try to reopen
->interpreter if MISC_FMT_OPEN_FILE is set but ->interp_file is NULL.

And I can't understand why do we use filp_close(), not fput().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 fs/binfmt_misc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index f4de5ae..040ed26 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -591,8 +591,13 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode)
 
 static void bm_evict_inode(struct inode *inode)
 {
+	Node *e = inode->i_private;
+
+	if ((e->flags & MISC_FMT_OPEN_FILE) && e->interp_file)
+		filp_close(e->interp_file, NULL);
+
 	clear_inode(inode);
-	kfree(inode->i_private);
+	kfree(e);
 }
 
 static void kill_node(Node *e)
@@ -603,11 +608,6 @@ static void kill_node(Node *e)
 	list_del_init(&e->list);
 	write_unlock(&entries_lock);
 
-	if ((e->flags & MISC_FMT_OPEN_FILE) && e->interp_file) {
-		filp_close(e->interp_file, NULL);
-		e->interp_file = NULL;
-	}
-
 	dentry = e->dentry;
 	drop_nlink(d_inode(dentry));
 	d_drop(dentry);
-- 
2.5.0

  parent reply	other threads:[~2017-09-22 14:37 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-22 14:36 [PATCH 0/5] exec: binfmt_misc: fix use-after-free, kill iname[BINPRM_BUF_SIZE] Oleg Nesterov
2017-09-22 14:36 ` [PATCH 1/5] exec: binfmt_misc: don't nullify Node->dentry in kill_node() Oleg Nesterov
2017-09-22 14:36 ` Oleg Nesterov [this message]
2017-09-22 14:36 ` [PATCH 3/5] exec: binfmt_misc: remove the confusing e->interp_file != NULL checks Oleg Nesterov
2017-09-22 14:36 ` [PATCH 4/5] exec: binfmt_misc: fix race between load_misc_binary() and kill_node() Oleg Nesterov
2017-09-22 14:36 ` [PATCH 5/5] exec: binfmt_misc: kill the onstack iname[BINPRM_BUF_SIZE] array Oleg Nesterov
2017-09-22 15:24 ` [PATCH 0/5] exec: binfmt_misc: fix use-after-free, kill iname[BINPRM_BUF_SIZE] Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170922143644.GA17216@redhat.com \
    --to=oleg@redhat.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=akpm@linux-foundation.org \
    --cc=foraker1@llnl.gov \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tgummels@redhat.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=woodard@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.