Re: [PATCH bpf-next 01/11] bpf: make bpf_d_path() helper use probe-read semantics

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey Christian,

On Wed, Feb 21, 2024 at 08:55:25AM +0100, Christian Brauner wrote:
> On Tue, Feb 20, 2024 at 01:22:14PM +0000, Matt Bobrowski wrote:
> > 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
> 
> The ugliness of the duplicated code made me start my mail with NAK. It
> would've been enough to just say no.
> 
> > 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
> 
> No, that's fine and was the initial proposal anyway. You're already
> using the existing d_path() anway in that bpf_d_path() thing. So
> exposing another variant with KF_TRUSTED_ARGS restriction is fine. But
> not hacking up a custom d_path() variant.

OK, thank you for clarifying. Perhaps we should just make a remark in
the form of a comment against bpf_d_path() stating that this BPF
helper is considered unsafe and users should look to migrate to the
newly added KF_TRUSTED_ARGS variant if at all possible.

> > [0]. Or, is it the other supporting reference counting based BPF
> > kfuncs [1, 2] that have irked you and aren't supportive of either?
> 
> Yes, because you're exposing fs_root, fs_pwd, path_put() and fdput(),
> get_task_exe_file(), get_mm_exe_file(). None of that I see being turned
> into kfuncs.

Hm, OK, but do know that BPF kfuncs do not make any promises around
being a stable interface, they never have and never will. Therefore,
it's not like introducing this kind of dependency on such APIs from
BPF kfuncs would hinder you from fundamentally modifying them moving
forward?

Additionally, given that these new acquire/release based BPF kfuncs
which rely on APIs like get_fs_root() and path_put() are in fact
restricted to BPF LSM programs, usage of such BPF kfuncs from the
context of a BPF LSM program would rather be analogous to a
pre-existing LSM module calling get_fs_root() and path_put()
explicitly within one of its implemented hooks, no? IOW, once a BPF
LSM program is loaded and JITed, what's the fundamental difference
between a baked in LSM module hook implementation which calls
get_fs_root() and a BPF LSM program which calls
bpf_get_task_fs_root()?  They're both being used in a perfectly
reasonable and sane like-for-like context, so what's the issue with
exposing such APIs as BPF kfuncs if they're being used appropriately?
It really doesn't make sense to provide independent reference counting
implementations just for BPF if there's some pre-existing
infrastructure in the kernel that does it the right way.

Also note that without such new reference counting BPF kfuncs which
I've proposed within this patch series the KF_TRUSTED_ARGS variant of
bpf_d_path() that we've agreed on becomes somewhat difficult to use in
practice. It'd essentially only be usable from LSM hooks that pass in
a struct path via the context parameter. Whilst in reality, it's
considered rather typical to also pass a struct path like
&current->mm->exe_file->f_path and &current->fs->pwd to bpf_d_path()
and friends from within the the implementation of an LSM hook. Such
struct path objects nested some levels deep isn't considered as being
trusted and therefore cannot be passed to a BPF kfunc that enforces
the KF_TRUSTED_ARGS constraint. The only way to acquire trust on a
pointer after performing such a struct walk is by grabbing a reference
on it, and hence why this KF_TRUSTED_ARGS change to d_path() and these
new BPF kfuncs go hand in hand.

Apologies about all the questions and comments here, but I'm really
just trying to understand why there's so much push back with regards
adding these reference counting BPF kfuncs?

/M




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux