linux-kernel - Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED

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] [day] [month] [year] [list]

Message-ID: <CAEXv5_gR1=OcH9dKg3TA1MGkq8dRSNX=phuNK6n6UzD=eh6cjQ@mail.gmail.com>
Date: Fri, 12 Sep 2025 19:26:46 -0500
From: David Windsor <dwindsor@...il.com>
To: Song Liu <song@...nel.org>
Cc: bpf@...r.kernel.org, linux-kernel@...r.kernel.org, martin.lau@...ux.dev, 
	ast@...nel.org, daniel@...earbox.net, andrii@...nel.org, eddyz87@...il.com, 
	yonghong.song@...ux.dev, john.fastabend@...il.com, kpsingh@...nel.org, 
	sdf@...ichev.me, haoluo@...gle.com, jolsa@...nel.org
Subject: Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED_STORAGE map type and kfuncs

On Fri, Sep 12, 2025 at 7:12 PM Song Liu <song@...nel.org> wrote:
>
> On Fri, Sep 12, 2025 at 3:25 PM David Windsor <dwindsor@...il.com> wrote:
> >
> > All other bpf local storage is obtained using helpers which benefit from
> > RET_PTR_TO_MAP_VALUE_OR_NULL, so can return void * pointers directly to
> > map values. kfuncs don't have that, so return struct
> > bpf_local_storage_data * and access map values through sdata->data.
> >
> > Signed-off-by: David Windsor <dwindsor@...il.com>
>
> Maybe I missed something, but I think you haven't addressed Alexei's
> question in v1: why this is needed and why hash map is not sufficient.
>
> Other local storage types (task, inode, sk storage) may get a large
> number of entries in a system, and thus would benefit from object
> local storage. I don't think we expect too many creds in a system.
> hash map of a smallish size should be good in most cases, and be
> faster than cred local storage.
>
> Did I get this right?
>
> Thanks,
> Song
>

Yes I think I addressed in the cover letter of -v2:

"Like other local storage types (task, inode, sk), this provides automatic
lifecycle management and is useful for LSM programs tracking credential
state across LSM calls. Lifetime management is necessary for detecting
credential leaks and enforcing time-based security policies."

You're right it's faster and there aren't many creds, but I feel like
in this case, it'll be a nightmare to manual cleanup with hashmaps. I
think the correctness we get with lifetime management is worth it in
this case, but could be convinced otherwise. Many cred usage patterns
are short lived and a hash map could quickly become stale...






> The following are some quick feedbacks of the patch, but let's
> first address the question above.
>
> This is hitting KASAN BUG in CI:
>
> https://github.com/kernel-patches/bpf/actions/runs/17687566710/job/50275683479
>
> (You may need to log in GitHub to see details).
>
> [...]
>
> > +
> > +__bpf_kfunc int bpf_cred_storage_delete(struct bpf_map *map, struct cred *cred)
> > +{
> > +       if (!cred)
> > +               return -EINVAL;
> > +
> > +       return cred_storage_delete(cred, map);
> > +}
> > +
> > +BTF_KFUNCS_START(bpf_cred_storage_kfunc_ids)
> > +BTF_ID_FLAGS(func, bpf_cred_storage_delete, 0)
> > +BTF_ID_FLAGS(func, bpf_cred_storage_get, KF_RET_NULL)
> > +BTF_KFUNCS_END(bpf_cred_storage_kfunc_ids)
> > +
> > +static const struct btf_kfunc_id_set bpf_cred_storage_kfunc_set = {
> > +       .owner = THIS_MODULE,
> > +       .set   = &bpf_cred_storage_kfunc_ids,
> > +};
> > +
> > +static int __init bpf_cred_storage_init(void)
> > +{
> > +       int err;
>
> We need an empty line after the declaration.
> scripts/checkpatch.pl should warn this. Please fix other warnings
> from checkpatch.pl.
>
> > +       err = register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_cred_storage_kfunc_set);
> > +       if (err) {
>
> [...]
>
> > diff --git a/kernel/cred.c b/kernel/cred.c
> > index 9676965c0981..a1be27fe5f4c 100644
> > --- a/kernel/cred.c
> > +++ b/kernel/cred.c
> > @@ -38,6 +38,10 @@ static struct kmem_cache *cred_jar;
> >  /* init to 2 - one for init_task, one to ensure it is never freed */
> >  static struct group_info init_groups = { .usage = REFCOUNT_INIT(2) };
> >
> > +#ifdef CONFIG_BPF_LSM
> > +#include <linux/bpf_lsm.h>
> > +#endif
>
> We defined a dummy version of bpf_cred_storage_free
> in bpf_lsm.h, so the ifdef here is not needed.
>
> > +
> >  /*
> >   * The initial credentials for the initial task
> >   */
> > @@ -76,6 +80,9 @@ static void put_cred_rcu(struct rcu_head *rcu)
> >                       cred, atomic_long_read(&cred->usage));
> >
> >         security_cred_free(cred);
> > +#ifdef CONFIG_BPF_LSM
> > +       bpf_cred_storage_free(cred);
> > +#endif
>
> Ditto.
>
> >         key_put(cred->session_keyring);
> >         key_put(cred->process_keyring);
> >         key_put(cred->thread_keyring);
> [...]