2023-07-24 15:36:29

by Roberto Sassu

[permalink] [raw]
Subject: [PATCH v2 5/5] ramfs: Initialize security of in-memory inodes

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.

Signed-off-by: Roberto Sassu <[email protected]>
---
fs/ramfs/inode.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
index fef477c7810..ac90ebd9dbd 100644
--- a/fs/ramfs/inode.c
+++ b/fs/ramfs/inode.c
@@ -102,6 +102,14 @@ 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);
+ return error;
+ }
+
d_instantiate(dentry, inode);
dget(dentry); /* Extra count - pin the dentry in core */
error = 0;
@@ -134,6 +142,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);
+ return error;
+ }
+
error = page_symlink(inode, symname, l);
if (!error) {
d_instantiate(dentry, inode);
@@ -149,10 +166,20 @@ 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);
+ return error;
+ }
+
d_tmpfile(file, inode);
return finish_open_simple(file, 0);
}
--
2.34.1



2023-11-15 08:02:37

by Roberto Sassu

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] ramfs: Initialize security of in-memory inodes

On Mon, 2023-07-24 at 17:13 +0200, 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.
>
> Signed-off-by: Roberto Sassu <[email protected]>

+ Andrew

Hi Andrew

I'm proposing an extension to initialize the inode security field at
inode creation time for filesystems that don't support xattrs (ramfs in
this case).

The LSM infrastructure already supports setting the inode security
field, but only at run-time, with the inode_setsecurity hook.

I developed this to do some testing on the Smack LSM, and I thought it
could be useful anyway.

Casey would need your acked-by, to carry this patch in his repository.
I'm not completely sure if you are the maintainer, but in the past you
accepted a patch for ramfs.

If you have time and you could have a look, that would be great!

Thanks

Roberto

> ---
> fs/ramfs/inode.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index fef477c7810..ac90ebd9dbd 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -102,6 +102,14 @@ 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);
> + return error;
> + }
> +
> d_instantiate(dentry, inode);
> dget(dentry); /* Extra count - pin the dentry in core */
> error = 0;
> @@ -134,6 +142,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);
> + return error;
> + }
> +
> error = page_symlink(inode, symname, l);
> if (!error) {
> d_instantiate(dentry, inode);
> @@ -149,10 +166,20 @@ 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);
> + return error;
> + }
> +
> d_tmpfile(file, inode);
> return finish_open_simple(file, 0);
> }

2023-11-15 22:25:21

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] ramfs: Initialize security of in-memory inodes

On Wed, 15 Nov 2023 09:01:52 +0100 Roberto Sassu <[email protected]> wrote:

> On Mon, 2023-07-24 at 17:13 +0200, 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.
> >
> > Signed-off-by: Roberto Sassu <[email protected]>
>
> + Andrew
>
> Hi Andrew
>
> I'm proposing an extension to initialize the inode security field at
> inode creation time for filesystems that don't support xattrs (ramfs in
> this case).
>
> The LSM infrastructure already supports setting the inode security
> field, but only at run-time, with the inode_setsecurity hook.
>
> I developed this to do some testing on the Smack LSM, and I thought it
> could be useful anyway.
>
> Casey would need your acked-by, to carry this patch in his repository.
> I'm not completely sure if you are the maintainer, but in the past you
> accepted a patch for ramfs.
>
> If you have time and you could have a look, that would be great!

Patch looks OK to me. Please cc Hugh and myself on a resend.

One little thing:

> > +++ b/fs/ramfs/inode.c
> > @@ -102,6 +102,14 @@ 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);
> > + return error;

A `break' here would be better. To avoid having multiple return
points, which are often a maintenance hassle. Same treatment at
the other sites.