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

[Casey Schaufler <casey@schaufler-ca.com>]

On 11/14/2024 8:36 AM, Dr. Greg wrote:
> On Wed, Nov 13, 2024 at 10:57:05AM -0800, Song Liu wrote:
>
> Good morning, I hope the week is going well for everyone.
>
>> On Wed, Nov 13, 2024 at 10:06???AM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 11/12/2024 5:37 PM, Song Liu wrote:
>> [...]
>>>> Could you provide more information on the definition of "more
>>>> consistent" LSM infrastructure?
>>> We're doing several things. The management of security blobs
>>> (e.g. inode->i_security) has been moved out of the individual
>>> modules and into the infrastructure. The use of a u32 secid is
>>> being replaced with a more general lsm_prop structure, except
>>> where networking code won't allow it. A good deal of work has
>>> gone into making the return values of LSM hooks consistent.
>> Thanks for the information. Unifying per-object memory usage of
>> different LSMs makes sense. However, I don't think we are limiting
>> any LSM to only use memory from the lsm_blobs. The LSMs still
>> have the freedom to use other memory allocators. BPF inode
>> local storage, just like other BPF maps, is a way to manage
>> memory. BPF LSM programs have full access to BPF maps. So
>> I don't think it makes sense to say this BPF map is used by tracing,
>> so we should not allow LSM to use it.
>>
>> Does this make sense?
> As involved bystanders, some questions and thoughts that may help
> further the discussion.
>
> With respect to inode specific storage, the currently accepted pattern
> in the LSM world is roughly as follows:
>
> The LSM initialization code, at boot, computes the total amount of
> storage needed by all of the LSM's that are requesting inode specific
> storage.  A single pointer to that 'blob' of storage is included in
> the inode structure.
>
> In an include file, an inline function similar to the following is
> declared, whose purpose is to return the location inside of the
> allocated storage or 'LSM inode blob' where a particular LSM's inode
> specific data structure is located:
>
> static inline struct tsem_inode *tsem_inode(struct inode *inode)
> {
> 	return inode->i_security + tsem_blob_sizes.lbs_inode;
> }
>
> In an LSM's implementation code, the function gets used in something
> like the following manner:
>
> static int tsem_inode_alloc_security(struct inode *inode)
> {
> 	struct tsem_inode *tsip = tsem_inode(inode);
>
> 	/* Do something with the structure pointed to by tsip. */
> }
>
> Christian appears to have already chimed in and indicated that there
> is no appetite to add another pointer member to the inode structure.
>
> So, if this were to proceed forward, is it proposed that there will be
> a 'flag day' requirement to have each LSM that uses inode specific
> storage implement a security_inode_alloc() event handler that creates
> an LSM specific BPF map key/value pair for that inode?
>
> Which, in turn, would require that the accessor functions be converted
> to use a bpf key request to return the LSM specific information for
> that inode?
>
> A flag day event is always somewhat of a concern, but the larger
> concern may be the substitution of simple pointer arithmetic for a
> body of more complex code.  One would assume with something like this,
> that there may be a need for a shake-out period to determine what type
> of potential regressions the more complex implementation may generate,
> with regressions in security sensitive code always a concern.
>
> In a larger context.  Given that the current implementation works on
> simple pointer arithmetic over a common block of storage, there is not
> much of a safety guarantee that one LSM couldn't interfere with the
> inode storage of another LSM.  However, using a generic BPF construct
> such as a map, would presumably open the level of influence over LSM
> specific inode storage to a much larger audience, presumably any BPF
> program that would be loaded.
>
> 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.

>
> Song, FWIW going forward.  I don't know how closely you follow LSM
> development, but we believe an unbiased observer would conclude that
> there is some degree of reticence about BPF's involvement with the LSM
> infrastructure by some of the core LSM maintainers, that in turn makes
> these types of conversations technically sensitive.
>
>> Song
> We will look forward to your thoughts on the above.
>
> Have a good week.
>
> As always,
> Dr. Greg
>
> The Quixote Project - Flailing at the Travails of Cybersecurity
>               https://github.com/Quixote-Project