| Message ID | 20241112082600.298035-1-song@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Make inode storage available to tracing prog | expand |
> 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
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
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 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