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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Thu, 2024-11-14 at 18:08 +0000, Song Liu wrote:
> 
> 
> > 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?

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.

Regards,

James