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] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAEXv5_hKQqFH_7zmxr7moBpt07B-+ZWB=qfWOb+Rn9Vj=7EX+g@mail.gmail.com>
Date: Tue, 16 Sep 2025 11:25:03 -0400
From: David Windsor <dwindsor@...il.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Song Liu <song@...nel.org>, bpf <bpf@...r.kernel.org>, 
	LKML <linux-kernel@...r.kernel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Eduard <eddyz87@...il.com>, 
	Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>
Subject: Re: [PATCH 1/2] bpf: Add BPF_MAP_TYPE_CRED_STORAGE map type and kfuncs

On Sun, Sep 14, 2025 at 10:10 PM David Windsor <dwindsor@...il.com> wrote:
>
> On Sun, Sep 14, 2025 at 9:10 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Sat, Sep 13, 2025 at 3:27 PM David Windsor <dwindsor@...il.com> wrote:
> > >
> > >
> > >
> > > On Sat, Sep 13, 2025 at 5:58 PM Song Liu <song@...nel.org> wrote:
> > >>
> > >> On Fri, Sep 12, 2025 at 5:27 PM David Windsor <dwindsor@...il.com> wrote:
> > >> [...]
> > >> > >
> > >> > > 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...
> > >>
> > >> We can clean up the hashmap in hook cred_free, no? The following
> > >> check in security_cred_free() seems problematic:
> > >>
> > >>         if (unlikely(cred->security == NULL))
> > >>                 return;
> > >>
> > >> But as far as I can tell, it is not really useful, and can be removed.
> > >> With this removed, hash map will work just as well. Did I miss
> > >> something?
> > >
> > >
> > > No I think actually this is easier.
> > >
> > > I will prepare a patch for the race in cleanup I stumbled on earlier which is still there and could affect other users.
> > >
> > > That said, is there any use case for local storage for these structs:
> > >
> > > - struct file
> > > - struct msg_msg
> > > - struct ipc
> > >
> > > I can off the top of my head think of some security use cases for these but not sure if hashmaps are needed, perhaps struct file
> >
> > Sorry, no. This is not a copy paste territory.
>
> no i get it's not copy/paste but I have the series for struct file
> ready for submission, with selftests. this is also a performance
> critical use case and there will be numerous struct file on edge
> servers.
>
> > The existing local storage maps were added because
> > performance was critical for those use cases,
> > but we made a few mistakes. There is a performance
> > cliff that has to be fixed before we adopt it to
> > other kernel objects.
>
> ahh wasn't aware of this.
>
> > Please use hash map and consider wrapping rhashtable
> > as a new bpf map type if fixed max_entries is problematic.
> >
>
> makes sense thanks
>

Hi,

Thinking about this more, hashmaps are still problematic for this case.

Meaning, placing a hook on security_cred_free alone for garbage
collection / end-of-life processing isn't enough - we still have to
deal with prepare/commit_creds. This flow works by having
prepare_creds clone an existing cred object, then commit_creds works
by swapping old creds with new one atomically, then later freeing the
original cred. If we are not very careful there will be a period of
time during which both cred objects could be valid, and I think this
is worth the feature alone.

Also, the main reason we want local storage for these structs is that
LSMs use them. Every classic LSM (SELinux, Smack, AppArmor, TOMOYO,
Landlock, Yama) has a cred blob, all of them have file blobs, ipc
blobs, superblock blobs, etc. struct cred is the basis for subject
identities in at least SELinux, probably others I'm sure.

With respect to performance issues, correct thing to do is still build
out these local storage types for in-kernel LSMs but then fix the
performance issue.

kp do you have any thoughts on this?

> > pw-bot: cr