From: Jann Horn <jannh@google.com>
To: Kees Cook <keescook@chromium.org>
Cc: Christian Brauner <brauner@kernel.org>,
Alexander Viro <viro@zeniv.linux.org.uk>, Jan Kara <jack@suse.cz>,
linux-fsdevel@vger.kernel.org,
Zack Rusin <zack.rusin@broadcom.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@broadcom.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
Jani Nikula <jani.nikula@linux.intel.com>,
Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Tvrtko Ursulin <tursulin@ursulin.net>,
Andi Shyti <andi.shyti@linux.intel.com>,
Lucas De Marchi <lucas.demarchi@intel.com>,
Matt Atwood <matthew.s.atwood@intel.com>,
Matthew Auld <matthew.auld@intel.com>,
Nirmoy Das <nirmoy.das@intel.com>,
Jonathan Cavitt <jonathan.cavitt@intel.com>,
Will Deacon <will@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Boqun Feng <boqun.feng@gmail.com>,
Mark Rutland <mark.rutland@arm.com>,
Kent Overstreet <kent.overstreet@linux.dev>,
Masahiro Yamada <masahiroy@kernel.org>,
Nathan Chancellor <nathan@kernel.org>,
Nicolas Schier <nicolas@fjasle.eu>,
Andrew Morton <akpm@linux-foundation.org>,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org, linux-kbuild@vger.kernel.org,
linux-hardening@vger.kernel.org
Subject: Re: [PATCH 1/5] fs: Do not allow get_file() to resurrect 0 f_count
Date: Fri, 3 May 2024 00:53:56 +0200 [thread overview]
Message-ID: <CAG48ez0d81xbOHqTUbWcBFWx5WY=RM8MM++ug79wXe0O-NKLig@mail.gmail.com> (raw)
In-Reply-To: <20240502223341.1835070-1-keescook@chromium.org>
On Fri, May 3, 2024 at 12:34 AM Kees Cook <keescook@chromium.org> wrote:
> If f_count reaches 0, calling get_file() should be a failure. Adjust to
> use atomic_long_inc_not_zero() and return NULL on failure. In the future
> get_file() can be annotated with __must_check, though that is not
> currently possible.
[...]
> static inline struct file *get_file(struct file *f)
> {
> - atomic_long_inc(&f->f_count);
> + if (unlikely(!atomic_long_inc_not_zero(&f->f_count)))
> + return NULL;
> return f;
> }
Oh, I really don't like this...
In most code, if you call get_file() on a file and see refcount zero,
that basically means you're in a UAF write situation, or that you
could be in such a situation if you had raced differently. It's
basically just like refcount_inc() in that regard.
And get_file() has semantics just like refcount_inc(): The caller
guarantees that it is already holding a reference to the file; and if
the caller is wrong about that, their subsequent attempt to clean up
the reference that they think they were already holding will likely
lead to UAF too. If get_file() sees a zero refcount, there is no safe
way to continue. And all existing callers of get_file() expect the
return value to be the same as the non-NULL pointer they passed in, so
they'll either ignore the result of this check and barrel on, or oops
with a NULL deref.
For callers that want to actually try incrementing file refcounts that
could be zero, which is only possible under specific circumstances, we
have helpers like get_file_rcu() and get_file_active().
Can't you throw a CHECK_DATA_CORRUPTION() or something like that in
there instead?
next prev parent reply other threads:[~2024-05-02 22:54 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-02 22:33 [PATCH 0/5] fs: Do not allow get_file() to resurrect 0 f_count Kees Cook
2024-05-02 22:33 ` [PATCH 1/5] " Kees Cook
2024-05-02 22:53 ` Jann Horn [this message]
2024-05-02 23:03 ` Kees Cook
2024-05-03 9:02 ` Christian Brauner
2024-05-02 22:33 ` [PATCH 2/5] drm/vmwgfx: Do not directly manipulate file->f_count Kees Cook
2024-05-02 22:33 ` [PATCH 3/5] drm/i915: " Kees Cook
2024-05-02 22:33 ` [PATCH 4/5] refcount: Introduce refcount_long_t and APIs Kees Cook
2024-05-06 8:04 ` kernel test robot
2024-05-02 22:33 ` [PATCH 5/5] fs: Convert struct file::f_count to refcount_long_t Kees Cook
2024-05-02 22:42 ` Al Viro
2024-05-02 22:52 ` Kees Cook
2024-05-02 23:12 ` Al Viro
2024-05-02 23:21 ` Kees Cook
2024-05-02 23:41 ` Al Viro
2024-05-03 0:10 ` Kees Cook
2024-05-03 0:14 ` Al Viro
2024-05-03 0:41 ` Kees Cook
2024-05-03 9:37 ` Christian Brauner
2024-05-03 10:36 ` Peter Zijlstra
2024-05-03 11:35 ` Christian Brauner
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='CAG48ez0d81xbOHqTUbWcBFWx5WY=RM8MM++ug79wXe0O-NKLig@mail.gmail.com' \
--to=jannh@google.com \
--cc=airlied@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=andi.shyti@linux.intel.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=boqun.feng@gmail.com \
--cc=brauner@kernel.org \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=jack@suse.cz \
--cc=jani.nikula@linux.intel.com \
--cc=jonathan.cavitt@intel.com \
--cc=joonas.lahtinen@linux.intel.com \
--cc=keescook@chromium.org \
--cc=kent.overstreet@linux.dev \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-hardening@vger.kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lucas.demarchi@intel.com \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mark.rutland@arm.com \
--cc=masahiroy@kernel.org \
--cc=matthew.auld@intel.com \
--cc=matthew.s.atwood@intel.com \
--cc=mripard@kernel.org \
--cc=nathan@kernel.org \
--cc=nicolas@fjasle.eu \
--cc=nirmoy.das@intel.com \
--cc=peterz@infradead.org \
--cc=rodrigo.vivi@intel.com \
--cc=tursulin@ursulin.net \
--cc=tzimmermann@suse.de \
--cc=viro@zeniv.linux.org.uk \
--cc=will@kernel.org \
--cc=zack.rusin@broadcom.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).