On Thu, Mar 13, 2025 at 4:50 AM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > ---
> > > 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
