* [PATCH 1/2] fs: convert simple_xattrs to RCU list
2022-09-01 15:26 [PATCH 0/2] fs: fix capable() call in simple_xattr_list() Ondrej Mosnacek
@ 2022-09-01 15:26 ` Ondrej Mosnacek
2022-09-01 15:26 ` [PATCH 2/2] fs: don't call capable() prematurely in simple_xattr_list() Ondrej Mosnacek
2022-09-05 9:08 ` [PATCH 0/2] fs: fix capable() call " Christian Brauner
2 siblings, 0 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2022-09-01 15:26 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-fsdevel, linux-security-module, selinux, rcu, linux-kernel,
Martin Pitt
Use the RCU list mechanism instead of a simple lock to access/modify
simple_xattrs. The performance benefit is probably negligible, but it
will help avoid lock nesting concerns for an upcoming patch.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
fs/xattr.c | 36 ++++++++++++++++++++++--------------
include/linux/xattr.h | 1 +
2 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c
index a1f4998bc6be..fad2344f1168 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -22,6 +22,8 @@
#include <linux/audit.h>
#include <linux/vmalloc.h>
#include <linux/posix_acl_xattr.h>
+#include <linux/rculist.h>
+#include <linux/rcupdate.h>
#include <linux/uaccess.h>
@@ -1030,8 +1032,8 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
struct simple_xattr *xattr;
int ret = -ENODATA;
- spin_lock(&xattrs->lock);
- list_for_each_entry(xattr, &xattrs->head, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(xattr, &xattrs->head, list) {
if (strcmp(name, xattr->name))
continue;
@@ -1044,10 +1046,18 @@ int simple_xattr_get(struct simple_xattrs *xattrs, const char *name,
}
break;
}
- spin_unlock(&xattrs->lock);
+ rcu_read_unlock();
return ret;
}
+static void simple_xattr_free_rcu(struct rcu_head *rcu)
+{
+ struct simple_xattr *xattr = container_of(rcu, struct simple_xattr, rcu);
+
+ kfree(xattr->name);
+ kvfree(xattr);
+}
+
/**
* simple_xattr_set - xattr SET operation for in-memory/pseudo filesystems
* @xattrs: target simple_xattr list
@@ -1094,11 +1104,11 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
xattr = new_xattr;
err = -EEXIST;
} else if (new_xattr) {
- list_replace(&xattr->list, &new_xattr->list);
+ list_replace_rcu(&xattr->list, &new_xattr->list);
if (removed_size)
*removed_size = xattr->size;
} else {
- list_del(&xattr->list);
+ list_del_rcu(&xattr->list);
if (removed_size)
*removed_size = xattr->size;
}
@@ -1109,15 +1119,13 @@ int simple_xattr_set(struct simple_xattrs *xattrs, const char *name,
xattr = new_xattr;
err = -ENODATA;
} else {
- list_add(&new_xattr->list, &xattrs->head);
+ list_add_rcu(&new_xattr->list, &xattrs->head);
xattr = NULL;
}
out:
spin_unlock(&xattrs->lock);
- if (xattr) {
- kfree(xattr->name);
- kvfree(xattr);
- }
+ if (xattr)
+ call_rcu(&xattr->rcu, simple_xattr_free_rcu);
return err;
}
@@ -1169,8 +1177,8 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
}
#endif
- spin_lock(&xattrs->lock);
- list_for_each_entry(xattr, &xattrs->head, list) {
+ rcu_read_lock();
+ list_for_each_entry_rcu(xattr, &xattrs->head, list) {
/* skip "trusted." attributes for unprivileged callers */
if (!trusted && xattr_is_trusted(xattr->name))
continue;
@@ -1179,7 +1187,7 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
if (err)
break;
}
- spin_unlock(&xattrs->lock);
+ rcu_read_unlock();
return err ? err : size - remaining_size;
}
@@ -1191,6 +1199,6 @@ void simple_xattr_list_add(struct simple_xattrs *xattrs,
struct simple_xattr *new_xattr)
{
spin_lock(&xattrs->lock);
- list_add(&new_xattr->list, &xattrs->head);
+ list_add_rcu(&new_xattr->list, &xattrs->head);
spin_unlock(&xattrs->lock);
}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 979a9d3e5bfb..3236c469aaac 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -86,6 +86,7 @@ struct simple_xattrs {
struct simple_xattr {
struct list_head list;
+ struct rcu_head rcu;
char *name;
size_t size;
char value[];
--
2.37.2
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 2/2] fs: don't call capable() prematurely in simple_xattr_list()
2022-09-01 15:26 [PATCH 0/2] fs: fix capable() call in simple_xattr_list() Ondrej Mosnacek
2022-09-01 15:26 ` [PATCH 1/2] fs: convert simple_xattrs to RCU list Ondrej Mosnacek
@ 2022-09-01 15:26 ` Ondrej Mosnacek
2022-09-05 9:08 ` [PATCH 0/2] fs: fix capable() call " Christian Brauner
2 siblings, 0 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2022-09-01 15:26 UTC (permalink / raw)
To: Alexander Viro
Cc: linux-fsdevel, linux-security-module, selinux, rcu, linux-kernel,
Martin Pitt
Calling capable() pre-emptively causes a problem for SELinux, which will
normally log a denial whenever capable() is called and the task's
SELinux context doesn't have the corresponding capability permission
allowed.
With the current implementation of simple_xattr_list(), any time a
process without CAP_SYS_ADMIN calls listxattr(2) or similar on a
filesystem that uses this function, a denial is logged even if there are
no trusted.* xattrs on the inode in question. In such situation, policy
writers are forced to chose one of the following options:
1. Grant CAP_SYS_ADMIN to the given SELinux domain even though it
doesn't really need it. (Not good for security.)
2. Add a rule to the policy that will silence CAP_SYS_ADMIN denials for
the given domain without actually granting it. (Not good, because now
denials that make actual difference may be hidden, making
troubleshooting harder.)
3. Do nothing and let the denials appear. (Not good, because the audit
spam could obscure actual important denials.)
To avoid this misery, only call capable() when an actual trusted.* xattr
is encountered. This is somewhat less optimal, since capable() will now
be called once per each trusted.* xattr, but that's pretty unlikely to
matter in practice.
Even after this fix any process listing xattrs on an inode that has one
or more trusted.* ones may trigger an "irrelevant" denial if it doesn't
actually care about the trusted.* xattrs, but such cases should be rare
and thus silencing the denial in such cases would not be as big of a
deal.
Fixes: b09e0fa4b4ea ("tmpfs: implement generic xattr support")
Reported-by: Martin Pitt <mpitt@redhat.com>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
fs/xattr.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/fs/xattr.c b/fs/xattr.c
index fad2344f1168..84a459ac779a 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -1155,7 +1155,6 @@ static int xattr_list_one(char **buffer, ssize_t *remaining_size,
ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
char *buffer, size_t size)
{
- bool trusted = capable(CAP_SYS_ADMIN);
struct simple_xattr *xattr;
ssize_t remaining_size = size;
int err = 0;
@@ -1180,7 +1179,7 @@ ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
rcu_read_lock();
list_for_each_entry_rcu(xattr, &xattrs->head, list) {
/* skip "trusted." attributes for unprivileged callers */
- if (!trusted && xattr_is_trusted(xattr->name))
+ if (xattr_is_trusted(xattr->name) && !capable(CAP_SYS_ADMIN))
continue;
err = xattr_list_one(&buffer, &remaining_size, xattr->name);
--
2.37.2
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 0/2] fs: fix capable() call in simple_xattr_list()
2022-09-01 15:26 [PATCH 0/2] fs: fix capable() call in simple_xattr_list() Ondrej Mosnacek
2022-09-01 15:26 ` [PATCH 1/2] fs: convert simple_xattrs to RCU list Ondrej Mosnacek
2022-09-01 15:26 ` [PATCH 2/2] fs: don't call capable() prematurely in simple_xattr_list() Ondrej Mosnacek
@ 2022-09-05 9:08 ` Christian Brauner
2022-09-05 10:15 ` Ondrej Mosnacek
2 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2022-09-05 9:08 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: Alexander Viro, linux-fsdevel, linux-security-module, selinux,
rcu, linux-kernel, Martin Pitt, Vasily Averin
On Thu, Sep 01, 2022 at 05:26:30PM +0200, Ondrej Mosnacek wrote:
> The goal of these patches is to avoid calling capable() unconditionally
> in simple_xattr_list(), which causes issues under SELinux (see
> explanation in the second patch).
>
> The first patch tries to make this change safer by converting
> simple_xattrs to use the RCU mechanism, so that capable() is not called
> while the xattrs->lock is held. I didn't find evidence that this is an
> issue in the current code, but it can't hurt to make that change
> either way (and it was quite straightforward).
Hey Ondrey,
There's another patchset I'd like to see first which switches from a
linked list to an rbtree to get rid of performance issues in this code
that can be used to dos tmpfs in containers:
https://lore.kernel.org/lkml/d73bd478-e373-f759-2acb-2777f6bba06f@openvz.org
I don't think Vasily has time to continue with this so I'll just pick it
up hopefully this or the week after LPC.
Christian
^ permalink raw reply [flat|nested] 6+ messages in thread