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=-2.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham 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 BC7FAC10F11 for ; Wed, 24 Apr 2019 13:07:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 96F1721773 for ; Wed, 24 Apr 2019 13:07:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730377AbfDXNHQ (ORCPT ); Wed, 24 Apr 2019 09:07:16 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:52800 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729384AbfDXNHQ (ORCPT ); Wed, 24 Apr 2019 09:07:16 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.92 #3 (Red Hat Linux)) id 1hJHcF-0002pH-9h; Wed, 24 Apr 2019 13:07:11 +0000 Date: Wed, 24 Apr 2019 14:07:11 +0100 From: Al Viro To: Mukesh Ojha Cc: "dmitry.torokhov@gmail.com" , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Gaurav Kohli , Peter Hutterer , Martin Kepplinger , "Paul E. McKenney" Subject: Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock Message-ID: <20190424130711.GP2217@ZenIV.linux.org.uk> References: <20190418014321.dptin7tpxpldhsns@penguin> <20190419071152.x5ghvbybjhv76uxt@penguin> <20190423032839.xvbldglrmjxkdntj@penguin> <17f4a0be-ab04-8537-9197-32fbca807f3f@codeaurora.org> <20190423084944.gj2boxfcg7lp4zad@penguin> <20190423110611.GL2217@ZenIV.linux.org.uk> <5614f04f-827d-1668-9ed0-60d93e110b8e@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <5614f04f-827d-1668-9ed0-60d93e110b8e@codeaurora.org> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 24, 2019 at 05:40:40PM +0530, Mukesh Ojha wrote: > > Al, > > i tried to put traceprintk inside ioctl after fdget and fdput on a simple > call of open  => ioctl => close in a loop, and multithreaded, presumably? > on /dev/uinput. > >           uinput-532   [002] ....    45.312044: SYSC_ioctl: 2     <= f_count > >              uinput-532   [002] ....    45.312055: SYSC_ioctl: 2            >           uinput-532   [004] ....    45.313766: uinput_open: uinput: 1 >           uinput-532   [004] ....    45.313783: SYSC_ioctl: 1 >           uinput-532   [004] ....    45.313788: uinput_ioctl_handler: > uinput: uinput_ioctl_handler, 1 >           uinput-532   [004] ....    45.313835: SYSC_ioctl: 1 >           uinput-532   [004] ....    45.313843: uinput_release: uinput:  0 > > > So while a ioctl is running the f_count is 1, so a fput could be run and do > atomic_long_dec_and_test > this could call release right ? Look at ksys_ioctl(): int ksys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg) { int error; struct fd f = fdget(fd); an error or refcount bumped if (!f.file) return -EBADF; not an error, then. We know that ->release() won't be called until we drop the reference we've just acquired. error = security_file_ioctl(f.file, cmd, arg); if (!error) error = do_vfs_ioctl(f.file, fd, cmd, arg); ... and we are done with calling ->ioctl(), so fdput(f); ... we drop the reference we'd acquired. Seeing refcount 1 inside ->ioctl() is possible, all right: CPU1: ioctl(2) resolves fd to struct file *, refcount 2 CPU2: close(2) rips struct file * from descriptor table and does fput() to drop it; refcount reaches 1 and fput() is done; no call of ->release() yet. CPU1: we get arouund to ->ioctl(), where your trace sees refcount 1 CPU1: done with ->ioctl(), drop our reference. *NOW* refcount gets to 0, and ->release() is called. IOW, in your trace fput() has already been run by close(2); having somebody else do that again while we are in ->ioctl() would be a bug (to start with, where did they get that struct file * and why wasn't that reference contributing to struct file refcount?) In all cases we only call ->release() once all references gone - both the one(s) in descriptor tables and any transient ones acquired by fdget(), etc. I would really like to see a reproducer for the original use-after-free report...