[RFC PATCH 0/6] fs, lsm: mediate O_PATH fd creation in file_open hook

[RFC PATCH 0/6] fs, lsm: mediate O_PATH fd creation in file_open hook

[Ryan Lee]

Calls to the openat(2) family of syscalls are mediated by the file_open LSM
hook, but the opening of O_PATH file descriptors completely bypasses LSM
mediation, preventing LSMs from initializing LSM file security context
blobs for such file descriptors for use in other mediation hooks.

This patchset enables mediation of O_PATH file descriptors through the
file_open hook and updates the LSMs using that hook to unconditionally
allow creation of O_PATH fds, in order to preserve the existing behavior.
However, the LSM patches are primarily meant as a starting point for
discussions on how each one wants to handle O_PATH fd creation.

Ryan Lee (6):
  fs: invoke LSM file_open hook in do_dentry_open for O_PATH fds as well
  apparmor: explicitly skip mediation of O_PATH file descriptors
  landlock: explicitly skip mediation of O_PATH file descriptors
  selinux: explicitly skip mediation of O_PATH file descriptors
  smack: explicitly skip mediation of O_PATH file descriptors
  tomoyo: explicitly skip mediation of O_PATH file descriptors

 fs/open.c                  |  7 ++++++-
 security/apparmor/lsm.c    | 10 ++++++++++
 security/landlock/fs.c     |  8 ++++++++
 security/selinux/hooks.c   |  5 +++++
 security/smack/smack_lsm.c |  4 ++++
 security/tomoyo/file.c     |  4 ++++
 6 files changed, 37 insertions(+), 1 deletion(-)

-- 
2.43.0

[RFC PATCH 1/6] fs: invoke LSM file_open hook in do_dentry_open for O_PATH fds as well

[Ryan Lee]

Currently, opening O_PATH file descriptors completely bypasses the LSM
infrastructure. Invoking the LSM file_open hook for O_PATH fds will
be necessary for e.g. mediating the fsmount() syscall.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 fs/open.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index 30bfcddd505d..0f8542bf6cd4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -921,8 +921,13 @@ static int do_dentry_open(struct file *f,
 	if (unlikely(f->f_flags & O_PATH)) {
 		f->f_mode = FMODE_PATH | FMODE_OPENED;
 		file_set_fsnotify_mode(f, FMODE_NONOTIFY);
 		f->f_op = &empty_fops;
-		return 0;
+		/*
+		 * do_o_path in fs/namei.c unconditionally invokes path_put
+		 * after this function returns, so don't path_put the path
+		 * upon LSM rejection of O_PATH opening
+		 */
+		return security_file_open(f);
 	}
 
 	if ((f->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ) {
-- 
2.43.0

base-kernel: v6.14-rc6

[RFC PATCH 2/6] apparmor: explicitly skip mediation of O_PATH file descriptors

[Ryan Lee]

Previously, we saw O_PATH fds only when mediating fd inheritance on exec,
but because they would have no request associated with them, we would
unconditionally let them be inherited. Until we have better handling of
O_PATH fds, preserve the existing behavior of unconditionally allowing
them.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 security/apparmor/lsm.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 11ace667cbbf..2349a1dd41f4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -647,6 +647,16 @@ static int apparmor_file_open(struct file *file)
 		return 0;
 	}
 
+	/*
+	 * Preserve the behavior of O_PATH fd creation not being mediated.
+	 *
+	 * TODO: we weren't handling O_PATH fds in aa_inherit_files anyways
+	 * (all-zero request -> fds unconditionally inherited), so proper
+	 * mediation of those will require changes in multiple places.
+	 */
+	if (file->f_flags & O_PATH)
+		return 0;
+
 	label = aa_get_newest_cred_label_condref(file->f_cred, &needput);
 	if (!unconfined(label)) {
 		struct mnt_idmap *idmap = file_mnt_idmap(file);
-- 
2.43.0

base-kernel: v6.14-rc6

[RFC PATCH 3/6] landlock: explicitly skip mediation of O_PATH file descriptors

[Ryan Lee]

Landlock currently does not have handling of O_PATH fds. Now that they
are being passed to the file_open hook, explicitly skip mediation of
them until we can handle them.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 security/landlock/fs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 0804f76a67be..37b2167bf4c6 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1522,6 +1522,14 @@ static int hook_file_open(struct file *const file)
 	if (!dom)
 		return 0;
 
+	/*
+	 * Preserve the behavior of O_PATH fd creation not being mediated, for
+	 * now.  Remove this when the comment below about handling O_PATH fds
+	 * is resolved.
+	 */
+	if (file->f_flags & O_PATH)
+		return 0;
+
 	/*
 	 * Because a file may be opened with O_PATH, get_required_file_open_access()
 	 * may return 0.  This case will be handled with a future Landlock
-- 
2.43.0

base-kernel: v6.14-rc6

[RFC PATCH 4/6] selinux: explicitly skip mediation of O_PATH file descriptors

[Ryan Lee]

Now that O_PATH fds are being passed to the file_open hook,
unconditionally skip mediation of them to preserve existing behavior.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 security/selinux/hooks.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 07f71e6c2660..886ee9381507 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4009,6 +4009,11 @@ static int selinux_file_open(struct file *file)
 	 */
 	fsec->isid = isec->sid;
 	fsec->pseqno = avc_policy_seqno();
+
+	/* Preserve the behavior of O_PATH fd creation not being mediated */
+	if (file->f_flags & O_PATH)
+		return 0;
+
 	/*
 	 * Since the inode label or policy seqno may have changed
 	 * between the selinux_inode_permission check and the saving
-- 
2.43.0

base-kernel: v6.14-rc6

[RFC PATCH 5/6] smack: explicitly skip mediation of O_PATH file descriptors

[Ryan Lee]

Now that O_PATH fds are being passed to the file_open hook,
unconditionally skip mediation of them to preserve existing behavior.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 security/smack/smack_lsm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 2f65eb392bc0..c05e223bfb33 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -2062,6 +2062,10 @@ static int smack_file_open(struct file *file)
 	struct smk_audit_info ad;
 	int rc;
 
+	/* Preserve the behavior of O_PATH fd creation not being mediated */
+	if (file->f_flags & O_PATH)
+		return 0;
+
 	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
 	smk_ad_setfield_u_fs_path(&ad, file->f_path);
 	rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);
-- 
2.43.0

base-kernel: v6.14-rc6

[RFC PATCH 6/6] tomoyo: explicitly skip mediation of O_PATH file descriptors

[Ryan Lee]

Now that O_PATH fds are being passed to the file_open hook,
unconditionally skip mediation of them to preserve existing behavior.

Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
---
 security/tomoyo/file.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/security/tomoyo/file.c b/security/tomoyo/file.c
index 8f3b90b6e03d..efecfa7d15b2 100644
--- a/security/tomoyo/file.c
+++ b/security/tomoyo/file.c
@@ -762,6 +762,10 @@ int tomoyo_check_open_permission(struct tomoyo_domain_info *domain,
 	};
 	int idx;
 
+	/* Preserve the behavior of O_PATH fd creation not being mediated */
+	if (flag & O_PATH)
+		return 0;
+
 	buf.name = NULL;
 	r.mode = TOMOYO_CONFIG_DISABLED;
 	idx = tomoyo_read_lock();
-- 
2.43.0

base-kernel: v6.14-rc6

Re: [RFC PATCH 1/6] fs: invoke LSM file_open hook in do_dentry_open for O_PATH fds as well

[Al Viro]

On Wed, Mar 12, 2025 at 02:21:41PM -0700, Ryan Lee wrote:
> Currently, opening O_PATH file descriptors completely bypasses the LSM
> infrastructure. Invoking the LSM file_open hook for O_PATH fds will
> be necessary for e.g. mediating the fsmount() syscall.
> 
> Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
> ---
>  fs/open.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 30bfcddd505d..0f8542bf6cd4 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -921,8 +921,13 @@ static int do_dentry_open(struct file *f,
>  	if (unlikely(f->f_flags & O_PATH)) {
>  		f->f_mode = FMODE_PATH | FMODE_OPENED;
>  		file_set_fsnotify_mode(f, FMODE_NONOTIFY);
>  		f->f_op = &empty_fops;
> -		return 0;
> +		/*
> +		 * do_o_path in fs/namei.c unconditionally invokes path_put
> +		 * after this function returns, so don't path_put the path
> +		 * upon LSM rejection of O_PATH opening
> +		 */
> +		return security_file_open(f);

Unconditional path_put() in do_o_path() has nothing to do with that -
what gets dropped there is the reference acquired there; the reference
acquired (and not dropped) here is the one that went into ->f_path.
Since you are leaving FMODE_OPENED set, you will have __fput() drop
that reference.

Basically, you are simulating behaviour on the O_DIRECT open of
something that does not support O_DIRECT - return an error, with
->f_path and FMODE_OPENED left in place.

Said that, what I do not understand is the point of that exercise -
why does LSM need to veto anything for those and why is security_file_open()
the right place for such checks?

The second part is particularly interesting...

Re: [RFC PATCH 5/6] smack: explicitly skip mediation of O_PATH file descriptors

[Casey Schaufler]

On 3/12/2025 2:21 PM, Ryan Lee wrote:
> Now that O_PATH fds are being passed to the file_open hook,
> unconditionally skip mediation of them to preserve existing behavior.
>
> Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
> ---
>  security/smack/smack_lsm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 2f65eb392bc0..c05e223bfb33 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -2062,6 +2062,10 @@ static int smack_file_open(struct file *file)
>  	struct smk_audit_info ad;
>  	int rc;
>  
> +	/* Preserve the behavior of O_PATH fd creation not being mediated */

In Smack the single line comment is discouraged. Please use

+	/*
+	 * Preserve the behavior of O_PATH fd creation not being mediated
+	 */

> +	if (file->f_flags & O_PATH)
> +		return 0;
> +
>  	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_PATH);
>  	smk_ad_setfield_u_fs_path(&ad, file->f_path);
>  	rc = smk_tskacc(tsp, smk_of_inode(inode), MAY_READ, &ad);

Re: [RFC PATCH 1/6] fs: invoke LSM file_open hook in do_dentry_open for O_PATH fds as well

[Christian Brauner]

On Wed, Mar 12, 2025 at 09:37:14PM +0000, Al Viro wrote:
> On Wed, Mar 12, 2025 at 02:21:41PM -0700, Ryan Lee wrote:
> > Currently, opening O_PATH file descriptors completely bypasses the LSM
> > infrastructure. Invoking the LSM file_open hook for O_PATH fds will
> > be necessary for e.g. mediating the fsmount() syscall.

LSM mediation for the mount api should be done by adding appropriate
hooks to the new mount api.

> > 
> > Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
> > ---
> >  fs/open.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/open.c b/fs/open.c
> > index 30bfcddd505d..0f8542bf6cd4 100644
> > --- a/fs/open.c
> > +++ b/fs/open.c
> > @@ -921,8 +921,13 @@ static int do_dentry_open(struct file *f,
> >  	if (unlikely(f->f_flags & O_PATH)) {
> >  		f->f_mode = FMODE_PATH | FMODE_OPENED;
> >  		file_set_fsnotify_mode(f, FMODE_NONOTIFY);
> >  		f->f_op = &empty_fops;
> > -		return 0;
> > +		/*
> > +		 * do_o_path in fs/namei.c unconditionally invokes path_put
> > +		 * after this function returns, so don't path_put the path
> > +		 * upon LSM rejection of O_PATH opening
> > +		 */
> > +		return security_file_open(f);
> 
> Unconditional path_put() in do_o_path() has nothing to do with that -
> what gets dropped there is the reference acquired there; the reference
> acquired (and not dropped) here is the one that went into ->f_path.
> Since you are leaving FMODE_OPENED set, you will have __fput() drop
> that reference.
> 
> Basically, you are simulating behaviour on the O_DIRECT open of
> something that does not support O_DIRECT - return an error, with
> ->f_path and FMODE_OPENED left in place.
> 
> Said that, what I do not understand is the point of that exercise -
> why does LSM need to veto anything for those and why is security_file_open()

I really think this is misguided. This should be done via proper hooks
into apis that use O_PATH file descriptors for specific purposes but not
for the generic open() path.

> the right place for such checks?

It isn't.

> 
> The second part is particularly interesting...

Re: [RFC PATCH 1/6] fs: invoke LSM file_open hook in do_dentry_open for O_PATH fds as well

[Paul Moore]

On Thu, Mar 13, 2025 at 4:50 AM Christian Brauner <brauner@kernel.org> wrote:
> On Wed, Mar 12, 2025 at 09:37:14PM +0000, Al Viro wrote:
> > On Wed, Mar 12, 2025 at 02:21:41PM -0700, Ryan Lee wrote:
> > > Currently, opening O_PATH file descriptors completely bypasses the LSM
> > > infrastructure. Invoking the LSM file_open hook for O_PATH fds will
> > > be necessary for e.g. mediating the fsmount() syscall.
>
> LSM mediation for the mount api should be done by adding appropriate
> hooks to the new mount api.
>
> > >
> > > Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
> > > ---
> > >  fs/open.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/open.c b/fs/open.c
> > > index 30bfcddd505d..0f8542bf6cd4 100644
> > > --- a/fs/open.c
> > > +++ b/fs/open.c
> > > @@ -921,8 +921,13 @@ static int do_dentry_open(struct file *f,
> > >     if (unlikely(f->f_flags & O_PATH)) {
> > >             f->f_mode = FMODE_PATH | FMODE_OPENED;
> > >             file_set_fsnotify_mode(f, FMODE_NONOTIFY);
> > >             f->f_op = &empty_fops;
> > > -           return 0;
> > > +           /*
> > > +            * do_o_path in fs/namei.c unconditionally invokes path_put
> > > +            * after this function returns, so don't path_put the path
> > > +            * upon LSM rejection of O_PATH opening
> > > +            */
> > > +           return security_file_open(f);
> >
> > Unconditional path_put() in do_o_path() has nothing to do with that -
> > what gets dropped there is the reference acquired there; the reference
> > acquired (and not dropped) here is the one that went into ->f_path.
> > Since you are leaving FMODE_OPENED set, you will have __fput() drop
> > that reference.
> >
> > Basically, you are simulating behaviour on the O_DIRECT open of
> > something that does not support O_DIRECT - return an error, with
> > ->f_path and FMODE_OPENED left in place.
> >
> > Said that, what I do not understand is the point of that exercise -
> > why does LSM need to veto anything for those and why is security_file_open()
>
> I really think this is misguided. This should be done via proper hooks
> into apis that use O_PATH file descriptors for specific purposes but not
> for the generic open() path.

I agree that this patchset is at best incomplete, we don't add LSM
hooks without at least one in-tree LSM demonstrating a need for it,
and we don't see any of the LSMs actually making use of this new hook
placement in this patchset.  In the future Ryan, please ensure that
the patchset actually does "something" visible, e.g. new
functionality, bug fixes, etc.  I understand part of your intent was
to spark some discussion around O_PATH files, but without some initial
code to do something meaningful, it's hard to have any real discussion
that doesn't get lost in some rathole or tangent.

Beyond that, I can only speculate on Ryan's intent here, but based on
some off-list discussions, it's possible Ryan is (re)using the
security_file_open() hook in the O_PATH case not necessarily to gate
the creation of an O_PATH file, but rather to capture some of the
context when the O_PATH file is created.  However, if that was the
case I would think Ryan should be able to do that using the
security_file_alloc() hook, although we would need to pass the file
flags to that hook if Ryan wanted to do any special handling around
O_PATH.  Regardless, it's just guessing at this point and I've got
enough things asking for attention that I can't spend any more time on
this patchset simply guessing ...

-- 
paul-moore.com