From patchwork Fri May 3 18:26:32 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 13653310 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 8EE6EC4345F for ; Fri, 3 May 2024 18:26:35 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C86F41131D5; Fri, 3 May 2024 18:26:34 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="d/XXHdU+"; dkim-atps=neutral Received: from mail-pf1-f177.google.com (mail-pf1-f177.google.com [209.85.210.177]) by gabe.freedesktop.org (Postfix) with ESMTPS id C12181131D5 for ; Fri, 3 May 2024 18:26:33 +0000 (UTC) Received: by mail-pf1-f177.google.com with SMTP id d2e1a72fcca58-6f44b296e1fso7298b3a.3 for ; Fri, 03 May 2024 11:26:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1714760793; x=1715365593; darn=lists.freedesktop.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=jqBoL43PMGyCqFXn3ehXdH+uWie7AbSm2FUzxDYORqI=; b=d/XXHdU+OrJ4YuJcPaXK0mnINgszaglLd29H9W0t/XzaXpaml45It+KiJkFz1O54kg SOirnFtwpcssMH61rG8IHXA/x/wU6sAvSHPoP9rkItuMvVWlnLCrJDI0Zp2toLg7spVx EWRZT3c9E2yWV/NF8SIVNclYnb5SEumoLnSPw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714760793; x=1715365593; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=jqBoL43PMGyCqFXn3ehXdH+uWie7AbSm2FUzxDYORqI=; b=eERquaDEoe8sniCfoG3FkNVIXL0GOEWc6i6g4lWb6DSb/Z4UqUxtHjarjzgvzUwPxN w9utPVVMs8Lclrb93HWLvgqMsNUbEmjvZMxZMp86UJkzl4P3SxJSJf1CqK/gNXf/lVSW 8QGJ1j5ICxYAkHNLkxgjjIsKHFrF7WsQCruT6OxBZumr8ia+MhNg0YMlIwdJKfmwp1mP UPWQB0sWxykwdPRYZIY4kpX0o1uZeAWRNcsccAwO2fgQDDFqkZTpQgMggLMINfKpQ1vL YLFAsCA2tBcRE6GQecfUNDWOs21TYNqGsMxbQ6gdm/LKX9Z30rOBJXw2iEtRgwZKPQwA wHzQ== X-Forwarded-Encrypted: i=1; AJvYcCW8Se5hN4pKCs8ky/eTg5EoXTmG8puyeBHVoQph8dypKo0cYq5UpylCoyvJVJhtgRNr7fzqYNW1kXacbJS5gq4IBTSTP5Vp9Y1WIsEYmOrD X-Gm-Message-State: AOJu0Yx4nI+GgMlWEJVy291mLsAdDAlM0J0IXPR1qaLzfJ5XNVvqJrNH 7XxE2Lfhy89Kv+3c8XJo9CHw7X6OsfXDfsLmQ0nInttC3bNz7LIaNBg9xElo5g== X-Google-Smtp-Source: AGHT+IGWOc/11Q1zIvnXaqaqAVGNo4zxAI/bv3cFFxYSIO/ngCUn47liMIMHwFEFiX6Gt/zTa/ffcw== X-Received: by 2002:a05:6a00:3d06:b0:6e7:20a7:9fc0 with SMTP id lo6-20020a056a003d0600b006e720a79fc0mr3768000pfb.34.1714760793183; Fri, 03 May 2024 11:26:33 -0700 (PDT) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id gp9-20020a056a003b8900b006ea8cc9250bsm3361952pfb.44.2024.05.03.11.26.32 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 03 May 2024 11:26:32 -0700 (PDT) Date: Fri, 3 May 2024 11:26:32 -0700 From: Kees Cook To: Bui Quang Minh , Al Viro , Christian Brauner Cc: syzbot , axboe@kernel.dk, brauner@kernel.org, io-uring@vger.kernel.org, jack@suse.cz, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com, viro@zeniv.linux.org.uk, Sumit Semwal , Christian =?iso-8859-1?q?K=F6nig?= , linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org, linaro-mm-sig@lists.linaro.org, Laura Abbott Subject: get_file() unsafe under epoll (was Re: [syzbot] [fs?] [io-uring?] general protection fault in __ep_remove) Message-ID: <202405031110.6F47982593@keescook> References: <0000000000002d631f0615918f1e@google.com> <7c41cf3c-2a71-4dbb-8f34-0337890906fc@gmail.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7c41cf3c-2a71-4dbb-8f34-0337890906fc@gmail.com> X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Fri, May 03, 2024 at 06:54:22PM +0700, Bui Quang Minh wrote: > [...] > Root cause: > AFAIK, eventpoll (epoll) does not increase the registered file's reference. > To ensure the safety, when the registered file is deallocated in __fput, > eventpoll_release is called to unregister the file from epoll. When calling > poll on epoll, epoll will loop through registered files and call vfs_poll on > these files. In the file's poll, file is guaranteed to be alive, however, as > epoll does not increase the registered file's reference, the file may be > dying > and it's not safe the get the file for later use. And dma_buf_poll violates > this. In the dma_buf_poll, it tries to get_file to use in the callback. This > leads to a race where the dmabuf->file can be fput twice. > > Here is the race occurs in the above proof-of-concept > > close(dmabuf->file) > __fput_sync (f_count == 1, last ref) > f_count-- (f_count == 0 now) > __fput > epoll_wait > vfs_poll(dmabuf->file) > get_file(dmabuf->file)(f_count == 1) > eventpoll_release > dmabuf->file deallocation > fput(dmabuf->file) (f_count == 1) > f_count-- > dmabuf->file deallocation > > I am not familiar with the dma_buf so I don't know the proper fix for the > issue. About the rule that don't get the file for later use in poll callback > of > file, I wonder if it is there when only select/poll exist or just after > epoll > appears. > > I hope the analysis helps us to fix the issue. Thanks for doing this analysis! I suspect at least a start of a fix would be this: What's the right way to deal with the dmabuf situation? (And I suspect it applies to get_dma_buf_unless_doomed() as well...) -Kees diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 8fe5aa67b167..15e8f74ee0f2 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -267,9 +267,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) if (events & EPOLLOUT) { /* Paired with fput in dma_buf_poll_cb */ - get_file(dmabuf->file); - - if (!dma_buf_poll_add_cb(resv, true, dcb)) + if (!atomic_long_inc_not_zero(&dmabuf->file) && + !dma_buf_poll_add_cb(resv, true, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else @@ -290,9 +289,8 @@ static __poll_t dma_buf_poll(struct file *file, poll_table *poll) if (events & EPOLLIN) { /* Paired with fput in dma_buf_poll_cb */ - get_file(dmabuf->file); - - if (!dma_buf_poll_add_cb(resv, false, dcb)) + if (!atomic_long_inc_not_zero(&dmabuf->file) && + !dma_buf_poll_add_cb(resv, false, dcb)) /* No callback queued, wake up any other waiters */ dma_buf_poll_cb(NULL, &dcb->cb); else But this ends up leaving "active" non-zero, and at close time it runs into: BUG_ON(dmabuf->cb_in.active || dmabuf->cb_out.active); But the bottom line is that get_file() is unsafe to use in some places, one of which appears to be in the poll handler. There are maybe some other fragile places too, like in drivers/gpu/drm/vmwgfx/ttm_object.c: static bool __must_check get_dma_buf_unless_doomed(struct dma_buf *dmabuf) { return atomic_long_inc_not_zero(&dmabuf->file->f_count) != 0L; } Which I also note involves a dmabuf... Due to this issue I've proposed fixing get_file() to detect pathological states: https://lore.kernel.org/lkml/20240502222252.work.690-kees@kernel.org/ But that has run into some push-back. I'm hoping that seeing this epoll example will help illustrate what needs fixing a little better. I think the best current proposal is to just WARN sooner instead of a full refcount_t implementation: diff --git a/include/linux/fs.h b/include/linux/fs.h index 8dfd53b52744..e09107d0a3d6 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1040,7 +1040,8 @@ struct file_handle { static inline struct file *get_file(struct file *f) { - atomic_long_inc(&f->f_count); + long prior = atomic_long_fetch_inc_relaxed(&f->f_count); + WARN_ONCE(!prior, "struct file::f_count incremented from zero; use-after-free condition present!\n"); return f; }