| Message ID | 20240324223231.6249-1-greg@enjellic.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Do not require attributes for security_inode_init_security. | expand |
On Sun, Mar 24, 2024 at 6:33 PM Greg Wettstein <greg@enjellic.com> wrote: > > The integration of the Integrity Measurement Architecture (IMA) > into the LSM infrastructure introduced a conditional check that > denies access to the security_inode_init_security() event handler > if the LSM extended attribute 'blob' size is 0. > > This changes the previous behavior of this event handler and > results in variable behavior of LSM's depending on the LSM boot > configuration. > > Modify the function so that it removes the need for a non-zero > extended attribute blob size and bypasses the memory allocation > and freeing that is not needed if the LSM infrastructure is not > using extended attributes. > > Use a break statement to exit the loop that is iterating over the > defined handlers for this event if a halting error condition is > generated by one of the invoked LSM handlers. The checks for how > to handle cleanup are executed at the end of the loop regardless > of how the loop terminates. > > A two exit label strategy is implemented. One of the exit > labels is a target for the no attribute case while the second is > the target for the case where memory allocated for processing of > extended attributes needs to be freed. > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > --- > security/security.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/security/security.c b/security/security.c > index 7035ee35a393..a0b52b964688 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1717,10 +1717,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > if (unlikely(IS_PRIVATE(inode))) > return 0; > > - if (!blob_sizes.lbs_xattr_count) > - return 0; > - > - if (initxattrs) { > + if (blob_sizes.lbs_xattr_count && initxattrs) { > /* Allocate +1 for EVM and +1 as terminator. */ > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2, > sizeof(*new_xattrs), GFP_NOFS); > @@ -1733,7 +1730,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs, > &xattr_count); > if (ret && ret != -EOPNOTSUPP) > - goto out; > + break; > /* > * As documented in lsm_hooks.h, -EOPNOTSUPP in this context > * means that the LSM is not willing to provide an xattr, not > @@ -1742,19 +1739,22 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > */ > } > > - /* If initxattrs() is NULL, xattr_count is zero, skip the call. */ > - if (!xattr_count) > - goto out; > + /* Skip xattr processing if no attributes are in use. */ > + if (!blob_sizes.lbs_xattr_count) > + goto out2; > + /* No attrs or an LSM returned an actionable error code. */ > + if (!xattr_count || (ret && ret != -EOPNOTSUPP)) > + goto out1; > > ret = evm_inode_init_security(inode, dir, qstr, new_xattrs, > &xattr_count); > - if (ret) > - goto out; > - ret = initxattrs(inode, new_xattrs, fs_data); > -out: > + if (!ret) > + ret = initxattrs(inode, new_xattrs, fs_data); > + out1: > for (; xattr_count > 0; xattr_count--) > kfree(new_xattrs[xattr_count - 1].value); > kfree(new_xattrs); > + out2: > return (ret == -EOPNOTSUPP) ? 0 : ret; > } > EXPORT_SYMBOL(security_inode_init_security); > -- > 2.39.1 Looking at this quickly, why does something like the following not work? [WARNING: copy-n-paste patch, likely whitespace damaged] diff --git a/security/security.c b/security/security.c index 7e118858b545..007ce438e636 100644 --- a/security/security.c +++ b/security/security.c @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str uct inode *dir, if (unlikely(IS_PRIVATE(inode))) return 0; - if (!blob_sizes.lbs_xattr_count) - return 0; - - if (initxattrs) { + if (initxattrs && blob_sizes.lbs_xattr_count) { /* Allocate +1 as terminator. */ new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, sizeof(*new_xattrs), GFP_NOFS);
On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote: Good morning, I hope the week is going well. > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg@enjellic.com> wrote: > > > > The integration of the Integrity Measurement Architecture (IMA) > > into the LSM infrastructure introduced a conditional check that > > denies access to the security_inode_init_security() event handler > > if the LSM extended attribute 'blob' size is 0. > > > > This changes the previous behavior of this event handler and > > results in variable behavior of LSM's depending on the LSM boot > > configuration. > > > > Modify the function so that it removes the need for a non-zero > > extended attribute blob size and bypasses the memory allocation > > and freeing that is not needed if the LSM infrastructure is not > > using extended attributes. > > > > Use a break statement to exit the loop that is iterating over the > > defined handlers for this event if a halting error condition is > > generated by one of the invoked LSM handlers. The checks for how > > to handle cleanup are executed at the end of the loop regardless > > of how the loop terminates. > > > > A two exit label strategy is implemented. One of the exit > > labels is a target for the no attribute case while the second is > > the target for the case where memory allocated for processing of > > extended attributes needs to be freed. > > > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > > --- > > security/security.c | 24 ++++++++++++------------ > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/security/security.c b/security/security.c > > index 7035ee35a393..a0b52b964688 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1717,10 +1717,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > > if (unlikely(IS_PRIVATE(inode))) > > return 0; > > > > - if (!blob_sizes.lbs_xattr_count) > > - return 0; > > - > > - if (initxattrs) { > > + if (blob_sizes.lbs_xattr_count && initxattrs) { > > /* Allocate +1 for EVM and +1 as terminator. */ > > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2, > > sizeof(*new_xattrs), GFP_NOFS); > > @@ -1733,7 +1730,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > > ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs, > > &xattr_count); > > if (ret && ret != -EOPNOTSUPP) > > - goto out; > > + break; > > /* > > * As documented in lsm_hooks.h, -EOPNOTSUPP in this context > > * means that the LSM is not willing to provide an xattr, not > > @@ -1742,19 +1739,22 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, > > */ > > } > > > > - /* If initxattrs() is NULL, xattr_count is zero, skip the call. */ > > - if (!xattr_count) > > - goto out; > > + /* Skip xattr processing if no attributes are in use. */ > > + if (!blob_sizes.lbs_xattr_count) > > + goto out2; > > + /* No attrs or an LSM returned an actionable error code. */ > > + if (!xattr_count || (ret && ret != -EOPNOTSUPP)) > > + goto out1; > > > > ret = evm_inode_init_security(inode, dir, qstr, new_xattrs, > > &xattr_count); > > - if (ret) > > - goto out; > > - ret = initxattrs(inode, new_xattrs, fs_data); > > -out: > > + if (!ret) > > + ret = initxattrs(inode, new_xattrs, fs_data); > > + out1: > > for (; xattr_count > 0; xattr_count--) > > kfree(new_xattrs[xattr_count - 1].value); > > kfree(new_xattrs); > > + out2: > > return (ret == -EOPNOTSUPP) ? 0 : ret; > > } > > EXPORT_SYMBOL(security_inode_init_security); > > -- > > 2.39.1 > > Looking at this quickly, why does something like the following not work? > > [Warning: copy-n-paste patch, likely whitespace damaged] > > diff --git a/security/security.c b/security/security.c > index 7e118858b545..007ce438e636 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str > uct inode *dir, > if (unlikely(IS_PRIVATE(inode))) > return 0; > > - if (!blob_sizes.lbs_xattr_count) > - return 0; > - > - if (initxattrs) { > + if (initxattrs && blob_sizes.lbs_xattr_count) { > /* Allocate +1 as terminator. */ > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > sizeof(*new_xattrs), GFP_NOFS); We ran with something similar to the above for several days of TSEMv3 testing. For the patch that we submitted upstream, we elected to take a 'belt and suspenders' approach that isolated the 'no attributes' execution flow from the flow followed if extended attributes are present. The approach used doesn't make any difference to us as long as we get the functionality of the hook restored. If you go with the simpler approach, it may be worthwhile to at least simplify the handling of the call to the initxattr() function after the evm_inode_init_security() call. It seems simpler and with more clear intent, to use a negated conditional check of the 'ret' value from evm_inode_init_security() to call the initxattr() function, rather than using the return value to jump over the call. Once again, your choice, no preferences on our part. > paul-moore.com Have a good day. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
On Tue, Mar 26, 2024 at 6:31 AM Dr. Greg <greg@enjellic.com> wrote: > On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote: > > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg@enjellic.com> wrote: > > > > > > The integration of the Integrity Measurement Architecture (IMA) > > > into the LSM infrastructure introduced a conditional check that > > > denies access to the security_inode_init_security() event handler > > > if the LSM extended attribute 'blob' size is 0. > > > > > > This changes the previous behavior of this event handler and > > > results in variable behavior of LSM's depending on the LSM boot > > > configuration. > > > > > > Modify the function so that it removes the need for a non-zero > > > extended attribute blob size and bypasses the memory allocation > > > and freeing that is not needed if the LSM infrastructure is not > > > using extended attributes. > > > > > > Use a break statement to exit the loop that is iterating over the > > > defined handlers for this event if a halting error condition is > > > generated by one of the invoked LSM handlers. The checks for how > > > to handle cleanup are executed at the end of the loop regardless > > > of how the loop terminates. > > > > > > A two exit label strategy is implemented. One of the exit > > > labels is a target for the no attribute case while the second is > > > the target for the case where memory allocated for processing of > > > extended attributes needs to be freed. > > > > > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > > > --- > > > security/security.c | 24 ++++++++++++------------ > > > 1 file changed, 12 insertions(+), 12 deletions(-) ... > > Looking at this quickly, why does something like the following not work? > > > > [Warning: copy-n-paste patch, likely whitespace damaged] > > > > diff --git a/security/security.c b/security/security.c > > index 7e118858b545..007ce438e636 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str > > uct inode *dir, > > if (unlikely(IS_PRIVATE(inode))) > > return 0; > > > > - if (!blob_sizes.lbs_xattr_count) > > - return 0; > > - > > - if (initxattrs) { > > + if (initxattrs && blob_sizes.lbs_xattr_count) { > > /* Allocate +1 as terminator. */ > > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > > sizeof(*new_xattrs), GFP_NOFS); > > We ran with something similar to the above for several days of TSEMv3 > testing. > > For the patch that we submitted upstream, we elected to take a 'belt > and suspenders' approach that isolated the 'no attributes' execution > flow from the flow followed if extended attributes are present. > > The approach used doesn't make any difference to us as long as we get > the functionality of the hook restored. I'd prefer the simpler approach. I'd likely also prefer we park this patch until it is needed upstream, or am I misunderstanding things and upstream is currently broken without a fix like this? > If you go with the simpler approach, it may be worthwhile to at least > simplify the handling of the call to the initxattr() function after > the evm_inode_init_security() call. Starting with v6.9-rc1 there is no longer an explicit call to evm_inode_init_security() as it is incorporated into the normal LSM hook processing, e.g. `hp->hook.inode_init_security(...)`. I'm also not sure we need to worry about the initxattrs() call near the bottom of security_inode_init_security() since in the no @blob.lbs_xattr_count case the @xattr_count variable will also be zero so the initxattrs() call will be skipped. Or were you talking about something else? > It seems simpler and with more clear intent, to use a negated > conditional check of the 'ret' value from evm_inode_init_security() to > call the initxattr() function, rather than using the return value to > jump over the call. > > Once again, your choice, no preferences on our part.
On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote: Good morning to everyone. > On Tue, Mar 26, 2024 at 6:31???AM Dr. Greg <greg@enjellic.com> wrote: > > On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote: > > > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg@enjellic.com> wrote: > > > > > > > > The integration of the Integrity Measurement Architecture (IMA) > > > > into the LSM infrastructure introduced a conditional check that > > > > denies access to the security_inode_init_security() event handler > > > > if the LSM extended attribute 'blob' size is 0. > > > > > > > > This changes the previous behavior of this event handler and > > > > results in variable behavior of LSM's depending on the LSM boot > > > > configuration. > > > > > > > > Modify the function so that it removes the need for a non-zero > > > > extended attribute blob size and bypasses the memory allocation > > > > and freeing that is not needed if the LSM infrastructure is not > > > > using extended attributes. > > > > > > > > Use a break statement to exit the loop that is iterating over the > > > > defined handlers for this event if a halting error condition is > > > > generated by one of the invoked LSM handlers. The checks for how > > > > to handle cleanup are executed at the end of the loop regardless > > > > of how the loop terminates. > > > > > > > > A two exit label strategy is implemented. One of the exit > > > > labels is a target for the no attribute case while the second is > > > > the target for the case where memory allocated for processing of > > > > extended attributes needs to be freed. > > > > > > > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > > > > --- > > > > security/security.c | 24 ++++++++++++------------ > > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > ... > > > > Looking at this quickly, why does something like the following not work? > > > > > > [Warning: copy-n-paste patch, likely whitespace damaged] > > > > > > diff --git a/security/security.c b/security/security.c > > > index 7e118858b545..007ce438e636 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str > > > uct inode *dir, > > > if (unlikely(IS_PRIVATE(inode))) > > > return 0; > > > > > > - if (!blob_sizes.lbs_xattr_count) > > > - return 0; > > > - > > > - if (initxattrs) { > > > + if (initxattrs && blob_sizes.lbs_xattr_count) { > > > /* Allocate +1 as terminator. */ > > > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > > > sizeof(*new_xattrs), GFP_NOFS); > > > > We ran with something similar to the above for several days of TSEMv3 > > testing. > > > > For the patch that we submitted upstream, we elected to take a 'belt > > and suspenders' approach that isolated the 'no attributes' execution > > flow from the flow followed if extended attributes are present. > > > > The approach used doesn't make any difference to us as long as we get > > the functionality of the hook restored. > I'd prefer the simpler approach. I'd likely also prefer we park > this patch until it is needed upstream, or am I misunderstanding > things and upstream is currently broken without a fix like this? As of the 6.8 release, a security handler that previously functioned in a consistent manner now functions inconsistently depending on the LSM stacking configuration that is in effect. Perhaps more problematically, when the handler does not function correctly, there is no indication of that fact passed upward to the LSM invoking the handler. This would cause the LSM to conclude that a security relevant action was conducted when it did not actually occur. I believe we would all universally conclude that having security critical infrastructure function in a consistent and reliable manner is of benefit, so we should return the previous behavior of the handler, particularly since it can be done with a one line fix if that is your preference. If you would be so kind, please put a 'Reported-by:' tag on whatever you commit upstream. > > If you go with the simpler approach, it may be worthwhile to at least > > simplify the handling of the call to the initxattr() function after > > the evm_inode_init_security() call. > > Starting with v6.9-rc1 there is no longer an explicit call to > evm_inode_init_security() as it is incorporated into the normal LSM > hook processing, e.g. `hp->hook.inode_init_security(...)`. I'm also > not sure we need to worry about the initxattrs() call near the bottom > of security_inode_init_security() since in the no > @blob.lbs_xattr_count case the @xattr_count variable will also be zero > so the initxattrs() call will be skipped. > > Or were you talking about something else? We were discussing something else but it isn't as important as getting the security handler fixed, so lets just proceed with that and we can call it a day. > paul-moore.com Have a good day. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
On Wed, Mar 27, 2024 at 5:17 AM Dr. Greg <greg@enjellic.com> wrote: > On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote: > > On Tue, Mar 26, 2024 at 6:31???AM Dr. Greg <greg@enjellic.com> wrote: > > > On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote: > > > > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg@enjellic.com> wrote: > > > > > > > > > > The integration of the Integrity Measurement Architecture (IMA) > > > > > into the LSM infrastructure introduced a conditional check that > > > > > denies access to the security_inode_init_security() event handler > > > > > if the LSM extended attribute 'blob' size is 0. > > > > > > > > > > This changes the previous behavior of this event handler and > > > > > results in variable behavior of LSM's depending on the LSM boot > > > > > configuration. > > > > > > > > > > Modify the function so that it removes the need for a non-zero > > > > > extended attribute blob size and bypasses the memory allocation > > > > > and freeing that is not needed if the LSM infrastructure is not > > > > > using extended attributes. > > > > > > > > > > Use a break statement to exit the loop that is iterating over the > > > > > defined handlers for this event if a halting error condition is > > > > > generated by one of the invoked LSM handlers. The checks for how > > > > > to handle cleanup are executed at the end of the loop regardless > > > > > of how the loop terminates. > > > > > > > > > > A two exit label strategy is implemented. One of the exit > > > > > labels is a target for the no attribute case while the second is > > > > > the target for the case where memory allocated for processing of > > > > > extended attributes needs to be freed. > > > > > > > > > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > > > > > --- > > > > > security/security.c | 24 ++++++++++++------------ > > > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > ... > > > > > > Looking at this quickly, why does something like the following not work? > > > > > > > > [Warning: copy-n-paste patch, likely whitespace damaged] > > > > > > > > diff --git a/security/security.c b/security/security.c > > > > index 7e118858b545..007ce438e636 100644 > > > > --- a/security/security.c > > > > +++ b/security/security.c > > > > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str > > > > uct inode *dir, > > > > if (unlikely(IS_PRIVATE(inode))) > > > > return 0; > > > > > > > > - if (!blob_sizes.lbs_xattr_count) > > > > - return 0; > > > > - > > > > - if (initxattrs) { > > > > + if (initxattrs && blob_sizes.lbs_xattr_count) { > > > > /* Allocate +1 as terminator. */ > > > > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > > > > sizeof(*new_xattrs), GFP_NOFS); > > > > > > We ran with something similar to the above for several days of TSEMv3 > > > testing. > > > > > > For the patch that we submitted upstream, we elected to take a 'belt > > > and suspenders' approach that isolated the 'no attributes' execution > > > flow from the flow followed if extended attributes are present. > > > > > > The approach used doesn't make any difference to us as long as we get > > > the functionality of the hook restored. > > > I'd prefer the simpler approach. I'd likely also prefer we park > > this patch until it is needed upstream, or am I misunderstanding > > things and upstream is currently broken without a fix like this? > > As of the 6.8 release, a security handler that previously functioned > in a consistent manner now functions inconsistently depending on the > LSM stacking configuration that is in effect. In Linux v6.8[1] only Smack and SELinux provide implementations for the security_inode_init_security() hook, and both also increment the associated lsm_blob_sizes::lbs_xattr_count field. While the behavior of the hook may have changed, I see no indications of any harm with respect to the standard upstream Linux kernel. We obviously want to ensure that we work to fix harmful behavior, but I simply don't see that here; convince me there is a problem, send me a patch as we've discussed, and I'll merge it. If we are talking about future code, simply include the change with the associated patchset. If we are talking about out-of-tree code, that's something else. [1] In Linux v6.9-rc1 this grows to include EVM, but EVM also provides both a hook implementation and a lbs_xattr_count bump. > Perhaps more problematically, when the handler does not function > correctly, there is no indication of that fact passed upward to the > LSM invoking the handler. This would cause the LSM to conclude that a > security relevant action was conducted when it did not actually occur. > > I believe we would all universally conclude that having security > critical infrastructure function in a consistent and reliable manner > is of benefit, so we should return the previous behavior of the > handler, particularly since it can be done with a one line fix if that > is your preference. You need to demonstrate the harm caused to the upstream Linux kernel, either a proper tagged release in Linus' tree, the current development code in Linus tree, or a subsystem branch/repository. > If you would be so kind, please put a 'Reported-by:' tag on whatever > you commit upstream. As you initially submitted a patch for this, it would be preferable if you would send a patch ... if necessary (see above comments). Of course if you are unable to do so, and we all agree that a problem in the upstream kernel exists, I can submit a patch with the appropriate credit. I will mention that bug fixes like this are a great way for new contributors to gain experience working with the upstream Linux kernel; I would encourage you to see this through. As frustrating as this might be, debates like this are part of the process :)
On Wed, Mar 27, 2024 at 11:18:47AM -0400, Paul Moore wrote: Good morning to everyone. > On Wed, Mar 27, 2024 at 5:17???AM Dr. Greg <greg@enjellic.com> wrote: > > On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote: > > > On Tue, Mar 26, 2024 at 6:31???AM Dr. Greg <greg@enjellic.com> wrote: > > > > On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote: > > > > > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg@enjellic.com> wrote: > > > > > > > > > > > > The integration of the Integrity Measurement Architecture (IMA) > > > > > > into the LSM infrastructure introduced a conditional check that > > > > > > denies access to the security_inode_init_security() event handler > > > > > > if the LSM extended attribute 'blob' size is 0. > > > > > > > > > > > > This changes the previous behavior of this event handler and > > > > > > results in variable behavior of LSM's depending on the LSM boot > > > > > > configuration. > > > > > > > > > > > > Modify the function so that it removes the need for a non-zero > > > > > > extended attribute blob size and bypasses the memory allocation > > > > > > and freeing that is not needed if the LSM infrastructure is not > > > > > > using extended attributes. > > > > > > > > > > > > Use a break statement to exit the loop that is iterating over the > > > > > > defined handlers for this event if a halting error condition is > > > > > > generated by one of the invoked LSM handlers. The checks for how > > > > > > to handle cleanup are executed at the end of the loop regardless > > > > > > of how the loop terminates. > > > > > > > > > > > > A two exit label strategy is implemented. One of the exit > > > > > > labels is a target for the no attribute case while the second is > > > > > > the target for the case where memory allocated for processing of > > > > > > extended attributes needs to be freed. > > > > > > > > > > > > Signed-off-by: Greg Wettstein <greg@enjellic.com> > > > > > > --- > > > > > > security/security.c | 24 ++++++++++++------------ > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > > > ... > > > > > > > > Looking at this quickly, why does something like the following not work? > > > > > > > > > > [Warning: copy-n-paste patch, likely whitespace damaged] > > > > > > > > > > diff --git a/security/security.c b/security/security.c > > > > > index 7e118858b545..007ce438e636 100644 > > > > > --- a/security/security.c > > > > > +++ b/security/security.c > > > > > @@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str > > > > > uct inode *dir, > > > > > if (unlikely(IS_PRIVATE(inode))) > > > > > return 0; > > > > > > > > > > - if (!blob_sizes.lbs_xattr_count) > > > > > - return 0; > > > > > - > > > > > - if (initxattrs) { > > > > > + if (initxattrs && blob_sizes.lbs_xattr_count) { > > > > > /* Allocate +1 as terminator. */ > > > > > new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1, > > > > > sizeof(*new_xattrs), GFP_NOFS); > > > > > > > > We ran with something similar to the above for several days of TSEMv3 > > > > testing. > > > > > > > > For the patch that we submitted upstream, we elected to take a 'belt > > > > and suspenders' approach that isolated the 'no attributes' execution > > > > flow from the flow followed if extended attributes are present. > > > > > > > > The approach used doesn't make any difference to us as long as we get > > > > the functionality of the hook restored. > > > > > I'd prefer the simpler approach. I'd likely also prefer we park > > > this patch until it is needed upstream, or am I misunderstanding > > > things and upstream is currently broken without a fix like this? > > > > As of the 6.8 release, a security handler that previously functioned > > in a consistent manner now functions inconsistently depending on the > > LSM stacking configuration that is in effect. > In Linux v6.8[1] only Smack and SELinux provide implementations for > the security_inode_init_security() hook, and both also increment the > associated lsm_blob_sizes::lbs_xattr_count field. While the > behavior of the hook may have changed, I see no indications of any > harm with respect to the standard upstream Linux kernel. We > obviously want to ensure that we work to fix harmful behavior, but I > simply don't see that here; convince me there is a problem, send me > a patch as we've discussed, and I'll merge it. BPF provides an implementation and would be affected. Simply grepping for 'inode_init_security', in the security sub-directory of the kernel source tree, will miss the dependency. Google initialized the BPF LSM hooks by redefining the LSM_HOOK macro before including the include/linux/lsm_hook_defs.h file. This causes all of the LSM event handlers to be defined as BPF LSM hooks but doesn't explicitly disclose any of the event handlers by name. We just checked our documentation on BPF LSM tap points for CrowdStrike's Falcon agent, which is a couple of months old by now, and don't see them using it, but that is only a small and dated sample. You probably remember, I believe you were there for an LSM update panel, that Alexei Starovoitov commented in his keynote talk at the European Linux Security Summit last September, that he estimated 90% of the use of the BPF LSM is not publically disclosed. Given that, affecting any functionality, particularly the way this issue does, would seem to be imprudent. > If we are talking about future code, simply include the change with > the associated patchset. Given the dynamics of all this, it would seem to make sense for this to be a straight forward fix that would get directed to stable. > If we are talking about out-of-tree code, that's something else. We understand Linux kernel dynamics pretty well [1], we wouldn't advocate for changes to mainline for such initiatives. > [1] In Linux v6.9-rc1 this grows to include EVM, but EVM also provides > both a hook implementation and a lbs_xattr_count bump. BPF initialization, as of 6.8 does not include an xattr request. > > Perhaps more problematically, when the handler does not function > > correctly, there is no indication of that fact passed upward to the > > LSM invoking the handler. This would cause the LSM to conclude that a > > security relevant action was conducted when it did not actually occur. > > > > I believe we would all universally conclude that having security > > critical infrastructure function in a consistent and reliable manner > > is of benefit, so we should return the previous behavior of the > > handler, particularly since it can be done with a one line fix if that > > is your preference. > You need to demonstrate the harm caused to the upstream Linux kernel, > either a proper tagged release in Linus' tree, the current development > code in Linus tree, or a subsystem branch/repository. BPF would be currently affected, including its derivatives, from 6.8 forward. > > If you would be so kind, please put a 'Reported-by:' tag on whatever > > you commit upstream. > As you initially submitted a patch for this, it would be preferable > if you would send a patch ... if necessary (see above comments). Of > course if you are unable to do so, and we all agree that a problem > in the upstream kernel exists, I can submit a patch with the > appropriate credit. The one-liner is a simple, straight forward and acceptable fix but it would not be our preference for fixing it. Given that, it would probably be best to go in under alternate authorship. > I will mention that bug fixes like this are a great way for new > contributors to gain experience working with the upstream Linux > kernel; I would encourage you to see this through. As frustrating > as this might be, debates like this are part of the process :) [1]: With respect to a new contributor gaining experience. For whatever it is worth moving forward, personally, I've been spinning patches for the Linux kernel since December of 1991. So I have a high degree of familiarity with respect to the dynamics of Linux kernel development and how they have evolved over time. Personally I also enjoy a substantive background in forensic exchange, so debate is not a foreign concept by any means. Given that a one-liner restores long standing behavior and potentially fixes what could be silent corruption of desired security functionality, there would seem to be little basis for debate, as entertaining as that may be... :-) > paul-moore.com Have a good remainder of the day. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
On 3/28/2024 8:38 AM, Dr. Greg wrote: > ... >> In Linux v6.8[1] only Smack and SELinux provide implementations for >> the security_inode_init_security() hook, and both also increment the >> associated lsm_blob_sizes::lbs_xattr_count field. While the >> behavior of the hook may have changed, I see no indications of any >> harm with respect to the standard upstream Linux kernel. We >> obviously want to ensure that we work to fix harmful behavior, but I >> simply don't see that here; convince me there is a problem, send me >> a patch as we've discussed, and I'll merge it. > BPF provides an implementation and would be affected. BPF has chosen to implement its LSM hooks their own way. As it is impossible for the infrastructure developers to predict what the behavior of those hooks may be, it is unreasonable to constrain them based on hypothetical or rumored use cases. The implementation of BPF precludes its use of LSM blobs that are infrastructure managed. That ought to be obvious. BPF could include a non-zero lbs_xattr_count just in case, and your problem would be solved, but at a cost. > Bear poking trimmed ... > > [1] In Linux v6.9-rc1 this grows to include EVM, but EVM also provides > both a hook implementation and a lbs_xattr_count bump. > BPF initialization, as of 6.8 does not include an xattr request. Just so. If BPF wants to use the aforementioned interface, it needs to include an xattr request. Just like any other LSM.
On Thu, Mar 28, 2024 at 11:38 AM Dr. Greg <greg@enjellic.com> wrote: > > BPF provides an implementation and would be affected ... Casey pretty much summed up my thoughts fairly well, including the "Bear poking trimmed" comment, which was worth a good laugh :)
On Thu, Mar 28, 2024 at 08:26:11PM -0400, Paul Moore wrote: > On Thu, Mar 28, 2024 at 11:38???AM Dr. Greg <greg@enjellic.com> wrote: > > > > BPF provides an implementation and would be affected ... > Casey pretty much summed up my thoughts fairly well, including the > "Bear poking trimmed" comment, which was worth a good laugh :) Very good, we will take Casey's e-mail as the official position of the Linux security maintainers on the functionality under discussion and similar issues moving forward. > paul-moore.com Have a good weekend. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
On Thu, Mar 28, 2024 at 09:34:39AM -0700, Casey Schaufler wrote: Good afternoon, I hope the weekend is going well for everyone. > On 3/28/2024 8:38 AM, Dr. Greg wrote: > > ... > >> In Linux v6.8[1] only Smack and SELinux provide implementations for > >> the security_inode_init_security() hook, and both also increment the > >> associated lsm_blob_sizes::lbs_xattr_count field. While the > >> behavior of the hook may have changed, I see no indications of any > >> harm with respect to the standard upstream Linux kernel. We > >> obviously want to ensure that we work to fix harmful behavior, but I > >> simply don't see that here; convince me there is a problem, send me > >> a patch as we've discussed, and I'll merge it. > > BPF provides an implementation and would be affected. > BPF has chosen to implement its LSM hooks their own way. As it is > impossible for the infrastructure developers to predict what the > behavior of those hooks may be, it is unreasonable to constrain them > based on hypothetical or rumored use cases. We were asked to identify a case where upstream could be possibly broken by the change in behavior, we did that. It is now perfectly clear that the LSM maintainers don't consider the possibility of breaking upstream BPF to be an issue of concern, no doubt an important clarification for everyone moving forward. > The implementation of BPF precludes its use of LSM blobs that are > infrastructure managed. That ought to be obvious. BPF could include > a non-zero lbs_xattr_count just in case, and your problem would be > solved, but at a cost. FWIW, it would not seem unreasonable to assume that an LSM, BPF included, may want to be notified of the the instantiation of the security state of an inode, regardless of whether or not the LSM is using extended attributes. > > Bear poking trimmed ... > > > > [1] In Linux v6.9-rc1 this grows to include EVM, but EVM also provides > > both a hook implementation and a lbs_xattr_count bump. > > BPF initialization, as of 6.8 does not include an xattr request. > Just so. If BPF wants to use the aforementioned interface, it needs to > include an xattr request. Just like any other LSM. Requirement so noted. Have a good week. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity https://github.com/Quixote-Project
On Sat, Mar 30, 2024 at 10:46 AM Dr. Greg <greg@enjellic.com> wrote: > On Thu, Mar 28, 2024 at 08:26:11PM -0400, Paul Moore wrote: > > On Thu, Mar 28, 2024 at 11:38???AM Dr. Greg <greg@enjellic.com> wrote: > > > > > > BPF provides an implementation and would be affected ... > > > Casey pretty much summed up my thoughts fairly well, including the > > "Bear poking trimmed" comment, which was worth a good laugh :) > > Very good, we will take Casey's e-mail as the official position of the > Linux security maintainers on the functionality under discussion and > similar issues moving forward. You're welcome to take whatever lessons you want from this thread, that is your choice, but please understand that your interpretation of this thread may not accurately reflect the opinions or policies, either now or in the future, of the subsystem maintainers. I understand that developers/engineers like hard rules, it's reassuring and comforting; I'm right there with you. Unfortunately, the Linux kernel is a bizarrely complex beast with changes happening on a regular basis and in an often unpredictable way. While I do attempt to provide guidelines on certain things, e.g. new LSMs, new LSM hooks, etc., ultimately decisions still boil down to the wonderfully/frustratingly vague "maintainer's discretion". In this thread, especially the last few messages, the only "position" I would suggest one take as a lesson, is that the LSM developers don't need to be told about the BPF LSM, or BPF in general, because we have all be struggling (?) with the challenges it brings for many, many years already. That isn't to say the BPF LSM, or eBPF in general, is a bad technology - you can definitely do some cool things with it - but integrating it into the kernel, and determining the appropriate boundaries between BPF code and the kernel internals, has been (and continues to be) a struggle. Simply dig through the archives and you'll see more than a few threads on this subject.
On Sat, Mar 30, 2024 at 4:14 PM Dr. Greg <greg@enjellic.com> wrote: > > It is now perfectly clear that the LSM maintainers don't consider the > possibility of breaking upstream BPF to be an issue of concern, no > doubt an important clarification for everyone moving forward. I've said this before on-list, but I'll repeat it here for those who might not follow every thread or email. The BPF LSM is a bit of an odd case as while there is a very simple structure (framework?) present in-tree, the actual implementation of the LSM is out-of-tree. While one could draw some parallels between BPF LSM implementations and other LSMs with loadable security policies, there is an important difference in that the conventional LSMs with loadable security policies separate the security policy from the enforcement engine code and maintain the enforcement engine code in the upstream Linux kernel. The BPF LSM maintains the enforcement engine code outside the upstream Linux kernel and because of that it is impossible for us, the upstream Linux devs, to do any meaningful analysis of BPF LSM behaviors. The result of this is that I currently give individual BPF LSMs largely the same consideration I would give out-of-tree kernel code: I am not going to go out of my way to block, or otherwise negatively impact the implementations, but I'm not going to sacrifice the development of any of the in-tree LSMs, or the LSM layer itself, solely for the advantage of an out-of-tree implementation. If you're really craving an official policy, that's the policy-of-the-moment.
diff --git a/security/security.c b/security/security.c index 7035ee35a393..a0b52b964688 100644 --- a/security/security.c +++ b/security/security.c @@ -1717,10 +1717,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, if (unlikely(IS_PRIVATE(inode))) return 0; - if (!blob_sizes.lbs_xattr_count) - return 0; - - if (initxattrs) { + if (blob_sizes.lbs_xattr_count && initxattrs) { /* Allocate +1 for EVM and +1 as terminator. */ new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2, sizeof(*new_xattrs), GFP_NOFS); @@ -1733,7 +1730,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs, &xattr_count); if (ret && ret != -EOPNOTSUPP) - goto out; + break; /* * As documented in lsm_hooks.h, -EOPNOTSUPP in this context * means that the LSM is not willing to provide an xattr, not @@ -1742,19 +1739,22 @@ int security_inode_init_security(struct inode *inode, struct inode *dir, */ } - /* If initxattrs() is NULL, xattr_count is zero, skip the call. */ - if (!xattr_count) - goto out; + /* Skip xattr processing if no attributes are in use. */ + if (!blob_sizes.lbs_xattr_count) + goto out2; + /* No attrs or an LSM returned an actionable error code. */ + if (!xattr_count || (ret && ret != -EOPNOTSUPP)) + goto out1; ret = evm_inode_init_security(inode, dir, qstr, new_xattrs, &xattr_count); - if (ret) - goto out; - ret = initxattrs(inode, new_xattrs, fs_data); -out: + if (!ret) + ret = initxattrs(inode, new_xattrs, fs_data); + out1: for (; xattr_count > 0; xattr_count--) kfree(new_xattrs[xattr_count - 1].value); kfree(new_xattrs); + out2: return (ret == -EOPNOTSUPP) ? 0 : ret; } EXPORT_SYMBOL(security_inode_init_security);
The integration of the Integrity Measurement Architecture (IMA) into the LSM infrastructure introduced a conditional check that denies access to the security_inode_init_security() event handler if the LSM extended attribute 'blob' size is 0. This changes the previous behavior of this event handler and results in variable behavior of LSM's depending on the LSM boot configuration. Modify the function so that it removes the need for a non-zero extended attribute blob size and bypasses the memory allocation and freeing that is not needed if the LSM infrastructure is not using extended attributes. Use a break statement to exit the loop that is iterating over the defined handlers for this event if a halting error condition is generated by one of the invoked LSM handlers. The checks for how to handle cleanup are executed at the end of the loop regardless of how the loop terminates. A two exit label strategy is implemented. One of the exit labels is a target for the no attribute case while the second is the target for the case where memory allocated for processing of extended attributes needs to be freed. Signed-off-by: Greg Wettstein <greg@enjellic.com> --- security/security.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)