From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756091AbdJJLQu (ORCPT ); Tue, 10 Oct 2017 07:16:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45620 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755400AbdJJLQt (ORCPT ); Tue, 10 Oct 2017 07:16:49 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1754CC058EBF Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=oleg@redhat.com Date: Tue, 10 Oct 2017 13:16:47 +0200 From: Oleg Nesterov To: Tycho Andersen Cc: Kees Cook , linux-kernel@vger.kernel.org Subject: Re: null dereference in binfmt misc Message-ID: <20171010111647.GA27310@redhat.com> References: <20171009211940.rtgjt7zayj5kftic@smitten> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171009211940.rtgjt7zayj5kftic@smitten> User-Agent: Mutt/1.5.24 (2015-08-30) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 10 Oct 2017 11:16:49 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/09, Tycho Andersen wrote: > Hi, > > It looks like eb23aa031 ("exec: binfmt_misc: remove the confusing > e->interp_file != NULL checks") uncovered a bug for me (see the trace below, > which I'm afraid isn't very helpful). Well, I think this commit uncovered the fact I am stupid, although there is nothing new. I forgot about iput() in bm_register_write's error paths, it can be called with MISC_FMT_OPEN_FILE && interp_file == NULL. I'll try to cleanup bm_register_write() to make this impossible, or perhaps I will just restore the interp_file != NULL check in evict. Before that, could you please try the debugging patch below? To ensure you didn't hit another problem. Thanks! Oleg. --- a/fs/binfmt_misc.c +++ b/fs/binfmt_misc.c @@ -589,12 +589,18 @@ static struct inode *bm_get_inode(struct super_block *sb, int mode) return inode; } +#define XXX (void*)1234 + static void bm_evict_inode(struct inode *inode) { Node *e = inode->i_private; - if (e->flags & MISC_FMT_OPEN_FILE) - filp_close(e->interp_file, NULL); + if (e->flags & MISC_FMT_OPEN_FILE) { + if (e->interp_file == XXX) + pr_err("register: hit XXX\n"); + else + filp_close(e->interp_file, NULL); + } clear_inode(inode); kfree(e); @@ -687,7 +693,6 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, int err = 0; e = create_entry(buffer, count); - if (IS_ERR(e)) return PTR_ERR(e); @@ -709,6 +714,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count); if (err) { + pr_err("register: failed to pin, f=%d", !!(e->flags & MISC_FMT_OPEN_FILE)); + if (e->flags & MISC_FMT_OPEN_FILE) + e->interp_file = XXX; iput(inode); inode = NULL; goto out2; @@ -720,7 +728,8 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer, f = open_exec(e->interpreter); if (IS_ERR(f)) { err = PTR_ERR(f); - pr_notice("register: failed to install interpreter file %s\n", e->interpreter); + pr_err("register: failed to install interpreter\n"); + e->interp_file = XXX; simple_release_fs(&bm_mnt, &entry_count); iput(inode); inode = NULL;