From: Roberto Sassu <[email protected]>
The first two patches are obvious fixes, the first restricts setting the
SMACK64TRANSMUTE xattr only for directories, and the second makes it
possible to set SMACK64TRANSMUTE if the filesystem does not support xattrs
(e.g. ramfs).
The remaining fixes are optional, and only required if we want filesystems
without xattr support behave like those with xattr support. Since we have
the inode_setsecurity and inode_getsecurity hooks to make the first group
work, it seems useful to fix inode creation too (SELinux should be fine).
The third patch is merely a code move out of the 'if (xattr)' condition.
The fourth updates the security field of the in-memory inode directly in
smack_inode_init_security() and marks the inode as instantiated, and the
fifth adds a security_inode_init_security() call in ramfs to initialize the
security field of the in-memory inodes (needed to test transmuting
directories).
Both the Smack (on xfs) and IMA test suite succeed with all patches
applied. Tests were not executed on v3 (trivial changes).
By executing the tests in a ramfs, the results are:
Without the patches:
86 Passed, 9 Failed, 90% Success rate
With the patches:
93 Passed, 2 Failed, 97% Success rate
The remaining two failures are:
2151 ioctl(4, BTRFS_IOC_CLONE or FICLONE, 3) = -1 EOPNOTSUPP (Operation not supported)
2152 lsetxattr("./targets/proc-attr-Snap", "security.SMACK64EXEC", "Pop", 3, 0) = -1 EOPNOTSUPP (Operation not supported)
The first one is likely due ramfs lack of support for ioctl() while the
second could be fixed by handling SMACK64EXEC in smack_inode_setsecurity().
The patch set applies on top of lsm/dev, commit e246777e2a03 ("MAINTAINERS:
update the LSM entry").
The ramfs patch potentially could be useful to correctly initialize the
label of new inodes in the initramfs, assuming that it will be fully
labeled with support for xattrs in the cpio image:
https://lore.kernel.org/linux-integrity/[email protected]/
Ramfs inode labels will be set from xattrs with the inode_setsecurity hook.
Changelog
v2:
- Replace return with goto in the ramfs patch, for better maintainability
(suggested by Andrew Morton)
v1:
- Rebase on top of latest lsm/next
- Remove -EOPNOTSUPP check in patch 5 (cannot happen)
Roberto Sassu (5):
smack: Set SMACK64TRANSMUTE only for dirs in smack_inode_setxattr()
smack: Handle SMACK64TRANSMUTE in smack_inode_setsecurity()
smack: Always determine inode labels in smack_inode_init_security()
smack: Initialize the in-memory inode in smack_inode_init_security()
ramfs: Initialize security of in-memory inodes
fs/ramfs/inode.c | 32 ++++++++++++-
security/smack/smack_lsm.c | 95 ++++++++++++++++++++++----------------
2 files changed, 86 insertions(+), 41 deletions(-)
--
2.34.1
From: Roberto Sassu <[email protected]>
If the SMACK64TRANSMUTE xattr is provided, and the inode is a directory,
update the in-memory inode flags by setting SMK_INODE_TRANSMUTE.
Cc: [email protected]
Fixes: 5c6d1125f8db ("Smack: Transmute labels on specified directories") # v2.6.38.x
Signed-off-by: Roberto Sassu <[email protected]>
---
security/smack/smack_lsm.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0027803a43ac..7b6d7ddd6d36 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2855,6 +2855,15 @@ static int smack_inode_setsecurity(struct inode *inode, const char *name,
if (value == NULL || size > SMK_LONGLABEL || size == 0)
return -EINVAL;
+ if (strcmp(name, XATTR_SMACK_TRANSMUTE) == 0) {
+ if (!S_ISDIR(inode->i_mode) || size != TRANS_TRUE_SIZE ||
+ strncmp(value, TRANS_TRUE, TRANS_TRUE_SIZE) != 0)
+ return -EINVAL;
+
+ nsp->smk_flags |= SMK_INODE_TRANSMUTE;
+ return 0;
+ }
+
skp = smk_import_entry(value, size);
if (IS_ERR(skp))
return PTR_ERR(skp);
--
2.34.1
From: Roberto Sassu <[email protected]>
The inode_init_security hook is already a good place to initialize the
in-memory inode. And that is also what SELinux does.
In preparation for this, move the existing smack_inode_init_security() code
outside the 'if (xattr)' condition, and set the xattr, if provided.
This change does not have any impact on the current code, since every time
security_inode_init_security() is called, the initxattr() callback is
passed and, thus, xattr is non-NULL.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/smack/smack_lsm.c | 78 +++++++++++++++++++-------------------
1 file changed, 39 insertions(+), 39 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 7b6d7ddd6d36..72f97492f5c3 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -999,51 +999,51 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
struct xattr *xattr = lsm_get_xattr_slot(xattrs, xattr_count);
int may;
- if (xattr) {
- /*
- * If equal, transmuting already occurred in
- * smack_dentry_create_files_as(). No need to check again.
- */
- if (tsp->smk_task != tsp->smk_transmuted) {
- rcu_read_lock();
- may = smk_access_entry(skp->smk_known, dsp->smk_known,
- &skp->smk_rules);
- rcu_read_unlock();
- }
+ /*
+ * If equal, transmuting already occurred in
+ * smack_dentry_create_files_as(). No need to check again.
+ */
+ if (tsp->smk_task != tsp->smk_transmuted) {
+ rcu_read_lock();
+ may = smk_access_entry(skp->smk_known, dsp->smk_known,
+ &skp->smk_rules);
+ rcu_read_unlock();
+ }
+
+ /*
+ * In addition to having smk_task equal to smk_transmuted,
+ * if the access rule allows transmutation and the directory
+ * requests transmutation then by all means transmute.
+ * Mark the inode as changed.
+ */
+ if ((tsp->smk_task == tsp->smk_transmuted) ||
+ (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
+ smk_inode_transmutable(dir))) {
+ struct xattr *xattr_transmute;
/*
- * In addition to having smk_task equal to smk_transmuted,
- * if the access rule allows transmutation and the directory
- * requests transmutation then by all means transmute.
- * Mark the inode as changed.
+ * The caller of smack_dentry_create_files_as()
+ * should have overridden the current cred, so the
+ * inode label was already set correctly in
+ * smack_inode_alloc_security().
*/
- if ((tsp->smk_task == tsp->smk_transmuted) ||
- (may > 0 && ((may & MAY_TRANSMUTE) != 0) &&
- smk_inode_transmutable(dir))) {
- struct xattr *xattr_transmute;
+ if (tsp->smk_task != tsp->smk_transmuted)
+ isp = dsp;
+ xattr_transmute = lsm_get_xattr_slot(xattrs,
+ xattr_count);
+ if (xattr_transmute) {
+ xattr_transmute->value = kmemdup(TRANS_TRUE,
+ TRANS_TRUE_SIZE,
+ GFP_NOFS);
+ if (!xattr_transmute->value)
+ return -ENOMEM;
- /*
- * The caller of smack_dentry_create_files_as()
- * should have overridden the current cred, so the
- * inode label was already set correctly in
- * smack_inode_alloc_security().
- */
- if (tsp->smk_task != tsp->smk_transmuted)
- isp = dsp;
- xattr_transmute = lsm_get_xattr_slot(xattrs,
- xattr_count);
- if (xattr_transmute) {
- xattr_transmute->value = kmemdup(TRANS_TRUE,
- TRANS_TRUE_SIZE,
- GFP_NOFS);
- if (!xattr_transmute->value)
- return -ENOMEM;
-
- xattr_transmute->value_len = TRANS_TRUE_SIZE;
- xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
- }
+ xattr_transmute->value_len = TRANS_TRUE_SIZE;
+ xattr_transmute->name = XATTR_SMACK_TRANSMUTE;
}
+ }
+ if (xattr) {
xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
if (!xattr->value)
return -ENOMEM;
--
2.34.1
From: Roberto Sassu <[email protected]>
Add a call security_inode_init_security() after ramfs_get_inode(), to let
LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
initialization is done through the sb_set_mnt_opts hook.
Calling security_inode_init_security() call inside ramfs_get_inode() is
not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
the latter.
Pass NULL as initxattrs() callback to security_inode_init_security(), since
the purpose of the call is only to initialize the in-memory inodes.
Cc: Hugh Dickins <[email protected]>
Cc: Andrew Morton <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
---
fs/ramfs/inode.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index 4ac05a9e25bc..8006faaaf0ec 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
int error = -ENOSPC;
if (inode) {
+ error = security_inode_init_security(inode, dir,
+ &dentry->d_name, NULL,
+ NULL);
+ if (error) {
+ iput(inode);
+ goto out;
+ }
+
d_instantiate(dentry, inode);
dget(dentry); /* Extra count - pin the dentry in core */
error = 0;
inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
}
+out:
return error;
}
@@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
if (inode) {
int l = strlen(symname)+1;
+
+ error = security_inode_init_security(inode, dir,
+ &dentry->d_name, NULL,
+ NULL);
+ if (error) {
+ iput(inode);
+ goto out;
+ }
+
error = page_symlink(inode, symname, l);
if (!error) {
d_instantiate(dentry, inode);
@@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
} else
iput(inode);
}
+out:
return error;
}
@@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
struct inode *dir, struct file *file, umode_t mode)
{
struct inode *inode;
+ int error;
inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
if (!inode)
return -ENOSPC;
+
+ error = security_inode_init_security(inode, dir,
+ &file_dentry(file)->d_name, NULL,
+ NULL);
+ if (error) {
+ iput(inode);
+ goto out;
+ }
+
d_tmpfile(file, inode);
- return finish_open_simple(file, 0);
+out:
+ return finish_open_simple(file, error);
}
static const struct inode_operations ramfs_dir_inode_operations = {
--
2.34.1
From: Roberto Sassu <[email protected]>
Currently, Smack initializes in-memory new inodes in three steps. It first
sets the xattrs in smack_inode_init_security(), fetches them in
smack_d_instantiate() and finally, in the same function, sets the in-memory
inodes depending on xattr values, unless they are in specially-handled
filesystems.
Other than being inefficient, this also prevents filesystems not supporting
xattrs from working properly since, without xattrs, there is no way to pass
the label determined in smack_inode_init_security() to
smack_d_instantiate().
Since the LSM infrastructure allows setting and getting the security field
without xattrs through the inode_setsecurity and inode_getsecurity hooks,
make the inode creation work too, by initializing the in-memory inode
earlier in smack_inode_init_security().
Also mark the inode as instantiated, to prevent smack_d_instantiate() from
overwriting the security field. As mentioned above, this potentially has
impact for inodes in specially-handled filesystems in
smack_d_instantiate(), if they are not handled in the same way in
smack_inode_init_security().
Filesystems other than tmpfs don't call security_inode_init_security(), so
they would be always initialized in smack_d_instantiate(), as before. For
tmpfs, the current behavior is to assign to inodes the label '*', but
actually that label is overwritten with the one fetched from the SMACK64
xattr, set in smack_inode_init_security() (default: '_').
Initializing the in-memory inode is straightforward: if not transmuting,
nothing more needs to be done; if transmuting, overwrite the current inode
label with the one from the parent directory, and set SMK_INODE_TRANSMUTE.
Finally, set SMK_INODE_INSTANT for all cases, to mark the inode as
instantiated.
Signed-off-by: Roberto Sassu <[email protected]>
---
security/smack/smack_lsm.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 72f97492f5c3..43e9389cbdfa 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -993,6 +993,7 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
struct xattr *xattrs, int *xattr_count)
{
struct task_smack *tsp = smack_cred(current_cred());
+ struct inode_smack *issp = smack_inode(inode);
struct smack_known *skp = smk_of_task(tsp);
struct smack_known *isp = smk_of_inode(inode);
struct smack_known *dsp = smk_of_inode(dir);
@@ -1028,7 +1029,9 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
* smack_inode_alloc_security().
*/
if (tsp->smk_task != tsp->smk_transmuted)
- isp = dsp;
+ isp = issp->smk_inode = dsp;
+
+ issp->smk_flags |= SMK_INODE_TRANSMUTE;
xattr_transmute = lsm_get_xattr_slot(xattrs,
xattr_count);
if (xattr_transmute) {
@@ -1043,6 +1046,8 @@ static int smack_inode_init_security(struct inode *inode, struct inode *dir,
}
}
+ issp->smk_flags |= SMK_INODE_INSTANT;
+
if (xattr) {
xattr->value = kstrdup(isp->smk_known, GFP_NOFS);
if (!xattr->value)
--
2.34.1
On Thu, 2023-11-16 at 10:01 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> Add a call security_inode_init_security() after ramfs_get_inode(), to let
> LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
> initialization is done through the sb_set_mnt_opts hook.
>
> Calling security_inode_init_security() call inside ramfs_get_inode() is
> not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
> the latter.
>
> Pass NULL as initxattrs() callback to security_inode_init_security(), since
> the purpose of the call is only to initialize the in-memory inodes.
Hugh, Andrew, is the patch fine for you? Casey would make a PR for the
patch set.
Thanks
Roberto
> Cc: Hugh Dickins <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> fs/ramfs/inode.c | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index 4ac05a9e25bc..8006faaaf0ec 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> int error = -ENOSPC;
>
> if (inode) {
> + error = security_inode_init_security(inode, dir,
> + &dentry->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> d_instantiate(dentry, inode);
> dget(dentry); /* Extra count - pin the dentry in core */
> error = 0;
> inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
> }
> +out:
> return error;
> }
>
> @@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
> if (inode) {
> int l = strlen(symname)+1;
> +
> + error = security_inode_init_security(inode, dir,
> + &dentry->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> error = page_symlink(inode, symname, l);
> if (!error) {
> d_instantiate(dentry, inode);
> @@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> } else
> iput(inode);
> }
> +out:
> return error;
> }
>
> @@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
> struct inode *dir, struct file *file, umode_t mode)
> {
> struct inode *inode;
> + int error;
>
> inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
> if (!inode)
> return -ENOSPC;
> +
> + error = security_inode_init_security(inode, dir,
> + &file_dentry(file)->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> d_tmpfile(file, inode);
> - return finish_open_simple(file, 0);
> +out:
> + return finish_open_simple(file, error);
> }
>
> static const struct inode_operations ramfs_dir_inode_operations = {
On Fri, 2024-01-12 at 09:17 +0100, Roberto Sassu wrote:
> On Thu, 2023-11-16 at 10:01 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > Add a call security_inode_init_security() after ramfs_get_inode(), to let
> > LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
> > initialization is done through the sb_set_mnt_opts hook.
> >
> > Calling security_inode_init_security() call inside ramfs_get_inode() is
> > not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
> > the latter.
> >
> > Pass NULL as initxattrs() callback to security_inode_init_security(), since
> > the purpose of the call is only to initialize the in-memory inodes.
>
> Hugh, Andrew, is the patch fine for you? Casey would make a PR for the
> patch set.
(I guess putting the people of interest in To instead of CC could
help...)
Friendly ping... it would be awesome if you could Ack this patch so
that Casey can still ask Linus to pull.
Thanks
Roberto
> Thanks
>
> Roberto
>
> > Cc: Hugh Dickins <[email protected]>
> > Cc: Andrew Morton <[email protected]>
> > Signed-off-by: Roberto Sassu <[email protected]>
> > ---
> > fs/ramfs/inode.c | 32 +++++++++++++++++++++++++++++++-
> > 1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> > index 4ac05a9e25bc..8006faaaf0ec 100644
> > --- a/fs/ramfs/inode.c
> > +++ b/fs/ramfs/inode.c
> > @@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> > int error = -ENOSPC;
> >
> > if (inode) {
> > + error = security_inode_init_security(inode, dir,
> > + &dentry->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > d_instantiate(dentry, inode);
> > dget(dentry); /* Extra count - pin the dentry in core */
> > error = 0;
> > inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
> > }
> > +out:
> > return error;
> > }
> >
> > @@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
> > if (inode) {
> > int l = strlen(symname)+1;
> > +
> > + error = security_inode_init_security(inode, dir,
> > + &dentry->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > error = page_symlink(inode, symname, l);
> > if (!error) {
> > d_instantiate(dentry, inode);
> > @@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > } else
> > iput(inode);
> > }
> > +out:
> > return error;
> > }
> >
> > @@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
> > struct inode *dir, struct file *file, umode_t mode)
> > {
> > struct inode *inode;
> > + int error;
> >
> > inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
> > if (!inode)
> > return -ENOSPC;
> > +
> > + error = security_inode_init_security(inode, dir,
> > + &file_dentry(file)->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > d_tmpfile(file, inode);
> > - return finish_open_simple(file, 0);
> > +out:
> > + return finish_open_simple(file, error);
> > }
> >
> > static const struct inode_operations ramfs_dir_inode_operations = {
>
On 11/16/2023 1:01 AM, Roberto Sassu wrote:
> From: Roberto Sassu <[email protected]>
>
> The first two patches are obvious fixes, the first restricts setting the
> SMACK64TRANSMUTE xattr only for directories, and the second makes it
> possible to set SMACK64TRANSMUTE if the filesystem does not support xattrs
> (e.g. ramfs).
>
> The remaining fixes are optional, and only required if we want filesystems
> without xattr support behave like those with xattr support. Since we have
> the inode_setsecurity and inode_getsecurity hooks to make the first group
> work, it seems useful to fix inode creation too (SELinux should be fine).
>
> The third patch is merely a code move out of the 'if (xattr)' condition.
> The fourth updates the security field of the in-memory inode directly in
> smack_inode_init_security() and marks the inode as instantiated,
I have taken patches 1-4 in smack-next. I'm still waiting on a convincing
approval for patch 5.
> and the
> fifth adds a security_inode_init_security() call in ramfs to initialize the
> security field of the in-memory inodes (needed to test transmuting
> directories).
>
> Both the Smack (on xfs) and IMA test suite succeed with all patches
> applied. Tests were not executed on v3 (trivial changes).
>
> By executing the tests in a ramfs, the results are:
>
> Without the patches:
> 86 Passed, 9 Failed, 90% Success rate
>
> With the patches:
> 93 Passed, 2 Failed, 97% Success rate
>
> The remaining two failures are:
> 2151 ioctl(4, BTRFS_IOC_CLONE or FICLONE, 3) = -1 EOPNOTSUPP (Operation not supported)
> 2152 lsetxattr("./targets/proc-attr-Snap", "security.SMACK64EXEC", "Pop", 3, 0) = -1 EOPNOTSUPP (Operation not supported)
>
> The first one is likely due ramfs lack of support for ioctl() while the
> second could be fixed by handling SMACK64EXEC in smack_inode_setsecurity().
>
> The patch set applies on top of lsm/dev, commit e246777e2a03 ("MAINTAINERS:
> update the LSM entry").
>
> The ramfs patch potentially could be useful to correctly initialize the
> label of new inodes in the initramfs, assuming that it will be fully
> labeled with support for xattrs in the cpio image:
>
> https://lore.kernel.org/linux-integrity/[email protected]/
>
> Ramfs inode labels will be set from xattrs with the inode_setsecurity hook.
>
> Changelog
>
> v2:
> - Replace return with goto in the ramfs patch, for better maintainability
> (suggested by Andrew Morton)
>
> v1:
> - Rebase on top of latest lsm/next
> - Remove -EOPNOTSUPP check in patch 5 (cannot happen)
>
> Roberto Sassu (5):
> smack: Set SMACK64TRANSMUTE only for dirs in smack_inode_setxattr()
> smack: Handle SMACK64TRANSMUTE in smack_inode_setsecurity()
> smack: Always determine inode labels in smack_inode_init_security()
> smack: Initialize the in-memory inode in smack_inode_init_security()
> ramfs: Initialize security of in-memory inodes
>
> fs/ramfs/inode.c | 32 ++++++++++++-
> security/smack/smack_lsm.c | 95 ++++++++++++++++++++++----------------
> 2 files changed, 86 insertions(+), 41 deletions(-)
>
On Wed, 2024-01-24 at 14:31 -0800, Casey Schaufler wrote:
> On 11/16/2023 1:01 AM, Roberto Sassu wrote:
> > From: Roberto Sassu <[email protected]>
> >
> > The first two patches are obvious fixes, the first restricts setting the
> > SMACK64TRANSMUTE xattr only for directories, and the second makes it
> > possible to set SMACK64TRANSMUTE if the filesystem does not support xattrs
> > (e.g. ramfs).
> >
> > The remaining fixes are optional, and only required if we want filesystems
> > without xattr support behave like those with xattr support. Since we have
> > the inode_setsecurity and inode_getsecurity hooks to make the first group
> > work, it seems useful to fix inode creation too (SELinux should be fine).
> >
> > The third patch is merely a code move out of the 'if (xattr)' condition.
> > The fourth updates the security field of the in-memory inode directly in
> > smack_inode_init_security() and marks the inode as instantiated,
>
> I have taken patches 1-4 in smack-next. I'm still waiting on a convincing
> approval for patch 5.
Thanks Casey. I also hope to receive an Ack from the maintainers.
Roberto
> > and the
> > fifth adds a security_inode_init_security() call in ramfs to initialize the
> > security field of the in-memory inodes (needed to test transmuting
> > directories).
> >
> > Both the Smack (on xfs) and IMA test suite succeed with all patches
> > applied. Tests were not executed on v3 (trivial changes).
> >
> > By executing the tests in a ramfs, the results are:
> >
> > Without the patches:
> > 86 Passed, 9 Failed, 90% Success rate
> >
> > With the patches:
> > 93 Passed, 2 Failed, 97% Success rate
> >
> > The remaining two failures are:
> > 2151 ioctl(4, BTRFS_IOC_CLONE or FICLONE, 3) = -1 EOPNOTSUPP (Operation not supported)
> > 2152 lsetxattr("./targets/proc-attr-Snap", "security.SMACK64EXEC", "Pop", 3, 0) = -1 EOPNOTSUPP (Operation not supported)
> >
> > The first one is likely due ramfs lack of support for ioctl() while the
> > second could be fixed by handling SMACK64EXEC in smack_inode_setsecurity().
> >
> > The patch set applies on top of lsm/dev, commit e246777e2a03 ("MAINTAINERS:
> > update the LSM entry").
> >
> > The ramfs patch potentially could be useful to correctly initialize the
> > label of new inodes in the initramfs, assuming that it will be fully
> > labeled with support for xattrs in the cpio image:
> >
> > https://lore.kernel.org/linux-integrity/[email protected]/
> >
> > Ramfs inode labels will be set from xattrs with the inode_setsecurity hook.
> >
> > Changelog
> >
> > v2:
> > - Replace return with goto in the ramfs patch, for better maintainability
> > (suggested by Andrew Morton)
> >
> > v1:
> > - Rebase on top of latest lsm/next
> > - Remove -EOPNOTSUPP check in patch 5 (cannot happen)
> >
> > Roberto Sassu (5):
> > smack: Set SMACK64TRANSMUTE only for dirs in smack_inode_setxattr()
> > smack: Handle SMACK64TRANSMUTE in smack_inode_setsecurity()
> > smack: Always determine inode labels in smack_inode_init_security()
> > smack: Initialize the in-memory inode in smack_inode_init_security()
> > ramfs: Initialize security of in-memory inodes
> >
> > fs/ramfs/inode.c | 32 ++++++++++++-
> > security/smack/smack_lsm.c | 95 ++++++++++++++++++++++----------------
> > 2 files changed, 86 insertions(+), 41 deletions(-)
> >
On Thu, 16 Nov 2023 10:01:25 +0100 Roberto Sassu <[email protected]> wrote:
> From: Roberto Sassu <[email protected]>
>
> Add a call security_inode_init_security() after ramfs_get_inode(), to let
> LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
> initialization is done through the sb_set_mnt_opts hook.
>
> Calling security_inode_init_security() call inside ramfs_get_inode() is
> not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
> the latter.
>
> Pass NULL as initxattrs() callback to security_inode_init_security(), since
> the purpose of the call is only to initialize the in-memory inodes.
>
fwiw,
Acked-by: Andrew Morton <[email protected]>
Please include this in the relevant security tree.
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index 4ac05a9e25bc..8006faaaf0ec 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> int error = -ENOSPC;
>
> if (inode) {
> + error = security_inode_init_security(inode, dir,
> + &dentry->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> d_instantiate(dentry, inode);
> dget(dentry); /* Extra count - pin the dentry in core */
> error = 0;
> inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
> }
> +out:
> return error;
> }
>
> @@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
> if (inode) {
> int l = strlen(symname)+1;
> +
> + error = security_inode_init_security(inode, dir,
> + &dentry->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> error = page_symlink(inode, symname, l);
> if (!error) {
> d_instantiate(dentry, inode);
> @@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> } else
> iput(inode);
> }
> +out:
> return error;
> }
>
> @@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
> struct inode *dir, struct file *file, umode_t mode)
> {
> struct inode *inode;
> + int error;
>
> inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
> if (!inode)
> return -ENOSPC;
> +
> + error = security_inode_init_security(inode, dir,
> + &file_dentry(file)->d_name, NULL,
> + NULL);
> + if (error) {
> + iput(inode);
> + goto out;
> + }
> +
> d_tmpfile(file, inode);
> - return finish_open_simple(file, 0);
> +out:
> + return finish_open_simple(file, error);
> }
>
> static const struct inode_operations ramfs_dir_inode_operations = {
> --
> 2.34.1
On Thu, 2024-01-25 at 17:08 -0800, Andrew Morton wrote:
> On Thu, 16 Nov 2023 10:01:25 +0100 Roberto Sassu <[email protected]> wrote:
>
> > From: Roberto Sassu <[email protected]>
> >
> > Add a call security_inode_init_security() after ramfs_get_inode(), to let
> > LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
> > initialization is done through the sb_set_mnt_opts hook.
> >
> > Calling security_inode_init_security() call inside ramfs_get_inode() is
> > not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
> > the latter.
> >
> > Pass NULL as initxattrs() callback to security_inode_init_security(), since
> > the purpose of the call is only to initialize the in-memory inodes.
> >
>
> fwiw,
>
> Acked-by: Andrew Morton <[email protected]>
>
> Please include this in the relevant security tree.
Thanks a lot!
Roberto
> > diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> > index 4ac05a9e25bc..8006faaaf0ec 100644
> > --- a/fs/ramfs/inode.c
> > +++ b/fs/ramfs/inode.c
> > @@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
> > int error = -ENOSPC;
> >
> > if (inode) {
> > + error = security_inode_init_security(inode, dir,
> > + &dentry->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > d_instantiate(dentry, inode);
> > dget(dentry); /* Extra count - pin the dentry in core */
> > error = 0;
> > inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
> > }
> > +out:
> > return error;
> > }
> >
> > @@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
> > if (inode) {
> > int l = strlen(symname)+1;
> > +
> > + error = security_inode_init_security(inode, dir,
> > + &dentry->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > error = page_symlink(inode, symname, l);
> > if (!error) {
> > d_instantiate(dentry, inode);
> > @@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
> > } else
> > iput(inode);
> > }
> > +out:
> > return error;
> > }
> >
> > @@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
> > struct inode *dir, struct file *file, umode_t mode)
> > {
> > struct inode *inode;
> > + int error;
> >
> > inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
> > if (!inode)
> > return -ENOSPC;
> > +
> > + error = security_inode_init_security(inode, dir,
> > + &file_dentry(file)->d_name, NULL,
> > + NULL);
> > + if (error) {
> > + iput(inode);
> > + goto out;
> > + }
> > +
> > d_tmpfile(file, inode);
> > - return finish_open_simple(file, 0);
> > +out:
> > + return finish_open_simple(file, error);
> > }
> >
> > static const struct inode_operations ramfs_dir_inode_operations = {
> > --
> > 2.34.1
On 1/25/2024 11:53 PM, Roberto Sassu wrote:
> On Thu, 2024-01-25 at 17:08 -0800, Andrew Morton wrote:
>> On Thu, 16 Nov 2023 10:01:25 +0100 Roberto Sassu <[email protected]> wrote:
>>
>>> From: Roberto Sassu <[email protected]>
>>>
>>> Add a call security_inode_init_security() after ramfs_get_inode(), to let
>>> LSMs initialize the inode security field. Skip ramfs_fill_super(), as the
>>> initialization is done through the sb_set_mnt_opts hook.
>>>
>>> Calling security_inode_init_security() call inside ramfs_get_inode() is
>>> not possible since, for CONFIG_SHMEM=n, tmpfs also calls the former after
>>> the latter.
>>>
>>> Pass NULL as initxattrs() callback to security_inode_init_security(), since
>>> the purpose of the call is only to initialize the in-memory inodes.
>>>
>> fwiw,
>>
>> Acked-by: Andrew Morton <[email protected]>
>>
>> Please include this in the relevant security tree.
I will take this in the Smack tree. Thank you.
> Thanks a lot!
>
> Roberto
>
>>> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
>>> index 4ac05a9e25bc..8006faaaf0ec 100644
>>> --- a/fs/ramfs/inode.c
>>> +++ b/fs/ramfs/inode.c
>>> @@ -102,11 +102,20 @@ ramfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
>>> int error = -ENOSPC;
>>>
>>> if (inode) {
>>> + error = security_inode_init_security(inode, dir,
>>> + &dentry->d_name, NULL,
>>> + NULL);
>>> + if (error) {
>>> + iput(inode);
>>> + goto out;
>>> + }
>>> +
>>> d_instantiate(dentry, inode);
>>> dget(dentry); /* Extra count - pin the dentry in core */
>>> error = 0;
>>> inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir));
>>> }
>>> +out:
>>> return error;
>>> }
>>>
>>> @@ -134,6 +143,15 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>>> inode = ramfs_get_inode(dir->i_sb, dir, S_IFLNK|S_IRWXUGO, 0);
>>> if (inode) {
>>> int l = strlen(symname)+1;
>>> +
>>> + error = security_inode_init_security(inode, dir,
>>> + &dentry->d_name, NULL,
>>> + NULL);
>>> + if (error) {
>>> + iput(inode);
>>> + goto out;
>>> + }
>>> +
>>> error = page_symlink(inode, symname, l);
>>> if (!error) {
>>> d_instantiate(dentry, inode);
>>> @@ -143,6 +161,7 @@ static int ramfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>>> } else
>>> iput(inode);
>>> }
>>> +out:
>>> return error;
>>> }
>>>
>>> @@ -150,12 +169,23 @@ static int ramfs_tmpfile(struct mnt_idmap *idmap,
>>> struct inode *dir, struct file *file, umode_t mode)
>>> {
>>> struct inode *inode;
>>> + int error;
>>>
>>> inode = ramfs_get_inode(dir->i_sb, dir, mode, 0);
>>> if (!inode)
>>> return -ENOSPC;
>>> +
>>> + error = security_inode_init_security(inode, dir,
>>> + &file_dentry(file)->d_name, NULL,
>>> + NULL);
>>> + if (error) {
>>> + iput(inode);
>>> + goto out;
>>> + }
>>> +
>>> d_tmpfile(file, inode);
>>> - return finish_open_simple(file, 0);
>>> +out:
>>> + return finish_open_simple(file, error);
>>> }
>>>
>>> static const struct inode_operations ramfs_dir_inode_operations = {
>>> --
>>> 2.34.1
>