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: <4BF6D271-51D5-4768-A460-0853ABC5602D@fb.com>
Date: Thu, 14 Nov 2024 18:08:06 +0000
From: Song Liu <songliubraving@...a.com>
To: Casey Schaufler <casey@...aufler-ca.com>
CC: "Dr. Greg" <greg@...ellic.com>, Song Liu <song@...nel.org>,
        Song Liu
	<songliubraving@...a.com>,
        "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>,
        "brauner@...nel.org" <brauner@...nel.org>,
        "jack@...e.cz" <jack@...e.cz>,
        "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



> On Nov 14, 2024, at 9:29 AM, Casey Schaufler <casey@...aufler-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