Re: [PATCH v3 1/2] lsm: add backing_file LSM hooks

On Fri, Mar 27, 2026 at 11:05 PM Paul Moore <[email protected]> wrote:
>
> Stacked filesystems such as overlayfs do not currently provide the
> necessary mechanisms for LSMs to properly enforce access controls on the
> mmap() and mprotect() operations.  In order to resolve this gap, a LSM
> security blob is being added to the backing_file struct and the following
> new LSM hooks are being created:
>
>  security_backing_file_alloc()
>  security_backing_file_free()
>  security_mmap_backing_file()
>
> The first two hooks are to manage the lifecycle of the LSM security blob
> in the backing_file struct, while the third provides a new mmap() access
> control point for the underlying backing file.  It is also expected that
> LSMs will likely want to update their security_file_mprotect() callback
> to address issues with their mprotect() controls, but that does not
> require a change to the security_file_mprotect() LSM hook.
>
> There are a two other small changes to support these new LSM hooks.  We
> pass the user file associated with a backing file down to
> alloc_empty_backing_file() so it can be included in the
> security_backing_file_alloc() hook, and we constify the file struct field
> in the LSM common_audit_data struct to better support LSMs that need to
> pass a const file struct pointer into the common LSM audit code.
>
> Thanks to Arnd Bergmann for identifying the missing EXPORT_SYMBOL_GPL()
> and supplying a fixup.
>
> Cc: [email protected]
> Acked-by: Christian Brauner <[email protected]>
> Signed-off-by: Paul Moore <[email protected]>
> ---
>  fs/backing-file.c             |  18 ++++--
>  fs/erofs/ishare.c             |  10 +++-
>  fs/file_table.c               |  21 ++++++-
>  fs/fuse/passthrough.c         |   2 +-
>  fs/internal.h                 |   3 +-
>  fs/overlayfs/dir.c            |   2 +-
>  fs/overlayfs/file.c           |   2 +-
>  include/linux/backing-file.h  |   4 +-
>  include/linux/fs.h            |   1 +
>  include/linux/lsm_audit.h     |   2 +-
>  include/linux/lsm_hook_defs.h |   5 ++
>  include/linux/lsm_hooks.h     |   1 +
>  include/linux/security.h      |  22 ++++++++
>  security/lsm.h                |   1 +
>  security/lsm_init.c           |   9 +++
>  security/security.c           | 100 ++++++++++++++++++++++++++++++++++
>  16 files changed, 187 insertions(+), 16 deletions(-)
>

That looks like a nice clean abstraction to me.

...

> diff --git a/fs/file_table.c b/fs/file_table.c
> index aaa5faaace1e..0bdc26cae138 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -50,6 +50,7 @@ struct backing_file {
>                 struct path user_path;
>                 freeptr_t bf_freeptr;
>         };
> +       void *security;

This needs ifdef SECURITY
and the name should be user_security

>  };
>
>  #define backing_file(f) container_of(f, struct backing_file, file)
> @@ -66,6 +67,11 @@ void backing_file_set_user_path(struct file *f, const struct path *path)
>  }
>  EXPORT_SYMBOL_GPL(backing_file_set_user_path);
>
> +void *backing_file_security(const struct file *f)
> +{
> +       return backing_file(f)->security;
> +}

I prefer the name backing_file_user_security()

Terminology here is very confusing but when saying
"backing file" it is more natural that one is referring to the
backing xfs file with overlayfs has opened.

The "backing file" already has an LSM blob f->f_security
which is fair the call it the "backing file's LSM blob"

Therefore, I think we need to make a distinction, as we did
with backing_file_user_path() and refer to this as something along
the lines of the "backing file's user LSM blob".

> +
>  static inline void file_free(struct file *f)
>  {
>         security_file_free(f);
> @@ -73,8 +79,11 @@ static inline void file_free(struct file *f)
>                 percpu_counter_dec(&nr_files);
>         put_cred(f->f_cred);
>         if (unlikely(f->f_mode & FMODE_BACKING)) {
> -               path_put(backing_file_user_path(f));
> -               kmem_cache_free(bfilp_cachep, backing_file(f));
> +               struct backing_file *ff = backing_file(f);
> +
> +               security_backing_file_free(&ff->security);
> +               path_put(&ff->user_path);
> +               kmem_cache_free(bfilp_cachep, ff);

Not directly related to your patch, but as this is growing, IMO
this would look cleaner with backing_file_free() inline helper
(see attached path).

>         } else {
>                 kmem_cache_free(filp_cachep, f);
>         }
> @@ -290,7 +299,8 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
>   * This is only for kernel internal use, and the allocate file must not be
>   * installed into file tables or such.
>   */
> -struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
> +struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
> +                                     const struct file *user_file)
>  {
>         struct backing_file *ff;
>         int error;
> @@ -306,6 +316,11 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
>         }
>
>         ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
> +       error = security_backing_file_alloc(&ff->security, user_file);
> +       if (unlikely(error)) {
> +               fput(&ff->file);
> +               return ERR_PTR(error);
> +       }
>         return &ff->file;
>  }

There is an API issue here.
in order to call fput() we must ensure that user_security was initialized to
NULL (or allocated).

I don't think that we want security_backing_file_alloc() to provide this
semantic and the current patch does not implement it.

Furthermore, user_path is also not initialized in the error case.

Attached UNTESTED fixup patch to suggest a cleanup with
init_backing_file() helper.

It also changes the variable and helper name to user_security
and plays some trick to avoid many ifdef SECURITY.
Feel free to take whichever bits you like with/without attribution.

If you prefer, attached also a proper prep patch.
compile tested only.

Thanks,
Amir.

From 4858f610d960454ab4de0f29f3557016e80848bd Mon Sep 17 00:00:00 2001
From: Amir Goldstein <[email protected]>
Date: Mon, 30 Mar 2026 08:26:01 +0200
Subject: [PATCH] backing_file_user_security

---
 fs/file_table.c    | 47 ++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h |  2 +-
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index 0bdc26cae1389..4666e88ba687d 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -43,14 +43,19 @@ static struct kmem_cache *bfilp_cachep __ro_after_init;
 
 static struct percpu_counter nr_files __cacheline_aligned_in_smp;
 
-/* Container for backing file with optional user path */
+/* Container for backing file with optional user path and security blob */
 struct backing_file {
 	struct file file;
 	union {
 		struct path user_path;
 		freeptr_t bf_freeptr;
+		void *dummy_security;
 	};
-	void *security;
+#ifdef CONFIG_SECURITY
+	void *user_security;
+#else
+#define user_security dummy_security
+#endif
 };
 
 #define backing_file(f) container_of(f, struct backing_file, file)
@@ -67,9 +72,16 @@ void backing_file_set_user_path(struct file *f, const struct path *path)
 }
 EXPORT_SYMBOL_GPL(backing_file_set_user_path);
 
-void *backing_file_security(const struct file *f)
+void *backing_file_user_security(struct file *f)
 {
-	return backing_file(f)->security;
+	return backing_file(f)->user_security;
+}
+
+static inline void backing_file_free(struct backing_file *ff)
+{
+	security_backing_file_free(&ff->user_security);
+	path_put(&ff->user_path);
+	kmem_cache_free(bfilp_cachep, ff);
 }
 
 static inline void file_free(struct file *f)
@@ -79,11 +91,7 @@ static inline void file_free(struct file *f)
 		percpu_counter_dec(&nr_files);
 	put_cred(f->f_cred);
 	if (unlikely(f->f_mode & FMODE_BACKING)) {
-		struct backing_file *ff = backing_file(f);
-
-		security_backing_file_free(&ff->security);
-		path_put(&ff->user_path);
-		kmem_cache_free(bfilp_cachep, ff);
+		backing_file_free(backing_file(f));
 	} else {
 		kmem_cache_free(filp_cachep, f);
 	}
@@ -292,6 +300,23 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
 	return f;
 }
 
+static int init_backing_file(struct backing_file *ff,
+			     const struct file *user_file)
+{
+	int error;
+
+	memset(&ff->user_path, 0, sizeof(ff->user_path));
+	ff->user_security = NULL;
+
+	error = security_backing_file_alloc(&ff->user_security, user_file);
+	if (unlikely(error)) {
+		fput(&ff->file);
+		return ERR_PTR(error);
+	}
+
+	return 0;
+}
+
 /*
  * Variant of alloc_empty_file() that allocates a backing_file container
  * and doesn't check and modify nr_files.
@@ -315,12 +340,14 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred,
 		return ERR_PTR(error);
 	}
 
+	// The f_mode flags must be set before fput()
 	ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
-	error = security_backing_file_alloc(&ff->security, user_file);
+	error = init_backing_file(ff, user_file);
 	if (unlikely(error)) {
 		fput(&ff->file);
 		return ERR_PTR(error);
 	}
+
 	return &ff->file;
 }
 EXPORT_SYMBOL_GPL(alloc_empty_backing_file);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 8f5702cfb5e0b..60450a0790add 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2474,7 +2474,7 @@ struct file *dentry_open_nonotify(const struct path *path, int flags,
 struct file *dentry_create(struct path *path, int flags, umode_t mode,
 			   const struct cred *cred);
 const struct path *backing_file_user_path(const struct file *f);
-void *backing_file_security(const struct file *f);
+void *backing_file_user_security(const struct file *f);
 
 /*
  * When mmapping a file on a stackable filesystem (e.g., overlayfs), the file
-- 
2.53.0

From cad1df280bcc935289c787f5f4deb4a23ea20fcd Mon Sep 17 00:00:00 2001
From: Amir Goldstein <[email protected]>
Date: Mon, 30 Mar 2026 10:27:51 +0200
Subject: [PATCH] fs: prepare for adding user_security block to backing_file

In preparation to adding user_security blob to backing_file struct,
factor out helpers init_backing_file() and backing_file_free().

Signed-off-by: Amir Goldstein <[email protected]>
---
 fs/file_table.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index aaa5faaace1e9..9e02c1f16db3c 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -66,6 +66,12 @@ void backing_file_set_user_path(struct file *f, const struct path *path)
 }
 EXPORT_SYMBOL_GPL(backing_file_set_user_path);
 
+static inline void backing_file_free(struct backing_file *ff)
+{
+	path_put(&ff->user_path);
+	kmem_cache_free(bfilp_cachep, ff);
+}
+
 static inline void file_free(struct file *f)
 {
 	security_file_free(f);
@@ -73,8 +79,7 @@ static inline void file_free(struct file *f)
 		percpu_counter_dec(&nr_files);
 	put_cred(f->f_cred);
 	if (unlikely(f->f_mode & FMODE_BACKING)) {
-		path_put(backing_file_user_path(f));
-		kmem_cache_free(bfilp_cachep, backing_file(f));
+		backing_file_free(backing_file(f));
 	} else {
 		kmem_cache_free(filp_cachep, f);
 	}
@@ -283,6 +288,12 @@ struct file *alloc_empty_file_noaccount(int flags, const struct cred *cred)
 	return f;
 }
 
+static int init_backing_file(struct backing_file *ff)
+{
+	memset(&ff->user_path, 0, sizeof(ff->user_path));
+	return 0;
+}
+
 /*
  * Variant of alloc_empty_file() that allocates a backing_file container
  * and doesn't check and modify nr_files.
@@ -305,7 +316,14 @@ struct file *alloc_empty_backing_file(int flags, const struct cred *cred)
 		return ERR_PTR(error);
 	}
 
+	// The f_mode flags must be set before fput()
 	ff->file.f_mode |= FMODE_BACKING | FMODE_NOACCOUNT;
+	error = init_backing_file(ff);
+	if (unlikely(error)) {
+		fput(&ff->file);
+		return ERR_PTR(error);
+	}
+
 	return &ff->file;
 }
 EXPORT_SYMBOL_GPL(alloc_empty_backing_file);
-- 
2.53.0