* Re: [PATCH] security: apparmor: Fix a possible sleep-in-atomic-context bug in find_attach()
2019-12-26 22:30 ` Al Viro
@ 2020-01-02 19:19 ` John Johansen
0 siblings, 0 replies; 3+ messages in thread
From: John Johansen @ 2020-01-02 19:19 UTC (permalink / raw)
To: Al Viro, Jia-Ju Bai; +Cc: jmorris, serge, linux-security-module, linux-kernel
On 12/26/19 2:30 PM, Al Viro wrote:
> On Tue, Dec 17, 2019 at 09:12:20PM +0800, Jia-Ju Bai wrote:
>> The kernel may sleep while holding a RCU lock.
>> The function call path (from bottom to top) in Linux 4.19 is:
>>
>> security/apparmor/domain.c, 331:
>> vfs_getxattr_alloc(GFP_KERNEL) in aa_xattrs_match
>> security/apparmor/domain.c, 425:
>> aa_xattrs_match in __attach_match
>> security/apparmor/domain.c, 485:
>> __attach_match in find_attach
>> security/apparmor/domain.c, 484:
>> rcu_read_lock in find_attach
>>
>> vfs_getxattr_alloc(GFP_KERNEL) can sleep at runtime.
>>
>> To fix this possible bug, GFP_KERNEL is replaced with GFP_ATOMIC for
>> vfs_getxattr_alloc().
>>
>> This bug is found by a static analysis tool STCheck written by myself.
>>
>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>> ---
>> security/apparmor/domain.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>> index 9be7ccb8379e..60b54ce57d1f 100644
>> --- a/security/apparmor/domain.c
>> +++ b/security/apparmor/domain.c
>> @@ -325,7 +325,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
>>
>> for (i = 0; i < profile->xattr_count; i++) {
>> size = vfs_getxattr_alloc(d, profile->xattrs[i], &value,
>> - value_size, GFP_KERNEL);
>> + value_size, GFP_ATOMIC);
>
> <stares>
>
> How can that possibly make any sense? We are, by the look of it, trying
> to read extended attributes of some sort here. How the hell can that
> possibly hope to be non-blocking?
>
it can't
> <goes to read>
> Yup, vfs_getxattr_alloc() does call xattr ->get() method, which certainly
> can cause IO. GFP_ATOMIC affects only the allocation done in
> vfs_getxattr_alloc() itself, ->get() call doesn't even see it.
>
> AFAICS, that "fix" is pure cargo-culting; if that code *can* be called in
> non-blocking context, we are fucked, GFP_ATOMIC or no GFP_ATOMIC.
>
> Let's look at your call chain... find_attach() calls __attach_match()
> under rcu_read_lock(). Yes, it does, and by the look of the function
> itself it does expect to be called thus.
>
> Call of aa_xattrs_match() in there. Present, no obvious dropping/regaining
> rcu_read_lock() around it. Conditional upon perm & MAY_EXEC, but that
> doesn't seem to be provably excludable by the arguments of this particular
> call. And yes, aa_xattrs_match() is very obviously blocking.
>
yep this is broken. this needs to either break the rcu_read_lock or be switched
to a lock that can sleep.
> So you've caught a real bug, but the "fix" doesn't fix anything whatsoever;
> reading xattrs *does* block, no matter what.
>
> By the look at git blame, we have commit 8e51f9087f4024d20f70f4d9831e1f45d8088331
> Author: Matthew Garrett <mjg59@google.com>
> Date: Thu Feb 8 12:37:19 2018 -0800
>
> apparmor: Add support for attaching profiles via xattr, presence and value
>
> Make it possible to tie Apparmor profiles to the presence of one or more
> extended attributes, and optionally their values. An example usecase for
> this is to automatically transition to a more privileged Apparmor profile
> if an executable has a valid IMA signature, which can then be appraised
> by the IMA subsystem.
>
> Signed-off-by: Matthew Garrett <mjg59@google.com>
> Signed-off-by: John Johansen <john.johansen@canonical.com>
>
> to thank for that. And by the time of that commit the call chain already
> existed, complete with rcu_read_lock().
>
> AFAICS, it really is buggered by design - you can't read xattrs in such
> context. Again, GFP_ATOMIC is useless here - the problem is not in
> allocation, it's IO, possibly over network, etc.
>
indeed, below is a first pass at breaking the rcu_read_lock done against 5.5-rc4
> Morever, *anything* that passes GFP_ATOMIC to vfs_getxattr_alloc() is
> deeply suspect - it's almost certainly cargo-culting in attempt to
> do inherently blocking operation in non-blocking context. <greps>
> No GFP_ATOMIC (thankfully), but there's a bunch of GFP_NOFS and
> I really wonder if _those_ make any sense here...
>
---
commit b5b26347adae89f45a871f30f53d4f17b37cf2d4
Author: John Johansen <john.johansen@canonical.com>
Date: Thu Jan 2 05:31:22 2020 -0800
apparmor: fix aa_xattrs_match() may sleep while holding a RCU lock
aa_xattrs_match() is unfortunately calling vfs_getxattr_alloc() from a
context protected by an rcu_read_lock. This can not be done as
vfs_getxattr_alloc() may sleep regardles of the gfp_t value being
passed to it.
Fix this by breaking the rcu_read_lock on the policy search when the
xattr match feature is requested and restarting the search if policy
changes occur.
Fixes: 8e51f9087f40 ("apparmor: Add support for attaching profiles via xattr, presence and value")
Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: John Johansen <john.johansen@canonical.com>
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 09996f2552ee..47aff8700547 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -623,7 +623,7 @@ static __poll_t ns_revision_poll(struct file *file, poll_table *pt)
void __aa_bump_ns_revision(struct aa_ns *ns)
{
- ns->revision++;
+ WRITE_ONCE(ns->revision, ns->revision + 1);
wake_up_interruptible(&ns->wait);
}
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 9be7ccb8379e..6ceb74e0f789 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -317,6 +317,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
if (!bprm || !profile->xattr_count)
return 0;
+ might_sleep();
/* transition from exec match to xattr set */
state = aa_dfa_null_transition(profile->xmatch, state);
@@ -361,10 +362,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
}
/**
- * __attach_match_ - find an attachment match
+ * find_attach - do attachment search for unconfined processes
* @bprm - binprm structure of transitioning task
- * @name - to match against (NOT NULL)
+ * @ns: the current namespace (NOT NULL)
* @head - profile list to walk (NOT NULL)
+ * @name - to match against (NOT NULL)
* @info - info message if there was an error (NOT NULL)
*
* Do a linear search on the profiles in the list. There is a matching
@@ -374,12 +376,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
*
* Requires: @head not be shared or have appropriate locks held
*
- * Returns: profile or NULL if no match found
+ * Returns: label or NULL if no match found
*/
-static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
- const char *name,
- struct list_head *head,
- const char **info)
+static struct aa_label *find_attach(const struct linux_binprm *bprm,
+ struct aa_ns *ns, struct list_head *head,
+ const char *name, const char **info)
{
int candidate_len = 0, candidate_xattrs = 0;
bool conflict = false;
@@ -388,6 +389,8 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
AA_BUG(!name);
AA_BUG(!head);
+ rcu_read_lock();
+restart:
list_for_each_entry_rcu(profile, head, base.list) {
if (profile->label.flags & FLAG_NULL &&
&profile->label == ns_unconfined(profile->ns))
@@ -413,16 +416,32 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
perm = dfa_user_allow(profile->xmatch, state);
/* any accepting state means a valid match. */
if (perm & MAY_EXEC) {
- int ret;
+ int ret = 0;
if (count < candidate_len)
continue;
- ret = aa_xattrs_match(bprm, profile, state);
- /* Fail matching if the xattrs don't match */
- if (ret < 0)
- continue;
-
+ if (bprm && profile->xattr_count) {
+ long rev = READ_ONCE(ns->revision);
+
+ if (!aa_get_profile_not0(profile))
+ goto restart;
+ rcu_read_unlock();
+ ret = aa_xattrs_match(bprm, profile,
+ state);
+ rcu_read_lock();
+ aa_put_profile(profile);
+ if (rev !=
+ READ_ONCE(ns->revision))
+ /* policy changed */
+ goto restart;
+ /*
+ * Fail matching if the xattrs don't
+ * match
+ */
+ if (ret < 0)
+ continue;
+ }
/*
* TODO: allow for more flexible best match
*
@@ -445,43 +464,28 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
candidate_xattrs = ret;
conflict = false;
}
- } else if (!strcmp(profile->base.name, name))
+ } else if (!strcmp(profile->base.name, name)) {
/*
* old exact non-re match, without conditionals such
* as xattrs. no more searching required
*/
- return profile;
+ candidate = profile;
+ goto out;
+ }
}
- if (conflict) {
- *info = "conflicting profile attachments";
+ if (!candidate || conflict) {
+ if (conflict)
+ *info = "conflicting profile attachments";
+ rcu_read_unlock();
return NULL;
}
- return candidate;
-}
-
-/**
- * find_attach - do attachment search for unconfined processes
- * @bprm - binprm structure of transitioning task
- * @ns: the current namespace (NOT NULL)
- * @list: list to search (NOT NULL)
- * @name: the executable name to match against (NOT NULL)
- * @info: info message if there was an error
- *
- * Returns: label or NULL if no match found
- */
-static struct aa_label *find_attach(const struct linux_binprm *bprm,
- struct aa_ns *ns, struct list_head *list,
- const char *name, const char **info)
-{
- struct aa_profile *profile;
-
- rcu_read_lock();
- profile = aa_get_profile(__attach_match(bprm, name, list, info));
+out:
+ candidate = aa_get_newest_profile(candidate);
rcu_read_unlock();
- return profile ? &profile->label : NULL;
+ return &candidate->label;
}
static const char *next_name(int xtype, const char *name)
^ permalink raw reply related [flat|nested] 3+ messages in thread