From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E6258C10DCE for ; Thu, 12 Mar 2020 14:38:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C93142071B for ; Thu, 12 Mar 2020 14:38:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727541AbgCLOiG (ORCPT ); Thu, 12 Mar 2020 10:38:06 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:52972 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727340AbgCLOiF (ORCPT ); Thu, 12 Mar 2020 10:38:05 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1jCOyH-00ABbH-Uc; Thu, 12 Mar 2020 14:38:02 +0000 Date: Thu, 12 Mar 2020 14:38:01 +0000 From: Al Viro To: Tetsuo Handa Cc: linux-fsdevel , Alexei Starovoitov , "David S. Miller" Subject: Re: [PATCH] umh: fix refcount underflow in fork_usermode_blob(). Message-ID: <20200312143801.GJ23230@ZenIV.linux.org.uk> References: <2a8775b4-1dd5-9d5c-aa42-9872445e0942@i-love.sakura.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <2a8775b4-1dd5-9d5c-aa42-9872445e0942@i-love.sakura.ne.jp> Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org On Thu, Mar 12, 2020 at 10:43:00PM +0900, Tetsuo Handa wrote: > Before thinking how to fix a bug that tomoyo_realpath_nofollow() from > tomoyo_find_next_domain() likely fails with -ENOENT whenever > fork_usermode_blob() is used because 449325b52b7a6208 did not take into > account that TOMOYO security module needs to calculate symlink's pathname, > is this a correct fix for a bug that file_inode(file)->i_writecount != 0 > and file->f_count < 0 ? > - if (!file) > + if (!file) { > file = do_open_execat(fd, filename, flags); > - retval = PTR_ERR(file); > - if (IS_ERR(file)) > - goto out_unmark; > + retval = PTR_ERR(file); > + if (IS_ERR(file)) > + goto out_unmark; > + } else { > + retval = deny_write_access(file); > + if (retval) > + goto out_unmark; > + get_file(file); > + } *UGH* Something's certainly fishy with the refcounting there. First of all, bprm->file is a counting reference (observe what free_bprm() is doing). So as it is, on success __do_execve_file() consumes the reference passed to it in 'file', ditto for do_execve_file(). However, it's inconsistent - failure of e.g. bprm allocation leaves the reference unconsumed. Your change makes it consistent in that respect, but it means that in normal case you are getting refcount higher by 1 than the mainline. Does the mainline have an extra fput() *in* *normal* *case*? I can easily believe in buggered cleanups on failure, but... Has that code ever been tested? It _does_ look like that double-fput() is real, but I'd like a confirmation before going further - umh is convoluted enough for something subtle to be hidden there. Alexei, what the refcounting behaviour was supposed to be? As in "this function consumes the reference passed to it in this argument", etc.