| Message ID | 20191119184057.14961-3-will@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Avoid blocking in selinux inode callbacks on RCU walk | expand |
On 11/19/19 1:40 PM, Will Deacon wrote: > 'selinux_inode_follow_link()' can be called as part of an RCU path walk, > and is passed a 'bool rcu' parameter to indicate whether or not it is > being called from within an RCU read-side critical section. > > Unfortunately, this knowledge is not propagated further and, instead, > 'avc_has_perm()' unconditionally passes a flags argument of '0' to both > 'avc_has_perm_noaudit()' and 'avc_audit()' which may block. > > Introduce 'avc_has_perm_flags()' which can be used safely from within an > RCU read-side critical section. Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop passing MAY_NOT_BLOCK to the AVC upon follow_link"). > > Signed-off-by: Will Deacon <will@kernel.org> > --- > security/selinux/avc.c | 12 +++++++----- > security/selinux/hooks.c | 5 +++-- > security/selinux/include/avc.h | 12 ++++++++---- > 3 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/security/selinux/avc.c b/security/selinux/avc.c > index 9c183c899e92..7d99dadd24d0 100644 > --- a/security/selinux/avc.c > +++ b/security/selinux/avc.c > @@ -1177,11 +1177,12 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, > } > > /** > - * avc_has_perm - Check permissions and perform any appropriate auditing. > + * avc_has_perm_flags - Check permissions and perform any appropriate auditing. > * @ssid: source security identifier > * @tsid: target security identifier > * @tclass: target security class > * @requested: requested permissions, interpreted based on @tclass > + * @flags: AVC_STRICT, AVC_NONBLOCKING, or 0 > * @auditdata: auxiliary audit data > * > * Check the AVC to determine whether the @requested permissions are granted > @@ -1192,17 +1193,18 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, > * permissions are granted, -%EACCES if any permissions are denied, or > * another -errno upon other errors. > */ > -int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass, > - u32 requested, struct common_audit_data *auditdata) > +int avc_has_perm_flags(struct selinux_state *state, u32 ssid, u32 tsid, > + u16 tclass, u32 requested, unsigned int flags, > + struct common_audit_data *auditdata) > { > struct av_decision avd; > int rc, rc2; > > - rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, 0, > + rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, flags, > &avd); > > rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc, > - auditdata, 0); > + auditdata, flags); > if (rc2) > return rc2; > return rc; > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 9625b99e677f..0c09f59a2740 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -3008,8 +3008,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode, > if (IS_ERR(isec)) > return PTR_ERR(isec); > > - return avc_has_perm(&selinux_state, > - sid, isec->sid, isec->sclass, FILE__READ, &ad); > + return avc_has_perm_flags(&selinux_state, sid, isec->sid, isec->sclass, > + rcu ? AVC_NONBLOCKING : 0, > + FILE__READ, &ad); > } > > static noinline int audit_inode_permission(struct inode *inode, > diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h > index 7be0e1e90e8b..0450e1b88182 100644 > --- a/security/selinux/include/avc.h > +++ b/security/selinux/include/avc.h > @@ -149,10 +149,14 @@ int avc_has_perm_noaudit(struct selinux_state *state, > unsigned flags, > struct av_decision *avd); > > -int avc_has_perm(struct selinux_state *state, > - u32 ssid, u32 tsid, > - u16 tclass, u32 requested, > - struct common_audit_data *auditdata); > +int avc_has_perm_flags(struct selinux_state *state, > + u32 ssid, u32 tsid, > + u16 tclass, u32 requested, > + unsigned flags, > + struct common_audit_data *auditdata); > + > +#define avc_has_perm(state, ssid, tsid, tclass, requested, auditdata) \ > + avc_has_perm_flags(state, ssid, tsid, tclass, requested, 0, auditdata) > > int avc_has_extended_perms(struct selinux_state *state, > u32 ssid, u32 tsid, u16 tclass, u32 requested, >
Hi Stephen, Thanks for the quick review. On Tue, Nov 19, 2019 at 01:46:37PM -0500, Stephen Smalley wrote: > On 11/19/19 1:40 PM, Will Deacon wrote: > > 'selinux_inode_follow_link()' can be called as part of an RCU path walk, > > and is passed a 'bool rcu' parameter to indicate whether or not it is > > being called from within an RCU read-side critical section. > > > > Unfortunately, this knowledge is not propagated further and, instead, > > 'avc_has_perm()' unconditionally passes a flags argument of '0' to both > > 'avc_has_perm_noaudit()' and 'avc_audit()' which may block. > > > > Introduce 'avc_has_perm_flags()' which can be used safely from within an > > RCU read-side critical section. > > Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop passing > MAY_NOT_BLOCK to the AVC upon follow_link"). Ha, not sure how I missed that -- my patch is almost a direct revert, including the name 'avs_has_perm_flags()'! My only concern is that the commit message for e46e01eebbbc asserts that the only use of MAY_NOT_BLOCK is in slow_avc_audit(), but AVC_NONBLOCKING is used more widely than that. For example: selinux_inode_follow_link() -> avc_has_perm() -> avc_has_perm_noaudit() -> avc_denied() -> avc_update_node() where we return early if AVC_NONBLOCKING is set, except flags are always zero on this path. Will
On 11/20/19 8:13 AM, Will Deacon wrote: > Hi Stephen, > > Thanks for the quick review. > > On Tue, Nov 19, 2019 at 01:46:37PM -0500, Stephen Smalley wrote: >> On 11/19/19 1:40 PM, Will Deacon wrote: >>> 'selinux_inode_follow_link()' can be called as part of an RCU path walk, >>> and is passed a 'bool rcu' parameter to indicate whether or not it is >>> being called from within an RCU read-side critical section. >>> >>> Unfortunately, this knowledge is not propagated further and, instead, >>> 'avc_has_perm()' unconditionally passes a flags argument of '0' to both >>> 'avc_has_perm_noaudit()' and 'avc_audit()' which may block. >>> >>> Introduce 'avc_has_perm_flags()' which can be used safely from within an >>> RCU read-side critical section. >> >> Please see e46e01eebbbcf2ff6d28ee7cae9f117e9d1572c8 ("selinux: stop passing >> MAY_NOT_BLOCK to the AVC upon follow_link"). > > Ha, not sure how I missed that -- my patch is almost a direct revert, > including the name 'avs_has_perm_flags()'! My only concern is that the > commit message for e46e01eebbbc asserts that the only use of MAY_NOT_BLOCK > is in slow_avc_audit(), but AVC_NONBLOCKING is used more widely than that. > > For example: > > selinux_inode_follow_link() > -> avc_has_perm() > -> avc_has_perm_noaudit() > -> avc_denied() > -> avc_update_node() > > where we return early if AVC_NONBLOCKING is set, except flags are always > zero on this path. That was introduced by 3a28cff3bd4bf43f02be0c4e7933aebf3dc8197e ("selinux: avoid silent denials in permissive mode under RCU walk") and is only needed if we have to pass MAY_NOT_BLOCK to slow_avc_audit(), which is only presently needed in the selinux_inode_permission() case AFAICT. Both AVC_NONBLOCKING and MAY_NOT_BLOCK are misnomers wrt the AVC since it should never block regardless; the issue IIUC was rather the inability to safely collect the dentry name in an audit message during RCU walk per commit 0dc1ba24f7fff659725eecbba2c9ad679a0954cd (" SELINUX: Make selinux cache VFS RCU walks safe"). I'm not 100% certain about this; it is possible that the test in slow_avc_audit() is wrong and we ought to be doing this for any of LSM_AUDIT_DATA_PATH, _DENTRY, or _INODE (these were split out of LSM_AUDIT_DATA_FS). In that case, we should revert my earlier commit for follow_link and fix the test inside of slow_avc_audit() instead. I cc'd some additional folks who may have insight. Al, tell us if we got it wrong please!
diff --git a/security/selinux/avc.c b/security/selinux/avc.c index 9c183c899e92..7d99dadd24d0 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -1177,11 +1177,12 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, } /** - * avc_has_perm - Check permissions and perform any appropriate auditing. + * avc_has_perm_flags - Check permissions and perform any appropriate auditing. * @ssid: source security identifier * @tsid: target security identifier * @tclass: target security class * @requested: requested permissions, interpreted based on @tclass + * @flags: AVC_STRICT, AVC_NONBLOCKING, or 0 * @auditdata: auxiliary audit data * * Check the AVC to determine whether the @requested permissions are granted @@ -1192,17 +1193,18 @@ inline int avc_has_perm_noaudit(struct selinux_state *state, * permissions are granted, -%EACCES if any permissions are denied, or * another -errno upon other errors. */ -int avc_has_perm(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass, - u32 requested, struct common_audit_data *auditdata) +int avc_has_perm_flags(struct selinux_state *state, u32 ssid, u32 tsid, + u16 tclass, u32 requested, unsigned int flags, + struct common_audit_data *auditdata) { struct av_decision avd; int rc, rc2; - rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, 0, + rc = avc_has_perm_noaudit(state, ssid, tsid, tclass, requested, flags, &avd); rc2 = avc_audit(state, ssid, tsid, tclass, requested, &avd, rc, - auditdata, 0); + auditdata, flags); if (rc2) return rc2; return rc; diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 9625b99e677f..0c09f59a2740 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3008,8 +3008,9 @@ static int selinux_inode_follow_link(struct dentry *dentry, struct inode *inode, if (IS_ERR(isec)) return PTR_ERR(isec); - return avc_has_perm(&selinux_state, - sid, isec->sid, isec->sclass, FILE__READ, &ad); + return avc_has_perm_flags(&selinux_state, sid, isec->sid, isec->sclass, + rcu ? AVC_NONBLOCKING : 0, + FILE__READ, &ad); } static noinline int audit_inode_permission(struct inode *inode, diff --git a/security/selinux/include/avc.h b/security/selinux/include/avc.h index 7be0e1e90e8b..0450e1b88182 100644 --- a/security/selinux/include/avc.h +++ b/security/selinux/include/avc.h @@ -149,10 +149,14 @@ int avc_has_perm_noaudit(struct selinux_state *state, unsigned flags, struct av_decision *avd); -int avc_has_perm(struct selinux_state *state, - u32 ssid, u32 tsid, - u16 tclass, u32 requested, - struct common_audit_data *auditdata); +int avc_has_perm_flags(struct selinux_state *state, + u32 ssid, u32 tsid, + u16 tclass, u32 requested, + unsigned flags, + struct common_audit_data *auditdata); + +#define avc_has_perm(state, ssid, tsid, tclass, requested, auditdata) \ + avc_has_perm_flags(state, ssid, tsid, tclass, requested, 0, auditdata) int avc_has_extended_perms(struct selinux_state *state, u32 ssid, u32 tsid, u16 tclass, u32 requested,
'selinux_inode_follow_link()' can be called as part of an RCU path walk, and is passed a 'bool rcu' parameter to indicate whether or not it is being called from within an RCU read-side critical section. Unfortunately, this knowledge is not propagated further and, instead, 'avc_has_perm()' unconditionally passes a flags argument of '0' to both 'avc_has_perm_noaudit()' and 'avc_audit()' which may block. Introduce 'avc_has_perm_flags()' which can be used safely from within an RCU read-side critical section. Signed-off-by: Will Deacon <will@kernel.org> --- security/selinux/avc.c | 12 +++++++----- security/selinux/hooks.c | 5 +++-- security/selinux/include/avc.h | 12 ++++++++---- 3 files changed, 18 insertions(+), 11 deletions(-)