* [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-10-12 18:06 ` Vivek Goyal
0 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-10-12 18:06 UTC (permalink / raw)
To: linux-fsdevel, selinux, linux-security-module, miklos
Cc: stephen.smalley.work, omosnace, virtio-fs, casey, vgoyal
When a new inode is created, send its security context to server along
with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
This gives server an opportunity to create new file and set security
context (possibly atomically). In all the configurations it might not
be possible to set context atomically.
Like nfs and ceph, use security_dentry_init_security() to dermine security
context of inode and send it with create, mkdir, mknod, and symlink requests.
Following is the information sent to server.
- struct fuse_secctxs.
This contains total number of security contexts being sent.
- struct fuse_secctx.
This contains total size of security context which follows this structure.
There is one fuse_secctx instance per security context.
- xattr name string.
This string represents name of xattr which should be used while setting
security context. As of now it is hardcoded to "security.selinux".
- security context.
This is the actual security context whose size is specified in fuse_secctx
struct.
This patch is modified version of patch from
Chirantan Ekbote <chirantan@chromium.org>
v2:
- Added "fuse_secctxs" structure where one can specify how many security
contexts are being sent. This can be useful down the line if we
have more than one security contexts being set.
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
fs/fuse/fuse_i.h | 3 +
fs/fuse/inode.c | 4 +-
include/uapi/linux/fuse.h | 20 +++++++
4 files changed, 136 insertions(+), 6 deletions(-)
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index d9b977c0f38d..ce62593a61f9 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -17,6 +17,9 @@
#include <linux/xattr.h>
#include <linux/iversion.h>
#include <linux/posix_acl.h>
+#include <linux/security.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
static void fuse_advise_use_readdirplus(struct inode *dir)
{
@@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
return ERR_PTR(err);
}
+static int get_security_context(struct dentry *entry, umode_t mode,
+ void **security_ctx, u32 *security_ctxlen)
+{
+ struct fuse_secctx *fsecctx;
+ struct fuse_secctxs *fsecctxs;
+ void *ctx, *full_ctx;
+ u32 ctxlen, full_ctxlen;
+ int err = 0;
+ const char *name;
+
+ err = security_dentry_init_security(entry, mode, &entry->d_name,
+ &name, &ctx, &ctxlen);
+ if (err) {
+ if (err != -EOPNOTSUPP)
+ goto out_err;
+ /* No LSM is supporting this security hook. Ignore error */
+ err = 0;
+ ctxlen = 0;
+ }
+
+ if (ctxlen > 0) {
+ void *ptr;
+
+ full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
+ strlen(name) + ctxlen + 1;
+ full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+ if (!full_ctx) {
+ err = -ENOMEM;
+ kfree(ctx);
+ goto out_err;
+ }
+
+ ptr = full_ctx;
+ fsecctxs = (struct fuse_secctxs*) ptr;
+ fsecctxs->nr_secctx = 1;
+ ptr += sizeof(*fsecctxs);
+
+ fsecctx = (struct fuse_secctx*) ptr;
+ fsecctx->size = ctxlen;
+ ptr += sizeof(*fsecctx);
+
+ strcpy(ptr, name);
+ ptr += strlen(name) + 1;
+ memcpy(ptr, ctx, ctxlen);
+ kfree(ctx);
+ } else {
+ full_ctxlen = sizeof(*fsecctxs);
+ full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
+ if (!full_ctx) {
+ err = -ENOMEM;
+ goto out_err;
+ }
+ }
+
+ *security_ctxlen = full_ctxlen;
+ *security_ctx = full_ctx;
+out_err:
+ return err;
+}
+
/*
* Atomic create+open operation
*
@@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
struct fuse_entry_out outentry;
struct fuse_inode *fi;
struct fuse_file *ff;
+ void *security_ctx = NULL;
+ u32 security_ctxlen;
/* Userspace expects S_IFREG in create mode */
BUG_ON((mode & S_IFMT) != S_IFREG);
@@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
args.out_args[0].value = &outentry;
args.out_args[1].size = sizeof(outopen);
args.out_args[1].value = &outopen;
+
+ if (fm->fc->init_security) {
+ err = get_security_context(entry, mode, &security_ctx,
+ &security_ctxlen);
+ if (err)
+ goto out_put_forget_req;
+
+ args.in_numargs = 3;
+ args.in_args[2].size = security_ctxlen;
+ args.in_args[2].value = security_ctx;
+ }
+
err = fuse_simple_request(fm, &args);
if (err)
goto out_free_ff;
@@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
out_free_ff:
fuse_file_free(ff);
+ kfree(security_ctx);
out_put_forget_req:
kfree(forget);
out_err:
@@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
*/
static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
struct inode *dir, struct dentry *entry,
- umode_t mode)
+ umode_t mode, bool init_security)
{
struct fuse_entry_out outarg;
struct inode *inode;
struct dentry *d;
int err;
struct fuse_forget_link *forget;
+ void *security_ctx = NULL;
+ u32 security_ctxlen = 0;
if (fuse_is_bad(dir))
return -EIO;
@@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
args->out_numargs = 1;
args->out_args[0].size = sizeof(outarg);
args->out_args[0].value = &outarg;
+
+ if (init_security) {
+ unsigned short idx = args->in_numargs;
+
+ if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
+ err = -ENOMEM;
+ goto out_put_forget_req;
+ }
+
+ err = get_security_context(entry, mode, &security_ctx,
+ &security_ctxlen);
+ if (err)
+ goto out_put_forget_req;
+
+ if (security_ctxlen > 0) {
+ args->in_args[idx].size = security_ctxlen;
+ args->in_args[idx].value = security_ctx;
+ args->in_numargs++;
+ }
+ }
+
err = fuse_simple_request(fm, args);
+ kfree(security_ctx);
if (err)
goto out_put_forget_req;
@@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
args.in_args[0].value = &inarg;
args.in_args[1].size = entry->d_name.len + 1;
args.in_args[1].value = entry->d_name.name;
- return create_new_entry(fm, &args, dir, entry, mode);
+ return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
}
static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
@@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
args.in_args[0].value = &inarg;
args.in_args[1].size = entry->d_name.len + 1;
args.in_args[1].value = entry->d_name.name;
- return create_new_entry(fm, &args, dir, entry, S_IFDIR);
+ return create_new_entry(fm, &args, dir, entry, S_IFDIR,
+ fm->fc->init_security);
}
static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
@@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
args.in_args[0].value = entry->d_name.name;
args.in_args[1].size = len;
args.in_args[1].value = link;
- return create_new_entry(fm, &args, dir, entry, S_IFLNK);
+ return create_new_entry(fm, &args, dir, entry, S_IFLNK,
+ fm->fc->init_security);
}
void fuse_update_ctime(struct inode *inode)
@@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
args.in_args[0].value = &inarg;
args.in_args[1].size = newent->d_name.len + 1;
args.in_args[1].value = newent->d_name.name;
- err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
+ err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
+ false);
/* Contrary to "normal" filesystems it can happen that link
makes two "logical" inodes point to the same "physical"
inode. We invalidate the attributes of the old one, so it
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 319596df5dc6..885f34f9967f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -765,6 +765,9 @@ struct fuse_conn {
/* Propagate syncfs() to server */
unsigned int sync_fs:1;
+ /* Initialize security xattrs when creating a new inode */
+ unsigned int init_security:1;
+
/** The number of requests waiting for completion */
atomic_t num_waiting;
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 36cd03114b6d..343bc9cfbd92 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
}
if (arg->flags & FUSE_SETXATTR_EXT)
fc->setxattr_ext = 1;
+ if (arg->flags & FUSE_SECURITY_CTX)
+ fc->init_security = 1;
} else {
ra_pages = fc->max_read / PAGE_SIZE;
fc->no_lock = 1;
@@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
- FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
+ FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
#ifdef CONFIG_FUSE_DAX
if (fm->fc->dax)
ia->in.flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 2fe54c80051a..b31a0f79fde8 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -986,4 +986,24 @@ struct fuse_syncfs_in {
uint64_t padding;
};
+/*
+ * For each security context, send fuse_secctx with size of security context
+ * fuse_secctx will be followed by security context name and this in turn
+ * will be followed by actual context label.
+ * fuse_secctx, name, context
+ * */
+struct fuse_secctx {
+ uint32_t size;
+ uint32_t padding;
+};
+
+/*
+ * Contains the information about how many fuse_secctx structures are being
+ * sent.
+ */
+struct fuse_secctxs {
+ uint32_t nr_secctx;
+ uint32_t padding;
+};
+
#endif /* _LINUX_FUSE_H */
--
2.31.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-12 18:06 ` [Virtio-fs] " Vivek Goyal
@ 2021-10-12 18:24 ` Casey Schaufler
-1 siblings, 0 replies; 29+ messages in thread
From: Casey Schaufler @ 2021-10-12 18:24 UTC (permalink / raw)
To: Vivek Goyal, linux-fsdevel, selinux, linux-security-module, miklos
Cc: virtio-fs, chirantan, stephen.smalley.work, dwalsh, omosnace,
Casey Schaufler
On 10/12/2021 11:06 AM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctxs.
> This contains total number of security contexts being sent.
>
> - struct fuse_secctx.
> This contains total size of security context which follows this structure.
> There is one fuse_secctx instance per security context.
>
> - xattr name string.
> This string represents name of xattr which should be used while setting
> security context. As of now it is hardcoded to "security.selinux".
Where is the name hardcoded? I looks as if you're getting the attribute
name along with the value from security_dentry_init_security().
>
> - security context.
> This is the actual security context whose size is specified in fuse_secctx
> struct.
>
> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> v2:
> - Added "fuse_secctxs" structure where one can specify how many security
> contexts are being sent. This can be useful down the line if we
> have more than one security contexts being set.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> fs/fuse/fuse_i.h | 3 +
> fs/fuse/inode.c | 4 +-
> include/uapi/linux/fuse.h | 20 +++++++
> 4 files changed, 136 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..ce62593a61f9 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
> #include <linux/xattr.h>
> #include <linux/iversion.h>
> #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>
> static void fuse_advise_use_readdirplus(struct inode *dir)
> {
> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> return ERR_PTR(err);
> }
>
> +static int get_security_context(struct dentry *entry, umode_t mode,
> + void **security_ctx, u32 *security_ctxlen)
> +{
> + struct fuse_secctx *fsecctx;
> + struct fuse_secctxs *fsecctxs;
> + void *ctx, *full_ctx;
> + u32 ctxlen, full_ctxlen;
> + int err = 0;
> + const char *name;
> +
> + err = security_dentry_init_security(entry, mode, &entry->d_name,
> + &name, &ctx, &ctxlen);
> + if (err) {
> + if (err != -EOPNOTSUPP)
> + goto out_err;
> + /* No LSM is supporting this security hook. Ignore error */
> + err = 0;
> + ctxlen = 0;
> + }
> +
> + if (ctxlen > 0) {
> + void *ptr;
> +
> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> + strlen(name) + ctxlen + 1;
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + kfree(ctx);
> + goto out_err;
> + }
> +
> + ptr = full_ctx;
> + fsecctxs = (struct fuse_secctxs*) ptr;
> + fsecctxs->nr_secctx = 1;
> + ptr += sizeof(*fsecctxs);
> +
> + fsecctx = (struct fuse_secctx*) ptr;
> + fsecctx->size = ctxlen;
> + ptr += sizeof(*fsecctx);
> +
> + strcpy(ptr, name);
> + ptr += strlen(name) + 1;
> + memcpy(ptr, ctx, ctxlen);
> + kfree(ctx);
> + } else {
> + full_ctxlen = sizeof(*fsecctxs);
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> + }
> +
> + *security_ctxlen = full_ctxlen;
> + *security_ctx = full_ctx;
> +out_err:
> + return err;
> +}
> +
> /*
> * Atomic create+open operation
> *
> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> struct fuse_entry_out outentry;
> struct fuse_inode *fi;
> struct fuse_file *ff;
> + void *security_ctx = NULL;
> + u32 security_ctxlen;
>
> /* Userspace expects S_IFREG in create mode */
> BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> args.out_args[0].value = &outentry;
> args.out_args[1].size = sizeof(outopen);
> args.out_args[1].value = &outopen;
> +
> + if (fm->fc->init_security) {
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + args.in_numargs = 3;
> + args.in_args[2].size = security_ctxlen;
> + args.in_args[2].value = security_ctx;
> + }
> +
> err = fuse_simple_request(fm, &args);
> if (err)
> goto out_free_ff;
> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>
> out_free_ff:
> fuse_file_free(ff);
> + kfree(security_ctx);
> out_put_forget_req:
> kfree(forget);
> out_err:
> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> */
> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> struct inode *dir, struct dentry *entry,
> - umode_t mode)
> + umode_t mode, bool init_security)
> {
> struct fuse_entry_out outarg;
> struct inode *inode;
> struct dentry *d;
> int err;
> struct fuse_forget_link *forget;
> + void *security_ctx = NULL;
> + u32 security_ctxlen = 0;
>
> if (fuse_is_bad(dir))
> return -EIO;
> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> args->out_numargs = 1;
> args->out_args[0].size = sizeof(outarg);
> args->out_args[0].value = &outarg;
> +
> + if (init_security) {
> + unsigned short idx = args->in_numargs;
> +
> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> + err = -ENOMEM;
> + goto out_put_forget_req;
> + }
> +
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + if (security_ctxlen > 0) {
> + args->in_args[idx].size = security_ctxlen;
> + args->in_args[idx].value = security_ctx;
> + args->in_numargs++;
> + }
> + }
> +
> err = fuse_simple_request(fm, args);
> + kfree(security_ctx);
> if (err)
> goto out_put_forget_req;
>
> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, mode);
> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> }
>
> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> + fm->fc->init_security);
> }
>
> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = entry->d_name.name;
> args.in_args[1].size = len;
> args.in_args[1].value = link;
> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> + fm->fc->init_security);
> }
>
> void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = newent->d_name.len + 1;
> args.in_args[1].value = newent->d_name.name;
> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> + false);
> /* Contrary to "normal" filesystems it can happen that link
> makes two "logical" inodes point to the same "physical"
> inode. We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
> /* Propagate syncfs() to server */
> unsigned int sync_fs:1;
>
> + /* Initialize security xattrs when creating a new inode */
> + unsigned int init_security:1;
> +
> /** The number of requests waiting for completion */
> atomic_t num_waiting;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> }
> if (arg->flags & FUSE_SETXATTR_EXT)
> fc->setxattr_ext = 1;
> + if (arg->flags & FUSE_SECURITY_CTX)
> + fc->init_security = 1;
> } else {
> ra_pages = fc->max_read / PAGE_SIZE;
> fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> #ifdef CONFIG_FUSE_DAX
> if (fm->fc->dax)
> ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..b31a0f79fde8 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> uint64_t padding;
> };
>
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> + uint32_t size;
> + uint32_t padding;
> +};
> +
> +/*
> + * Contains the information about how many fuse_secctx structures are being
> + * sent.
> + */
> +struct fuse_secctxs {
> + uint32_t nr_secctx;
> + uint32_t padding;
> +};
> +
> #endif /* _LINUX_FUSE_H */
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-10-12 18:24 ` Casey Schaufler
0 siblings, 0 replies; 29+ messages in thread
From: Casey Schaufler @ 2021-10-12 18:24 UTC (permalink / raw)
To: Vivek Goyal, linux-fsdevel, selinux, linux-security-module, miklos
Cc: stephen.smalley.work, omosnace, virtio-fs, Casey Schaufler
On 10/12/2021 11:06 AM, Vivek Goyal wrote:
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctxs.
> This contains total number of security contexts being sent.
>
> - struct fuse_secctx.
> This contains total size of security context which follows this structure.
> There is one fuse_secctx instance per security context.
>
> - xattr name string.
> This string represents name of xattr which should be used while setting
> security context. As of now it is hardcoded to "security.selinux".
Where is the name hardcoded? I looks as if you're getting the attribute
name along with the value from security_dentry_init_security().
>
> - security context.
> This is the actual security context whose size is specified in fuse_secctx
> struct.
>
> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> v2:
> - Added "fuse_secctxs" structure where one can specify how many security
> contexts are being sent. This can be useful down the line if we
> have more than one security contexts being set.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> fs/fuse/fuse_i.h | 3 +
> fs/fuse/inode.c | 4 +-
> include/uapi/linux/fuse.h | 20 +++++++
> 4 files changed, 136 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..ce62593a61f9 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
> #include <linux/xattr.h>
> #include <linux/iversion.h>
> #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>
> static void fuse_advise_use_readdirplus(struct inode *dir)
> {
> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> return ERR_PTR(err);
> }
>
> +static int get_security_context(struct dentry *entry, umode_t mode,
> + void **security_ctx, u32 *security_ctxlen)
> +{
> + struct fuse_secctx *fsecctx;
> + struct fuse_secctxs *fsecctxs;
> + void *ctx, *full_ctx;
> + u32 ctxlen, full_ctxlen;
> + int err = 0;
> + const char *name;
> +
> + err = security_dentry_init_security(entry, mode, &entry->d_name,
> + &name, &ctx, &ctxlen);
> + if (err) {
> + if (err != -EOPNOTSUPP)
> + goto out_err;
> + /* No LSM is supporting this security hook. Ignore error */
> + err = 0;
> + ctxlen = 0;
> + }
> +
> + if (ctxlen > 0) {
> + void *ptr;
> +
> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> + strlen(name) + ctxlen + 1;
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + kfree(ctx);
> + goto out_err;
> + }
> +
> + ptr = full_ctx;
> + fsecctxs = (struct fuse_secctxs*) ptr;
> + fsecctxs->nr_secctx = 1;
> + ptr += sizeof(*fsecctxs);
> +
> + fsecctx = (struct fuse_secctx*) ptr;
> + fsecctx->size = ctxlen;
> + ptr += sizeof(*fsecctx);
> +
> + strcpy(ptr, name);
> + ptr += strlen(name) + 1;
> + memcpy(ptr, ctx, ctxlen);
> + kfree(ctx);
> + } else {
> + full_ctxlen = sizeof(*fsecctxs);
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> + }
> +
> + *security_ctxlen = full_ctxlen;
> + *security_ctx = full_ctx;
> +out_err:
> + return err;
> +}
> +
> /*
> * Atomic create+open operation
> *
> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> struct fuse_entry_out outentry;
> struct fuse_inode *fi;
> struct fuse_file *ff;
> + void *security_ctx = NULL;
> + u32 security_ctxlen;
>
> /* Userspace expects S_IFREG in create mode */
> BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> args.out_args[0].value = &outentry;
> args.out_args[1].size = sizeof(outopen);
> args.out_args[1].value = &outopen;
> +
> + if (fm->fc->init_security) {
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + args.in_numargs = 3;
> + args.in_args[2].size = security_ctxlen;
> + args.in_args[2].value = security_ctx;
> + }
> +
> err = fuse_simple_request(fm, &args);
> if (err)
> goto out_free_ff;
> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>
> out_free_ff:
> fuse_file_free(ff);
> + kfree(security_ctx);
> out_put_forget_req:
> kfree(forget);
> out_err:
> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> */
> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> struct inode *dir, struct dentry *entry,
> - umode_t mode)
> + umode_t mode, bool init_security)
> {
> struct fuse_entry_out outarg;
> struct inode *inode;
> struct dentry *d;
> int err;
> struct fuse_forget_link *forget;
> + void *security_ctx = NULL;
> + u32 security_ctxlen = 0;
>
> if (fuse_is_bad(dir))
> return -EIO;
> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> args->out_numargs = 1;
> args->out_args[0].size = sizeof(outarg);
> args->out_args[0].value = &outarg;
> +
> + if (init_security) {
> + unsigned short idx = args->in_numargs;
> +
> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> + err = -ENOMEM;
> + goto out_put_forget_req;
> + }
> +
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + if (security_ctxlen > 0) {
> + args->in_args[idx].size = security_ctxlen;
> + args->in_args[idx].value = security_ctx;
> + args->in_numargs++;
> + }
> + }
> +
> err = fuse_simple_request(fm, args);
> + kfree(security_ctx);
> if (err)
> goto out_put_forget_req;
>
> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, mode);
> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> }
>
> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> + fm->fc->init_security);
> }
>
> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = entry->d_name.name;
> args.in_args[1].size = len;
> args.in_args[1].value = link;
> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> + fm->fc->init_security);
> }
>
> void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = newent->d_name.len + 1;
> args.in_args[1].value = newent->d_name.name;
> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> + false);
> /* Contrary to "normal" filesystems it can happen that link
> makes two "logical" inodes point to the same "physical"
> inode. We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
> /* Propagate syncfs() to server */
> unsigned int sync_fs:1;
>
> + /* Initialize security xattrs when creating a new inode */
> + unsigned int init_security:1;
> +
> /** The number of requests waiting for completion */
> atomic_t num_waiting;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> }
> if (arg->flags & FUSE_SETXATTR_EXT)
> fc->setxattr_ext = 1;
> + if (arg->flags & FUSE_SECURITY_CTX)
> + fc->init_security = 1;
> } else {
> ra_pages = fc->max_read / PAGE_SIZE;
> fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> #ifdef CONFIG_FUSE_DAX
> if (fm->fc->dax)
> ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..b31a0f79fde8 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> uint64_t padding;
> };
>
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> + uint32_t size;
> + uint32_t padding;
> +};
> +
> +/*
> + * Contains the information about how many fuse_secctx structures are being
> + * sent.
> + */
> +struct fuse_secctxs {
> + uint32_t nr_secctx;
> + uint32_t padding;
> +};
> +
> #endif /* _LINUX_FUSE_H */
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-12 18:24 ` [Virtio-fs] " Casey Schaufler
@ 2021-10-12 18:34 ` Vivek Goyal
-1 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-10-12 18:34 UTC (permalink / raw)
To: Casey Schaufler
Cc: linux-fsdevel, selinux, linux-security-module, miklos, virtio-fs,
chirantan, stephen.smalley.work, dwalsh, omosnace
On Tue, Oct 12, 2021 at 11:24:23AM -0700, Casey Schaufler wrote:
> On 10/12/2021 11:06 AM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctxs.
> > This contains total number of security contexts being sent.
> >
> > - struct fuse_secctx.
> > This contains total size of security context which follows this structure.
> > There is one fuse_secctx instance per security context.
> >
> > - xattr name string.
> > This string represents name of xattr which should be used while setting
> > security context. As of now it is hardcoded to "security.selinux".
>
> Where is the name hardcoded? I looks as if you're getting the attribute
> name along with the value from security_dentry_init_security().
Sorry, I copied pasted this description from V1 where I was hardcoding
the name to "security.selinux". But V2 got rid of that hardcoding and
that's why this patch series is dependent on the other patch which
modifies security_dentry_init_security() signature.
https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c@redhat.com/
Thanks
Vivek
>
> >
> > - security context.
> > This is the actual security context whose size is specified in fuse_secctx
> > struct.
> >
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > v2:
> > - Added "fuse_secctxs" structure where one can specify how many security
> > contexts are being sent. This can be useful down the line if we
> > have more than one security contexts being set.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> > fs/fuse/fuse_i.h | 3 +
> > fs/fuse/inode.c | 4 +-
> > include/uapi/linux/fuse.h | 20 +++++++
> > 4 files changed, 136 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..ce62593a61f9 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> > #include <linux/xattr.h>
> > #include <linux/iversion.h>
> > #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >
> > static void fuse_advise_use_readdirplus(struct inode *dir)
> > {
> > @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> > return ERR_PTR(err);
> > }
> >
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > + void **security_ctx, u32 *security_ctxlen)
> > +{
> > + struct fuse_secctx *fsecctx;
> > + struct fuse_secctxs *fsecctxs;
> > + void *ctx, *full_ctx;
> > + u32 ctxlen, full_ctxlen;
> > + int err = 0;
> > + const char *name;
> > +
> > + err = security_dentry_init_security(entry, mode, &entry->d_name,
> > + &name, &ctx, &ctxlen);
> > + if (err) {
> > + if (err != -EOPNOTSUPP)
> > + goto out_err;
> > + /* No LSM is supporting this security hook. Ignore error */
> > + err = 0;
> > + ctxlen = 0;
> > + }
> > +
> > + if (ctxlen > 0) {
> > + void *ptr;
> > +
> > + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> > + strlen(name) + ctxlen + 1;
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + kfree(ctx);
> > + goto out_err;
> > + }
> > +
> > + ptr = full_ctx;
> > + fsecctxs = (struct fuse_secctxs*) ptr;
> > + fsecctxs->nr_secctx = 1;
> > + ptr += sizeof(*fsecctxs);
> > +
> > + fsecctx = (struct fuse_secctx*) ptr;
> > + fsecctx->size = ctxlen;
> > + ptr += sizeof(*fsecctx);
> > +
> > + strcpy(ptr, name);
> > + ptr += strlen(name) + 1;
> > + memcpy(ptr, ctx, ctxlen);
> > + kfree(ctx);
> > + } else {
> > + full_ctxlen = sizeof(*fsecctxs);
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > + }
> > +
> > + *security_ctxlen = full_ctxlen;
> > + *security_ctx = full_ctx;
> > +out_err:
> > + return err;
> > +}
> > +
> > /*
> > * Atomic create+open operation
> > *
> > @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > struct fuse_entry_out outentry;
> > struct fuse_inode *fi;
> > struct fuse_file *ff;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen;
> >
> > /* Userspace expects S_IFREG in create mode */
> > BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > args.out_args[0].value = &outentry;
> > args.out_args[1].size = sizeof(outopen);
> > args.out_args[1].value = &outopen;
> > +
> > + if (fm->fc->init_security) {
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + args.in_numargs = 3;
> > + args.in_args[2].size = security_ctxlen;
> > + args.in_args[2].value = security_ctx;
> > + }
> > +
> > err = fuse_simple_request(fm, &args);
> > if (err)
> > goto out_free_ff;
> > @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >
> > out_free_ff:
> > fuse_file_free(ff);
> > + kfree(security_ctx);
> > out_put_forget_req:
> > kfree(forget);
> > out_err:
> > @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > */
> > static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > struct inode *dir, struct dentry *entry,
> > - umode_t mode)
> > + umode_t mode, bool init_security)
> > {
> > struct fuse_entry_out outarg;
> > struct inode *inode;
> > struct dentry *d;
> > int err;
> > struct fuse_forget_link *forget;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen = 0;
> >
> > if (fuse_is_bad(dir))
> > return -EIO;
> > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > args->out_numargs = 1;
> > args->out_args[0].size = sizeof(outarg);
> > args->out_args[0].value = &outarg;
> > +
> > + if (init_security) {
> > + unsigned short idx = args->in_numargs;
> > +
> > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > + err = -ENOMEM;
> > + goto out_put_forget_req;
> > + }
> > +
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + if (security_ctxlen > 0) {
> > + args->in_args[idx].size = security_ctxlen;
> > + args->in_args[idx].value = security_ctx;
> > + args->in_numargs++;
> > + }
> > + }
> > +
> > err = fuse_simple_request(fm, args);
> > + kfree(security_ctx);
> > if (err)
> > goto out_put_forget_req;
> >
> > @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, mode);
> > + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> > }
> >
> > static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > + fm->fc->init_security);
> > }
> >
> > static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = entry->d_name.name;
> > args.in_args[1].size = len;
> > args.in_args[1].value = link;
> > - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > + fm->fc->init_security);
> > }
> >
> > void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = newent->d_name.len + 1;
> > args.in_args[1].value = newent->d_name.name;
> > - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > + false);
> > /* Contrary to "normal" filesystems it can happen that link
> > makes two "logical" inodes point to the same "physical"
> > inode. We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> > /* Propagate syncfs() to server */
> > unsigned int sync_fs:1;
> >
> > + /* Initialize security xattrs when creating a new inode */
> > + unsigned int init_security:1;
> > +
> > /** The number of requests waiting for completion */
> > atomic_t num_waiting;
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > }
> > if (arg->flags & FUSE_SETXATTR_EXT)
> > fc->setxattr_ext = 1;
> > + if (arg->flags & FUSE_SECURITY_CTX)
> > + fc->init_security = 1;
> > } else {
> > ra_pages = fc->max_read / PAGE_SIZE;
> > fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> > #ifdef CONFIG_FUSE_DAX
> > if (fm->fc->dax)
> > ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..b31a0f79fde8 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> > uint64_t padding;
> > };
> >
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > + uint32_t size;
> > + uint32_t padding;
> > +};
> > +
> > +/*
> > + * Contains the information about how many fuse_secctx structures are being
> > + * sent.
> > + */
> > +struct fuse_secctxs {
> > + uint32_t nr_secctx;
> > + uint32_t padding;
> > +};
> > +
> > #endif /* _LINUX_FUSE_H */
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-10-12 18:34 ` Vivek Goyal
0 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-10-12 18:34 UTC (permalink / raw)
To: Casey Schaufler
Cc: miklos, selinux, stephen.smalley.work, omosnace, virtio-fs,
linux-security-module, linux-fsdevel
On Tue, Oct 12, 2021 at 11:24:23AM -0700, Casey Schaufler wrote:
> On 10/12/2021 11:06 AM, Vivek Goyal wrote:
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctxs.
> > This contains total number of security contexts being sent.
> >
> > - struct fuse_secctx.
> > This contains total size of security context which follows this structure.
> > There is one fuse_secctx instance per security context.
> >
> > - xattr name string.
> > This string represents name of xattr which should be used while setting
> > security context. As of now it is hardcoded to "security.selinux".
>
> Where is the name hardcoded? I looks as if you're getting the attribute
> name along with the value from security_dentry_init_security().
Sorry, I copied pasted this description from V1 where I was hardcoding
the name to "security.selinux". But V2 got rid of that hardcoding and
that's why this patch series is dependent on the other patch which
modifies security_dentry_init_security() signature.
https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c@redhat.com/
Thanks
Vivek
>
> >
> > - security context.
> > This is the actual security context whose size is specified in fuse_secctx
> > struct.
> >
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > v2:
> > - Added "fuse_secctxs" structure where one can specify how many security
> > contexts are being sent. This can be useful down the line if we
> > have more than one security contexts being set.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> > fs/fuse/fuse_i.h | 3 +
> > fs/fuse/inode.c | 4 +-
> > include/uapi/linux/fuse.h | 20 +++++++
> > 4 files changed, 136 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..ce62593a61f9 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> > #include <linux/xattr.h>
> > #include <linux/iversion.h>
> > #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >
> > static void fuse_advise_use_readdirplus(struct inode *dir)
> > {
> > @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> > return ERR_PTR(err);
> > }
> >
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > + void **security_ctx, u32 *security_ctxlen)
> > +{
> > + struct fuse_secctx *fsecctx;
> > + struct fuse_secctxs *fsecctxs;
> > + void *ctx, *full_ctx;
> > + u32 ctxlen, full_ctxlen;
> > + int err = 0;
> > + const char *name;
> > +
> > + err = security_dentry_init_security(entry, mode, &entry->d_name,
> > + &name, &ctx, &ctxlen);
> > + if (err) {
> > + if (err != -EOPNOTSUPP)
> > + goto out_err;
> > + /* No LSM is supporting this security hook. Ignore error */
> > + err = 0;
> > + ctxlen = 0;
> > + }
> > +
> > + if (ctxlen > 0) {
> > + void *ptr;
> > +
> > + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> > + strlen(name) + ctxlen + 1;
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + kfree(ctx);
> > + goto out_err;
> > + }
> > +
> > + ptr = full_ctx;
> > + fsecctxs = (struct fuse_secctxs*) ptr;
> > + fsecctxs->nr_secctx = 1;
> > + ptr += sizeof(*fsecctxs);
> > +
> > + fsecctx = (struct fuse_secctx*) ptr;
> > + fsecctx->size = ctxlen;
> > + ptr += sizeof(*fsecctx);
> > +
> > + strcpy(ptr, name);
> > + ptr += strlen(name) + 1;
> > + memcpy(ptr, ctx, ctxlen);
> > + kfree(ctx);
> > + } else {
> > + full_ctxlen = sizeof(*fsecctxs);
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > + }
> > +
> > + *security_ctxlen = full_ctxlen;
> > + *security_ctx = full_ctx;
> > +out_err:
> > + return err;
> > +}
> > +
> > /*
> > * Atomic create+open operation
> > *
> > @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > struct fuse_entry_out outentry;
> > struct fuse_inode *fi;
> > struct fuse_file *ff;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen;
> >
> > /* Userspace expects S_IFREG in create mode */
> > BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > args.out_args[0].value = &outentry;
> > args.out_args[1].size = sizeof(outopen);
> > args.out_args[1].value = &outopen;
> > +
> > + if (fm->fc->init_security) {
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + args.in_numargs = 3;
> > + args.in_args[2].size = security_ctxlen;
> > + args.in_args[2].value = security_ctx;
> > + }
> > +
> > err = fuse_simple_request(fm, &args);
> > if (err)
> > goto out_free_ff;
> > @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >
> > out_free_ff:
> > fuse_file_free(ff);
> > + kfree(security_ctx);
> > out_put_forget_req:
> > kfree(forget);
> > out_err:
> > @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > */
> > static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > struct inode *dir, struct dentry *entry,
> > - umode_t mode)
> > + umode_t mode, bool init_security)
> > {
> > struct fuse_entry_out outarg;
> > struct inode *inode;
> > struct dentry *d;
> > int err;
> > struct fuse_forget_link *forget;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen = 0;
> >
> > if (fuse_is_bad(dir))
> > return -EIO;
> > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > args->out_numargs = 1;
> > args->out_args[0].size = sizeof(outarg);
> > args->out_args[0].value = &outarg;
> > +
> > + if (init_security) {
> > + unsigned short idx = args->in_numargs;
> > +
> > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > + err = -ENOMEM;
> > + goto out_put_forget_req;
> > + }
> > +
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + if (security_ctxlen > 0) {
> > + args->in_args[idx].size = security_ctxlen;
> > + args->in_args[idx].value = security_ctx;
> > + args->in_numargs++;
> > + }
> > + }
> > +
> > err = fuse_simple_request(fm, args);
> > + kfree(security_ctx);
> > if (err)
> > goto out_put_forget_req;
> >
> > @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, mode);
> > + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> > }
> >
> > static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > + fm->fc->init_security);
> > }
> >
> > static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = entry->d_name.name;
> > args.in_args[1].size = len;
> > args.in_args[1].value = link;
> > - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > + fm->fc->init_security);
> > }
> >
> > void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = newent->d_name.len + 1;
> > args.in_args[1].value = newent->d_name.name;
> > - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > + false);
> > /* Contrary to "normal" filesystems it can happen that link
> > makes two "logical" inodes point to the same "physical"
> > inode. We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> > /* Propagate syncfs() to server */
> > unsigned int sync_fs:1;
> >
> > + /* Initialize security xattrs when creating a new inode */
> > + unsigned int init_security:1;
> > +
> > /** The number of requests waiting for completion */
> > atomic_t num_waiting;
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > }
> > if (arg->flags & FUSE_SETXATTR_EXT)
> > fc->setxattr_ext = 1;
> > + if (arg->flags & FUSE_SECURITY_CTX)
> > + fc->init_security = 1;
> > } else {
> > ra_pages = fc->max_read / PAGE_SIZE;
> > fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> > #ifdef CONFIG_FUSE_DAX
> > if (fm->fc->dax)
> > ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..b31a0f79fde8 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> > uint64_t padding;
> > };
> >
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > + uint32_t size;
> > + uint32_t padding;
> > +};
> > +
> > +/*
> > + * Contains the information about how many fuse_secctx structures are being
> > + * sent.
> > + */
> > +struct fuse_secctxs {
> > + uint32_t nr_secctx;
> > + uint32_t padding;
> > +};
> > +
> > #endif /* _LINUX_FUSE_H */
>
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-12 18:34 ` [Virtio-fs] " Vivek Goyal
@ 2021-10-12 18:41 ` Casey Schaufler
-1 siblings, 0 replies; 29+ messages in thread
From: Casey Schaufler @ 2021-10-12 18:41 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, selinux, linux-security-module, miklos, virtio-fs,
chirantan, stephen.smalley.work, dwalsh, omosnace,
Casey Schaufler
On 10/12/2021 11:34 AM, Vivek Goyal wrote:
> On Tue, Oct 12, 2021 at 11:24:23AM -0700, Casey Schaufler wrote:
>> On 10/12/2021 11:06 AM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctxs.
>>> This contains total number of security contexts being sent.
>>>
>>> - struct fuse_secctx.
>>> This contains total size of security context which follows this structure.
>>> There is one fuse_secctx instance per security context.
>>>
>>> - xattr name string.
>>> This string represents name of xattr which should be used while setting
>>> security context. As of now it is hardcoded to "security.selinux".
>> Where is the name hardcoded? I looks as if you're getting the attribute
>> name along with the value from security_dentry_init_security().
> Sorry, I copied pasted this description from V1 where I was hardcoding
> the name to "security.selinux".
That's what I suspected. Thanks.
> But V2 got rid of that hardcoding and
> that's why this patch series is dependent on the other patch which
> modifies security_dentry_init_security() signature.
>
> https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c@redhat.com/
>
> Thanks
> Vivek
>
>>> - security context.
>>> This is the actual security context whose size is specified in fuse_secctx
>>> struct.
>>>
>>> This patch is modified version of patch from
>>> Chirantan Ekbote <chirantan@chromium.org>
>>>
>>> v2:
>>> - Added "fuse_secctxs" structure where one can specify how many security
>>> contexts are being sent. This can be useful down the line if we
>>> have more than one security contexts being set.
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
>>> fs/fuse/fuse_i.h | 3 +
>>> fs/fuse/inode.c | 4 +-
>>> include/uapi/linux/fuse.h | 20 +++++++
>>> 4 files changed, 136 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>> index d9b977c0f38d..ce62593a61f9 100644
>>> --- a/fs/fuse/dir.c
>>> +++ b/fs/fuse/dir.c
>>> @@ -17,6 +17,9 @@
>>> #include <linux/xattr.h>
>>> #include <linux/iversion.h>
>>> #include <linux/posix_acl.h>
>>> +#include <linux/security.h>
>>> +#include <linux/types.h>
>>> +#include <linux/kernel.h>
>>>
>>> static void fuse_advise_use_readdirplus(struct inode *dir)
>>> {
>>> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>>> return ERR_PTR(err);
>>> }
>>>
>>> +static int get_security_context(struct dentry *entry, umode_t mode,
>>> + void **security_ctx, u32 *security_ctxlen)
>>> +{
>>> + struct fuse_secctx *fsecctx;
>>> + struct fuse_secctxs *fsecctxs;
>>> + void *ctx, *full_ctx;
>>> + u32 ctxlen, full_ctxlen;
>>> + int err = 0;
>>> + const char *name;
>>> +
>>> + err = security_dentry_init_security(entry, mode, &entry->d_name,
>>> + &name, &ctx, &ctxlen);
>>> + if (err) {
>>> + if (err != -EOPNOTSUPP)
>>> + goto out_err;
>>> + /* No LSM is supporting this security hook. Ignore error */
>>> + err = 0;
>>> + ctxlen = 0;
>>> + }
>>> +
>>> + if (ctxlen > 0) {
>>> + void *ptr;
>>> +
>>> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
>>> + strlen(name) + ctxlen + 1;
>>> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> + if (!full_ctx) {
>>> + err = -ENOMEM;
>>> + kfree(ctx);
>>> + goto out_err;
>>> + }
>>> +
>>> + ptr = full_ctx;
>>> + fsecctxs = (struct fuse_secctxs*) ptr;
>>> + fsecctxs->nr_secctx = 1;
>>> + ptr += sizeof(*fsecctxs);
>>> +
>>> + fsecctx = (struct fuse_secctx*) ptr;
>>> + fsecctx->size = ctxlen;
>>> + ptr += sizeof(*fsecctx);
>>> +
>>> + strcpy(ptr, name);
>>> + ptr += strlen(name) + 1;
>>> + memcpy(ptr, ctx, ctxlen);
>>> + kfree(ctx);
>>> + } else {
>>> + full_ctxlen = sizeof(*fsecctxs);
>>> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> + if (!full_ctx) {
>>> + err = -ENOMEM;
>>> + goto out_err;
>>> + }
>>> + }
>>> +
>>> + *security_ctxlen = full_ctxlen;
>>> + *security_ctx = full_ctx;
>>> +out_err:
>>> + return err;
>>> +}
>>> +
>>> /*
>>> * Atomic create+open operation
>>> *
>>> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>> struct fuse_entry_out outentry;
>>> struct fuse_inode *fi;
>>> struct fuse_file *ff;
>>> + void *security_ctx = NULL;
>>> + u32 security_ctxlen;
>>>
>>> /* Userspace expects S_IFREG in create mode */
>>> BUG_ON((mode & S_IFMT) != S_IFREG);
>>> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>> args.out_args[0].value = &outentry;
>>> args.out_args[1].size = sizeof(outopen);
>>> args.out_args[1].value = &outopen;
>>> +
>>> + if (fm->fc->init_security) {
>>> + err = get_security_context(entry, mode, &security_ctx,
>>> + &security_ctxlen);
>>> + if (err)
>>> + goto out_put_forget_req;
>>> +
>>> + args.in_numargs = 3;
>>> + args.in_args[2].size = security_ctxlen;
>>> + args.in_args[2].value = security_ctx;
>>> + }
>>> +
>>> err = fuse_simple_request(fm, &args);
>>> if (err)
>>> goto out_free_ff;
>>> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>
>>> out_free_ff:
>>> fuse_file_free(ff);
>>> + kfree(security_ctx);
>>> out_put_forget_req:
>>> kfree(forget);
>>> out_err:
>>> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>>> */
>>> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>> struct inode *dir, struct dentry *entry,
>>> - umode_t mode)
>>> + umode_t mode, bool init_security)
>>> {
>>> struct fuse_entry_out outarg;
>>> struct inode *inode;
>>> struct dentry *d;
>>> int err;
>>> struct fuse_forget_link *forget;
>>> + void *security_ctx = NULL;
>>> + u32 security_ctxlen = 0;
>>>
>>> if (fuse_is_bad(dir))
>>> return -EIO;
>>> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>> args->out_numargs = 1;
>>> args->out_args[0].size = sizeof(outarg);
>>> args->out_args[0].value = &outarg;
>>> +
>>> + if (init_security) {
>>> + unsigned short idx = args->in_numargs;
>>> +
>>> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
>>> + err = -ENOMEM;
>>> + goto out_put_forget_req;
>>> + }
>>> +
>>> + err = get_security_context(entry, mode, &security_ctx,
>>> + &security_ctxlen);
>>> + if (err)
>>> + goto out_put_forget_req;
>>> +
>>> + if (security_ctxlen > 0) {
>>> + args->in_args[idx].size = security_ctxlen;
>>> + args->in_args[idx].value = security_ctx;
>>> + args->in_numargs++;
>>> + }
>>> + }
>>> +
>>> err = fuse_simple_request(fm, args);
>>> + kfree(security_ctx);
>>> if (err)
>>> goto out_put_forget_req;
>>>
>>> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = entry->d_name.len + 1;
>>> args.in_args[1].value = entry->d_name.name;
>>> - return create_new_entry(fm, &args, dir, entry, mode);
>>> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>>> }
>>>
>>> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = entry->d_name.len + 1;
>>> args.in_args[1].value = entry->d_name.name;
>>> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
>>> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
>>> + fm->fc->init_security);
>>> }
>>>
>>> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = entry->d_name.name;
>>> args.in_args[1].size = len;
>>> args.in_args[1].value = link;
>>> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
>>> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
>>> + fm->fc->init_security);
>>> }
>>>
>>> void fuse_update_ctime(struct inode *inode)
>>> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = newent->d_name.len + 1;
>>> args.in_args[1].value = newent->d_name.name;
>>> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
>>> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
>>> + false);
>>> /* Contrary to "normal" filesystems it can happen that link
>>> makes two "logical" inodes point to the same "physical"
>>> inode. We invalidate the attributes of the old one, so it
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 319596df5dc6..885f34f9967f 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -765,6 +765,9 @@ struct fuse_conn {
>>> /* Propagate syncfs() to server */
>>> unsigned int sync_fs:1;
>>>
>>> + /* Initialize security xattrs when creating a new inode */
>>> + unsigned int init_security:1;
>>> +
>>> /** The number of requests waiting for completion */
>>> atomic_t num_waiting;
>>>
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 36cd03114b6d..343bc9cfbd92 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>> }
>>> if (arg->flags & FUSE_SETXATTR_EXT)
>>> fc->setxattr_ext = 1;
>>> + if (arg->flags & FUSE_SECURITY_CTX)
>>> + fc->init_security = 1;
>>> } else {
>>> ra_pages = fc->max_read / PAGE_SIZE;
>>> fc->no_lock = 1;
>>> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>>> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>>> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>>> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
>>> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
>>> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>>> #ifdef CONFIG_FUSE_DAX
>>> if (fm->fc->dax)
>>> ia->in.flags |= FUSE_MAP_ALIGNMENT;
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index 2fe54c80051a..b31a0f79fde8 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
>>> uint64_t padding;
>>> };
>>>
>>> +/*
>>> + * For each security context, send fuse_secctx with size of security context
>>> + * fuse_secctx will be followed by security context name and this in turn
>>> + * will be followed by actual context label.
>>> + * fuse_secctx, name, context
>>> + * */
>>> +struct fuse_secctx {
>>> + uint32_t size;
>>> + uint32_t padding;
>>> +};
>>> +
>>> +/*
>>> + * Contains the information about how many fuse_secctx structures are being
>>> + * sent.
>>> + */
>>> +struct fuse_secctxs {
>>> + uint32_t nr_secctx;
>>> + uint32_t padding;
>>> +};
>>> +
>>> #endif /* _LINUX_FUSE_H */
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-10-12 18:41 ` Casey Schaufler
0 siblings, 0 replies; 29+ messages in thread
From: Casey Schaufler @ 2021-10-12 18:41 UTC (permalink / raw)
To: Vivek Goyal
Cc: miklos, selinux, Casey Schaufler, stephen.smalley.work, omosnace,
virtio-fs, linux-security-module, linux-fsdevel
On 10/12/2021 11:34 AM, Vivek Goyal wrote:
> On Tue, Oct 12, 2021 at 11:24:23AM -0700, Casey Schaufler wrote:
>> On 10/12/2021 11:06 AM, Vivek Goyal wrote:
>>> When a new inode is created, send its security context to server along
>>> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
>>> This gives server an opportunity to create new file and set security
>>> context (possibly atomically). In all the configurations it might not
>>> be possible to set context atomically.
>>>
>>> Like nfs and ceph, use security_dentry_init_security() to dermine security
>>> context of inode and send it with create, mkdir, mknod, and symlink requests.
>>>
>>> Following is the information sent to server.
>>>
>>> - struct fuse_secctxs.
>>> This contains total number of security contexts being sent.
>>>
>>> - struct fuse_secctx.
>>> This contains total size of security context which follows this structure.
>>> There is one fuse_secctx instance per security context.
>>>
>>> - xattr name string.
>>> This string represents name of xattr which should be used while setting
>>> security context. As of now it is hardcoded to "security.selinux".
>> Where is the name hardcoded? I looks as if you're getting the attribute
>> name along with the value from security_dentry_init_security().
> Sorry, I copied pasted this description from V1 where I was hardcoding
> the name to "security.selinux".
That's what I suspected. Thanks.
> But V2 got rid of that hardcoding and
> that's why this patch series is dependent on the other patch which
> modifies security_dentry_init_security() signature.
>
> https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c@redhat.com/
>
> Thanks
> Vivek
>
>>> - security context.
>>> This is the actual security context whose size is specified in fuse_secctx
>>> struct.
>>>
>>> This patch is modified version of patch from
>>> Chirantan Ekbote <chirantan@chromium.org>
>>>
>>> v2:
>>> - Added "fuse_secctxs" structure where one can specify how many security
>>> contexts are being sent. This can be useful down the line if we
>>> have more than one security contexts being set.
>>>
>>> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
>>> ---
>>> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
>>> fs/fuse/fuse_i.h | 3 +
>>> fs/fuse/inode.c | 4 +-
>>> include/uapi/linux/fuse.h | 20 +++++++
>>> 4 files changed, 136 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>> index d9b977c0f38d..ce62593a61f9 100644
>>> --- a/fs/fuse/dir.c
>>> +++ b/fs/fuse/dir.c
>>> @@ -17,6 +17,9 @@
>>> #include <linux/xattr.h>
>>> #include <linux/iversion.h>
>>> #include <linux/posix_acl.h>
>>> +#include <linux/security.h>
>>> +#include <linux/types.h>
>>> +#include <linux/kernel.h>
>>>
>>> static void fuse_advise_use_readdirplus(struct inode *dir)
>>> {
>>> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
>>> return ERR_PTR(err);
>>> }
>>>
>>> +static int get_security_context(struct dentry *entry, umode_t mode,
>>> + void **security_ctx, u32 *security_ctxlen)
>>> +{
>>> + struct fuse_secctx *fsecctx;
>>> + struct fuse_secctxs *fsecctxs;
>>> + void *ctx, *full_ctx;
>>> + u32 ctxlen, full_ctxlen;
>>> + int err = 0;
>>> + const char *name;
>>> +
>>> + err = security_dentry_init_security(entry, mode, &entry->d_name,
>>> + &name, &ctx, &ctxlen);
>>> + if (err) {
>>> + if (err != -EOPNOTSUPP)
>>> + goto out_err;
>>> + /* No LSM is supporting this security hook. Ignore error */
>>> + err = 0;
>>> + ctxlen = 0;
>>> + }
>>> +
>>> + if (ctxlen > 0) {
>>> + void *ptr;
>>> +
>>> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
>>> + strlen(name) + ctxlen + 1;
>>> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> + if (!full_ctx) {
>>> + err = -ENOMEM;
>>> + kfree(ctx);
>>> + goto out_err;
>>> + }
>>> +
>>> + ptr = full_ctx;
>>> + fsecctxs = (struct fuse_secctxs*) ptr;
>>> + fsecctxs->nr_secctx = 1;
>>> + ptr += sizeof(*fsecctxs);
>>> +
>>> + fsecctx = (struct fuse_secctx*) ptr;
>>> + fsecctx->size = ctxlen;
>>> + ptr += sizeof(*fsecctx);
>>> +
>>> + strcpy(ptr, name);
>>> + ptr += strlen(name) + 1;
>>> + memcpy(ptr, ctx, ctxlen);
>>> + kfree(ctx);
>>> + } else {
>>> + full_ctxlen = sizeof(*fsecctxs);
>>> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>>> + if (!full_ctx) {
>>> + err = -ENOMEM;
>>> + goto out_err;
>>> + }
>>> + }
>>> +
>>> + *security_ctxlen = full_ctxlen;
>>> + *security_ctx = full_ctx;
>>> +out_err:
>>> + return err;
>>> +}
>>> +
>>> /*
>>> * Atomic create+open operation
>>> *
>>> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>> struct fuse_entry_out outentry;
>>> struct fuse_inode *fi;
>>> struct fuse_file *ff;
>>> + void *security_ctx = NULL;
>>> + u32 security_ctxlen;
>>>
>>> /* Userspace expects S_IFREG in create mode */
>>> BUG_ON((mode & S_IFMT) != S_IFREG);
>>> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>> args.out_args[0].value = &outentry;
>>> args.out_args[1].size = sizeof(outopen);
>>> args.out_args[1].value = &outopen;
>>> +
>>> + if (fm->fc->init_security) {
>>> + err = get_security_context(entry, mode, &security_ctx,
>>> + &security_ctxlen);
>>> + if (err)
>>> + goto out_put_forget_req;
>>> +
>>> + args.in_numargs = 3;
>>> + args.in_args[2].size = security_ctxlen;
>>> + args.in_args[2].value = security_ctx;
>>> + }
>>> +
>>> err = fuse_simple_request(fm, &args);
>>> if (err)
>>> goto out_free_ff;
>>> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>>>
>>> out_free_ff:
>>> fuse_file_free(ff);
>>> + kfree(security_ctx);
>>> out_put_forget_req:
>>> kfree(forget);
>>> out_err:
>>> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>>> */
>>> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>> struct inode *dir, struct dentry *entry,
>>> - umode_t mode)
>>> + umode_t mode, bool init_security)
>>> {
>>> struct fuse_entry_out outarg;
>>> struct inode *inode;
>>> struct dentry *d;
>>> int err;
>>> struct fuse_forget_link *forget;
>>> + void *security_ctx = NULL;
>>> + u32 security_ctxlen = 0;
>>>
>>> if (fuse_is_bad(dir))
>>> return -EIO;
>>> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
>>> args->out_numargs = 1;
>>> args->out_args[0].size = sizeof(outarg);
>>> args->out_args[0].value = &outarg;
>>> +
>>> + if (init_security) {
>>> + unsigned short idx = args->in_numargs;
>>> +
>>> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
>>> + err = -ENOMEM;
>>> + goto out_put_forget_req;
>>> + }
>>> +
>>> + err = get_security_context(entry, mode, &security_ctx,
>>> + &security_ctxlen);
>>> + if (err)
>>> + goto out_put_forget_req;
>>> +
>>> + if (security_ctxlen > 0) {
>>> + args->in_args[idx].size = security_ctxlen;
>>> + args->in_args[idx].value = security_ctx;
>>> + args->in_numargs++;
>>> + }
>>> + }
>>> +
>>> err = fuse_simple_request(fm, args);
>>> + kfree(security_ctx);
>>> if (err)
>>> goto out_put_forget_req;
>>>
>>> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = entry->d_name.len + 1;
>>> args.in_args[1].value = entry->d_name.name;
>>> - return create_new_entry(fm, &args, dir, entry, mode);
>>> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
>>> }
>>>
>>> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = entry->d_name.len + 1;
>>> args.in_args[1].value = entry->d_name.name;
>>> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
>>> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
>>> + fm->fc->init_security);
>>> }
>>>
>>> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
>>> args.in_args[0].value = entry->d_name.name;
>>> args.in_args[1].size = len;
>>> args.in_args[1].value = link;
>>> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
>>> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
>>> + fm->fc->init_security);
>>> }
>>>
>>> void fuse_update_ctime(struct inode *inode)
>>> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
>>> args.in_args[0].value = &inarg;
>>> args.in_args[1].size = newent->d_name.len + 1;
>>> args.in_args[1].value = newent->d_name.name;
>>> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
>>> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
>>> + false);
>>> /* Contrary to "normal" filesystems it can happen that link
>>> makes two "logical" inodes point to the same "physical"
>>> inode. We invalidate the attributes of the old one, so it
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 319596df5dc6..885f34f9967f 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -765,6 +765,9 @@ struct fuse_conn {
>>> /* Propagate syncfs() to server */
>>> unsigned int sync_fs:1;
>>>
>>> + /* Initialize security xattrs when creating a new inode */
>>> + unsigned int init_security:1;
>>> +
>>> /** The number of requests waiting for completion */
>>> atomic_t num_waiting;
>>>
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 36cd03114b6d..343bc9cfbd92 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>> }
>>> if (arg->flags & FUSE_SETXATTR_EXT)
>>> fc->setxattr_ext = 1;
>>> + if (arg->flags & FUSE_SECURITY_CTX)
>>> + fc->init_security = 1;
>>> } else {
>>> ra_pages = fc->max_read / PAGE_SIZE;
>>> fc->no_lock = 1;
>>> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
>>> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
>>> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
>>> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
>>> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
>>> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
>>> #ifdef CONFIG_FUSE_DAX
>>> if (fm->fc->dax)
>>> ia->in.flags |= FUSE_MAP_ALIGNMENT;
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index 2fe54c80051a..b31a0f79fde8 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
>>> uint64_t padding;
>>> };
>>>
>>> +/*
>>> + * For each security context, send fuse_secctx with size of security context
>>> + * fuse_secctx will be followed by security context name and this in turn
>>> + * will be followed by actual context label.
>>> + * fuse_secctx, name, context
>>> + * */
>>> +struct fuse_secctx {
>>> + uint32_t size;
>>> + uint32_t padding;
>>> +};
>>> +
>>> +/*
>>> + * Contains the information about how many fuse_secctx structures are being
>>> + * sent.
>>> + */
>>> +struct fuse_secctxs {
>>> + uint32_t nr_secctx;
>>> + uint32_t padding;
>>> +};
>>> +
>>> #endif /* _LINUX_FUSE_H */
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-12 18:06 ` [Virtio-fs] " Vivek Goyal
(?)
(?)
@ 2021-10-13 4:04 ` kernel test robot
2021-10-13 12:50 ` Vivek Goyal
-1 siblings, 1 reply; 29+ messages in thread
From: kernel test robot @ 2021-10-13 4:04 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5648 bytes --]
Hi Vivek,
I love your patch! Yet something to improve:
[auto build test ERROR on v5.15-rc5]
[also build test ERROR on next-20211012]
[cannot apply to mszeredi-fuse/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
base: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
config: arc-randconfig-r043-20211012 (attached as .config)
compiler: arc-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3671f69d52ea6521c521ba6052be8e1b07e19ef7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
git checkout 3671f69d52ea6521c521ba6052be8e1b07e19ef7
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
fs/fuse/dir.c: In function 'get_security_context':
>> fs/fuse/dir.c:473:45: error: passing argument 4 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
473 | &name, &ctx, &ctxlen);
| ^~~~~
| |
| const char **
In file included from include/linux/fs_context.h:14,
from fs/fuse/dir.c:13:
include/linux/security.h:742:57: note: expected 'void **' but argument is of type 'const char **'
742 | void **ctx,
| ~~~~~~~^~~
fs/fuse/dir.c:473:52: error: passing argument 5 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
473 | &name, &ctx, &ctxlen);
| ^~~~
| |
| void **
In file included from include/linux/fs_context.h:14,
from fs/fuse/dir.c:13:
include/linux/security.h:743:55: note: expected 'u32 *' {aka 'unsigned int *'} but argument is of type 'void **'
743 | u32 *ctxlen)
| ~~~~~^~~~~~
>> fs/fuse/dir.c:472:15: error: too many arguments to function 'security_dentry_init_security'
472 | err = security_dentry_init_security(entry, mode, &entry->d_name,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from include/linux/fs_context.h:14,
from fs/fuse/dir.c:13:
include/linux/security.h:739:19: note: declared here
739 | static inline int security_dentry_init_security(struct dentry *dentry,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/security_dentry_init_security +473 fs/fuse/dir.c
461
462 static int get_security_context(struct dentry *entry, umode_t mode,
463 void **security_ctx, u32 *security_ctxlen)
464 {
465 struct fuse_secctx *fsecctx;
466 struct fuse_secctxs *fsecctxs;
467 void *ctx, *full_ctx;
468 u32 ctxlen, full_ctxlen;
469 int err = 0;
470 const char *name;
471
> 472 err = security_dentry_init_security(entry, mode, &entry->d_name,
> 473 &name, &ctx, &ctxlen);
474 if (err) {
475 if (err != -EOPNOTSUPP)
476 goto out_err;
477 /* No LSM is supporting this security hook. Ignore error */
478 err = 0;
479 ctxlen = 0;
480 }
481
482 if (ctxlen > 0) {
483 void *ptr;
484
485 full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
486 strlen(name) + ctxlen + 1;
487 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
488 if (!full_ctx) {
489 err = -ENOMEM;
490 kfree(ctx);
491 goto out_err;
492 }
493
494 ptr = full_ctx;
495 fsecctxs = (struct fuse_secctxs*) ptr;
496 fsecctxs->nr_secctx = 1;
497 ptr += sizeof(*fsecctxs);
498
499 fsecctx = (struct fuse_secctx*) ptr;
500 fsecctx->size = ctxlen;
501 ptr += sizeof(*fsecctx);
502
503 strcpy(ptr, name);
504 ptr += strlen(name) + 1;
505 memcpy(ptr, ctx, ctxlen);
506 kfree(ctx);
507 } else {
508 full_ctxlen = sizeof(*fsecctxs);
509 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
510 if (!full_ctx) {
511 err = -ENOMEM;
512 goto out_err;
513 }
514 }
515
516 *security_ctxlen = full_ctxlen;
517 *security_ctx = full_ctx;
518 out_err:
519 return err;
520 }
521
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 33898 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-13 4:04 ` kernel test robot
@ 2021-10-13 12:50 ` Vivek Goyal
2021-10-15 0:39 ` Chen, Rong A
0 siblings, 1 reply; 29+ messages in thread
From: Vivek Goyal @ 2021-10-13 12:50 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6376 bytes --]
On Wed, Oct 13, 2021 at 12:04:36PM +0800, kernel test robot wrote:
> Hi Vivek,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on v5.15-rc5]
> [also build test ERROR on next-20211012]
> [cannot apply to mszeredi-fuse/for-next]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
Actually, this patch series is dependent on another patch which is
posted on mailing list but has not been merged yet.
[PATCH v2] security: Return xattr name from security_dentry_init_security()
https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c(a)redhat.com/
So if you apply this patch first and then apply the patch series, it
should apply cleanly and compile.
Thanks
Vivek
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
>
> url: https://github.com/0day-ci/linux/commits/Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
> base: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
> config: arc-randconfig-r043-20211012 (attached as .config)
> compiler: arc-elf-gcc (GCC) 11.2.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/0day-ci/linux/commit/3671f69d52ea6521c521ba6052be8e1b07e19ef7
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
> git checkout 3671f69d52ea6521c521ba6052be8e1b07e19ef7
> # save the attached .config to linux build tree
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> fs/fuse/dir.c: In function 'get_security_context':
> >> fs/fuse/dir.c:473:45: error: passing argument 4 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 473 | &name, &ctx, &ctxlen);
> | ^~~~~
> | |
> | const char **
> In file included from include/linux/fs_context.h:14,
> from fs/fuse/dir.c:13:
> include/linux/security.h:742:57: note: expected 'void **' but argument is of type 'const char **'
> 742 | void **ctx,
> | ~~~~~~~^~~
> fs/fuse/dir.c:473:52: error: passing argument 5 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 473 | &name, &ctx, &ctxlen);
> | ^~~~
> | |
> | void **
> In file included from include/linux/fs_context.h:14,
> from fs/fuse/dir.c:13:
> include/linux/security.h:743:55: note: expected 'u32 *' {aka 'unsigned int *'} but argument is of type 'void **'
> 743 | u32 *ctxlen)
> | ~~~~~^~~~~~
> >> fs/fuse/dir.c:472:15: error: too many arguments to function 'security_dentry_init_security'
> 472 | err = security_dentry_init_security(entry, mode, &entry->d_name,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from include/linux/fs_context.h:14,
> from fs/fuse/dir.c:13:
> include/linux/security.h:739:19: note: declared here
> 739 | static inline int security_dentry_init_security(struct dentry *dentry,
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: some warnings being treated as errors
>
>
> vim +/security_dentry_init_security +473 fs/fuse/dir.c
>
> 461
> 462 static int get_security_context(struct dentry *entry, umode_t mode,
> 463 void **security_ctx, u32 *security_ctxlen)
> 464 {
> 465 struct fuse_secctx *fsecctx;
> 466 struct fuse_secctxs *fsecctxs;
> 467 void *ctx, *full_ctx;
> 468 u32 ctxlen, full_ctxlen;
> 469 int err = 0;
> 470 const char *name;
> 471
> > 472 err = security_dentry_init_security(entry, mode, &entry->d_name,
> > 473 &name, &ctx, &ctxlen);
> 474 if (err) {
> 475 if (err != -EOPNOTSUPP)
> 476 goto out_err;
> 477 /* No LSM is supporting this security hook. Ignore error */
> 478 err = 0;
> 479 ctxlen = 0;
> 480 }
> 481
> 482 if (ctxlen > 0) {
> 483 void *ptr;
> 484
> 485 full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> 486 strlen(name) + ctxlen + 1;
> 487 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> 488 if (!full_ctx) {
> 489 err = -ENOMEM;
> 490 kfree(ctx);
> 491 goto out_err;
> 492 }
> 493
> 494 ptr = full_ctx;
> 495 fsecctxs = (struct fuse_secctxs*) ptr;
> 496 fsecctxs->nr_secctx = 1;
> 497 ptr += sizeof(*fsecctxs);
> 498
> 499 fsecctx = (struct fuse_secctx*) ptr;
> 500 fsecctx->size = ctxlen;
> 501 ptr += sizeof(*fsecctx);
> 502
> 503 strcpy(ptr, name);
> 504 ptr += strlen(name) + 1;
> 505 memcpy(ptr, ctx, ctxlen);
> 506 kfree(ctx);
> 507 } else {
> 508 full_ctxlen = sizeof(*fsecctxs);
> 509 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> 510 if (!full_ctx) {
> 511 err = -ENOMEM;
> 512 goto out_err;
> 513 }
> 514 }
> 515
> 516 *security_ctxlen = full_ctxlen;
> 517 *security_ctx = full_ctx;
> 518 out_err:
> 519 return err;
> 520 }
> 521
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-13 12:50 ` Vivek Goyal
@ 2021-10-15 0:39 ` Chen, Rong A
0 siblings, 0 replies; 29+ messages in thread
From: Chen, Rong A @ 2021-10-15 0:39 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6973 bytes --]
On 10/13/2021 8:50 PM, Vivek Goyal wrote:
> On Wed, Oct 13, 2021 at 12:04:36PM +0800, kernel test robot wrote:
>> Hi Vivek,
>>
>> I love your patch! Yet something to improve:
>>
>> [auto build test ERROR on v5.15-rc5]
>> [also build test ERROR on next-20211012]
>> [cannot apply to mszeredi-fuse/for-next]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>
> Actually, this patch series is dependent on another patch which is
> posted on mailing list but has not been merged yet.
>
> [PATCH v2] security: Return xattr name from security_dentry_init_security()
> https://lore.kernel.org/linux-fsdevel/YWWMO%2FZDrvDZ5X4c(a)redhat.com/
>
> So if you apply this patch first and then apply the patch series, it
> should apply cleanly and compile.
Hi Vivek,
Thanks for the explanation, the bot is not clever on such case, we'll
take a look.
Best Regards,
Rong Chen
>
> Thanks
> Vivek
>
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch]
>>
>> url: https://github.com/0day-ci/linux/commits/Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
>> base: 64570fbc14f8d7cb3fe3995f20e26bc25ce4b2cc
>> config: arc-randconfig-r043-20211012 (attached as .config)
>> compiler: arc-elf-gcc (GCC) 11.2.0
>> reproduce (this is a W=1 build):
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> # https://github.com/0day-ci/linux/commit/3671f69d52ea6521c521ba6052be8e1b07e19ef7
>> git remote add linux-review https://github.com/0day-ci/linux
>> git fetch --no-tags linux-review Vivek-Goyal/fuse-Send-file-inode-security-context-during-creation/20211013-020827
>> git checkout 3671f69d52ea6521c521ba6052be8e1b07e19ef7
>> # save the attached .config to linux build tree
>> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=arc
>>
>> If you fix the issue, kindly add following tag as appropriate
>> Reported-by: kernel test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>> fs/fuse/dir.c: In function 'get_security_context':
>>>> fs/fuse/dir.c:473:45: error: passing argument 4 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
>> 473 | &name, &ctx, &ctxlen);
>> | ^~~~~
>> | |
>> | const char **
>> In file included from include/linux/fs_context.h:14,
>> from fs/fuse/dir.c:13:
>> include/linux/security.h:742:57: note: expected 'void **' but argument is of type 'const char **'
>> 742 | void **ctx,
>> | ~~~~~~~^~~
>> fs/fuse/dir.c:473:52: error: passing argument 5 of 'security_dentry_init_security' from incompatible pointer type [-Werror=incompatible-pointer-types]
>> 473 | &name, &ctx, &ctxlen);
>> | ^~~~
>> | |
>> | void **
>> In file included from include/linux/fs_context.h:14,
>> from fs/fuse/dir.c:13:
>> include/linux/security.h:743:55: note: expected 'u32 *' {aka 'unsigned int *'} but argument is of type 'void **'
>> 743 | u32 *ctxlen)
>> | ~~~~~^~~~~~
>>>> fs/fuse/dir.c:472:15: error: too many arguments to function 'security_dentry_init_security'
>> 472 | err = security_dentry_init_security(entry, mode, &entry->d_name,
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from include/linux/fs_context.h:14,
>> from fs/fuse/dir.c:13:
>> include/linux/security.h:739:19: note: declared here
>> 739 | static inline int security_dentry_init_security(struct dentry *dentry,
>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> cc1: some warnings being treated as errors
>>
>>
>> vim +/security_dentry_init_security +473 fs/fuse/dir.c
>>
>> 461
>> 462 static int get_security_context(struct dentry *entry, umode_t mode,
>> 463 void **security_ctx, u32 *security_ctxlen)
>> 464 {
>> 465 struct fuse_secctx *fsecctx;
>> 466 struct fuse_secctxs *fsecctxs;
>> 467 void *ctx, *full_ctx;
>> 468 u32 ctxlen, full_ctxlen;
>> 469 int err = 0;
>> 470 const char *name;
>> 471
>> > 472 err = security_dentry_init_security(entry, mode, &entry->d_name,
>> > 473 &name, &ctx, &ctxlen);
>> 474 if (err) {
>> 475 if (err != -EOPNOTSUPP)
>> 476 goto out_err;
>> 477 /* No LSM is supporting this security hook. Ignore error */
>> 478 err = 0;
>> 479 ctxlen = 0;
>> 480 }
>> 481
>> 482 if (ctxlen > 0) {
>> 483 void *ptr;
>> 484
>> 485 full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
>> 486 strlen(name) + ctxlen + 1;
>> 487 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>> 488 if (!full_ctx) {
>> 489 err = -ENOMEM;
>> 490 kfree(ctx);
>> 491 goto out_err;
>> 492 }
>> 493
>> 494 ptr = full_ctx;
>> 495 fsecctxs = (struct fuse_secctxs*) ptr;
>> 496 fsecctxs->nr_secctx = 1;
>> 497 ptr += sizeof(*fsecctxs);
>> 498
>> 499 fsecctx = (struct fuse_secctx*) ptr;
>> 500 fsecctx->size = ctxlen;
>> 501 ptr += sizeof(*fsecctx);
>> 502
>> 503 strcpy(ptr, name);
>> 504 ptr += strlen(name) + 1;
>> 505 memcpy(ptr, ctx, ctxlen);
>> 506 kfree(ctx);
>> 507 } else {
>> 508 full_ctxlen = sizeof(*fsecctxs);
>> 509 full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
>> 510 if (!full_ctx) {
>> 511 err = -ENOMEM;
>> 512 goto out_err;
>> 513 }
>> 514 }
>> 515
>> 516 *security_ctxlen = full_ctxlen;
>> 517 *security_ctx = full_ctx;
>> 518 out_err:
>> 519 return err;
>> 520 }
>> 521
>>
>> ---
>> 0-DAY CI Kernel Test Service, Intel Corporation
>> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
>
> _______________________________________________
> kbuild-all mailing list -- kbuild-all(a)lists.01.org
> To unsubscribe send an email to kbuild-all-leave(a)lists.01.org
>
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-10-12 18:06 ` [Virtio-fs] " Vivek Goyal
@ 2021-11-02 14:00 ` Miklos Szeredi
-1 siblings, 0 replies; 29+ messages in thread
From: Miklos Szeredi @ 2021-11-02 14:00 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, SElinux list, LSM, virtio-fs-list,
Chirantan Ekbote, Stephen Smalley, Daniel J Walsh,
Casey Schaufler, Ondrej Mosnacek
On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctxs.
> This contains total number of security contexts being sent.
>
> - struct fuse_secctx.
> This contains total size of security context which follows this structure.
> There is one fuse_secctx instance per security context.
>
> - xattr name string.
> This string represents name of xattr which should be used while setting
> security context. As of now it is hardcoded to "security.selinux".
>
> - security context.
> This is the actual security context whose size is specified in fuse_secctx
> struct.
>
> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> v2:
> - Added "fuse_secctxs" structure where one can specify how many security
> contexts are being sent. This can be useful down the line if we
> have more than one security contexts being set.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> fs/fuse/fuse_i.h | 3 +
> fs/fuse/inode.c | 4 +-
> include/uapi/linux/fuse.h | 20 +++++++
> 4 files changed, 136 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..ce62593a61f9 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
> #include <linux/xattr.h>
> #include <linux/iversion.h>
> #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>
> static void fuse_advise_use_readdirplus(struct inode *dir)
> {
> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> return ERR_PTR(err);
> }
>
> +static int get_security_context(struct dentry *entry, umode_t mode,
> + void **security_ctx, u32 *security_ctxlen)
> +{
> + struct fuse_secctx *fsecctx;
> + struct fuse_secctxs *fsecctxs;
> + void *ctx, *full_ctx;
> + u32 ctxlen, full_ctxlen;
> + int err = 0;
> + const char *name;
> +
> + err = security_dentry_init_security(entry, mode, &entry->d_name,
> + &name, &ctx, &ctxlen);
> + if (err) {
> + if (err != -EOPNOTSUPP)
> + goto out_err;
> + /* No LSM is supporting this security hook. Ignore error */
> + err = 0;
> + ctxlen = 0;
> + }
> +
> + if (ctxlen > 0) {
> + void *ptr;
> +
> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> + strlen(name) + ctxlen + 1;
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + kfree(ctx);
> + goto out_err;
> + }
> +
> + ptr = full_ctx;
> + fsecctxs = (struct fuse_secctxs*) ptr;
> + fsecctxs->nr_secctx = 1;
> + ptr += sizeof(*fsecctxs);
> +
> + fsecctx = (struct fuse_secctx*) ptr;
> + fsecctx->size = ctxlen;
> + ptr += sizeof(*fsecctx);
> +
> + strcpy(ptr, name);
> + ptr += strlen(name) + 1;
> + memcpy(ptr, ctx, ctxlen);
> + kfree(ctx);
> + } else {
> + full_ctxlen = sizeof(*fsecctxs);
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> + }
> +
> + *security_ctxlen = full_ctxlen;
> + *security_ctx = full_ctx;
> +out_err:
> + return err;
> +}
> +
> /*
> * Atomic create+open operation
> *
> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> struct fuse_entry_out outentry;
> struct fuse_inode *fi;
> struct fuse_file *ff;
> + void *security_ctx = NULL;
> + u32 security_ctxlen;
>
> /* Userspace expects S_IFREG in create mode */
> BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> args.out_args[0].value = &outentry;
> args.out_args[1].size = sizeof(outopen);
> args.out_args[1].value = &outopen;
> +
> + if (fm->fc->init_security) {
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + args.in_numargs = 3;
> + args.in_args[2].size = security_ctxlen;
> + args.in_args[2].value = security_ctx;
> + }
> +
> err = fuse_simple_request(fm, &args);
> if (err)
> goto out_free_ff;
> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>
> out_free_ff:
> fuse_file_free(ff);
> + kfree(security_ctx);
> out_put_forget_req:
> kfree(forget);
> out_err:
> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> */
> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> struct inode *dir, struct dentry *entry,
> - umode_t mode)
> + umode_t mode, bool init_security)
> {
> struct fuse_entry_out outarg;
> struct inode *inode;
> struct dentry *d;
> int err;
> struct fuse_forget_link *forget;
> + void *security_ctx = NULL;
> + u32 security_ctxlen = 0;
>
> if (fuse_is_bad(dir))
> return -EIO;
> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> args->out_numargs = 1;
> args->out_args[0].size = sizeof(outarg);
> args->out_args[0].value = &outarg;
> +
> + if (init_security) {
Instead of a new arg to create_new_entry(), this could check
args.opcode != FUSE_LINK.
> + unsigned short idx = args->in_numargs;
> +
> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> + err = -ENOMEM;
> + goto out_put_forget_req;
> + }
> +
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + if (security_ctxlen > 0) {
This doesn't seem right. How would the server know if this is arg is missing?
I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
will always need to be added to the MK* requests.
> + args->in_args[idx].size = security_ctxlen;
> + args->in_args[idx].value = security_ctx;
> + args->in_numargs++;
> + }
> + }
> +
> err = fuse_simple_request(fm, args);
> + kfree(security_ctx);
> if (err)
> goto out_put_forget_req;
>
> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, mode);
> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> }
>
> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> + fm->fc->init_security);
> }
>
> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = entry->d_name.name;
> args.in_args[1].size = len;
> args.in_args[1].value = link;
> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> + fm->fc->init_security);
> }
>
> void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = newent->d_name.len + 1;
> args.in_args[1].value = newent->d_name.name;
> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> + false);
> /* Contrary to "normal" filesystems it can happen that link
> makes two "logical" inodes point to the same "physical"
> inode. We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
> /* Propagate syncfs() to server */
> unsigned int sync_fs:1;
>
> + /* Initialize security xattrs when creating a new inode */
> + unsigned int init_security:1;
> +
> /** The number of requests waiting for completion */
> atomic_t num_waiting;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> }
> if (arg->flags & FUSE_SETXATTR_EXT)
> fc->setxattr_ext = 1;
> + if (arg->flags & FUSE_SECURITY_CTX)
> + fc->init_security = 1;
> } else {
> ra_pages = fc->max_read / PAGE_SIZE;
> fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> #ifdef CONFIG_FUSE_DAX
> if (fm->fc->dax)
> ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..b31a0f79fde8 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
I don't see why the API changes are split between the first and the
second patch in the series. Please either move all API changes to
1/2 or fold 1/2 into this patch.
> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> uint64_t padding;
> };
>
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> + uint32_t size;
> + uint32_t padding;
> +};
> +
> +/*
> + * Contains the information about how many fuse_secctx structures are being
> + * sent.
> + */
> +struct fuse_secctxs {
> + uint32_t nr_secctx;
> + uint32_t padding;
> +};
The name of this struct is very confusing due to similarity with
fuse_secctx. How about "fuse_secctx_header"?
Also I'd add the total length of the security context (including the
header), otherwise further args would need to parse the security
context completely to find the position of the next arg. The
counterexample is null-terminated names; while parsing these is pretty
trivial, in hindsight it would probably have been better to add a
header to names as well.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-11-02 14:00 ` Miklos Szeredi
0 siblings, 0 replies; 29+ messages in thread
From: Miklos Szeredi @ 2021-11-02 14:00 UTC (permalink / raw)
To: Vivek Goyal
Cc: SElinux list, Casey Schaufler, Stephen Smalley, Ondrej Mosnacek,
virtio-fs-list, LSM, linux-fsdevel
On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> When a new inode is created, send its security context to server along
> with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> This gives server an opportunity to create new file and set security
> context (possibly atomically). In all the configurations it might not
> be possible to set context atomically.
>
> Like nfs and ceph, use security_dentry_init_security() to dermine security
> context of inode and send it with create, mkdir, mknod, and symlink requests.
>
> Following is the information sent to server.
>
> - struct fuse_secctxs.
> This contains total number of security contexts being sent.
>
> - struct fuse_secctx.
> This contains total size of security context which follows this structure.
> There is one fuse_secctx instance per security context.
>
> - xattr name string.
> This string represents name of xattr which should be used while setting
> security context. As of now it is hardcoded to "security.selinux".
>
> - security context.
> This is the actual security context whose size is specified in fuse_secctx
> struct.
>
> This patch is modified version of patch from
> Chirantan Ekbote <chirantan@chromium.org>
>
> v2:
> - Added "fuse_secctxs" structure where one can specify how many security
> contexts are being sent. This can be useful down the line if we
> have more than one security contexts being set.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
> fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> fs/fuse/fuse_i.h | 3 +
> fs/fuse/inode.c | 4 +-
> include/uapi/linux/fuse.h | 20 +++++++
> 4 files changed, 136 insertions(+), 6 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index d9b977c0f38d..ce62593a61f9 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -17,6 +17,9 @@
> #include <linux/xattr.h>
> #include <linux/iversion.h>
> #include <linux/posix_acl.h>
> +#include <linux/security.h>
> +#include <linux/types.h>
> +#include <linux/kernel.h>
>
> static void fuse_advise_use_readdirplus(struct inode *dir)
> {
> @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> return ERR_PTR(err);
> }
>
> +static int get_security_context(struct dentry *entry, umode_t mode,
> + void **security_ctx, u32 *security_ctxlen)
> +{
> + struct fuse_secctx *fsecctx;
> + struct fuse_secctxs *fsecctxs;
> + void *ctx, *full_ctx;
> + u32 ctxlen, full_ctxlen;
> + int err = 0;
> + const char *name;
> +
> + err = security_dentry_init_security(entry, mode, &entry->d_name,
> + &name, &ctx, &ctxlen);
> + if (err) {
> + if (err != -EOPNOTSUPP)
> + goto out_err;
> + /* No LSM is supporting this security hook. Ignore error */
> + err = 0;
> + ctxlen = 0;
> + }
> +
> + if (ctxlen > 0) {
> + void *ptr;
> +
> + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> + strlen(name) + ctxlen + 1;
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + kfree(ctx);
> + goto out_err;
> + }
> +
> + ptr = full_ctx;
> + fsecctxs = (struct fuse_secctxs*) ptr;
> + fsecctxs->nr_secctx = 1;
> + ptr += sizeof(*fsecctxs);
> +
> + fsecctx = (struct fuse_secctx*) ptr;
> + fsecctx->size = ctxlen;
> + ptr += sizeof(*fsecctx);
> +
> + strcpy(ptr, name);
> + ptr += strlen(name) + 1;
> + memcpy(ptr, ctx, ctxlen);
> + kfree(ctx);
> + } else {
> + full_ctxlen = sizeof(*fsecctxs);
> + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> + if (!full_ctx) {
> + err = -ENOMEM;
> + goto out_err;
> + }
> + }
> +
> + *security_ctxlen = full_ctxlen;
> + *security_ctx = full_ctx;
> +out_err:
> + return err;
> +}
> +
> /*
> * Atomic create+open operation
> *
> @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> struct fuse_entry_out outentry;
> struct fuse_inode *fi;
> struct fuse_file *ff;
> + void *security_ctx = NULL;
> + u32 security_ctxlen;
>
> /* Userspace expects S_IFREG in create mode */
> BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> args.out_args[0].value = &outentry;
> args.out_args[1].size = sizeof(outopen);
> args.out_args[1].value = &outopen;
> +
> + if (fm->fc->init_security) {
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + args.in_numargs = 3;
> + args.in_args[2].size = security_ctxlen;
> + args.in_args[2].value = security_ctx;
> + }
> +
> err = fuse_simple_request(fm, &args);
> if (err)
> goto out_free_ff;
> @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>
> out_free_ff:
> fuse_file_free(ff);
> + kfree(security_ctx);
> out_put_forget_req:
> kfree(forget);
> out_err:
> @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> */
> static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> struct inode *dir, struct dentry *entry,
> - umode_t mode)
> + umode_t mode, bool init_security)
> {
> struct fuse_entry_out outarg;
> struct inode *inode;
> struct dentry *d;
> int err;
> struct fuse_forget_link *forget;
> + void *security_ctx = NULL;
> + u32 security_ctxlen = 0;
>
> if (fuse_is_bad(dir))
> return -EIO;
> @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> args->out_numargs = 1;
> args->out_args[0].size = sizeof(outarg);
> args->out_args[0].value = &outarg;
> +
> + if (init_security) {
Instead of a new arg to create_new_entry(), this could check
args.opcode != FUSE_LINK.
> + unsigned short idx = args->in_numargs;
> +
> + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> + err = -ENOMEM;
> + goto out_put_forget_req;
> + }
> +
> + err = get_security_context(entry, mode, &security_ctx,
> + &security_ctxlen);
> + if (err)
> + goto out_put_forget_req;
> +
> + if (security_ctxlen > 0) {
This doesn't seem right. How would the server know if this is arg is missing?
I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
will always need to be added to the MK* requests.
> + args->in_args[idx].size = security_ctxlen;
> + args->in_args[idx].value = security_ctx;
> + args->in_numargs++;
> + }
> + }
> +
> err = fuse_simple_request(fm, args);
> + kfree(security_ctx);
> if (err)
> goto out_put_forget_req;
>
> @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, mode);
> + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> }
>
> static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = entry->d_name.len + 1;
> args.in_args[1].value = entry->d_name.name;
> - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> + fm->fc->init_security);
> }
>
> static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> args.in_args[0].value = entry->d_name.name;
> args.in_args[1].size = len;
> args.in_args[1].value = link;
> - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> + fm->fc->init_security);
> }
>
> void fuse_update_ctime(struct inode *inode)
> @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> args.in_args[0].value = &inarg;
> args.in_args[1].size = newent->d_name.len + 1;
> args.in_args[1].value = newent->d_name.name;
> - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> + false);
> /* Contrary to "normal" filesystems it can happen that link
> makes two "logical" inodes point to the same "physical"
> inode. We invalidate the attributes of the old one, so it
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 319596df5dc6..885f34f9967f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -765,6 +765,9 @@ struct fuse_conn {
> /* Propagate syncfs() to server */
> unsigned int sync_fs:1;
>
> + /* Initialize security xattrs when creating a new inode */
> + unsigned int init_security:1;
> +
> /** The number of requests waiting for completion */
> atomic_t num_waiting;
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 36cd03114b6d..343bc9cfbd92 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> }
> if (arg->flags & FUSE_SETXATTR_EXT)
> fc->setxattr_ext = 1;
> + if (arg->flags & FUSE_SECURITY_CTX)
> + fc->init_security = 1;
> } else {
> ra_pages = fc->max_read / PAGE_SIZE;
> fc->no_lock = 1;
> @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> #ifdef CONFIG_FUSE_DAX
> if (fm->fc->dax)
> ia->in.flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 2fe54c80051a..b31a0f79fde8 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
I don't see why the API changes are split between the first and the
second patch in the series. Please either move all API changes to
1/2 or fold 1/2 into this patch.
> @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> uint64_t padding;
> };
>
> +/*
> + * For each security context, send fuse_secctx with size of security context
> + * fuse_secctx will be followed by security context name and this in turn
> + * will be followed by actual context label.
> + * fuse_secctx, name, context
> + * */
> +struct fuse_secctx {
> + uint32_t size;
> + uint32_t padding;
> +};
> +
> +/*
> + * Contains the information about how many fuse_secctx structures are being
> + * sent.
> + */
> +struct fuse_secctxs {
> + uint32_t nr_secctx;
> + uint32_t padding;
> +};
The name of this struct is very confusing due to similarity with
fuse_secctx. How about "fuse_secctx_header"?
Also I'd add the total length of the security context (including the
header), otherwise further args would need to parse the security
context completely to find the position of the next arg. The
counterexample is null-terminated names; while parsing these is pretty
trivial, in hindsight it would probably have been better to add a
header to names as well.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-11-02 14:00 ` [Virtio-fs] " Miklos Szeredi
@ 2021-11-02 15:30 ` Vivek Goyal
-1 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-11-02 15:30 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, SElinux list, LSM, virtio-fs-list,
Chirantan Ekbote, Stephen Smalley, Daniel J Walsh,
Casey Schaufler, Ondrej Mosnacek
On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctxs.
> > This contains total number of security contexts being sent.
> >
> > - struct fuse_secctx.
> > This contains total size of security context which follows this structure.
> > There is one fuse_secctx instance per security context.
> >
> > - xattr name string.
> > This string represents name of xattr which should be used while setting
> > security context. As of now it is hardcoded to "security.selinux".
> >
> > - security context.
> > This is the actual security context whose size is specified in fuse_secctx
> > struct.
> >
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > v2:
> > - Added "fuse_secctxs" structure where one can specify how many security
> > contexts are being sent. This can be useful down the line if we
> > have more than one security contexts being set.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> > fs/fuse/fuse_i.h | 3 +
> > fs/fuse/inode.c | 4 +-
> > include/uapi/linux/fuse.h | 20 +++++++
> > 4 files changed, 136 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..ce62593a61f9 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> > #include <linux/xattr.h>
> > #include <linux/iversion.h>
> > #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >
> > static void fuse_advise_use_readdirplus(struct inode *dir)
> > {
> > @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> > return ERR_PTR(err);
> > }
> >
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > + void **security_ctx, u32 *security_ctxlen)
> > +{
> > + struct fuse_secctx *fsecctx;
> > + struct fuse_secctxs *fsecctxs;
> > + void *ctx, *full_ctx;
> > + u32 ctxlen, full_ctxlen;
> > + int err = 0;
> > + const char *name;
> > +
> > + err = security_dentry_init_security(entry, mode, &entry->d_name,
> > + &name, &ctx, &ctxlen);
> > + if (err) {
> > + if (err != -EOPNOTSUPP)
> > + goto out_err;
> > + /* No LSM is supporting this security hook. Ignore error */
> > + err = 0;
> > + ctxlen = 0;
> > + }
> > +
> > + if (ctxlen > 0) {
> > + void *ptr;
> > +
> > + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> > + strlen(name) + ctxlen + 1;
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + kfree(ctx);
> > + goto out_err;
> > + }
> > +
> > + ptr = full_ctx;
> > + fsecctxs = (struct fuse_secctxs*) ptr;
> > + fsecctxs->nr_secctx = 1;
> > + ptr += sizeof(*fsecctxs);
> > +
> > + fsecctx = (struct fuse_secctx*) ptr;
> > + fsecctx->size = ctxlen;
> > + ptr += sizeof(*fsecctx);
> > +
> > + strcpy(ptr, name);
> > + ptr += strlen(name) + 1;
> > + memcpy(ptr, ctx, ctxlen);
> > + kfree(ctx);
> > + } else {
> > + full_ctxlen = sizeof(*fsecctxs);
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > + }
> > +
> > + *security_ctxlen = full_ctxlen;
> > + *security_ctx = full_ctx;
> > +out_err:
> > + return err;
> > +}
> > +
> > /*
> > * Atomic create+open operation
> > *
> > @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > struct fuse_entry_out outentry;
> > struct fuse_inode *fi;
> > struct fuse_file *ff;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen;
> >
> > /* Userspace expects S_IFREG in create mode */
> > BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > args.out_args[0].value = &outentry;
> > args.out_args[1].size = sizeof(outopen);
> > args.out_args[1].value = &outopen;
> > +
> > + if (fm->fc->init_security) {
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + args.in_numargs = 3;
> > + args.in_args[2].size = security_ctxlen;
> > + args.in_args[2].value = security_ctx;
> > + }
> > +
> > err = fuse_simple_request(fm, &args);
> > if (err)
> > goto out_free_ff;
> > @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >
> > out_free_ff:
> > fuse_file_free(ff);
> > + kfree(security_ctx);
> > out_put_forget_req:
> > kfree(forget);
> > out_err:
> > @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > */
> > static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > struct inode *dir, struct dentry *entry,
> > - umode_t mode)
> > + umode_t mode, bool init_security)
> > {
> > struct fuse_entry_out outarg;
> > struct inode *inode;
> > struct dentry *d;
> > int err;
> > struct fuse_forget_link *forget;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen = 0;
> >
> > if (fuse_is_bad(dir))
> > return -EIO;
> > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > args->out_numargs = 1;
> > args->out_args[0].size = sizeof(outarg);
> > args->out_args[0].value = &outarg;
> > +
> > + if (init_security) {
>
Hi Miklos,
> Instead of a new arg to create_new_entry(), this could check
> args.opcode != FUSE_LINK.
Will do.
>
> > + unsigned short idx = args->in_numargs;
> > +
> > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > + err = -ENOMEM;
> > + goto out_put_forget_req;
> > + }
> > +
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + if (security_ctxlen > 0) {
>
> This doesn't seem right. How would the server know if this is arg is missing?
>
> I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> will always need to be added to the MK* requests.
Even for the case of FUSE_LINK request? I think I put this check because
FUSE_LINK is not sending secctx header. Other requests are appending
this header even if a security module is not loaded/enabled.
I guess it makes more sense to add secctx header even for FUSE_LINK
request. Just that header will mention 0 security contexts are
following. This will interface more uniform. I will make this change.
>
> > + args->in_args[idx].size = security_ctxlen;
> > + args->in_args[idx].value = security_ctx;
> > + args->in_numargs++;
> > + }
> > + }
> > +
> > err = fuse_simple_request(fm, args);
> > + kfree(security_ctx);
> > if (err)
> > goto out_put_forget_req;
> >
> > @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, mode);
> > + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> > }
> >
> > static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > + fm->fc->init_security);
> > }
> >
> > static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = entry->d_name.name;
> > args.in_args[1].size = len;
> > args.in_args[1].value = link;
> > - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > + fm->fc->init_security);
> > }
> >
> > void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = newent->d_name.len + 1;
> > args.in_args[1].value = newent->d_name.name;
> > - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > + false);
> > /* Contrary to "normal" filesystems it can happen that link
> > makes two "logical" inodes point to the same "physical"
> > inode. We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> > /* Propagate syncfs() to server */
> > unsigned int sync_fs:1;
> >
> > + /* Initialize security xattrs when creating a new inode */
> > + unsigned int init_security:1;
> > +
> > /** The number of requests waiting for completion */
> > atomic_t num_waiting;
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > }
> > if (arg->flags & FUSE_SETXATTR_EXT)
> > fc->setxattr_ext = 1;
> > + if (arg->flags & FUSE_SECURITY_CTX)
> > + fc->init_security = 1;
> > } else {
> > ra_pages = fc->max_read / PAGE_SIZE;
> > fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> > #ifdef CONFIG_FUSE_DAX
> > if (fm->fc->dax)
> > ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..b31a0f79fde8 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
>
> I don't see why the API changes are split between the first and the
> second patch in the series. Please either move all API changes to
> 1/2 or fold 1/2 into this patch.
I guess I will fold first patch into this one, so that there is only
one patch.
>
> > @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> > uint64_t padding;
> > };
> >
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > + uint32_t size;
> > + uint32_t padding;
> > +};
> > +
> > +/*
> > + * Contains the information about how many fuse_secctx structures are being
> > + * sent.
> > + */
> > +struct fuse_secctxs {
> > + uint32_t nr_secctx;
> > + uint32_t padding;
> > +};
>
> The name of this struct is very confusing due to similarity with
> fuse_secctx. How about "fuse_secctx_header"?
Sounds good. Will do.
>
> Also I'd add the total length of the security context (including the
> header), otherwise further args would need to parse the security
> context completely to find the position of the next arg. The
> counterexample is null-terminated names; while parsing these is pretty
> trivial, in hindsight it would probably have been better to add a
> header to names as well.
Agreed. Will add total length also to "fuse_secctx_header". That will
act as a strong check/verification mechanism as well as allow to
quickly skip to next args if one does not want to parse security contexts
for whatever reason.
Thanks
Vivek
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-11-02 15:30 ` Vivek Goyal
0 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-11-02 15:30 UTC (permalink / raw)
To: Miklos Szeredi
Cc: SElinux list, Casey Schaufler, Stephen Smalley, Ondrej Mosnacek,
virtio-fs-list, LSM, linux-fsdevel
On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > When a new inode is created, send its security context to server along
> > with creation request (FUSE_CREAT, FUSE_MKNOD, FUSE_MKDIR and FUSE_SYMLINK).
> > This gives server an opportunity to create new file and set security
> > context (possibly atomically). In all the configurations it might not
> > be possible to set context atomically.
> >
> > Like nfs and ceph, use security_dentry_init_security() to dermine security
> > context of inode and send it with create, mkdir, mknod, and symlink requests.
> >
> > Following is the information sent to server.
> >
> > - struct fuse_secctxs.
> > This contains total number of security contexts being sent.
> >
> > - struct fuse_secctx.
> > This contains total size of security context which follows this structure.
> > There is one fuse_secctx instance per security context.
> >
> > - xattr name string.
> > This string represents name of xattr which should be used while setting
> > security context. As of now it is hardcoded to "security.selinux".
> >
> > - security context.
> > This is the actual security context whose size is specified in fuse_secctx
> > struct.
> >
> > This patch is modified version of patch from
> > Chirantan Ekbote <chirantan@chromium.org>
> >
> > v2:
> > - Added "fuse_secctxs" structure where one can specify how many security
> > contexts are being sent. This can be useful down the line if we
> > have more than one security contexts being set.
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> > fs/fuse/dir.c | 115 ++++++++++++++++++++++++++++++++++++--
> > fs/fuse/fuse_i.h | 3 +
> > fs/fuse/inode.c | 4 +-
> > include/uapi/linux/fuse.h | 20 +++++++
> > 4 files changed, 136 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index d9b977c0f38d..ce62593a61f9 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -17,6 +17,9 @@
> > #include <linux/xattr.h>
> > #include <linux/iversion.h>
> > #include <linux/posix_acl.h>
> > +#include <linux/security.h>
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> >
> > static void fuse_advise_use_readdirplus(struct inode *dir)
> > {
> > @@ -456,6 +459,66 @@ static struct dentry *fuse_lookup(struct inode *dir, struct dentry *entry,
> > return ERR_PTR(err);
> > }
> >
> > +static int get_security_context(struct dentry *entry, umode_t mode,
> > + void **security_ctx, u32 *security_ctxlen)
> > +{
> > + struct fuse_secctx *fsecctx;
> > + struct fuse_secctxs *fsecctxs;
> > + void *ctx, *full_ctx;
> > + u32 ctxlen, full_ctxlen;
> > + int err = 0;
> > + const char *name;
> > +
> > + err = security_dentry_init_security(entry, mode, &entry->d_name,
> > + &name, &ctx, &ctxlen);
> > + if (err) {
> > + if (err != -EOPNOTSUPP)
> > + goto out_err;
> > + /* No LSM is supporting this security hook. Ignore error */
> > + err = 0;
> > + ctxlen = 0;
> > + }
> > +
> > + if (ctxlen > 0) {
> > + void *ptr;
> > +
> > + full_ctxlen = sizeof(*fsecctxs) + sizeof(*fsecctx) +
> > + strlen(name) + ctxlen + 1;
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + kfree(ctx);
> > + goto out_err;
> > + }
> > +
> > + ptr = full_ctx;
> > + fsecctxs = (struct fuse_secctxs*) ptr;
> > + fsecctxs->nr_secctx = 1;
> > + ptr += sizeof(*fsecctxs);
> > +
> > + fsecctx = (struct fuse_secctx*) ptr;
> > + fsecctx->size = ctxlen;
> > + ptr += sizeof(*fsecctx);
> > +
> > + strcpy(ptr, name);
> > + ptr += strlen(name) + 1;
> > + memcpy(ptr, ctx, ctxlen);
> > + kfree(ctx);
> > + } else {
> > + full_ctxlen = sizeof(*fsecctxs);
> > + full_ctx = kzalloc(full_ctxlen, GFP_KERNEL);
> > + if (!full_ctx) {
> > + err = -ENOMEM;
> > + goto out_err;
> > + }
> > + }
> > +
> > + *security_ctxlen = full_ctxlen;
> > + *security_ctx = full_ctx;
> > +out_err:
> > + return err;
> > +}
> > +
> > /*
> > * Atomic create+open operation
> > *
> > @@ -476,6 +539,8 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > struct fuse_entry_out outentry;
> > struct fuse_inode *fi;
> > struct fuse_file *ff;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen;
> >
> > /* Userspace expects S_IFREG in create mode */
> > BUG_ON((mode & S_IFMT) != S_IFREG);
> > @@ -517,6 +582,18 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > args.out_args[0].value = &outentry;
> > args.out_args[1].size = sizeof(outopen);
> > args.out_args[1].value = &outopen;
> > +
> > + if (fm->fc->init_security) {
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + args.in_numargs = 3;
> > + args.in_args[2].size = security_ctxlen;
> > + args.in_args[2].value = security_ctx;
> > + }
> > +
> > err = fuse_simple_request(fm, &args);
> > if (err)
> > goto out_free_ff;
> > @@ -554,6 +631,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >
> > out_free_ff:
> > fuse_file_free(ff);
> > + kfree(security_ctx);
> > out_put_forget_req:
> > kfree(forget);
> > out_err:
> > @@ -613,13 +691,15 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > */
> > static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > struct inode *dir, struct dentry *entry,
> > - umode_t mode)
> > + umode_t mode, bool init_security)
> > {
> > struct fuse_entry_out outarg;
> > struct inode *inode;
> > struct dentry *d;
> > int err;
> > struct fuse_forget_link *forget;
> > + void *security_ctx = NULL;
> > + u32 security_ctxlen = 0;
> >
> > if (fuse_is_bad(dir))
> > return -EIO;
> > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > args->out_numargs = 1;
> > args->out_args[0].size = sizeof(outarg);
> > args->out_args[0].value = &outarg;
> > +
> > + if (init_security) {
>
Hi Miklos,
> Instead of a new arg to create_new_entry(), this could check
> args.opcode != FUSE_LINK.
Will do.
>
> > + unsigned short idx = args->in_numargs;
> > +
> > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > + err = -ENOMEM;
> > + goto out_put_forget_req;
> > + }
> > +
> > + err = get_security_context(entry, mode, &security_ctx,
> > + &security_ctxlen);
> > + if (err)
> > + goto out_put_forget_req;
> > +
> > + if (security_ctxlen > 0) {
>
> This doesn't seem right. How would the server know if this is arg is missing?
>
> I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> will always need to be added to the MK* requests.
Even for the case of FUSE_LINK request? I think I put this check because
FUSE_LINK is not sending secctx header. Other requests are appending
this header even if a security module is not loaded/enabled.
I guess it makes more sense to add secctx header even for FUSE_LINK
request. Just that header will mention 0 security contexts are
following. This will interface more uniform. I will make this change.
>
> > + args->in_args[idx].size = security_ctxlen;
> > + args->in_args[idx].value = security_ctx;
> > + args->in_numargs++;
> > + }
> > + }
> > +
> > err = fuse_simple_request(fm, args);
> > + kfree(security_ctx);
> > if (err)
> > goto out_put_forget_req;
> >
> > @@ -691,7 +793,7 @@ static int fuse_mknod(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, mode);
> > + return create_new_entry(fm, &args, dir, entry, mode, fm->fc->init_security);
> > }
> >
> > static int fuse_create(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -719,7 +821,8 @@ static int fuse_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = entry->d_name.len + 1;
> > args.in_args[1].value = entry->d_name.name;
> > - return create_new_entry(fm, &args, dir, entry, S_IFDIR);
> > + return create_new_entry(fm, &args, dir, entry, S_IFDIR,
> > + fm->fc->init_security);
> > }
> >
> > static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > @@ -735,7 +838,8 @@ static int fuse_symlink(struct user_namespace *mnt_userns, struct inode *dir,
> > args.in_args[0].value = entry->d_name.name;
> > args.in_args[1].size = len;
> > args.in_args[1].value = link;
> > - return create_new_entry(fm, &args, dir, entry, S_IFLNK);
> > + return create_new_entry(fm, &args, dir, entry, S_IFLNK,
> > + fm->fc->init_security);
> > }
> >
> > void fuse_update_ctime(struct inode *inode)
> > @@ -915,7 +1019,8 @@ static int fuse_link(struct dentry *entry, struct inode *newdir,
> > args.in_args[0].value = &inarg;
> > args.in_args[1].size = newent->d_name.len + 1;
> > args.in_args[1].value = newent->d_name.name;
> > - err = create_new_entry(fm, &args, newdir, newent, inode->i_mode);
> > + err = create_new_entry(fm, &args, newdir, newent, inode->i_mode,
> > + false);
> > /* Contrary to "normal" filesystems it can happen that link
> > makes two "logical" inodes point to the same "physical"
> > inode. We invalidate the attributes of the old one, so it
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 319596df5dc6..885f34f9967f 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -765,6 +765,9 @@ struct fuse_conn {
> > /* Propagate syncfs() to server */
> > unsigned int sync_fs:1;
> >
> > + /* Initialize security xattrs when creating a new inode */
> > + unsigned int init_security:1;
> > +
> > /** The number of requests waiting for completion */
> > atomic_t num_waiting;
> >
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 36cd03114b6d..343bc9cfbd92 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1152,6 +1152,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > }
> > if (arg->flags & FUSE_SETXATTR_EXT)
> > fc->setxattr_ext = 1;
> > + if (arg->flags & FUSE_SECURITY_CTX)
> > + fc->init_security = 1;
> > } else {
> > ra_pages = fc->max_read / PAGE_SIZE;
> > fc->no_lock = 1;
> > @@ -1195,7 +1197,7 @@ void fuse_send_init(struct fuse_mount *fm)
> > FUSE_PARALLEL_DIROPS | FUSE_HANDLE_KILLPRIV | FUSE_POSIX_ACL |
> > FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
> > FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
> > - FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT;
> > + FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_SECURITY_CTX;
> > #ifdef CONFIG_FUSE_DAX
> > if (fm->fc->dax)
> > ia->in.flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 2fe54c80051a..b31a0f79fde8 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
>
> I don't see why the API changes are split between the first and the
> second patch in the series. Please either move all API changes to
> 1/2 or fold 1/2 into this patch.
I guess I will fold first patch into this one, so that there is only
one patch.
>
> > @@ -986,4 +986,24 @@ struct fuse_syncfs_in {
> > uint64_t padding;
> > };
> >
> > +/*
> > + * For each security context, send fuse_secctx with size of security context
> > + * fuse_secctx will be followed by security context name and this in turn
> > + * will be followed by actual context label.
> > + * fuse_secctx, name, context
> > + * */
> > +struct fuse_secctx {
> > + uint32_t size;
> > + uint32_t padding;
> > +};
> > +
> > +/*
> > + * Contains the information about how many fuse_secctx structures are being
> > + * sent.
> > + */
> > +struct fuse_secctxs {
> > + uint32_t nr_secctx;
> > + uint32_t padding;
> > +};
>
> The name of this struct is very confusing due to similarity with
> fuse_secctx. How about "fuse_secctx_header"?
Sounds good. Will do.
>
> Also I'd add the total length of the security context (including the
> header), otherwise further args would need to parse the security
> context completely to find the position of the next arg. The
> counterexample is null-terminated names; while parsing these is pretty
> trivial, in hindsight it would probably have been better to add a
> header to names as well.
Agreed. Will add total length also to "fuse_secctx_header". That will
act as a strong check/verification mechanism as well as allow to
quickly skip to next args if one does not want to parse security contexts
for whatever reason.
Thanks
Vivek
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-11-02 15:30 ` [Virtio-fs] " Vivek Goyal
@ 2021-11-02 15:38 ` Miklos Szeredi
-1 siblings, 0 replies; 29+ messages in thread
From: Miklos Szeredi @ 2021-11-02 15:38 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-fsdevel, SElinux list, LSM, virtio-fs-list,
Chirantan Ekbote, Stephen Smalley, Daniel J Walsh,
Casey Schaufler, Ondrej Mosnacek
On Tue, 2 Nov 2021 at 16:30, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> > On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > > args->out_numargs = 1;
> > > args->out_args[0].size = sizeof(outarg);
> > > args->out_args[0].value = &outarg;
> > > +
> > > + if (init_security) {
> >
>
> Hi Miklos,
>
> > Instead of a new arg to create_new_entry(), this could check
> > args.opcode != FUSE_LINK.
>
> Will do.
>
> >
> > > + unsigned short idx = args->in_numargs;
> > > +
> > > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > > + err = -ENOMEM;
> > > + goto out_put_forget_req;
> > > + }
> > > +
> > > + err = get_security_context(entry, mode, &security_ctx,
> > > + &security_ctxlen);
> > > + if (err)
> > > + goto out_put_forget_req;
> > > +
> > > + if (security_ctxlen > 0) {
> >
> > This doesn't seem right. How would the server know if this is arg is missing?
> >
> > I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> > will always need to be added to the MK* requests.
>
> Even for the case of FUSE_LINK request? I think I put this check because
> FUSE_LINK is not sending secctx header. Other requests are appending
> this header even if a security module is not loaded/enabled.
No, FUSE_LINK wouldn't even get this far.
> I guess it makes more sense to add secctx header even for FUSE_LINK
> request. Just that header will mention 0 security contexts are
> following. This will interface more uniform. I will make this change.
Why? FUSE_LINK is not an inode creation op, it just shares the
instantiation part with creation.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-11-02 15:38 ` Miklos Szeredi
0 siblings, 0 replies; 29+ messages in thread
From: Miklos Szeredi @ 2021-11-02 15:38 UTC (permalink / raw)
To: Vivek Goyal
Cc: SElinux list, Casey Schaufler, Stephen Smalley, Ondrej Mosnacek,
virtio-fs-list, LSM, linux-fsdevel
On Tue, 2 Nov 2021 at 16:30, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> > On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > > args->out_numargs = 1;
> > > args->out_args[0].size = sizeof(outarg);
> > > args->out_args[0].value = &outarg;
> > > +
> > > + if (init_security) {
> >
>
> Hi Miklos,
>
> > Instead of a new arg to create_new_entry(), this could check
> > args.opcode != FUSE_LINK.
>
> Will do.
>
> >
> > > + unsigned short idx = args->in_numargs;
> > > +
> > > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > > + err = -ENOMEM;
> > > + goto out_put_forget_req;
> > > + }
> > > +
> > > + err = get_security_context(entry, mode, &security_ctx,
> > > + &security_ctxlen);
> > > + if (err)
> > > + goto out_put_forget_req;
> > > +
> > > + if (security_ctxlen > 0) {
> >
> > This doesn't seem right. How would the server know if this is arg is missing?
> >
> > I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> > will always need to be added to the MK* requests.
>
> Even for the case of FUSE_LINK request? I think I put this check because
> FUSE_LINK is not sending secctx header. Other requests are appending
> this header even if a security module is not loaded/enabled.
No, FUSE_LINK wouldn't even get this far.
> I guess it makes more sense to add secctx header even for FUSE_LINK
> request. Just that header will mention 0 security contexts are
> following. This will interface more uniform. I will make this change.
Why? FUSE_LINK is not an inode creation op, it just shares the
instantiation part with creation.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH v2 2/2] fuse: Send security context of inode on file creation
2021-11-02 15:38 ` [Virtio-fs] " Miklos Szeredi
@ 2021-11-02 19:09 ` Vivek Goyal
-1 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-11-02 19:09 UTC (permalink / raw)
To: Miklos Szeredi
Cc: linux-fsdevel, SElinux list, LSM, virtio-fs-list,
Chirantan Ekbote, Stephen Smalley, Daniel J Walsh,
Casey Schaufler, Ondrej Mosnacek
On Tue, Nov 02, 2021 at 04:38:06PM +0100, Miklos Szeredi wrote:
> On Tue, 2 Nov 2021 at 16:30, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> > > On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > > > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > > > args->out_numargs = 1;
> > > > args->out_args[0].size = sizeof(outarg);
> > > > args->out_args[0].value = &outarg;
> > > > +
> > > > + if (init_security) {
> > >
> >
> > Hi Miklos,
> >
> > > Instead of a new arg to create_new_entry(), this could check
> > > args.opcode != FUSE_LINK.
> >
> > Will do.
> >
> > >
> > > > + unsigned short idx = args->in_numargs;
> > > > +
> > > > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > > > + err = -ENOMEM;
> > > > + goto out_put_forget_req;
> > > > + }
> > > > +
> > > > + err = get_security_context(entry, mode, &security_ctx,
> > > > + &security_ctxlen);
> > > > + if (err)
> > > > + goto out_put_forget_req;
> > > > +
> > > > + if (security_ctxlen > 0) {
> > >
> > > This doesn't seem right. How would the server know if this is arg is missing?
> > >
> > > I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> > > will always need to be added to the MK* requests.
> >
> > Even for the case of FUSE_LINK request? I think I put this check because
> > FUSE_LINK is not sending secctx header. Other requests are appending
> > this header even if a security module is not loaded/enabled.
>
> No, FUSE_LINK wouldn't even get this far.
You are right. My bad. So looks like this check will always be true
given the current code. get_security_context() will either all
headers with security context or just return zeroed "struct fuse_secctxs",
indicating there are no security context.
If that's the case, I should be able to get rid of this check. I will
do some more testing.
>
> > I guess it makes more sense to add secctx header even for FUSE_LINK
> > request. Just that header will mention 0 security contexts are
> > following. This will interface more uniform. I will make this change.
>
> Why? FUSE_LINK is not an inode creation op, it just shares the
> instantiation part with creation.
Ok, got it. Makes sense. So no sending of zeroed security context header
with FUSE_LINK.
Vivek
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [Virtio-fs] [PATCH v2 2/2] fuse: Send security context of inode on file creation
@ 2021-11-02 19:09 ` Vivek Goyal
0 siblings, 0 replies; 29+ messages in thread
From: Vivek Goyal @ 2021-11-02 19:09 UTC (permalink / raw)
To: Miklos Szeredi
Cc: SElinux list, Casey Schaufler, Stephen Smalley, Ondrej Mosnacek,
virtio-fs-list, LSM, linux-fsdevel
On Tue, Nov 02, 2021 at 04:38:06PM +0100, Miklos Szeredi wrote:
> On Tue, 2 Nov 2021 at 16:30, Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Nov 02, 2021 at 03:00:30PM +0100, Miklos Szeredi wrote:
> > > On Tue, 12 Oct 2021 at 20:06, Vivek Goyal <vgoyal@redhat.com> wrote:
>
> > > > @@ -633,7 +713,29 @@ static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> > > > args->out_numargs = 1;
> > > > args->out_args[0].size = sizeof(outarg);
> > > > args->out_args[0].value = &outarg;
> > > > +
> > > > + if (init_security) {
> > >
> >
> > Hi Miklos,
> >
> > > Instead of a new arg to create_new_entry(), this could check
> > > args.opcode != FUSE_LINK.
> >
> > Will do.
> >
> > >
> > > > + unsigned short idx = args->in_numargs;
> > > > +
> > > > + if ((size_t)idx >= ARRAY_SIZE(args->in_args)) {
> > > > + err = -ENOMEM;
> > > > + goto out_put_forget_req;
> > > > + }
> > > > +
> > > > + err = get_security_context(entry, mode, &security_ctx,
> > > > + &security_ctxlen);
> > > > + if (err)
> > > > + goto out_put_forget_req;
> > > > +
> > > > + if (security_ctxlen > 0) {
> > >
> > > This doesn't seem right. How would the server know if this is arg is missing?
> > >
> > > I think if FUSE_SECURITY_CTX was negotiated, then the secctx header
> > > will always need to be added to the MK* requests.
> >
> > Even for the case of FUSE_LINK request? I think I put this check because
> > FUSE_LINK is not sending secctx header. Other requests are appending
> > this header even if a security module is not loaded/enabled.
>
> No, FUSE_LINK wouldn't even get this far.
You are right. My bad. So looks like this check will always be true
given the current code. get_security_context() will either all
headers with security context or just return zeroed "struct fuse_secctxs",
indicating there are no security context.
If that's the case, I should be able to get rid of this check. I will
do some more testing.
>
> > I guess it makes more sense to add secctx header even for FUSE_LINK
> > request. Just that header will mention 0 security contexts are
> > following. This will interface more uniform. I will make this change.
>
> Why? FUSE_LINK is not an inode creation op, it just shares the
> instantiation part with creation.
Ok, got it. Makes sense. So no sending of zeroed security context header
with FUSE_LINK.
Vivek
^ permalink raw reply [flat|nested] 29+ messages in thread