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: <b1e82da8daa1c372e4678b1984ac942c98db998d.camel@HansenPartnership.com>
Date: Thu, 14 Nov 2024 13:49:28 -0800
From: James Bottomley <James.Bottomley@...senPartnership.com>
To: Song Liu <songliubraving@...a.com>, Casey Schaufler
 <casey@...aufler-ca.com>
Cc: "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>,  "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 Thu, 2024-11-14 at 18:08 +0000, Song Liu wrote:
> 
> 
> > 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?

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