linux-kernel - Re: [PATCH v5 bpf-next 4/5] bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14610672-A596-4920-B15E-015506A1E3A1@fb.com>
Date: Wed, 18 Dec 2024 22:10:46 +0000
From: Song Liu <songliubraving@...a.com>
To: Song Liu <songliubraving@...a.com>
CC: Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Song Liu
	<song@...nel.org>, bpf <bpf@...r.kernel.org>,
        LKML
	<linux-kernel@...r.kernel.org>,
        LSM List
	<linux-security-module@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin KaFai Lau
	<martin.lau@...ux.dev>,
        KP Singh <kpsingh@...nel.org>,
        Matt Bobrowski
	<mattbobrowski@...gle.com>,
        Paul Moore <paul@...l-moore.com>, James Morris
	<jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>,
        Kumar Kartikeya
 Dwivedi <memxor@...il.com>
Subject: Re: [PATCH v5 bpf-next 4/5] bpf: fs/xattr: Add BPF kfuncs to set and
 remove xattrs



> On Dec 18, 2024, at 1:47 PM, Song Liu <songliubraving@...a.com> wrote:
> 
> Hi Alexei, 
> 
> Thanks for the review!
> 
>> On Dec 18, 2024, at 1:20 PM, Alexei Starovoitov <alexei.starovoitov@...il.com> wrote:
>> 
>> On Tue, Dec 17, 2024 at 8:48 PM Song Liu <song@...nel.org> wrote:
>>> 
>>> 
>>> BTF_KFUNCS_START(bpf_fs_kfunc_set_ids)
>>> @@ -170,6 +330,10 @@ BTF_ID_FLAGS(func, bpf_put_file, KF_RELEASE)
>>> BTF_ID_FLAGS(func, bpf_path_d_path, KF_TRUSTED_ARGS)
>>> BTF_ID_FLAGS(func, bpf_get_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> +BTF_ID_FLAGS(func, bpf_set_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> +BTF_ID_FLAGS(func, bpf_remove_dentry_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> +BTF_ID_FLAGS(func, bpf_set_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> +BTF_ID_FLAGS(func, bpf_remove_dentry_xattr_locked, KF_SLEEPABLE | KF_TRUSTED_ARGS)
>>> BTF_KFUNCS_END(bpf_fs_kfunc_set_ids)
>> 
>> The _locked() versions shouldn't be exposed to bpf prog.
>> Don't add them to the above set.
>> 
>> Also we need to somehow exclude them from being dumped into vmlinux.h
>> 
>>> static int bpf_fs_kfuncs_filter(const struct bpf_prog *prog, u32 kfunc_id)
>>> @@ -186,6 +350,37 @@ static const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
>>>       .filter = bpf_fs_kfuncs_filter,
>>> };
> 
> [...]
> 
>>> + */
>>> +static void remap_kfunc_locked_func_id(struct bpf_verifier_env *env, struct bpf_insn *insn)
>>> +{
>>> +       u32 func_id = insn->imm;
>>> +
>>> +       if (bpf_lsm_has_d_inode_locked(env->prog)) {
>>> +               if (func_id == special_kfunc_list[KF_bpf_set_dentry_xattr])
>>> +                       insn->imm =  special_kfunc_list[KF_bpf_set_dentry_xattr_locked];
>>> +               else if (func_id == special_kfunc_list[KF_bpf_remove_dentry_xattr])
>>> +                       insn->imm = special_kfunc_list[KF_bpf_remove_dentry_xattr_locked];
>>> +       } else {
>>> +               if (func_id == special_kfunc_list[KF_bpf_set_dentry_xattr_locked])
>>> +                       insn->imm =  special_kfunc_list[KF_bpf_set_dentry_xattr];
>> 
>> This part is not necessary.
>> _locked() shouldn't be exposed and it should be an error
>> if bpf prog attempts to use invalid kfunc.
> 
> I was implementing this in different way than the solution you and Kumar
> suggested. Instead of updating this in add_kfunc_call, check_kfunc_call, 
> and fixup_kfunc_call, remap_kfunc_locked_func_id happens before 
> add_kfunc_call. Then, for the rest of the process, the verifier handles
> _locked version and not _locked version as two different kfuncs. This is
> why we need the _locked version in bpf_fs_kfunc_set_ids. I personally 
> think this approach is a lot cleaner. 
> 
> I think the missing piece is to exclude the _locked version from 
> vmlinux.h. Maybe we can achieve this by adding a different DECL_TAG 
> to these kfuncs?

Looked into the code, I think it is doable:

1. Extend struct btf_kfunc_id_set with "struct btf_id_set8 *shadow_set",
   or a different name;
2. Add _locked kfuncs to shadow_set, and these kfuncs will not have 
   BTF_SET8_KFUNCS set. Then pahole will not generate DECL_TAG of 
   "bpf_kfunc" for these. 
3. __btf_kfunc_id_set_contains() will need to look up id from shadow_set.
   And the filter function needs to handle shadow_set. 

Does this sound sane? 

Thanks,
Song