Re: [PATCH bpf-next 2/2] selftests/bpf: Add tests for bpf_get_dentry_xattr

[Christian Brauner <brauner@kernel.org>]

On Fri, Jul 26, 2024 at 09:19:54AM GMT, Song Liu wrote:
> Hi Christian, 
> 
> > On Jul 26, 2024, at 12:06 AM, Christian Brauner <brauner@kernel.org> wrote:
> 
> [...]
> 
> >> +
> >> + for (i = 0; i < 10; i++) {
> >> + ret = bpf_get_dentry_xattr(dentry, "user.kfunc", &value_ptr);
> >> + if (ret == sizeof(expected_value) &&
> >> +    !bpf_strncmp(value, ret, expected_value))
> >> + matches++;
> >> +
> >> + prev_dentry = dentry;
> >> + dentry = bpf_dget_parent(prev_dentry);
> > 
> > Why do you need to walk upwards and instead of reading the xattr values
> > during security_inode_permission()?
> 
> In this use case, we would like to add xattr to the directory to cover
> all files under it. For example, assume we have the following xattrs:
> 
>   /bin  xattr: user.policy_A = value_A
>   /bin/gcc-6.9/ xattr: user.policy_A = value_B
>   /bin/gcc-6.9/gcc xattr: user.policy_A = value_C
> 
> /bin/gcc-6.9/gcc will use value_C;
> /bin/gcc-6.9/<other_files> will use value_B;
> /bin/<other_folder_or_file> will use value_A;
> 
> By walking upwards from security_file_open(), we can finish the logic 
> in a single LSM hook:
> 
>     repeat:
>         if (dentry have user.policy_A) {
>             /* make decision based on value */;
>         } else {
>             dentry = bpf_dget_parent();
>             goto repeat;
>         }
> 
> Does this make sense? Or maybe I misunderstood the suggestion?

Imho, what you're doing belongs into inode_permission() not into
security_file_open(). That's already too late and it's somewhat clear
from the example you're using that you're essentially doing permission
checking during path lookup.

Btw, what you're doing is potentially very heavy-handed because you're
retrieving xattrs for which no VFS cache exists so you might end up
causing a lot of io.

Say you have a 10000 deep directory hierarchy and you open a
file_at_level_10000. With that dget_parent() logic in the worst case you
end up walking up the whole hierarchy reading xattr values from disk
10000 times. You can achieve the same result and cleaner if you do the
checking in inode_permission() where it belongs and you only cause all
of that pain once and you abort path lookup correctly.

Also, I'm not even sure this is always correct because you're
retroactively checking what policy to apply based on the xattr value
walking up the parent chain. But a rename could happen and then the
ancestor chain you're checking is different from the current chain or
there's a bunch of mounts along the way.

Imho, that dget_parent() thing just encourages very badly written bpf
LSM programs. That's certainly not an interface we want to expose.

> Also, we don't have a bpf_get_inode_xattr() yet. I guess we will need
> it for the security_inode_permission approach. If we agree that's a 

Yes, that's fine.

You also need to ensure that you're only reading user.* xattrs. I know
you already do that for bpf_get_file_xattr() but this helper needs the
same treatment.

And you need to force a drop-out of RCU path lookup btw because you're
almost definitely going to block when you check the xattr.

> better approach, I more than happy to implement it that way. In fact,
> I think we will eventually need both bpf_get_inode_xattr() and 
> bpf_get_dentry_xattr(). 

I'm not sure about that because it's royally annoying in the first place
that we have to dentry and inode separately in the xattr handlers
because LSMs sometimes call them from a location when the dentry and
inode aren't yet fused together. The dentry is the wrong data structure
to care about here.