On Thu, 2024-11-14 at 18:08 +0000, Song Liu wrote: > > > > On Nov 14, 2024, at 9:29 AM, Casey Schaufler > > <casey@xxxxxxxxxxxxxxxx> 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
