From: Matt Bobrowski <mattbobrowski@google.com>
To: Christian Brauner <brauner@kernel.org>
Cc: bpf@vger.kernel.org, ast@kernel.org, andrii@kernel.org,
kpsingh@google.com, jannh@google.com, jolsa@kernel.org,
daniel@iogearbox.net, linux-fsdevel@vger.kernel.org,
Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH bpf-next 01/11] bpf: make bpf_d_path() helper use probe-read semantics
Date: Tue, 20 Feb 2024 13:22:14 +0000 [thread overview]
Message-ID: <ZdSnhqkO_JbRP5lO@google.com> (raw)
In-Reply-To: <20240220-erstochen-notwehr-755dbd0a02b3@brauner>
On Tue, Feb 20, 2024 at 10:48:10AM +0100, Christian Brauner wrote:
> On Tue, Feb 20, 2024 at 09:27:23AM +0000, Matt Bobrowski wrote:
> > There has now been several reported instances [0, 1, 2] where the
> > usage of the BPF helper bpf_d_path() has led to some form of memory
> > corruption issue.
> >
> > The fundamental reason behind why we repeatedly see bpf_d_path() being
> > susceptible to such memory corruption issues is because it only
> > enforces ARG_PTR_TO_BTF_ID constraints onto it's struct path
> > argument. This essentially means that it only requires an in-kernel
> > pointer of type struct path to be provided to it. Depending on the
> > underlying context and where the supplied struct path was obtained
> > from and when, depends on whether the struct path is fully intact or
> > not when calling bpf_d_path(). It's certainly possible to call
> > bpf_d_path() and subsequently d_path() from contexts where the
> > supplied struct path to bpf_d_path() has already started being torn
> > down by __fput() and such. An example of this is perfectly illustrated
> > in [0].
> >
> > Moving forward, we simply cannot enforce KF_TRUSTED_ARGS semantics
> > onto struct path of bpf_d_path(), as this approach would presumably
> > lead to some pretty wide scale and highly undesirable BPF program
> > breakage. To avoid breaking any pre-existing BPF program that is
> > dependent on bpf_d_path(), I propose that we take a different path and
> > re-implement an incredibly minimalistic and bare bone version of
> > d_path() which is entirely backed by kernel probe-read semantics. IOW,
> > a version of d_path() that is backed by
> > copy_from_kernel_nofault(). This ensures that any reads performed
> > against the supplied struct path to bpf_d_path() which may end up
> > faulting for whatever reason end up being gracefully handled and fixed
> > up.
> >
> > The caveats with such an approach is that we can't fully uphold all of
> > d_path()'s path resolution capabilities. Resolving a path which is
> > comprised of a dentry that make use of dynamic names via isn't
> > possible as we can't enforce probe-read semantics onto indirect
> > function calls performed via d_op as they're implementation
> > dependent. For such cases, we just return -EOPNOTSUPP. This might be a
> > little surprising to some users, especially those that are interested
> > in resolving paths that involve a dentry that resides on some
> > non-mountable pseudo-filesystem, being pipefs/sockfs/nsfs, but it's
> > arguably better than enforcing KF_TRUSTED_ARGS onto bpf_d_path() and
> > causing an unnecessary shemozzle for users. Additionally, we don't
>
> NAK. We're not going to add a semi-functional reimplementation of
> d_path() for bpf. This relied on VFS internals and guarantees that were
> never given. Restrict it to KF_TRUSTED_ARGS as it was suggested when
> this originally came up or fix it another way. But we're not adding a
> bunch of kfuncs to even more sensitive VFS machinery and then build a
> d_path() clone just so we can retroactively justify broken behavior.
OK, I agree, having a semi-functional re-implementation of d_path() is
indeed suboptimal. However, also understand that slapping the
KF_TRUSTED_ARGS constraint onto the pre-existing BPF helper
bpf_d_path() would outright break a lot of BPF programs out there, so
I can't see how taht would be an acceptable approach moving forward
here either.
Let's say that we decided to leave the pre-existing bpf_d_path()
implementation as is, accepting that it is fundamentally succeptible
to memory corruption issues, are you saying that you're also not for
adding the KF_TRUSTED_ARGS d_path() variant as I've done so here
[0]. Or, is it the other supporting reference counting based BPF
kfuncs [1, 2] that have irked you and aren't supportive of either?
[0] https://lore.kernel.org/bpf/20240220-erstochen-notwehr-755dbd0a02b3@brauner/T/#m542b86991b257cf9612406f1cc4d5692bcb75da8
[1] https://lore.kernel.org/bpf/20240220-erstochen-notwehr-755dbd0a02b3@brauner/T/#mc2aaadbe17490aeb1dde09071629b0b2a87d7436
[2] https://lore.kernel.org/bpf/20240220-erstochen-notwehr-755dbd0a02b3@brauner/T/#m07fa7a0c03af530d2ab3c4ef25c377b1d6ef17f8
/M
next prev parent reply other threads:[~2024-02-20 13:22 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-20 9:27 [PATCH bpf-next 00/11] bpf: probe-read bpf_d_path() and add new acquire/release BPF kfuncs Matt Bobrowski
2024-02-20 9:27 ` [PATCH bpf-next 01/11] bpf: make bpf_d_path() helper use probe-read semantics Matt Bobrowski
2024-02-20 9:48 ` Christian Brauner
2024-02-20 13:22 ` Matt Bobrowski [this message]
2024-02-21 7:55 ` Christian Brauner
2024-02-21 13:38 ` Matt Bobrowski
2024-02-20 9:27 ` [PATCH bpf-next 02/11] bpf/selftests: adjust selftests for BPF helper bpf_d_path() Matt Bobrowski
2024-02-20 9:27 ` [PATCH bpf-next 03/11] bpf: rename fs_kfunc_set_ids to lsm_kfunc_set_ids Matt Bobrowski
2024-02-20 9:27 ` [PATCH bpf-next 04/11] bpf: add new acquire/release BPF kfuncs for mm_struct Matt Bobrowski
2024-02-20 9:27 ` [PATCH bpf-next 05/11] bpf/selftests: add selftests for mm_struct acquire/release BPF kfuncs Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 06/11] bpf: add new acquire/release based BPF kfuncs for exe_file Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 07/11] bpf/selftests: add selftests for exe_file acquire/release BPF kfuncs Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 08/11] bpf: add acquire/release based BPF kfuncs for fs_struct's paths Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 09/11] bpf/selftests: add selftests for root/pwd path based BPF kfuncs Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 10/11] bpf: add trusted d_path() based BPF kfunc bpf_path_d_path() Matt Bobrowski
2024-02-20 9:28 ` [PATCH bpf-next 11/11] bpf/selftests: adapt selftests test_d_path for " Matt Bobrowski
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=ZdSnhqkO_JbRP5lO@google.com \
--to=mattbobrowski@google.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brauner@kernel.org \
--cc=daniel@iogearbox.net \
--cc=jannh@google.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@google.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.