linux-kernel - Re: [PATCH bpf-next 0/4] Make inode storage available to tracing prog

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: <A7017094-1A0C-42C8-BE9D-7352D2200ECC@fb.com>
Date: Sun, 17 Nov 2024 22:59:18 +0000
From: Song Liu <songliubraving@...a.com>
To: James Bottomley <James.Bottomley@...senPartnership.com>,
        "jack@...e.cz"
	<jack@...e.cz>,
        "brauner@...nel.org" <brauner@...nel.org>
CC: Song Liu <songliubraving@...a.com>,
        Casey Schaufler
	<casey@...aufler-ca.com>,
        "Dr. Greg" <greg@...ellic.com>, Song Liu
	<song@...nel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-security-module@...r.kernel.org"
	<linux-security-module@...r.kernel.org>,
        Kernel Team <kernel-team@...a.com>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "eddyz87@...il.com"
	<eddyz87@...il.com>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "martin.lau@...ux.dev"
	<martin.lau@...ux.dev>,
        "viro@...iv.linux.org.uk" <viro@...iv.linux.org.uk>,
        "kpsingh@...nel.org" <kpsingh@...nel.org>,
        "mattbobrowski@...gle.com"
	<mattbobrowski@...gle.com>,
        "amir73il@...il.com" <amir73il@...il.com>,
        "repnop@...gle.com" <repnop@...gle.com>,
        "jlayton@...nel.org"
	<jlayton@...nel.org>,
        Josef Bacik <josef@...icpanda.com>,
        "mic@...ikod.net"
	<mic@...ikod.net>,
        "gnoack@...gle.com" <gnoack@...gle.com>
Subject: Re: [PATCH bpf-next 0/4] Make inode storage available to tracing prog

Hi Christian, James and Jan, 

> On Nov 14, 2024, at 1:49 PM, James Bottomley <James.Bottomley@...senPartnership.com> wrote:

[...]

>> 
>> We can address this with something like following:
>> 
>> #ifdef CONFIG_SECURITY
>>         void                    *i_security;
>> #elif CONFIG_BPF_SYSCALL
>>         struct bpf_local_storage __rcu *i_bpf_storage;
>> #endif
>> 
>> This will help catch all misuse of the i_bpf_storage at compile
>> time, as i_bpf_storage doesn't exist with CONFIG_SECURITY=y. 
>> 
>> Does this make sense?
> 
> Got to say I'm with Casey here, this will generate horrible and failure
> prone code.
> 
> Since effectively you're making i_security always present anyway,
> simply do that and also pull the allocation code out of security.c in a
> way that it's always available?  That way you don't have to special
> case the code depending on whether CONFIG_SECURITY is defined. 
> Effectively this would give everyone a generic way to attach some
> memory area to an inode.  I know it's more complex than this because
> there are LSM hooks that run from security_inode_alloc() but if you can
> make it work generically, I'm sure everyone will benefit.

On a second thought, I think making i_security generic is not 
the right solution for "BPF inode storage in tracing use cases". 

This is because i_security serves a very specific use case: it 
points to a piece of memory whose size is calculated at system 
boot time. If some of the supported LSMs is not enabled by the 
lsm= kernel arg, the kernel will not allocate memory in 
i_security for them. The only way to change lsm= is to reboot 
the system. BPF LSM programs can be disabled at the boot time, 
which fits well in i_security. However, BPF tracing programs 
cannot be disabled at boot time (even we change the code to 
make it possible, we are not likely to disable BPF tracing). 
IOW, as long as CONFIG_BPF_SYSCALL is enabled, we expect some 
BPF tracing programs to load at some point of time, and these 
programs may use BPF inode storage. 

Therefore, with CONFIG_BPF_SYSCALL enabled, some extra memory 
always will be attached to i_security (maybe under a different 
name, say, i_generic) of every inode. In this case, we should 
really add i_bpf_storage directly to the inode, because another 
pointer jump via i_generic gives nothing but overhead. 

Does this make sense? Or did I misunderstand the suggestion?

Thanks,
Song