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

[Song Liu <songliubraving@meta.com>]

> On Nov 14, 2024, at 9:29 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:

[...]

>> 
>> 
>> The LSM inode information is obviously security sensitive, which I
>> presume would be be the motivation for Casey's concern that a 'mistake
>> by a BPF programmer could cause the whole system to blow up', which in
>> full disclosure is only a rough approximation of his statement.
>> 
>> We obviously can't speak directly to Casey's concerns.  Casey, any
>> specific technical comments on the challenges of using a common inode
>> specific storage architecture?
> 
> My objection to using a union for the BPF and LSM pointer is based
> on the observation that a lot of modern programmers don't know what
> a union does. The BPF programmer would see that there are two ways
> to accomplish their task, one for CONFIG_SECURITY=y and the other
> for when it isn't. The second is much simpler. Not understanding
> how kernel configuration works, nor being "real" C language savvy,
> the programmer installs code using the simpler interfaces on a
> Redhat system. The SELinux inode data is compromised by the BPF
> code, which thinks the data is its own. Hilarity ensues.

There must be some serious misunderstanding here. So let me 
explain the idea again. 

With CONFIG_SECURITY=y, the code will work the same as right now. 
BPF inode storage uses i_security, just as any other LSMs. 

With CONFIG_SECURITY=n, i_security does not exist, so the bpf
inode storage will use i_bpf_storage. 

Since this is a CONFIG_, all the logic got sorted out at compile
time. Thus the user API (for user space and for bpf programs) 
stays the same. 


Actually, I can understand the concern with union. Although, 
the logic is set at kernel compile time, it is still possible 
for kernel source code to use i_bpf_storage when 
CONFIG_SECURITY is enabled. (Yes, I guess now I finally understand
the concern). 

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?

Thanks,
Song