bpf inode local storage can be useful beyond LSM programs. For example,
bcc/libbpf-tools file* can use inode local storage to simplify the logic.
This set makes inode local storage available to tracing program.

1/4 is missing change for bpf task local storage. 2/4 move inode local
storage from security blob to inode.

Similar to task local storage in tracing program, it is necessary to add
recursion prevention logic for inode local storage. Patch 3/4 adds such
logic, and 4/4 add a test for the recursion prevention logic.

Song Liu (4):
  bpf: lsm: Remove hook to bpf_task_storage_free
  bpf: Make bpf inode storage available to tracing program
  bpf: Add recursion prevention logic for inode storage
  selftest/bpf: Test inode local storage recursion prevention

 fs/inode.c                                    |   1 +
 include/linux/bpf.h                           |   9 +
 include/linux/bpf_lsm.h                       |  29 ---
 include/linux/fs.h                            |   4 +
 kernel/bpf/Makefile                           |   3 +-
 kernel/bpf/bpf_inode_storage.c                | 185 +++++++++++++-----
 kernel/bpf/bpf_lsm.c                          |   4 -
 kernel/trace/bpf_trace.c                      |   8 +
 security/bpf/hooks.c                          |   7 -
 tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
 .../bpf/prog_tests/inode_local_storage.c      |  72 +++++++
 .../bpf/progs/inode_storage_recursion.c       |  90 +++++++++
 12 files changed, 320 insertions(+), 93 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/inode_local_storage.c
 create mode 100644 tools/testing/selftests/bpf/progs/inode_storage_recursion.c

--
2.43.5

> On Nov 12, 2024, at 12:25 AM, Song Liu <song@kernel.org> wrote:
> 
> bpf inode local storage can be useful beyond LSM programs. For example,
> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
> This set makes inode local storage available to tracing program.
> 
> 1/4 is missing change for bpf task local storage. 2/4 move inode local
> storage from security blob to inode.
> 
> Similar to task local storage in tracing program, it is necessary to add
> recursion prevention logic for inode local storage. Patch 3/4 adds such
> logic, and 4/4 add a test for the recursion prevention logic.
> 
> Song Liu (4):
>  bpf: lsm: Remove hook to bpf_task_storage_free
>  bpf: Make bpf inode storage available to tracing program
>  bpf: Add recursion prevention logic for inode storage
>  selftest/bpf: Test inode local storage recursion prevention

I accidentally sent some older .patch files together with this
set. Please ignore this version. I will resend v2. 

Thanks,
Song

> 
> fs/inode.c                                    |   1 +
> include/linux/bpf.h                           |   9 +
> include/linux/bpf_lsm.h                       |  29 ---
> include/linux/fs.h                            |   4 +
> kernel/bpf/Makefile                           |   3 +-
> kernel/bpf/bpf_inode_storage.c                | 185 +++++++++++++-----
> kernel/bpf/bpf_lsm.c                          |   4 -
> kernel/trace/bpf_trace.c                      |   8 +
> security/bpf/hooks.c                          |   7 -
> tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
> .../bpf/prog_tests/inode_local_storage.c      |  72 +++++++
> .../bpf/progs/inode_storage_recursion.c       |  90 +++++++++
> 12 files changed, 320 insertions(+), 93 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/inode_local_storage.c
> create mode 100644 tools/testing/selftests/bpf/progs/inode_storage_recursion.c
> 
> --
> 2.43.5

On 11/12/2024 12:25 AM, Song Liu wrote:
> bpf inode local storage can be useful beyond LSM programs. For example,
> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
> This set makes inode local storage available to tracing program.

Mixing the storage and scope of LSM data and tracing data leaves all sorts
of opportunities for abuse. Add inode data for tracing if you can get the
patch accepted, but do not move the LSM data out of i_security. Moving
the LSM data would break the integrity (such that there is) of the LSM
model.

>
> 1/4 is missing change for bpf task local storage. 2/4 move inode local
> storage from security blob to inode.
>
> Similar to task local storage in tracing program, it is necessary to add
> recursion prevention logic for inode local storage. Patch 3/4 adds such
> logic, and 4/4 add a test for the recursion prevention logic.
>
> Song Liu (4):
>   bpf: lsm: Remove hook to bpf_task_storage_free
>   bpf: Make bpf inode storage available to tracing program
>   bpf: Add recursion prevention logic for inode storage
>   selftest/bpf: Test inode local storage recursion prevention
>
>  fs/inode.c                                    |   1 +
>  include/linux/bpf.h                           |   9 +
>  include/linux/bpf_lsm.h                       |  29 ---
>  include/linux/fs.h                            |   4 +
>  kernel/bpf/Makefile                           |   3 +-
>  kernel/bpf/bpf_inode_storage.c                | 185 +++++++++++++-----
>  kernel/bpf/bpf_lsm.c                          |   4 -
>  kernel/trace/bpf_trace.c                      |   8 +
>  security/bpf/hooks.c                          |   7 -
>  tools/testing/selftests/bpf/DENYLIST.s390x    |   1 +
>  .../bpf/prog_tests/inode_local_storage.c      |  72 +++++++
>  .../bpf/progs/inode_storage_recursion.c       |  90 +++++++++
>  12 files changed, 320 insertions(+), 93 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/inode_local_storage.c
>  create mode 100644 tools/testing/selftests/bpf/progs/inode_storage_recursion.c
>
> --
> 2.43.5
>

Hi Casey, 

Thanks for your input. 

> On Nov 12, 2024, at 10:09 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> On 11/12/2024 12:25 AM, Song Liu wrote:
>> bpf inode local storage can be useful beyond LSM programs. For example,
>> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
>> This set makes inode local storage available to tracing program.
> 
> Mixing the storage and scope of LSM data and tracing data leaves all sorts
> of opportunities for abuse. Add inode data for tracing if you can get the
> patch accepted, but do not move the LSM data out of i_security. Moving
> the LSM data would break the integrity (such that there is) of the LSM
> model.

I honestly don't see how this would cause any issues. Each bpf inode 
storage maps are independent of each other, and the bpf local storage is 
designed to handle multiple inode storage maps properly. Therefore, if
the user decide to stick with only LSM hooks, there isn't any behavior 
change. OTOH, if the user decides some tracing hooks (on tracepoints, 
etc.) are needed, making a inode storage map available for both tracing 
programs and LSM programs would help simplify the logic. (Alternatively,
the tracing programs need to store per inode data in a hash map, and 
the LSM program would read that instead of the inode storage map.)

Does this answer the question and address the concerns?

Thanks,
Song

> 
>> 
>> 1/4 is missing change for bpf task local storage. 2/4 move inode local
>> storage from security blob to inode.
>> 
>> Similar to task local storage in tracing program, it is necessary to add
>> recursion prevention logic for inode local storage. Patch 3/4 adds such
>> logic, and 4/4 add a test for the recursion prevention logic.
>> 
>> Song Liu (4):
>>  bpf: lsm: Remove hook to bpf_task_storage_free
>>  bpf: Make bpf inode storage available to tracing program
>>  bpf: Add recursion prevention logic for inode storage
>>  selftest/bpf: Test inode local storage recursion prevention

[...]

On 11/12/2024 10:44 AM, Song Liu wrote:
> Hi Casey, 
>
> Thanks for your input. 
>
>> On Nov 12, 2024, at 10:09 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 11/12/2024 12:25 AM, Song Liu wrote:
>>> bpf inode local storage can be useful beyond LSM programs. For example,
>>> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
>>> This set makes inode local storage available to tracing program.
>> Mixing the storage and scope of LSM data and tracing data leaves all sorts
>> of opportunities for abuse. Add inode data for tracing if you can get the
>> patch accepted, but do not move the LSM data out of i_security. Moving
>> the LSM data would break the integrity (such that there is) of the LSM
>> model.
> I honestly don't see how this would cause any issues. Each bpf inode 
> storage maps are independent of each other, and the bpf local storage is 
> designed to handle multiple inode storage maps properly. Therefore, if
> the user decide to stick with only LSM hooks, there isn't any behavior 
> change. OTOH, if the user decides some tracing hooks (on tracepoints, 
> etc.) are needed, making a inode storage map available for both tracing 
> programs and LSM programs would help simplify the logic. (Alternatively,
> the tracing programs need to store per inode data in a hash map, and 
> the LSM program would read that instead of the inode storage map.)
>
> Does this answer the question and address the concerns?

First off, I had no question. No, this does not address my concern.
LSM data should be kept in and managed by the LSMs. We're making an
effort to make the LSM infrastructure more consistent. Moving some of
the LSM data into an LSM specific field in the inode structure goes
against this. What you're proposing is a one-off clever optimization
hack. We have too many of those already.



>
> Thanks,
> Song
>
>>> 1/4 is missing change for bpf task local storage. 2/4 move inode local
>>> storage from security blob to inode.
>>>
>>> Similar to task local storage in tracing program, it is necessary to add
>>> recursion prevention logic for inode local storage. Patch 3/4 adds such
>>> logic, and 4/4 add a test for the recursion prevention logic.
>>>
>>> Song Liu (4):
>>>  bpf: lsm: Remove hook to bpf_task_storage_free
>>>  bpf: Make bpf inode storage available to tracing program
>>>  bpf: Add recursion prevention logic for inode storage
>>>  selftest/bpf: Test inode local storage recursion prevention
> [...]
>

> On Nov 12, 2024, at 5:10 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> 
> On 11/12/2024 10:44 AM, Song Liu wrote:
>> Hi Casey, 
>> 
>> Thanks for your input. 
>> 
>>> On Nov 12, 2024, at 10:09 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> 
>>> On 11/12/2024 12:25 AM, Song Liu wrote:
>>>> bpf inode local storage can be useful beyond LSM programs. For example,
>>>> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
>>>> This set makes inode local storage available to tracing program.
>>> Mixing the storage and scope of LSM data and tracing data leaves all sorts
>>> of opportunities for abuse. Add inode data for tracing if you can get the
>>> patch accepted, but do not move the LSM data out of i_security. Moving
>>> the LSM data would break the integrity (such that there is) of the LSM
>>> model.
>> I honestly don't see how this would cause any issues. Each bpf inode 
>> storage maps are independent of each other, and the bpf local storage is 
>> designed to handle multiple inode storage maps properly. Therefore, if
>> the user decide to stick with only LSM hooks, there isn't any behavior 
>> change. OTOH, if the user decides some tracing hooks (on tracepoints, 
>> etc.) are needed, making a inode storage map available for both tracing 
>> programs and LSM programs would help simplify the logic. (Alternatively,
>> the tracing programs need to store per inode data in a hash map, and 
>> the LSM program would read that instead of the inode storage map.)
>> 
>> Does this answer the question and address the concerns?
> 
> First off, I had no question. No, this does not address my concern.
> LSM data should be kept in and managed by the LSMs. We're making an
> effort to make the LSM infrastructure more consistent.

Could you provide more information on the definition of "more 
consistent" LSM infrastructure? 

BPF LSM programs have full access to regular BPF maps (hash map, 
array, etc.). There was never a separation of LSM data vs. other 
data. 

AFAICT, other LSMs also use kzalloc and similar APIs for memory 
allocation. So data separation is not a goal for any LSM, right?

Thanks,
Song

> Moving some of
> the LSM data into an LSM specific field in the inode structure goes
> against this. What you're proposing is a one-off clever optimization
> hack. We have too many of those already.

On 11/12/2024 5:37 PM, Song Liu wrote:
>
>> On Nov 12, 2024, at 5:10 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 11/12/2024 10:44 AM, Song Liu wrote:
>>> Hi Casey, 
>>>
>>> Thanks for your input. 
>>>
>>>> On Nov 12, 2024, at 10:09 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>>
>>>> On 11/12/2024 12:25 AM, Song Liu wrote:
>>>>> bpf inode local storage can be useful beyond LSM programs. For example,
>>>>> bcc/libbpf-tools file* can use inode local storage to simplify the logic.
>>>>> This set makes inode local storage available to tracing program.
>>>> Mixing the storage and scope of LSM data and tracing data leaves all sorts
>>>> of opportunities for abuse. Add inode data for tracing if you can get the
>>>> patch accepted, but do not move the LSM data out of i_security. Moving
>>>> the LSM data would break the integrity (such that there is) of the LSM
>>>> model.
>>> I honestly don't see how this would cause any issues. Each bpf inode 
>>> storage maps are independent of each other, and the bpf local storage is 
>>> designed to handle multiple inode storage maps properly. Therefore, if
>>> the user decide to stick with only LSM hooks, there isn't any behavior 
>>> change. OTOH, if the user decides some tracing hooks (on tracepoints, 
>>> etc.) are needed, making a inode storage map available for both tracing 
>>> programs and LSM programs would help simplify the logic. (Alternatively,
>>> the tracing programs need to store per inode data in a hash map, and 
>>> the LSM program would read that instead of the inode storage map.)
>>>
>>> Does this answer the question and address the concerns?
>> First off, I had no question. No, this does not address my concern.
>> LSM data should be kept in and managed by the LSMs. We're making an
>> effort to make the LSM infrastructure more consistent.
> 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.

Some of this was done as part of the direct call change, and some
in support of LSM stacking. There are also some hardening changes
that aren't ready for prime-time, but that are in the works.
There have been concerns about the potential expoitability of the
LSM infrastructure, and we're serious about addressing those.

>
> BPF LSM programs have full access to regular BPF maps (hash map, 
> array, etc.). There was never a separation of LSM data vs. other 
> data. 
>
> AFAICT, other LSMs also use kzalloc and similar APIs for memory 
> allocation. So data separation is not a goal for any LSM, right?
>
> Thanks,
> Song
>
>> Moving some of
>> the LSM data into an LSM specific field in the inode structure goes
>> against this. What you're proposing is a one-off clever optimization
>> hack. We have too many of those already.
>
>

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?
Song

> Some of this was done as part of the direct call change, and some
> in support of LSM stacking. There are also some hardening changes
> that aren't ready for prime-time, but that are in the works.
> There have been concerns about the potential expoitability of the
> LSM infrastructure, and we're serious about addressing those.

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?

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

Hi Dr. Greg, 

Thanks for your input on this. 

> On Nov 14, 2024, at 8:36 AM, Dr. Greg <greg@enjellic.com> 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. */
> }

Yes, I am fully aware how most LSMs allocate and use these 
inode/task/etc. storage. 

> Christian appears to have already chimed in and indicated that there
> is no appetite to add another pointer member to the inode structure.

If I understand Christian correctly, his concern comes from the 
size of inode, and thus the impact on memory footprint and CPU
cache usage of all the inode in the system. While we got easier 
time adding a pointer to other data structures, for example socket,
I personally acknowledge Christian's concern and I am motivated to 
make changes to reduce the size of inode. 

> 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?

I never thought about asking other LSMs to make any changes. 
At the moment, none of the BPF maps are available to none BPF
code. 

> 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.

To be honest, I think bpf maps provide much better data isolation 
than a common block of storage. The creator of each bpf map has 
_full control_ who can access the map. The only exception is with
CAP_SYS_ADMIN, where the root user can access all bpf maps in the 
system. I don't think this has any security concern over the common
block of storage, as the root user can easily probe any data in the 
common block of storage via /proc/kcore. 

> 
> 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?
> 
> 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.

I think I indeed got much more push back than I would imagine. 
However, as always, I value everyone's perspective and I am
willing make reasonable changes to address valid concerns. 

Thanks,
Song

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

> 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?

Thanks,
Song

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

On Thu, Nov 14, 2024 at 4:49 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> 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.

My apologies on such a delayed response to this thread, I've had very
limited network access for a bit due to travel and the usual merge
window related distractions (and some others that were completely
unrelated) have left me with quite the mail backlog to sift through.

Enough with the excuses ...

Quickly skimming this thread and the v3 patchset, I would advise you
that there may be issues around using BPF LSMs and storing inode LSM
state outside the LSM managed inode storage blob.  Beyond the
conceptual objections that Casey has already mentioned, there have
been issues relating to the disjoint inode and inode->i_security
lifetimes.  While I believe we have an okay-ish solution in place now
for LSMs, I can't promise everything will work fine for BPF LSMs that
manage their inode LSM state outside of the LSM managed inode blob.
I'm sure you've already looked at it, but if you haven't it might be
worth looking at security_inode_free() to see some of the details.  In
a perfect world inode->i_security would be better synchronized with
the inode lifetime, but that would involve changes that the VFS folks
dislike.

However, while I will recommend against it, I'm not going to object to
you storing BPF LSM inode state elsewhere, that is up to you and KP
(he would need to ACK that as the BPF LSM maintainer).  I just want
you to know that if things break, there isn't much we (the LSM folks)
will be able to do to help other than suggest you go back to using the
LSM managed inode storage.

As far as some of the other ideas in this thread are concerned, at
this point in time I don't think we want to do any massive rework or
consolidation around i_security.  That's a critical field for the LSM
framework and many individual LSMs and there is work underway which
relies on this as a LSM specific inode storage blob; having to share
i_security with non-LSM users or moving the management of i_security
outside of the LSM is not something I'm overly excited about right
now.

-- 
paul-moore.com