Hi all,
While debugging a KASAN report in the selinux access vector cache hash
table, I noticed that it looks like we may block in the inode_follow_link()
and inode_permission() callbacks, even when called from the VFS layer as
part of an RCU-protected path walk.
These two patches attempt to fix that, but since I found this by
inspection and I'm not familiar with this code, I'm sending as an RFC in
case I missed something that means this cannot happen.
Comments very welcome,
Will
Cc: Paul Moore <[email protected]>
Cc: Ondrej Mosnacek <[email protected]>
Cc: Stephen Smalley <[email protected]>
Cc: Jeffrey Vander Stoep <[email protected]>
--->8
Will Deacon (2):
selinux: Don't call avc_compute_av() from RCU path walk
selinux: Propagate RCU walk status from 'security_inode_follow_link()'
security/selinux/avc.c | 21 +++++++++++++--------
security/selinux/hooks.c | 5 +++--
security/selinux/include/avc.h | 12 ++++++++----
3 files changed, 24 insertions(+), 14 deletions(-)
--
2.24.0.432.g9d3f5f5b63-goog
'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 <[email protected]>
---
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,
--
2.24.0.432.g9d3f5f5b63-goog
'avc_compute_av()' can block, so we carefully exit the RCU read-side
critical section before calling it in 'avc_has_perm_noaudit()'.
Unfortunately, if we're calling from the VFS layer on the RCU path walk
via 'selinux_inode_permission()' then we're still actually in an RCU
read-side critical section and must not block.
'avc_denied()' already handles this by simply returning success and
postponing the auditing until we're called again on the slowpath, so
follow the same approach here and return early if the node lookup fails
on the RCU walk path.
Signed-off-by: Will Deacon <[email protected]>
---
security/selinux/avc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index ecd3829996aa..9c183c899e92 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -1159,16 +1159,19 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
rcu_read_lock();
node = avc_lookup(state->avc, ssid, tsid, tclass);
- if (unlikely(!node))
+ if (unlikely(!node)) {
+ if (flags & AVC_NONBLOCKING)
+ goto out;
node = avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
- else
+ } else {
memcpy(avd, &node->ae.avd, sizeof(*avd));
+ }
denied = requested & ~(avd->allowed);
if (unlikely(denied))
rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
flags, avd);
-
+out:
rcu_read_unlock();
return rc;
}
--
2.24.0.432.g9d3f5f5b63-goog
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 <[email protected]>
> ---
> 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,
>
On 11/19/19 1:40 PM, Will Deacon wrote:
> 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> critical section before calling it in 'avc_has_perm_noaudit()'.
> Unfortunately, if we're calling from the VFS layer on the RCU path walk
> via 'selinux_inode_permission()' then we're still actually in an RCU
> read-side critical section and must not block.
avc_compute_av() should never block AFAIK. The blocking concern was with
slow_avc_audit(), and even that appears dubious to me. That seems to be
more about misuse of d_find_alias in dump_common_audit_data() than anything.
>
> 'avc_denied()' already handles this by simply returning success and
> postponing the auditing until we're called again on the slowpath, so
> follow the same approach here and return early if the node lookup fails
> on the RCU walk path.
>
> Signed-off-by: Will Deacon <[email protected]>
> ---
> security/selinux/avc.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index ecd3829996aa..9c183c899e92 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -1159,16 +1159,19 @@ inline int avc_has_perm_noaudit(struct selinux_state *state,
> rcu_read_lock();
>
> node = avc_lookup(state->avc, ssid, tsid, tclass);
> - if (unlikely(!node))
> + if (unlikely(!node)) {
> + if (flags & AVC_NONBLOCKING)
> + goto out;
> node = avc_compute_av(state, ssid, tsid, tclass, avd, &xp_node);
> - else
> + } else {
> memcpy(avd, &node->ae.avd, sizeof(*avd));
> + }
>
> denied = requested & ~(avd->allowed);
> if (unlikely(denied))
> rc = avc_denied(state, ssid, tsid, tclass, requested, 0, 0,
> flags, avd);
> -
> +out:
> rcu_read_unlock();
> return rc;
> }
>
Hi Stephen,
Thanks for the quick reply.
On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> On 11/19/19 1:40 PM, Will Deacon wrote:
> > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > critical section before calling it in 'avc_has_perm_noaudit()'.
> > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > via 'selinux_inode_permission()' then we're still actually in an RCU
> > read-side critical section and must not block.
>
> avc_compute_av() should never block AFAIK. The blocking concern was with
> slow_avc_audit(), and even that appears dubious to me. That seems to be more
> about misuse of d_find_alias in dump_common_audit_data() than anything.
Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
think it was propagated down to all of the potential allocations and
string functions. Having looked at it again, I can't see where it blocks.
Might be worth a comment in avc_compute_av(), because the temporary
dropping of rcu_read_lock() looks really dodgy when we could be running
on the RCU path walk path anyway.
Will
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!
On 11/20/19 8:12 AM, Will Deacon wrote:
> Hi Stephen,
>
> Thanks for the quick reply.
>
> On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
>> On 11/19/19 1:40 PM, Will Deacon wrote:
>>> 'avc_compute_av()' can block, so we carefully exit the RCU read-side
>>> critical section before calling it in 'avc_has_perm_noaudit()'.
>>> Unfortunately, if we're calling from the VFS layer on the RCU path walk
>>> via 'selinux_inode_permission()' then we're still actually in an RCU
>>> read-side critical section and must not block.
>>
>> avc_compute_av() should never block AFAIK. The blocking concern was with
>> slow_avc_audit(), and even that appears dubious to me. That seems to be more
>> about misuse of d_find_alias in dump_common_audit_data() than anything.
>
> Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> think it was propagated down to all of the potential allocations and
> string functions. Having looked at it again, I can't see where it blocks.
>
> Might be worth a comment in avc_compute_av(), because the temporary
> dropping of rcu_read_lock() looks really dodgy when we could be running
> on the RCU path walk path anyway.
I don't think that's a problem but I'll defer to the fsdevel and rcu
folks. The use of RCU within the SELinux AVC long predates the
introduction of RCU path walk, and the rcu_read_lock()/unlock() pairs
inside the AVC are not related in any way to RCU path walk. Hopefully
they don't break it. The SELinux security server (i.e.
security_compute_av() and the rest of security/selinux/ss/*) internally
has its own locking for its data structures, primarily the policy
rwlock. There was also a patch long ago to convert use of that policy
rwlock to RCU but it didn't seem justified at the time. We are
interested in revisiting that however. That would introduce its own set
of rcu_read_lock/unlock pairs inside of security_compute_av() as well.
On Wed, Nov 20, 2019 at 10:28:31AM -0500, Stephen Smalley wrote:
> On 11/20/19 8:12 AM, Will Deacon wrote:
> > Hi Stephen,
> >
> > Thanks for the quick reply.
> >
> > On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> > > On 11/19/19 1:40 PM, Will Deacon wrote:
> > > > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > > > critical section before calling it in 'avc_has_perm_noaudit()'.
> > > > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > > > via 'selinux_inode_permission()' then we're still actually in an RCU
> > > > read-side critical section and must not block.
> > >
> > > avc_compute_av() should never block AFAIK. The blocking concern was with
> > > slow_avc_audit(), and even that appears dubious to me. That seems to be more
> > > about misuse of d_find_alias in dump_common_audit_data() than anything.
> >
> > Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> > think it was propagated down to all of the potential allocations and
> > string functions. Having looked at it again, I can't see where it blocks.
> >
> > Might be worth a comment in avc_compute_av(), because the temporary
> > dropping of rcu_read_lock() looks really dodgy when we could be running
> > on the RCU path walk path anyway.
>
> I don't think that's a problem but I'll defer to the fsdevel and rcu folks.
> The use of RCU within the SELinux AVC long predates the introduction of RCU
> path walk, and the rcu_read_lock()/unlock() pairs inside the AVC are not
> related in any way to RCU path walk. Hopefully they don't break it. The
> SELinux security server (i.e. security_compute_av() and the rest of
> security/selinux/ss/*) internally has its own locking for its data
> structures, primarily the policy rwlock. There was also a patch long ago to
> convert use of that policy rwlock to RCU but it didn't seem justified at the
> time. We are interested in revisiting that however. That would introduce
> its own set of rcu_read_lock/unlock pairs inside of security_compute_av() as
> well.
RCU readers nest, so it should be fine. (Famous last words...)
Thanx, Paul
On Wed, Nov 20, 2019 at 11:07:43AM -0800, Paul E. McKenney wrote:
> On Wed, Nov 20, 2019 at 10:28:31AM -0500, Stephen Smalley wrote:
> > On 11/20/19 8:12 AM, Will Deacon wrote:
> > > Hi Stephen,
> > >
> > > Thanks for the quick reply.
> > >
> > > On Tue, Nov 19, 2019 at 01:59:40PM -0500, Stephen Smalley wrote:
> > > > On 11/19/19 1:40 PM, Will Deacon wrote:
> > > > > 'avc_compute_av()' can block, so we carefully exit the RCU read-side
> > > > > critical section before calling it in 'avc_has_perm_noaudit()'.
> > > > > Unfortunately, if we're calling from the VFS layer on the RCU path walk
> > > > > via 'selinux_inode_permission()' then we're still actually in an RCU
> > > > > read-side critical section and must not block.
> > > >
> > > > avc_compute_av() should never block AFAIK. The blocking concern was with
> > > > slow_avc_audit(), and even that appears dubious to me. That seems to be more
> > > > about misuse of d_find_alias in dump_common_audit_data() than anything.
> > >
> > > Apologies, I lost track of GFP_ATOMIC when I reading the code and didn't
> > > think it was propagated down to all of the potential allocations and
> > > string functions. Having looked at it again, I can't see where it blocks.
> > >
> > > Might be worth a comment in avc_compute_av(), because the temporary
> > > dropping of rcu_read_lock() looks really dodgy when we could be running
> > > on the RCU path walk path anyway.
> >
> > I don't think that's a problem but I'll defer to the fsdevel and rcu folks.
> > The use of RCU within the SELinux AVC long predates the introduction of RCU
> > path walk, and the rcu_read_lock()/unlock() pairs inside the AVC are not
> > related in any way to RCU path walk. Hopefully they don't break it. The
> > SELinux security server (i.e. security_compute_av() and the rest of
> > security/selinux/ss/*) internally has its own locking for its data
> > structures, primarily the policy rwlock. There was also a patch long ago to
> > convert use of that policy rwlock to RCU but it didn't seem justified at the
> > time. We are interested in revisiting that however. That would introduce
> > its own set of rcu_read_lock/unlock pairs inside of security_compute_av() as
> > well.
>
> RCU readers nest, so it should be fine. (Famous last words...)
Agreed. It was blocking that worried me, and it turns out that doesn't
happen for this code.
Will
FYI, we noticed the following commit (built with gcc-7):
commit: 5149a783b9655eb96bf418cf12a3d7786558df45 ("[RFC PATCH 2/2] selinux: Propagate RCU walk status from 'security_inode_follow_link()'")
url: https://github.com/0day-ci/linux/commits/Will-Deacon/selinux-Don-t-call-avc_compute_av-from-RCU-path-walk/20191122-104223
in testcase: boot
on test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 8G
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
+----------------------------------------------------------------------------+------------+------------+
| | c7e8a6ea45 | 5149a783b9 |
+----------------------------------------------------------------------------+------------+------------+
| boot_successes | 4 | 0 |
| boot_failures | 0 | 13 |
| WARNING:at_security/selinux/avc.c:#avc_has_perm_flags | 0 | 13 |
| RIP:avc_has_perm_flags | 0 | 13 |
| Kernel_panic-not_syncing:VFS:Unable_to_mount_root_fs_on_unknown-block(#,#) | 0 | 13 |
+----------------------------------------------------------------------------+------------+------------+
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
[ 4.885585] WARNING: CPU: 1 PID: 133 at security/selinux/avc.c:1156 avc_has_perm_flags+0x152/0x1f0
[ 4.885585] Modules linked in:
[ 4.885585] CPU: 1 PID: 133 Comm: kworker/u4:0 Not tainted 5.4.0-rc8-00017-g5149a783b9655 #1
[ 4.885585] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[ 4.885585] RIP: 0010:avc_has_perm_flags+0x152/0x1f0
[ 4.885585] Code: 04 75 aa 4c 8d 4c 24 20 4c 8d 44 24 0c 44 89 e1 44 89 ea 89 de 4c 89 f7 4c 89 1c 24 e8 f7 f0 ff ff 4c 8b 1c 24 e9 75 ff ff ff <0f> 0b b8 f3 ff ff ff eb a0 31 c0 44 8b 4c 24 14 41 21 d1 eb 8b 48
[ 4.885585] RSP: 0000:ffffb1fb4010fb00 EFLAGS: 00010246
[ 4.885585] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 000000000000000a
[ 4.885585] RDX: 0000000000000003 RSI: 0000000000000001 RDI: ffffffffb9d30620
[ 4.885585] RBP: 0000000000000001 R08: 0000000000000000 R09: 0000000000000002
[ 4.885585] R10: 0000000000000001 R11: ffffb1fb4010fba0 R12: ffff8c792a0e14d0
[ 4.885585] R13: ffff8c792a08da80 R14: ffffb1fb4010fd68 R15: ffff8c792a0e14d0
[ 4.885585] FS: 0000000000000000(0000) GS:ffff8c793fd00000(0000) knlGS:0000000000000000
[ 4.885585] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 4.885585] CR2: 00000000ffffffff CR3: 00000001e240a000 CR4: 00000000000406e0
[ 4.885585] Call Trace:
[ 4.885585] ? lookup_fast+0xc8/0x2b0
[ 4.885585] ? _cond_resched+0x19/0x30
[ 4.885585] selinux_inode_follow_link+0x84/0xb0
[ 4.885585] security_inode_follow_link+0x37/0x50
[ 4.885585] trailing_symlink+0xb7/0x260
[ 4.885585] path_openat+0xc6/0x1550
[ 4.885585] do_filp_open+0x9b/0x110
[ 4.885585] ? do_open_execat+0x83/0x1a0
[ 4.885585] do_open_execat+0x83/0x1a0
[ 4.885585] __do_execve_file+0x623/0x930
[ 4.885585] ? _cond_resched+0x19/0x30
[ 4.885585] ? yield_to+0x1c0/0x1c0
[ 4.885585] do_execve+0x21/0x30
[ 4.885585] call_usermodehelper_exec_async+0x19a/0x1b0
[ 4.885585] ? recalc_sigpending+0x17/0x50
[ 4.885585] ? call_usermodehelper+0xa0/0xa0
[ 4.885585] ret_from_fork+0x35/0x40
[ 4.885585] ---[ end trace b767db40447c2290 ]---
To reproduce:
# build kernel
cd linux
cp config-5.4.0-rc8-00017-g5149a783b9655 .config
make HOSTCC=gcc-7 CC=gcc-7 ARCH=x86_64 olddefconfig prepare modules_prepare bzImage
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
bin/lkp qemu -k <bzImage> job-script # job-script is attached in this email
Thanks,
lkp