* Re: [PATCH 09/19] fs: Refuse uid/gid changes which don't map into s_user_ns
From: Seth Forshee @ 2015-12-04 17:46 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204172738.GA2280@mail.hallyn.com>
On Fri, Dec 04, 2015 at 11:27:38AM -0600, Serge E. Hallyn wrote:
> On Wed, Dec 02, 2015 at 09:40:09AM -0600, Seth Forshee wrote:
> > Add checks to inode_change_ok to verify that uid and gid changes
> > will map into the superblock's user namespace. If they do not
> > fail with -EOVERFLOW. This cannot be overriden with ATTR_FORCE.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>
> ... although i could see root on the host being upset that it can't
> assign a uid not valid in the mounter's ns. But it does seem safer.
That change wouldn't be representable in the backing store though, and
that could lead to unexpected behaviour. It's better to tell root that
we can't make the requested change, in my opinion.
^ permalink raw reply
* [PATCH 1/2] md-cluster: update comments for MD_CLUSTER_SEND_LOCKED_ALREADY
From: Guoqing Jiang @ 2015-12-04 18:32 UTC (permalink / raw)
To: gqjiang, neilb; +Cc: linux-raid, rgoldwyn
1. fix unbalanced parentheses.
2. add more description about that MD_CLUSTER_SEND_LOCKED_ALREADY
will be cleared after set it in add_new_disk.
Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
drivers/md/md-cluster.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md-cluster.c b/drivers/md/md-cluster.c
index ad3ec7d..0ded8e9 100644
--- a/drivers/md/md-cluster.c
+++ b/drivers/md/md-cluster.c
@@ -53,11 +53,12 @@ struct resync_info {
* accomodate lock and hold. See next comment.
*/
#define MD_CLUSTER_SEND_LOCK 4
-/* If cluster operations must lock the communication channel,
- * so as to perform extra operations (and no other operation
- * is allowed on the MD, such as adding a disk. Token needs
- * to be locked and held until the operation completes with
- * a md_update_sb(), which would eventually release the lock.
+/* If cluster operations (such as adding a disk) must lock the
+ * communication channel, so as to perform extra operations
+ * (update metadata) and no other operation is allowed on the
+ * MD. Token needs to be locked and held until the operation
+ * completes witha md_update_sb(), which would eventually release
+ * the lock.
*/
#define MD_CLUSTER_SEND_LOCKED_ALREADY 5
@@ -1021,6 +1022,18 @@ static int add_new_disk(struct mddev *mddev, struct md_rdev *rdev)
unlock_comm(cinfo);
else {
dlm_lock_sync(cinfo->no_new_dev_lockres, DLM_LOCK_CR);
+ /* Since MD_CHANGE_DEVS will be set in add_bound_rdev which
+ * will run soon after add_new_disk, the below path will be
+ * invoked:
+ * md_wakeup_thread(mddev->thread)
+ * -> conf->thread (raid1d)
+ * -> md_check_recovery -> md_update_sb
+ * -> metadata_update_start/finish
+ * MD_CLUSTER_SEND_LOCKED_ALREADY will be cleared eventually.
+ *
+ * For other failure cases, metadata_update_cancel and
+ * add_new_disk_cancel also clear below bit as well.
+ * */
set_bit(MD_CLUSTER_SEND_LOCKED_ALREADY, &cinfo->state);
wake_up(&cinfo->wait);
}
--
2.1.4
^ permalink raw reply related
* [PATCH 2/2] md: update comment for md_allow_write
From: Guoqing Jiang @ 2015-12-04 18:32 UTC (permalink / raw)
To: gqjiang, neilb; +Cc: linux-raid, rgoldwyn
In-Reply-To: <1449253972-26997-1-git-send-email-gqjiang@suse.com>
MD_CHANGE_CLEAN had been replaced with MD_CHANGE_PENDING after
commit 070dc6 ("md: resolve confusion of MD_CHANGE_CLEAN"),
so make the change accordingly.
Signed-off-by: Guoqing Jiang <gqjiang@suse.com>
---
drivers/md/md.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 5ee18a0..0feff1c 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -7701,7 +7701,7 @@ EXPORT_SYMBOL(md_write_end);
* attempting a GFP_KERNEL allocation while holding the mddev lock.
* Must be called with mddev_lock held.
*
- * In the ->external case MD_CHANGE_CLEAN can not be cleared until mddev->lock
+ * In the ->external case MD_CHANGE_PENDING can not be cleared until mddev->lock
* is dropped, so return -EAGAIN after notifying userspace.
*/
int md_allow_write(struct mddev *mddev)
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 10/19] fs: Update posix_acl support to handle user namespace mounts
From: Serge E. Hallyn @ 2015-12-04 18:50 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <1449070821-73820-11-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> ids in on-disk ACLs should be converted to s_user_ns instead of
> init_user_ns as is done now. This introduces the possibility for
> id mappings to fail, and when this happens syscalls will return
> EOVERFLOW.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> fs/posix_acl.c | 67 ++++++++++++++++++++++++++---------------
> fs/xattr.c | 19 +++++++++---
> include/linux/posix_acl_xattr.h | 17 ++++++++---
> 3 files changed, 70 insertions(+), 33 deletions(-)
>
> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 4adde1e2cbec..a29442eb4af8 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c
> @@ -595,59 +595,77 @@ EXPORT_SYMBOL_GPL(posix_acl_create);
> /*
> * Fix up the uids and gids in posix acl extended attributes in place.
> */
> -static void posix_acl_fix_xattr_userns(
> +static int posix_acl_fix_xattr_userns(
> struct user_namespace *to, struct user_namespace *from,
> void *value, size_t size)
> {
> posix_acl_xattr_header *header = (posix_acl_xattr_header *)value;
> posix_acl_xattr_entry *entry = (posix_acl_xattr_entry *)(header+1), *end;
> int count;
> - kuid_t uid;
> - kgid_t gid;
> + kuid_t kuid;
> + uid_t uid;
> + kgid_t kgid;
> + gid_t gid;
>
> if (!value)
> - return;
> + return 0;
> if (size < sizeof(posix_acl_xattr_header))
> - return;
> + return 0;
> if (header->a_version != cpu_to_le32(POSIX_ACL_XATTR_VERSION))
> - return;
> + return 0;
>
> count = posix_acl_xattr_count(size);
> if (count < 0)
> - return;
> + return 0;
> if (count == 0)
> - return;
> + return 0;
>
> for (end = entry + count; entry != end; entry++) {
> switch(le16_to_cpu(entry->e_tag)) {
> case ACL_USER:
> - uid = make_kuid(from, le32_to_cpu(entry->e_id));
> - entry->e_id = cpu_to_le32(from_kuid(to, uid));
> + kuid = make_kuid(from, le32_to_cpu(entry->e_id));
> + if (!uid_valid(kuid))
> + return -EOVERFLOW;
> + uid = from_kuid(to, kuid);
> + if (uid == (uid_t)-1)
> + return -EOVERFLOW;
> + entry->e_id = cpu_to_le32(uid);
> break;
> case ACL_GROUP:
> - gid = make_kgid(from, le32_to_cpu(entry->e_id));
> - entry->e_id = cpu_to_le32(from_kgid(to, gid));
> + kgid = make_kgid(from, le32_to_cpu(entry->e_id));
> + if (!gid_valid(kgid))
> + return -EOVERFLOW;
> + gid = from_kgid(to, kgid);
> + if (gid == (gid_t)-1)
> + return -EOVERFLOW;
> + entry->e_id = cpu_to_le32(gid);
> break;
> default:
> break;
> }
> }
> +
> + return 0;
> }
>
> -void posix_acl_fix_xattr_from_user(void *value, size_t size)
> +int
> +posix_acl_fix_xattr_from_user(struct user_namespace *target_ns, void *value,
> + size_t size)
> {
> - struct user_namespace *user_ns = current_user_ns();
> - if (user_ns == &init_user_ns)
> - return;
> - posix_acl_fix_xattr_userns(&init_user_ns, user_ns, value, size);
> + struct user_namespace *source_ns = current_user_ns();
> + if (source_ns == target_ns)
> + return 0;
> + return posix_acl_fix_xattr_userns(target_ns, source_ns, value, size);
> }
>
> -void posix_acl_fix_xattr_to_user(void *value, size_t size)
> +int
> +posix_acl_fix_xattr_to_user(struct user_namespace *source_ns, void *value,
> + size_t size)
> {
> - struct user_namespace *user_ns = current_user_ns();
> - if (user_ns == &init_user_ns)
> - return;
> - posix_acl_fix_xattr_userns(user_ns, &init_user_ns, value, size);
> + struct user_namespace *target_ns = current_user_ns();
> + if (target_ns == source_ns)
> + return 0;
> + return posix_acl_fix_xattr_userns(target_ns, source_ns, value, size);
> }
>
> /*
> @@ -782,7 +800,7 @@ posix_acl_xattr_get(const struct xattr_handler *handler,
> if (acl == NULL)
> return -ENODATA;
>
> - error = posix_acl_to_xattr(&init_user_ns, acl, value, size);
> + error = posix_acl_to_xattr(dentry->d_sb->s_user_ns, acl, value, size);
> posix_acl_release(acl);
>
> return error;
> @@ -810,7 +828,8 @@ posix_acl_xattr_set(const struct xattr_handler *handler,
> return -EPERM;
>
> if (value) {
> - acl = posix_acl_from_xattr(&init_user_ns, value, size);
> + acl = posix_acl_from_xattr(dentry->d_sb->s_user_ns, value,
> + size);
> if (IS_ERR(acl))
> return PTR_ERR(acl);
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 9b932b95d74e..1268d8d5f74b 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -351,8 +351,12 @@ setxattr(struct dentry *d, const char __user *name, const void __user *value,
> goto out;
> }
> if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> - (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> - posix_acl_fix_xattr_from_user(kvalue, size);
> + (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0)) {
> + error = posix_acl_fix_xattr_from_user(d->d_sb->s_user_ns,
> + kvalue, size);
> + if (error)
> + goto out;
> + }
> }
>
> error = vfs_setxattr(d, kname, kvalue, size, flags);
> @@ -452,9 +456,14 @@ getxattr(struct dentry *d, const char __user *name, void __user *value,
> error = vfs_getxattr(d, kname, kvalue, size);
> if (error > 0) {
> if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
> - (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
> - posix_acl_fix_xattr_to_user(kvalue, size);
> - if (size && copy_to_user(value, kvalue, error))
> + (strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0)) {
> + int ret;
> + ret = posix_acl_fix_xattr_to_user(d->d_sb->s_user_ns,
> + kvalue, size);
> + if (ret)
> + error = ret;
> + }
> + if (error > 0 && size && copy_to_user(value, kvalue, error))
> error = -EFAULT;
> } else if (error == -ERANGE && size >= XATTR_SIZE_MAX) {
> /* The file system tried to returned a value bigger
> diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
> index 6f14ee295822..db63c57357b4 100644
> --- a/include/linux/posix_acl_xattr.h
> +++ b/include/linux/posix_acl_xattr.h
> @@ -53,14 +53,23 @@ posix_acl_xattr_count(size_t size)
> }
>
> #ifdef CONFIG_FS_POSIX_ACL
> -void posix_acl_fix_xattr_from_user(void *value, size_t size);
> -void posix_acl_fix_xattr_to_user(void *value, size_t size);
> +int posix_acl_fix_xattr_from_user(struct user_namespace *target_ns,
> + void *value, size_t size);
> +int posix_acl_fix_xattr_to_user(struct user_namespace *source_ns, void *value,
> + size_t size);
> #else
> -static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)
> +static inline int
> +posix_acl_fix_xattr_from_user(struct user_namespace *target_ns, void *value,
> + size_t size)
> {
> + return 0;
> }
> -static inline void posix_acl_fix_xattr_to_user(void *value, size_t size)
> +
> +static inline int
> +posix_acl_fix_xattr_to_user(struct user_namespace *source_ns, void *value,
> + size_t size)
> {
> + return 0;
> }
> #endif
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: RAID 5,6 sequential writing seems slower in newer kernels
From: Shaohua Li @ 2015-12-04 18:51 UTC (permalink / raw)
To: Robert Kierski; +Cc: Phil Turmel, Dallas Clement, linux-raid@vger.kernel.org
In-Reply-To: <F7761B9B1D11B64BBB666019E9378117FDE157@CFWEX01.americas.cray.com>
On Fri, Dec 04, 2015 at 01:40:02PM +0000, Robert Kierski wrote:
> It turns out the problem I'm experiencing is related to thread count. When I run XDD with a reasonable queuedepth parameter (32), I get horrible performance. When I run it with a small queuedepth (1-4), I get expected performance.
>
> Here are the command lines:
>
> Horrible Performance:
> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 32 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>
> GOOD Performance:
> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 1 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>
> BEST Performance:
> xdd -id commandline -dio -maxall -targets 1 /dev/md0 -queuedepth 3 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
>
> BAD Performance
> xdd -id commandline -dio -maxall -targets 1 /dev/md1 -queuedepth 5 -blocksize 1048576 -timelimit 10 -reqsize 1 -mbytes 5000 -passes 20 -verbose -op write -seek sequential
the performance issue only happens for directIO write, right? did you check
buffered write? The directIO case doesn't delay write, so will create more
read-modify-write. you can check with below debug code.
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 45933c1..d480cc3 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -5278,10 +5278,10 @@ static void make_request(struct mddev *mddev, struct bio * bi)
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
- if ((!sh->batch_head || sh == sh->batch_head) &&
- (bi->bi_rw & REQ_SYNC) &&
- !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
- atomic_inc(&conf->preread_active_stripes);
+// if ((!sh->batch_head || sh == sh->batch_head) &&
+// (bi->bi_rw & REQ_SYNC) &&
+// !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+// atomic_inc(&conf->preread_active_stripes);
release_stripe_plug(mddev, sh);
} else {
/* cannot get stripe for read-ahead, just give-up */
^ permalink raw reply related
* Re: [PATCH 11/19] fs: Ensure the mounter of a filesystem is privileged towards its inodes
From: Serge E. Hallyn @ 2015-12-04 19:00 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <1449070821-73820-12-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> The mounter of a filesystem should be privileged towards the
> inodes of that filesystem. Extend the checks in
> inode_owner_or_capable() and capable_wrt_inode_uidgid() to
> permit access by users priviliged in the user namespace of the
> inode's superblock.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> fs/inode.c | 3 +++
> kernel/capability.c | 13 +++++++++----
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 1be5f9003eb3..01c036fe1950 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1962,6 +1962,9 @@ bool inode_owner_or_capable(const struct inode *inode)
> ns = current_user_ns();
> if (ns_capable(ns, CAP_FOWNER) && kuid_has_mapping(ns, inode->i_uid))
> return true;
> +
> + if (ns_capable(inode->i_sb->s_user_ns, CAP_FOWNER))
> + return true;
> return false;
> }
> EXPORT_SYMBOL(inode_owner_or_capable);
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 45432b54d5c6..5137a38a5670 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -437,13 +437,18 @@ EXPORT_SYMBOL(file_ns_capable);
> *
> * Return true if the current task has the given capability targeted at
> * its own user namespace and that the given inode's uid and gid are
> - * mapped into the current user namespace.
> + * mapped into the current user namespace, or if the current task has
> + * the capability towards the user namespace of the inode's superblock.
> */
> bool capable_wrt_inode_uidgid(const struct inode *inode, int cap)
> {
> - struct user_namespace *ns = current_user_ns();
> + struct user_namespace *ns;
>
> - return ns_capable(ns, cap) && kuid_has_mapping(ns, inode->i_uid) &&
> - kgid_has_mapping(ns, inode->i_gid);
> + ns = current_user_ns();
> + if (ns_capable(ns, cap) && kuid_has_mapping(ns, inode->i_uid) &&
> + kgid_has_mapping(ns, inode->i_gid))
> + return true;
> +
> + return ns_capable(inode->i_sb->s_user_ns, cap);
> }
> EXPORT_SYMBOL(capable_wrt_inode_uidgid);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 12/19] fs: Don't remove suid for CAP_FSETID in s_user_ns
From: Serge E. Hallyn @ 2015-12-04 19:02 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <1449070821-73820-13-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> Expand the check in should_remove_suid() to keep privileges for
> CAP_FSETID in s_user_ns rather than init_user_ns.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> fs/inode.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 01c036fe1950..3e7c74da9304 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1684,7 +1684,8 @@ int should_remove_suid(struct dentry *dentry)
> if (unlikely((mode & S_ISGID) && (mode & S_IXGRP)))
> kill |= ATTR_KILL_SGID;
>
> - if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode)))
> + if (unlikely(kill && !ns_capable(dentry->d_sb->s_user_ns, CAP_FSETID) &&
> + S_ISREG(mode)))
> return kill;
>
> return 0;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 13/19] fs: Allow superblock owner to access do_remount_sb()
From: Serge E. Hallyn @ 2015-12-04 19:02 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <1449070821-73820-14-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> Superblock level remounts are currently restricted to global
> CAP_SYS_ADMIN, as is the path for changing the root mount to
> read only on umount. Loosen both of these permission checks to
> also allow CAP_SYS_ADMIN in any namespace which is privileged
> towards the userns which originally mounted the filesystem.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> ---
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> fs/namespace.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 18fc58760aec..b00a765895e7 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1510,7 +1510,7 @@ static int do_umount(struct mount *mnt, int flags)
> * Special case for "unmounting" root ...
> * we just try to remount it readonly.
> */
> - if (!capable(CAP_SYS_ADMIN))
> + if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
> return -EPERM;
> down_write(&sb->s_umount);
> if (!(sb->s_flags & MS_RDONLY))
> @@ -2199,7 +2199,7 @@ static int do_remount(struct path *path, int flags, int mnt_flags,
> down_write(&sb->s_umount);
> if (flags & MS_BIND)
> err = change_mount_flags(path->mnt, flags);
> - else if (!capable(CAP_SYS_ADMIN))
> + else if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
> err = -EPERM;
> else
> err = do_remount_sb(sb, flags, data, 0);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Serge E. Hallyn @ 2015-12-04 19:11 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <1449070821-73820-15-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> ---
> fs/ioctl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 5d01d2638ca5..45c371bed7ee 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -55,7 +55,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> /* do we support this mess? */
> if (!mapping->a_ops->bmap)
> return -EINVAL;
> - if (!capable(CAP_SYS_RAWIO))
> + if (!ns_capable(filp->f_inode->i_sb->s_user_ns, CAP_SYS_RAWIO))
> return -EPERM;
> res = get_user(block, p);
> if (res)
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 15/19] capabilities: Allow privileged user in s_user_ns to set file caps
From: Serge E. Hallyn @ 2015-12-04 19:42 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Serge Hallyn, James Morris, Serge E. Hallyn,
Alexander Viro, Richard Weinberger, Austin S Hemmelgarn,
Miklos Szeredi, linux-bcache, dm-devel, linux-raid, linux-kernel,
linux-mtd, linux-fsdevel, fuse-devel, linux-security-module,
selinux
In-Reply-To: <1449070821-73820-16-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> A privileged user in a super block's s_user_ns is privileged
> towards that file system and thus should be allowed to set file
> capabilities. The file capabilities will not be trusted outside
> of s_user_ns, so an unprivileged user cannot use this to gain
> privileges in a user namespace where they are not already
> privileged.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> security/commoncap.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 2119421613f6..d6c80c19c449 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -653,15 +653,17 @@ int cap_bprm_secureexec(struct linux_binprm *bprm)
> int cap_inode_setxattr(struct dentry *dentry, const char *name,
> const void *value, size_t size, int flags)
> {
> + struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> +
> if (!strcmp(name, XATTR_NAME_CAPS)) {
> - if (!capable(CAP_SETFCAP))
> + if (!ns_capable(user_ns, CAP_SETFCAP))
> return -EPERM;
This, for file capabilities, is fine,
> return 0;
> }
>
> if (!strncmp(name, XATTR_SECURITY_PREFIX,
> sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> - !capable(CAP_SYS_ADMIN))
> + !ns_capable(user_ns, CAP_SYS_ADMIN))
but this is for all other security.*.
It's probably still ok, but let's think about it a sec. MAC like
selinux or smack should be orthogonal to DAC. Capabilities are the
same in essence, but the reason they can be treated differently here
is because capabilties are in fact targated at a user namespace.
Apparmor namespaces, for instance, are completely orthogonal to user
namespaces, as are contexts in selinux.
Now, if smack or selinux xattrs are being set then those modules
should be gating these writes. Booting a kernel without those
modules should be a challenge for an untrusted user. But such a
situation could be exploited opportunistically if it were to happen.
The problem with simply not changing this here is that if selinux
or smack authorizes the xattr write, then commoncap shouldn't be
denying it.
I get the feeling we need cooperation among the modules (i.e. "if
the write is to 'security.$lsm' and $lsm is not loaded, then require
capable(CAP_SYS_ADMIN), else just allow) But that's not how things are
structured right now.
Maybe security.ko could grow central logic to 'assign' security.*
capabilities to specific lsms and gate writes to those if $lsm is not
loaded.
Does anything break if the second hunk in each fn in this patch is
not applied?
> return -EPERM;
> return 0;
> }
> @@ -679,15 +681,17 @@ int cap_inode_setxattr(struct dentry *dentry, const char *name,
> */
> int cap_inode_removexattr(struct dentry *dentry, const char *name)
> {
> + struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> +
> if (!strcmp(name, XATTR_NAME_CAPS)) {
> - if (!capable(CAP_SETFCAP))
> + if (!ns_capable(user_ns, CAP_SETFCAP))
> return -EPERM;
> return 0;
> }
>
> if (!strncmp(name, XATTR_SECURITY_PREFIX,
> sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> - !capable(CAP_SYS_ADMIN))
> + !ns_capable(user_ns, CAP_SYS_ADMIN))
> return -EPERM;
> return 0;
> }
> --
> 1.9.1
^ permalink raw reply
* Re: [PATCH 09/19] fs: Refuse uid/gid changes which don't map into s_user_ns
From: Serge E. Hallyn @ 2015-12-04 19:42 UTC (permalink / raw)
To: Seth Forshee
Cc: Serge E. Hallyn, Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204174605.GC147214@ubuntu-hedt>
Quoting Seth Forshee (seth.forshee@canonical.com):
> On Fri, Dec 04, 2015 at 11:27:38AM -0600, Serge E. Hallyn wrote:
> > On Wed, Dec 02, 2015 at 09:40:09AM -0600, Seth Forshee wrote:
> > > Add checks to inode_change_ok to verify that uid and gid changes
> > > will map into the superblock's user namespace. If they do not
> > > fail with -EOVERFLOW. This cannot be overriden with ATTR_FORCE.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >
> > Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> >
> > ... although i could see root on the host being upset that it can't
> > assign a uid not valid in the mounter's ns. But it does seem safer.
>
> That change wouldn't be representable in the backing store though, and
> that could lead to unexpected behaviour. It's better to tell root that
> we can't make the requested change, in my opinion.
Makes sense. Thanks.
^ permalink raw reply
* Re: [PATCH 17/19] fuse: Support fuse filesystems outside of init_user_ns
From: Serge E. Hallyn @ 2015-12-04 20:03 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Miklos Szeredi, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-bcache, dm-devel,
linux-raid, linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <1449070821-73820-18-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> Update fuse to translate uids and gids to/from the user namspace
> of the process servicing requests on /dev/fuse. Any ids which do
> not map into the namespace will result in errors. inodes will
> also be marked bad when unmappable ids are received from the
> userspace fuse process.
>
> Currently no use cases are known for letting the userspace fuse
> daemon switch namespaces after opening /dev/fuse. Given this
> fact, and in order to keep the implementation as simple as
> possible and ease security auditing, the user namespace from
> which /dev/fuse is opened is used for all id translations. This
> is required to be the same namespace as s_user_ns to maintain
> behavior consistent with other filesystems which can be mounted
> in user namespaces.
>
> For cuse the namespace used for the connection is also simply
> current_user_ns() at the time /dev/cuse is opened.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> fs/fuse/cuse.c | 3 +-
> fs/fuse/dev.c | 13 +++++----
> fs/fuse/dir.c | 81 ++++++++++++++++++++++++++++++++++-------------------
> fs/fuse/fuse_i.h | 14 ++++++----
> fs/fuse/inode.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------------
> 5 files changed, 136 insertions(+), 60 deletions(-)
>
> diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> index eae2c11268bc..a10aca57bfe4 100644
> --- a/fs/fuse/cuse.c
> +++ b/fs/fuse/cuse.c
> @@ -48,6 +48,7 @@
> #include <linux/stat.h>
> #include <linux/module.h>
> #include <linux/uio.h>
> +#include <linux/user_namespace.h>
>
> #include "fuse_i.h"
>
> @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
> if (!cc)
> return -ENOMEM;
>
> - fuse_conn_init(&cc->fc);
> + fuse_conn_init(&cc->fc, current_user_ns());
>
> fud = fuse_dev_alloc(&cc->fc);
> if (!fud) {
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index a4f6f30d6d86..11b4cb0a0e2f 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -127,8 +127,8 @@ static void __fuse_put_request(struct fuse_req *req)
>
> static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> {
> - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
> - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
> + req->in.h.uid = from_kuid(fc->user_ns, current_fsuid());
> + req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
> req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> }
>
> @@ -186,7 +186,8 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> __set_bit(FR_WAITING, &req->flags);
> if (for_background)
> __set_bit(FR_BACKGROUND, &req->flags);
> - if (req->in.h.pid == 0) {
> + if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 ||
> + req->in.h.gid == (gid_t)-1) {
> fuse_put_request(fc, req);
> return ERR_PTR(-EOVERFLOW);
> }
> @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
> struct fuse_in *in;
> unsigned reqsize;
>
> - if (task_active_pid_ns(current) != fc->pid_ns)
> + if (task_active_pid_ns(current) != fc->pid_ns ||
> + current_user_ns() != fc->user_ns)
Do you think this should be using current_in_user_ns(fc->user_ns) ?
Opening a file, forking (maybe unsharing) then acting on the file is
pretty standard fare which this would break.
(same for pidns, i guess, as well as for write below)
> return -EIO;
>
> restart:
> @@ -1880,7 +1882,8 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
> struct fuse_req *req;
> struct fuse_out_header oh;
>
> - if (task_active_pid_ns(current) != fc->pid_ns)
> + if (task_active_pid_ns(current) != fc->pid_ns ||
> + current_user_ns() != fc->user_ns)
> return -EIO;
>
> if (nbytes < sizeof(struct fuse_out_header))
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Theodore Ts'o @ 2015-12-04 20:05 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Seth Forshee, Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204191143.GE3624@mail.hallyn.com>
The fact that we need CAP_SYS_RAIO for FIBMAP is pretty silly, given
that FIEMAP does not require privileges --- and in fact the preferred
interface. Why not just simply drop the requirement for privileges
for FIBMAP?
(Seth, Serge, this isn't a real objection to your patch; but the fact
that FIBMAP requires root has always been a bit silly, and this would
be a great opportunity to simplify things a bit.)
- Ted
On Fri, Dec 04, 2015 at 01:11:43PM -0600, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@canonical.com):
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>
> > ---
> > fs/ioctl.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > index 5d01d2638ca5..45c371bed7ee 100644
> > --- a/fs/ioctl.c
> > +++ b/fs/ioctl.c
> > @@ -55,7 +55,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > /* do we support this mess? */
> > if (!mapping->a_ops->bmap)
> > return -EINVAL;
> > - if (!capable(CAP_SYS_RAWIO))
> > + if (!ns_capable(filp->f_inode->i_sb->s_user_ns, CAP_SYS_RAWIO))
> > return -EPERM;
> > res = get_user(block, p);
> > if (res)
> > --
> > 1.9.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 18/19] fuse: Restrict allow_other to the superblock's namespace or a descendant
From: Serge E. Hallyn @ 2015-12-04 20:05 UTC (permalink / raw)
To: Seth Forshee
Cc: Eric W. Biederman, Miklos Szeredi, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-bcache, dm-devel,
linux-raid, linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <1449070821-73820-19-git-send-email-seth.forshee@canonical.com>
Quoting Seth Forshee (seth.forshee@canonical.com):
> Unprivileged users are normally restricted from mounting with the
> allow_other option by system policy, but this could be bypassed
> for a mount done with user namespace root permissions. In such
> cases allow_other should not allow users outside the userns
> to access the mount as doing so would give the unprivileged user
> the ability to manipulate processes it would otherwise be unable
> to manipulate. Restrict allow_other to apply to users in the same
> userns used at mount or a descendant of that namespace.
>
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
> fs/fuse/dir.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index f67f4dd86b36..5b8edb1203b8 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1018,8 +1018,14 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> {
> const struct cred *cred;
>
> - if (fc->flags & FUSE_ALLOW_OTHER)
> - return 1;
> + if (fc->flags & FUSE_ALLOW_OTHER) {
> + struct user_namespace *ns;
> + for (ns = current_user_ns(); ns; ns = ns->parent) {
> + if (ns == fc->user_ns)
> + return 1;
> + }
use current_in_userns() ?
> + return 0;
> + }
>
> cred = current_cred();
> if (uid_eq(cred->euid, fc->user_id) &&
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Serge E. Hallyn @ 2015-12-04 20:07 UTC (permalink / raw)
To: Theodore Ts'o, Serge E. Hallyn, Seth Forshee,
Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204200528.GC18359@thunk.org>
Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
a little while ago :) The same question was asked 16 years ago. Apparently
the answer then was that it was easier than fixing the code.
Quoting Theodore Ts'o (tytso@mit.edu):
> The fact that we need CAP_SYS_RAIO for FIBMAP is pretty silly, given
> that FIEMAP does not require privileges --- and in fact the preferred
> interface. Why not just simply drop the requirement for privileges
> for FIBMAP?
>
> (Seth, Serge, this isn't a real objection to your patch; but the fact
> that FIBMAP requires root has always been a bit silly, and this would
> be a great opportunity to simplify things a bit.)
>
> - Ted
>
>
> On Fri, Dec 04, 2015 at 01:11:43PM -0600, Serge E. Hallyn wrote:
> > Quoting Seth Forshee (seth.forshee@canonical.com):
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >
> > Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> >
> > > ---
> > > fs/ioctl.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > index 5d01d2638ca5..45c371bed7ee 100644
> > > --- a/fs/ioctl.c
> > > +++ b/fs/ioctl.c
> > > @@ -55,7 +55,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > > /* do we support this mess? */
> > > if (!mapping->a_ops->bmap)
> > > return -EINVAL;
> > > - if (!capable(CAP_SYS_RAWIO))
> > > + if (!ns_capable(filp->f_inode->i_sb->s_user_ns, CAP_SYS_RAWIO))
> > > return -EPERM;
> > > res = get_user(block, p);
> > > if (res)
> > > --
> > > 1.9.1
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at http://www.tux.org/lkml/
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 15/19] capabilities: Allow privileged user in s_user_ns to set file caps
From: Seth Forshee @ 2015-12-04 20:36 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Eric W. Biederman, Serge Hallyn, James Morris, Alexander Viro,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204194206.GF3624@mail.hallyn.com>
On Fri, Dec 04, 2015 at 01:42:06PM -0600, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@canonical.com):
> > A privileged user in a super block's s_user_ns is privileged
> > towards that file system and thus should be allowed to set file
> > capabilities. The file capabilities will not be trusted outside
> > of s_user_ns, so an unprivileged user cannot use this to gain
> > privileges in a user namespace where they are not already
> > privileged.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> > security/commoncap.c | 12 ++++++++----
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 2119421613f6..d6c80c19c449 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -653,15 +653,17 @@ int cap_bprm_secureexec(struct linux_binprm *bprm)
> > int cap_inode_setxattr(struct dentry *dentry, const char *name,
> > const void *value, size_t size, int flags)
> > {
> > + struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> > +
> > if (!strcmp(name, XATTR_NAME_CAPS)) {
> > - if (!capable(CAP_SETFCAP))
> > + if (!ns_capable(user_ns, CAP_SETFCAP))
> > return -EPERM;
>
> This, for file capabilities, is fine,
>
> > return 0;
> > }
> >
> > if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> > - !capable(CAP_SYS_ADMIN))
> > + !ns_capable(user_ns, CAP_SYS_ADMIN))
>
> but this is for all other security.*.
>
> It's probably still ok, but let's think about it a sec. MAC like
> selinux or smack should be orthogonal to DAC. Capabilities are the
> same in essence, but the reason they can be treated differently here
> is because capabilties are in fact targated at a user namespace.
> Apparmor namespaces, for instance, are completely orthogonal to user
> namespaces, as are contexts in selinux.
>
> Now, if smack or selinux xattrs are being set then those modules
> should be gating these writes. Booting a kernel without those
> modules should be a challenge for an untrusted user. But such a
> situation could be exploited opportunistically if it were to happen.
>
> The problem with simply not changing this here is that if selinux
> or smack authorizes the xattr write, then commoncap shouldn't be
> denying it.
This is partly the logic behind the change, the other part being that
the user could already insert the xattrs directly into the backing store
so the LSMs must be prepared not to trust them in any case. But the
commit message doesn't explain that, my mistake. And it's a question
worth revisiting.
> I get the feeling we need cooperation among the modules (i.e. "if
> the write is to 'security.$lsm' and $lsm is not loaded, then require
> capable(CAP_SYS_ADMIN), else just allow) But that's not how things are
> structured right now.
>
> Maybe security.ko could grow central logic to 'assign' security.*
> capabilities to specific lsms and gate writes to those if $lsm is not
> loaded.
I don't see any meaningful difference between this case and the case of
inserting them into the backing store before mounting. We can't do
anything to prevent the latter, so LSMs just have to be aware of
unprivileged mounts and handle them with care. Previous patches do this
for SELinux and Smack by adopting a policy that doesn't respect security
labels on disk for these mounts. So I don't think that refusing to set
security.* xattrs for an LSM that isn't loaded really accomplishes
anything.
Then there's the case of setting xattrs for an LSM that is currently
loaded. I think that SELinux and Smack are both going to refuse these
writes, Smack rather directly by seeing that the user lacks global
CAP_MAC_ADMIN and SELinux by virtue of the fact that the previous patch
in this series applies mountpoint labeling to these mounts. As far as I
can tell the other LSMs don't take security policy from xattrs.
So, as far as I can tell, removing the check doesn't create any
vulnerabilities.
But that's not to say it's the right thing to do. After reconsidering
it, I'm inclined to be more conservative and to keep requiring
capable(CAP_SYS_ADMIN) until such time as there's a use case for
allowing a user privileged only in s_user_ns to write these xattrs.
> Does anything break if the second hunk in each fn in this patch is
> not applied?
Not that I'm aware of, no.
Seth
^ permalink raw reply
* Re: [PATCH 17/19] fuse: Support fuse filesystems outside of init_user_ns
From: Seth Forshee @ 2015-12-04 20:41 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Eric W. Biederman, Miklos Szeredi, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-bcache, dm-devel,
linux-raid, linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <20151204200355.GH3624@mail.hallyn.com>
On Fri, Dec 04, 2015 at 02:03:55PM -0600, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@canonical.com):
> > Update fuse to translate uids and gids to/from the user namspace
> > of the process servicing requests on /dev/fuse. Any ids which do
> > not map into the namespace will result in errors. inodes will
> > also be marked bad when unmappable ids are received from the
> > userspace fuse process.
> >
> > Currently no use cases are known for letting the userspace fuse
> > daemon switch namespaces after opening /dev/fuse. Given this
> > fact, and in order to keep the implementation as simple as
> > possible and ease security auditing, the user namespace from
> > which /dev/fuse is opened is used for all id translations. This
> > is required to be the same namespace as s_user_ns to maintain
> > behavior consistent with other filesystems which can be mounted
> > in user namespaces.
> >
> > For cuse the namespace used for the connection is also simply
> > current_user_ns() at the time /dev/cuse is opened.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> > fs/fuse/cuse.c | 3 +-
> > fs/fuse/dev.c | 13 +++++----
> > fs/fuse/dir.c | 81 ++++++++++++++++++++++++++++++++++-------------------
> > fs/fuse/fuse_i.h | 14 ++++++----
> > fs/fuse/inode.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------------
> > 5 files changed, 136 insertions(+), 60 deletions(-)
> >
> > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> > index eae2c11268bc..a10aca57bfe4 100644
> > --- a/fs/fuse/cuse.c
> > +++ b/fs/fuse/cuse.c
> > @@ -48,6 +48,7 @@
> > #include <linux/stat.h>
> > #include <linux/module.h>
> > #include <linux/uio.h>
> > +#include <linux/user_namespace.h>
> >
> > #include "fuse_i.h"
> >
> > @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
> > if (!cc)
> > return -ENOMEM;
> >
> > - fuse_conn_init(&cc->fc);
> > + fuse_conn_init(&cc->fc, current_user_ns());
> >
> > fud = fuse_dev_alloc(&cc->fc);
> > if (!fud) {
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index a4f6f30d6d86..11b4cb0a0e2f 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -127,8 +127,8 @@ static void __fuse_put_request(struct fuse_req *req)
> >
> > static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> > {
> > - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
> > - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
> > + req->in.h.uid = from_kuid(fc->user_ns, current_fsuid());
> > + req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
> > req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> > }
> >
> > @@ -186,7 +186,8 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> > __set_bit(FR_WAITING, &req->flags);
> > if (for_background)
> > __set_bit(FR_BACKGROUND, &req->flags);
> > - if (req->in.h.pid == 0) {
> > + if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 ||
> > + req->in.h.gid == (gid_t)-1) {
> > fuse_put_request(fc, req);
> > return ERR_PTR(-EOVERFLOW);
> > }
> > @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
> > struct fuse_in *in;
> > unsigned reqsize;
> >
> > - if (task_active_pid_ns(current) != fc->pid_ns)
> > + if (task_active_pid_ns(current) != fc->pid_ns ||
> > + current_user_ns() != fc->user_ns)
>
> Do you think this should be using current_in_user_ns(fc->user_ns) ?
>
> Opening a file, forking (maybe unsharing) then acting on the file is
> pretty standard fare which this would break.
>
> (same for pidns, i guess, as well as for write below)
I'd rather leave it as is. It might be okay, but even if current_user_ns
is a child of fc->user_ns it could end up seeing ids that aren't valid
for it's namespaces, and that might cause issues for filesystems which
aren't expecting it.
But if you have a use case for a process in a child namespace servicing
requests for a mount, I'd be willing to reconsider.
^ permalink raw reply
* Re: [PATCH 18/19] fuse: Restrict allow_other to the superblock's namespace or a descendant
From: Seth Forshee @ 2015-12-04 20:43 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Eric W. Biederman, Miklos Szeredi, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, linux-bcache, dm-devel,
linux-raid, linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <20151204200541.GI3624@mail.hallyn.com>
On Fri, Dec 04, 2015 at 02:05:41PM -0600, Serge E. Hallyn wrote:
> Quoting Seth Forshee (seth.forshee@canonical.com):
> > Unprivileged users are normally restricted from mounting with the
> > allow_other option by system policy, but this could be bypassed
> > for a mount done with user namespace root permissions. In such
> > cases allow_other should not allow users outside the userns
> > to access the mount as doing so would give the unprivileged user
> > the ability to manipulate processes it would otherwise be unable
> > to manipulate. Restrict allow_other to apply to users in the same
> > userns used at mount or a descendant of that namespace.
> >
> > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > ---
> > fs/fuse/dir.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index f67f4dd86b36..5b8edb1203b8 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -1018,8 +1018,14 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> > {
> > const struct cred *cred;
> >
> > - if (fc->flags & FUSE_ALLOW_OTHER)
> > - return 1;
> > + if (fc->flags & FUSE_ALLOW_OTHER) {
> > + struct user_namespace *ns;
> > + for (ns = current_user_ns(); ns; ns = ns->parent) {
> > + if (ns == fc->user_ns)
> > + return 1;
> > + }
>
> use current_in_userns() ?
Yes, it should. I wrote this before I wrote the patch which adds that
function and never thought to go back to change it here.
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Seth Forshee @ 2015-12-04 20:45 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Serge Hallyn, Theodore Ts'o, dm-devel-H+wXaHxf7aLQT0dZR+AlfA,
Miklos Szeredi, linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-bcache-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-raid-u79uwXL29TY76Z2rM5mHXA,
fuse-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f, Austin S Hemmelgarn,
linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Eric W. Biederman,
selinux-+05T5uksL2qpZYMLLGbcSA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Alexander Viro
In-Reply-To: <20151204200736.GJ3624-7LNsyQBKDXoIagZqoN9o3w@public.gmane.org>
On Fri, Dec 04, 2015 at 02:07:36PM -0600, Serge E. Hallyn wrote:
> Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
> a little while ago :) The same question was asked 16 years ago. Apparently
> the answer then was that it was easier than fixing the code.
So it seems then that either it still isn't safe and so unprivileged
users shouldn't be allowed to do it at all, or else it's safe and we
should drop the requirement completely. I can't say which is right,
unfortunately.
>
> Quoting Theodore Ts'o (tytso-3s7WtUTddSA@public.gmane.org):
> > The fact that we need CAP_SYS_RAIO for FIBMAP is pretty silly, given
> > that FIEMAP does not require privileges --- and in fact the preferred
> > interface. Why not just simply drop the requirement for privileges
> > for FIBMAP?
> >
> > (Seth, Serge, this isn't a real objection to your patch; but the fact
> > that FIBMAP requires root has always been a bit silly, and this would
> > be a great opportunity to simplify things a bit.)
> >
> > - Ted
> >
> >
> > On Fri, Dec 04, 2015 at 01:11:43PM -0600, Serge E. Hallyn wrote:
> > > Quoting Seth Forshee (seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org):
> > > > Signed-off-by: Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > >
> > > Acked-by: Serge Hallyn <serge.hallyn-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > >
> > > > ---
> > > > fs/ioctl.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/fs/ioctl.c b/fs/ioctl.c
> > > > index 5d01d2638ca5..45c371bed7ee 100644
> > > > --- a/fs/ioctl.c
> > > > +++ b/fs/ioctl.c
> > > > @@ -55,7 +55,7 @@ static int ioctl_fibmap(struct file *filp, int __user *p)
> > > > /* do we support this mess? */
> > > > if (!mapping->a_ops->bmap)
> > > > return -EINVAL;
> > > > - if (!capable(CAP_SYS_RAWIO))
> > > > + if (!ns_capable(filp->f_inode->i_sb->s_user_ns, CAP_SYS_RAWIO))
> > > > return -EPERM;
> > > > res = get_user(block, p);
> > > > if (res)
> > > > --
> > > > 1.9.1
> > > >
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > > Please read the FAQ at http://www.tux.org/lkml/
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
^ permalink raw reply
* Re: [PATCH 17/19] fuse: Support fuse filesystems outside of init_user_ns
From: Serge E. Hallyn @ 2015-12-04 21:57 UTC (permalink / raw)
To: Seth Forshee
Cc: Serge E. Hallyn, Eric W. Biederman, Miklos Szeredi,
Alexander Viro, Serge Hallyn, Richard Weinberger,
Austin S Hemmelgarn, linux-bcache, dm-devel, linux-raid,
linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <20151204204122.GE147214@ubuntu-hedt>
On Fri, Dec 04, 2015 at 02:41:22PM -0600, Seth Forshee wrote:
> On Fri, Dec 04, 2015 at 02:03:55PM -0600, Serge E. Hallyn wrote:
> > Quoting Seth Forshee (seth.forshee@canonical.com):
> > > Update fuse to translate uids and gids to/from the user namspace
> > > of the process servicing requests on /dev/fuse. Any ids which do
> > > not map into the namespace will result in errors. inodes will
> > > also be marked bad when unmappable ids are received from the
> > > userspace fuse process.
> > >
> > > Currently no use cases are known for letting the userspace fuse
> > > daemon switch namespaces after opening /dev/fuse. Given this
> > > fact, and in order to keep the implementation as simple as
> > > possible and ease security auditing, the user namespace from
> > > which /dev/fuse is opened is used for all id translations. This
> > > is required to be the same namespace as s_user_ns to maintain
> > > behavior consistent with other filesystems which can be mounted
> > > in user namespaces.
> > >
> > > For cuse the namespace used for the connection is also simply
> > > current_user_ns() at the time /dev/cuse is opened.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > > fs/fuse/cuse.c | 3 +-
> > > fs/fuse/dev.c | 13 +++++----
> > > fs/fuse/dir.c | 81 ++++++++++++++++++++++++++++++++++-------------------
> > > fs/fuse/fuse_i.h | 14 ++++++----
> > > fs/fuse/inode.c | 85 ++++++++++++++++++++++++++++++++++++++++++--------------
> > > 5 files changed, 136 insertions(+), 60 deletions(-)
> > >
> > > diff --git a/fs/fuse/cuse.c b/fs/fuse/cuse.c
> > > index eae2c11268bc..a10aca57bfe4 100644
> > > --- a/fs/fuse/cuse.c
> > > +++ b/fs/fuse/cuse.c
> > > @@ -48,6 +48,7 @@
> > > #include <linux/stat.h>
> > > #include <linux/module.h>
> > > #include <linux/uio.h>
> > > +#include <linux/user_namespace.h>
> > >
> > > #include "fuse_i.h"
> > >
> > > @@ -498,7 +499,7 @@ static int cuse_channel_open(struct inode *inode, struct file *file)
> > > if (!cc)
> > > return -ENOMEM;
> > >
> > > - fuse_conn_init(&cc->fc);
> > > + fuse_conn_init(&cc->fc, current_user_ns());
> > >
> > > fud = fuse_dev_alloc(&cc->fc);
> > > if (!fud) {
> > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > index a4f6f30d6d86..11b4cb0a0e2f 100644
> > > --- a/fs/fuse/dev.c
> > > +++ b/fs/fuse/dev.c
> > > @@ -127,8 +127,8 @@ static void __fuse_put_request(struct fuse_req *req)
> > >
> > > static void fuse_req_init_context(struct fuse_conn *fc, struct fuse_req *req)
> > > {
> > > - req->in.h.uid = from_kuid_munged(&init_user_ns, current_fsuid());
> > > - req->in.h.gid = from_kgid_munged(&init_user_ns, current_fsgid());
> > > + req->in.h.uid = from_kuid(fc->user_ns, current_fsuid());
> > > + req->in.h.gid = from_kgid(fc->user_ns, current_fsgid());
> > > req->in.h.pid = pid_nr_ns(task_pid(current), fc->pid_ns);
> > > }
> > >
> > > @@ -186,7 +186,8 @@ static struct fuse_req *__fuse_get_req(struct fuse_conn *fc, unsigned npages,
> > > __set_bit(FR_WAITING, &req->flags);
> > > if (for_background)
> > > __set_bit(FR_BACKGROUND, &req->flags);
> > > - if (req->in.h.pid == 0) {
> > > + if (req->in.h.pid == 0 || req->in.h.uid == (uid_t)-1 ||
> > > + req->in.h.gid == (gid_t)-1) {
> > > fuse_put_request(fc, req);
> > > return ERR_PTR(-EOVERFLOW);
> > > }
> > > @@ -1248,7 +1249,8 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
> > > struct fuse_in *in;
> > > unsigned reqsize;
> > >
> > > - if (task_active_pid_ns(current) != fc->pid_ns)
> > > + if (task_active_pid_ns(current) != fc->pid_ns ||
> > > + current_user_ns() != fc->user_ns)
> >
> > Do you think this should be using current_in_user_ns(fc->user_ns) ?
> >
> > Opening a file, forking (maybe unsharing) then acting on the file is
> > pretty standard fare which this would break.
> >
> > (same for pidns, i guess, as well as for write below)
>
> I'd rather leave it as is. It might be okay, but even if current_user_ns
> is a child of fc->user_ns it could end up seeing ids that aren't valid
> for it's namespaces, and that might cause issues for filesystems which
> aren't expecting it.
>
> But if you have a use case for a process in a child namespace servicing
> requests for a mount, I'd be willing to reconsider.
I'm pretty sure we'll get such uses, i.e. opening a file, unsharing userns,
and not mapping in any userids, to sandbox the program. But we can address
it when it happens, I think.
(sorry I need to re-read before acking the whole patch)
^ permalink raw reply
* Re: [PATCH 18/19] fuse: Restrict allow_other to the superblock's namespace or a descendant
From: Serge E. Hallyn @ 2015-12-04 21:57 UTC (permalink / raw)
To: Seth Forshee
Cc: Serge E. Hallyn, Eric W. Biederman, Miklos Szeredi,
Alexander Viro, Serge Hallyn, Richard Weinberger,
Austin S Hemmelgarn, linux-bcache, dm-devel, linux-raid,
linux-kernel, linux-mtd, linux-fsdevel, fuse-devel,
linux-security-module, selinux
In-Reply-To: <20151204204319.GF147214@ubuntu-hedt>
On Fri, Dec 04, 2015 at 02:43:19PM -0600, Seth Forshee wrote:
> On Fri, Dec 04, 2015 at 02:05:41PM -0600, Serge E. Hallyn wrote:
> > Quoting Seth Forshee (seth.forshee@canonical.com):
> > > Unprivileged users are normally restricted from mounting with the
> > > allow_other option by system policy, but this could be bypassed
> > > for a mount done with user namespace root permissions. In such
> > > cases allow_other should not allow users outside the userns
> > > to access the mount as doing so would give the unprivileged user
> > > the ability to manipulate processes it would otherwise be unable
> > > to manipulate. Restrict allow_other to apply to users in the same
> > > userns used at mount or a descendant of that namespace.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > > fs/fuse/dir.c | 10 ++++++++--
> > > 1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index f67f4dd86b36..5b8edb1203b8 100644
> > > --- a/fs/fuse/dir.c
> > > +++ b/fs/fuse/dir.c
> > > @@ -1018,8 +1018,14 @@ int fuse_allow_current_process(struct fuse_conn *fc)
> > > {
> > > const struct cred *cred;
> > >
> > > - if (fc->flags & FUSE_ALLOW_OTHER)
> > > - return 1;
> > > + if (fc->flags & FUSE_ALLOW_OTHER) {
> > > + struct user_namespace *ns;
> > > + for (ns = current_user_ns(); ns; ns = ns->parent) {
> > > + if (ns == fc->user_ns)
> > > + return 1;
> > > + }
> >
> > use current_in_userns() ?
>
> Yes, it should. I wrote this before I wrote the patch which adds that
> function and never thought to go back to change it here.
Ok -
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
thanks.
^ permalink raw reply
* Re: [PATCH 15/19] capabilities: Allow privileged user in s_user_ns to set file caps
From: Serge E. Hallyn @ 2015-12-04 22:05 UTC (permalink / raw)
To: Seth Forshee
Cc: Serge E. Hallyn, Eric W. Biederman, Serge Hallyn, James Morris,
Alexander Viro, Richard Weinberger, Austin S Hemmelgarn,
Miklos Szeredi, linux-bcache, dm-devel, linux-raid, linux-kernel,
linux-mtd, linux-fsdevel, fuse-devel, linux-security-module,
selinux
In-Reply-To: <20151204203627.GD147214@ubuntu-hedt>
On Fri, Dec 04, 2015 at 02:36:27PM -0600, Seth Forshee wrote:
> On Fri, Dec 04, 2015 at 01:42:06PM -0600, Serge E. Hallyn wrote:
> > Quoting Seth Forshee (seth.forshee@canonical.com):
> > > A privileged user in a super block's s_user_ns is privileged
> > > towards that file system and thus should be allowed to set file
> > > capabilities. The file capabilities will not be trusted outside
> > > of s_user_ns, so an unprivileged user cannot use this to gain
> > > privileges in a user namespace where they are not already
> > > privileged.
> > >
> > > Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> > > ---
> > > security/commoncap.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/security/commoncap.c b/security/commoncap.c
> > > index 2119421613f6..d6c80c19c449 100644
> > > --- a/security/commoncap.c
> > > +++ b/security/commoncap.c
> > > @@ -653,15 +653,17 @@ int cap_bprm_secureexec(struct linux_binprm *bprm)
> > > int cap_inode_setxattr(struct dentry *dentry, const char *name,
> > > const void *value, size_t size, int flags)
> > > {
> > > + struct user_namespace *user_ns = dentry->d_sb->s_user_ns;
> > > +
> > > if (!strcmp(name, XATTR_NAME_CAPS)) {
> > > - if (!capable(CAP_SETFCAP))
> > > + if (!ns_capable(user_ns, CAP_SETFCAP))
> > > return -EPERM;
> >
> > This, for file capabilities, is fine,
> >
> > > return 0;
> > > }
> > >
> > > if (!strncmp(name, XATTR_SECURITY_PREFIX,
> > > sizeof(XATTR_SECURITY_PREFIX) - 1) &&
> > > - !capable(CAP_SYS_ADMIN))
> > > + !ns_capable(user_ns, CAP_SYS_ADMIN))
> >
> > but this is for all other security.*.
> >
> > It's probably still ok, but let's think about it a sec. MAC like
> > selinux or smack should be orthogonal to DAC. Capabilities are the
> > same in essence, but the reason they can be treated differently here
> > is because capabilties are in fact targated at a user namespace.
> > Apparmor namespaces, for instance, are completely orthogonal to user
> > namespaces, as are contexts in selinux.
> >
> > Now, if smack or selinux xattrs are being set then those modules
> > should be gating these writes. Booting a kernel without those
> > modules should be a challenge for an untrusted user. But such a
> > situation could be exploited opportunistically if it were to happen.
> >
> > The problem with simply not changing this here is that if selinux
> > or smack authorizes the xattr write, then commoncap shouldn't be
> > denying it.
>
> This is partly the logic behind the change, the other part being that
> the user could already insert the xattrs directly into the backing store
> so the LSMs must be prepared not to trust them in any case. But the
> commit message doesn't explain that, my mistake. And it's a question
> worth revisiting.
>
> > I get the feeling we need cooperation among the modules (i.e. "if
> > the write is to 'security.$lsm' and $lsm is not loaded, then require
> > capable(CAP_SYS_ADMIN), else just allow) But that's not how things are
> > structured right now.
> >
> > Maybe security.ko could grow central logic to 'assign' security.*
> > capabilities to specific lsms and gate writes to those if $lsm is not
> > loaded.
>
> I don't see any meaningful difference between this case and the case of
> inserting them into the backing store before mounting. We can't do
> anything to prevent the latter, so LSMs just have to be aware of
> unprivileged mounts and handle them with care. Previous patches do this
> for SELinux and Smack by adopting a policy that doesn't respect security
> labels on disk for these mounts. So I don't think that refusing to set
> security.* xattrs for an LSM that isn't loaded really accomplishes
> anything.
Good point. I think that's the thing to point in the patch description.
(The original patch description doesn't mention any change apart from
file capabilities, which I think it should)
> Then there's the case of setting xattrs for an LSM that is currently
> loaded. I think that SELinux and Smack are both going to refuse these
> writes, Smack rather directly by seeing that the user lacks global
> CAP_MAC_ADMIN and SELinux by virtue of the fact that the previous patch
> in this series applies mountpoint labeling to these mounts. As far as I
> can tell the other LSMs don't take security policy from xattrs.
>
> So, as far as I can tell, removing the check doesn't create any
> vulnerabilities.
>
> But that's not to say it's the right thing to do. After reconsidering
> it, I'm inclined to be more conservative and to keep requiring
> capable(CAP_SYS_ADMIN) until such time as there's a use case for
> allowing a user privileged only in s_user_ns to write these xattrs.
>
> > Does anything break if the second hunk in each fn in this patch is
> > not applied?
>
> Not that I'm aware of, no.
That's ok, let's leave the patch as is, with updated description.
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
thanks!
-serge
^ permalink raw reply
* Re: [PATCH v2 0/3] Badblock tracking for gendisks
From: Verma, Vishal L @ 2015-12-04 22:53 UTC (permalink / raw)
To: linux-nvdimm@lists.01.org
Cc: linux-raid@vger.kernel.org, linux-scsi@vger.kernel.org,
linux-block@vger.kernel.org, neilb@suse.com, axboe@fb.com,
jmoyer@redhat.com
In-Reply-To: <1448477013-9174-1-git-send-email-vishal.l.verma@intel.com>
On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> v2:
> - In badblocks_free, make 'page' NULL (patch 1)
> - Move the core badblocks code to a new .c file (patch 1) (Jens)
> - Fix a sizeof usage in disk_alloc_badblocks (patch 2) (Dan)
> - Since disk_alloc_badblocks can fail, check disk->bb for NULL in
> the
> genhd wrappers (patch 2) (Jeff)
> - Update the md conversion to also ise the badblocks init and free
> functions (patch 3)
> - Remove the BB_* macros from md.h as they are now in badblocks.h
> (patch 3)
>
> Patch 1 copies badblock management code into a header of its own,
> making it generally available. It follows common libraries of code
> such as linked lists, where anyone may embed a core data structure
> in another place, and use the provided accessor functions to
> manipulate the data.
>
> Patch 2 adds badblock tracking to gendisks (in preparation for use
> by NVDIMM devices). Right now, it is turned on unconditionally - I'd
> appreciate comments on if that is the right path.
>
> Patch 3 converts md over to use the new badblocks 'library'. I have
> done some pretty simple testing on this - created a raid 1 device,
> made sure the sysfs entries show up, and can be used to add and view
> badblocks. A closer look by the md folks would be nice here.
>
>
> Vishal Verma (3):
> badblocks: Add core badblock management code
> block: Add badblock management for gendisks
> md: convert to use the generic badblocks code
>
Ping.
Jens, are you ok taking this through the block tree?
Any other comments from anyone else?
Thanks,
-Vishal
^ permalink raw reply
* Re: [PATCH 14/19] fs: Permit FIBMAP for users with CAP_SYS_RAWIO in s_user_ns
From: Theodore Ts'o @ 2015-12-04 23:11 UTC (permalink / raw)
To: Seth Forshee
Cc: Serge E. Hallyn, Eric W. Biederman, Alexander Viro, Serge Hallyn,
Richard Weinberger, Austin S Hemmelgarn, Miklos Szeredi,
linux-bcache, dm-devel, linux-raid, linux-kernel, linux-mtd,
linux-fsdevel, fuse-devel, linux-security-module, selinux
In-Reply-To: <20151204204532.GG147214@ubuntu-hedt>
On Fri, Dec 04, 2015 at 02:45:32PM -0600, Seth Forshee wrote:
> On Fri, Dec 04, 2015 at 02:07:36PM -0600, Serge E. Hallyn wrote:
> > Heh, I was looking over http://www.gossamer-threads.com/lists/linux/kernel/103611
> > a little while ago :) The same question was asked 16 years ago. Apparently
> > the answer then was that it was easier than fixing the code.
>
> So it seems then that either it still isn't safe and so unprivileged
> users shouldn't be allowed to do it at all, or else it's safe and we
> should drop the requirement completely. I can't say which is right,
> unfortunately.
It may not have been safe 16 years agoo, but giving invalid arguments
to FIBMAP is safe for ext4 and ext2. This is the sort of thing that
tools like trinity should and does test for, so I think it should be
fine to remove the root check for FIBMAP.
- Ted
^ permalink raw reply
* Re: [PATCH v2 1/3] badblocks: Add core badblock management code
From: James Bottomley @ 2015-12-04 23:30 UTC (permalink / raw)
To: Vishal Verma
Cc: linux-nvdimm, linux-block, linux-raid, linux-scsi, Jens Axboe,
NeilBrown, Jeff Moyer
In-Reply-To: <1448477013-9174-2-git-send-email-vishal.l.verma@intel.com>
On Wed, 2015-11-25 at 11:43 -0700, Vishal Verma wrote:
> Take the core badblocks implementation from md, and make it generally
> available. This follows the same style as kernel implementations of
> linked lists, rb-trees etc, where you can have a structure that can be
> embedded anywhere, and accessor functions to manipulate the data.
>
> The only changes in this copy of the code are ones to generalize
> function/variable names from md-specific ones. Also add init and free
> functions.
>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
> block/Makefile | 2 +-
> block/badblocks.c | 523 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/badblocks.h | 53 +++++
> 3 files changed, 577 insertions(+), 1 deletion(-)
> create mode 100644 block/badblocks.c
> create mode 100644 include/linux/badblocks.h
>
> diff --git a/block/Makefile b/block/Makefile
> index 00ecc97..db5f622 100644
> --- a/block/Makefile
> +++ b/block/Makefile
> @@ -8,7 +8,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-tag.o blk-sysfs.o \
> blk-iopoll.o blk-lib.o blk-mq.o blk-mq-tag.o \
> blk-mq-sysfs.o blk-mq-cpu.o blk-mq-cpumap.o ioctl.o \
> genhd.o scsi_ioctl.o partition-generic.o ioprio.o \
> - partitions/
> + badblocks.o partitions/
>
> obj-$(CONFIG_BOUNCE) += bounce.o
> obj-$(CONFIG_BLK_DEV_BSG) += bsg.o
> diff --git a/block/badblocks.c b/block/badblocks.c
> new file mode 100644
> index 0000000..6e07855
> --- /dev/null
> +++ b/block/badblocks.c
> @@ -0,0 +1,523 @@
> +/*
> + * Bad block management
> + *
> + * - Heavily based on MD badblocks code from Neil Brown
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/badblocks.h>
> +#include <linux/seqlock.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/stddef.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +
> +/*
> + * We can record which blocks on each device are 'bad' and so just
> + * fail those blocks, or that stripe, rather than the whole device.
> + * Entries in the bad-block table are 64bits wide. This comprises:
> + * Length of bad-range, in sectors: 0-511 for lengths 1-512
> + * Start of bad-range, sector offset, 54 bits (allows 8 exbibytes)
> + * A 'shift' can be set so that larger blocks are tracked and
> + * consequently larger devices can be covered.
> + * 'Acknowledged' flag - 1 bit. - the most significant bit.
> + *
> + * Locking of the bad-block table uses a seqlock so badblocks_check
> + * might need to retry if it is very unlucky.
> + * We will sometimes want to check for bad blocks in a bi_end_io function,
> + * so we use the write_seqlock_irq variant.
> + *
> + * When looking for a bad block we specify a range and want to
> + * know if any block in the range is bad. So we binary-search
> + * to the last range that starts at-or-before the given endpoint,
> + * (or "before the sector after the target range")
> + * then see if it ends after the given start.
> + * We return
> + * 0 if there are no known bad blocks in the range
> + * 1 if there are known bad block which are all acknowledged
> + * -1 if there are bad blocks which have not yet been acknowledged in metadata.
> + * plus the start/length of the first bad section we overlap.
> + */
This comment should be docbook.
> +int badblocks_check(struct badblocks *bb, sector_t s, int sectors,
> + sector_t *first_bad, int *bad_sectors)
[...]
> +
> +/*
> + * Add a range of bad blocks to the table.
> + * This might extend the table, or might contract it
> + * if two adjacent ranges can be merged.
> + * We binary-search to find the 'insertion' point, then
> + * decide how best to handle it.
> + */
And this one, plus you don't document returns. It looks like this
function returns 1 on success and zero on failure, which is really
counter-intuitive for the kernel: zero is usually returned on success
and negative error on failure.
> +int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
> + int acknowledged)
[...]
> +
> +/*
> + * Remove a range of bad blocks from the table.
> + * This may involve extending the table if we spilt a region,
> + * but it must not fail. So if the table becomes full, we just
> + * drop the remove request.
> + */
Docbook and document returns. This time they're the kernel standard of
0 on success and negative error on failure making the convention for
badblocks_set even more counterintuitive.
> +int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
> +{
[...]
> +#define DO_DEBUG 1
Why have this at all if it's unconditionally defined and always set.
> +ssize_t badblocks_store(struct badblocks *bb, const char *page, size_t len,
> + int unack)
[...]
> +int badblocks_init(struct badblocks *bb, int enable)
> +{
> + bb->count = 0;
> + if (enable)
> + bb->shift = 0;
> + else
> + bb->shift = -1;
> + bb->page = kmalloc(PAGE_SIZE, GFP_KERNEL);
Why not __get_free_page(GFP_KERNEL)? The problem with kmalloc of an
exactly known page sized quantity is that the slab tracker for this
requires two contiguous pages for each page because of the overhead.
James
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox