Message-ID: <20260115.b5977d57d52d@gnoack.org>
Date: Thu, 15 Jan 2026 11:10:02 +0100
From: Günther Noack <gnoack3000@...il.com>
To: Paul Moore <paul@...l-moore.com>,
	Christian Brauner <brauner@...nel.org>,
	Justin Suess <utilityemal77@...il.com>
Cc: Mickaël Salaün <mic@...ikod.net>,
	James Morris <jmorris@...ei.org>,
	"Serge E . Hallyn" <serge@...lyn.com>,
	linux-security-module@...r.kernel.org, Tingmao Wang <m@...wtm.org>,
	Samasth Norway Ananda <samasth.norway.ananda@...cle.com>,
	Matthieu Buffet <matthieu@...fet.re>,
	Mikhail Ivanov <ivanov.mikhail1@...wei-partners.com>,
	konstantin.meskhidze@...wei.com,
	Demi Marie Obenour <demiobenour@...il.com>,
	Alyssa Ross <hi@...ssa.is>, Jann Horn <jannh@...gle.com>,
	Tahera Fahimi <fahimitahera@...il.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v2 1/5] lsm: Add hook unix_path_connect

On Tue, Jan 13, 2026 at 06:27:15PM -0500, Paul Moore wrote:
> On Tue, Jan 13, 2026 at 4:34 AM Christian Brauner <brauner@...nel.org> wrote:
> > On Sat, Jan 10, 2026 at 03:32:57PM +0100, Günther Noack wrote:
> > > From: Justin Suess <utilityemal77@...il.com>
> > >
> > > Adds an LSM hook unix_path_connect.
> > >
> > > This hook is called to check the path of a named unix socket before a
> > > connection is initiated.
> > >
> > > Cc: Günther Noack <gnoack3000@...il.com>
> > > Signed-off-by: Justin Suess <utilityemal77@...il.com>
> > > ---
> > >  include/linux/lsm_hook_defs.h |  4 ++++
> > >  include/linux/security.h      | 11 +++++++++++
> > >  net/unix/af_unix.c            |  9 +++++++++
> > >  security/security.c           | 20 ++++++++++++++++++++
> > >  4 files changed, 44 insertions(+)
> 
> ...
> 
> > > diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> > > index 55cdebfa0da0..3aabe2d489ae 100644
> > > --- a/net/unix/af_unix.c
> > > +++ b/net/unix/af_unix.c
> > > @@ -1226,6 +1226,15 @@ static struct sock *unix_find_bsd(struct sockaddr_un *sunaddr, int addr_len,
> > >       if (!S_ISSOCK(inode->i_mode))
> > >               goto path_put;
> > >
> > > +     /*
> > > +      * We call the hook because we know that the inode is a socket
> > > +      * and we hold a valid reference to it via the path.
> > > +      */
> > > +     err = security_unix_path_connect(&path, type, flags);
> > > +     if (err)
> > > +             goto path_put;
> >
> > Couldn't we try reflowing the code here so the path is passed ...
> 
> It would be good if you could be a bit more specific about your
> desires here.  Are you talking about changing the
> unix_find_other()/unix_find_bsd() code path such that the path is
> available to unix_find_other() callers and not limited to the
> unix_find_bsd() scope?
> 
> > ... to
> > security_unix_stream_connect() and security_unix_may_send() so that all
> > LSMs get the same data and we don't have to have different LSMs hooks
> > into different callpaths that effectively do the same thing.
> >
> > I mean the objects are even in two completely different states between
> > those hooks. Even what type of sockets get a call to the LSM is
> > different between those two hooks.
> 
> I'm working on the assumption that you are talking about changing the
> UNIX socket code so that the path info is available to the existing
> _may_send() and _stream_connect() hooks.  If that isn't the case, and
> you're thinking of something different, disregard my comments below.
> 
> In both the unix_dgram_{connect(),sendmsg()}, aka
> security_unix_may_send(), cases and the unix_stream_connect(), aka
> security_unix_stream_connect(), case the call to unix_find_other() is
> done to lookup the other end of the communication channel, which does
> seem reasonably consistent to me.  Yes, of course, once you start
> getting into the specifics of the UNIX socket handling the unix_dgram_
> and unix_stream_ cases are very different, including their
> corresponding existing LSM hooks, but that doesn't mean in the context
> of unix_find_bsd() that security_unix_path_connect() doesn't have
> value.
> 
> The alternative would be some rather serious surgery in af_unix.c to
> persist the path struct from unix_find_bsd() until the later LSM hooks
> are executed.  It's certainly not impossible, but I'm not sure it is
> necessary or desirable at this point in time.  LSMs that wish to
> connect the information from _unix_path_connect() to either
> _unix_stream_connect() or _unix_may_send() can do so today without
> needing to substantially change af_unix.c.

Thanks for the review, Christan and Paul!

I am also unconvinced by the approach. It has also crossed my mind
before though, and my reasoning is as follows:

For reference, the function call hierarchy in af_unix.c is:

* unix_dgram_connect()   (*)
  * unix_find_other()
    * unix_find_bsd()
  * security_unix_may_send()

* unix_dgram_sendmsg()   (*)
  * unix_find_other()
    * unix_find_bsd()
  * security_unix_may_send()

* unix_stream_connect()  (*)
  * unix_find_other()
    * unix_find_bsd()
  * security_unix_stream_connect()

In my understanding, the hypothetical implementation would be:

* allocate a struct path on the stack of these proto_ops hook
  functions (marked with (*) above)
* pass a pointer to that path down to unix_find_other(), only to be
  filled out in the case that this is a pathname UNIX socket (not an
  abstract UNIX socket)
* pass a const pointer to that path to the LSM hooks

and then the LSM hooks would have to check whether the struct path has
non-zero pointers and could do the check.

This has the upside that it does not introduce a new LSM hook, but
only adds a "path" parameter to two existing LSM hooks.

On the other side, I see the following drawbacks:

* The more serious surgery in af_unix, which Paul also discussed:

  The function interface to unix_find_other() would need additional
  parameters for the sole purpose of supporting these LSM hooks and
  the refcounting of the struct path would have to be done in three
  functions instead of just in one.  That would complicate the
  af_unix.c logic, even when CONFIG_SECURITY_PATH is not set.

* The cases in which we pass a non-zero path argument to the LSM hooks
  would have surprising semantics IMHO, because it is not always set:

  * If a UNIX dgram user uses connect(2) and then calls sendmsg(2)
    without explicit recipient address, unix_dgram_sendmsg() would
    *not* do the look up any more and we can not provide the path to
    the security_unix_may_send() hook.
  * For abstract UNIX sockets it is not set either, of course.

  The path argument to the LSM hook would be present in the exact same
  cases where we now call the new UNIX path lookup LSM hook, but it
  would be invoked with a delay.

* Some properties of the resolved socket are still observable to
  userspace:

  When we only pass the path to a later LSM hook, there are a variety
  of additional error case checks in af_unix.c which are based on the
  "other" socket which we looked up through the path.  Examples:

  * was other shutdown(2)? (ECONNREFUSED on connect or EPIPE on dgram_sendmsg)
  * does other support SO_PASSRIGHTS (fd passing)? (EPERM on dgram_sendmsg)
  * would sendmsg pass sk_filter() (on dgram_sendmsg)

  For a LSM policy that is supposed to restrict the resolution of a
  UNIX socket by path, I would not expect such properties of the
  resolved socket to be observable?

  (And we also can't fix this up in the LSM by returning a matching
  error code, because at least unix_dgram_sendmsg() returns multiple
  different error codes in these error cases.)

  I would prefer if the correctness of our LSM did not depend on
  keeping track of the error scenarios in af_unix.c.  This seems
  brittle.

Overall, I am not convinced that using pre-existing hooks is the right
way and I would prefer the approach where we have a more dedicated LSM
hook for the path lookup.

Does that seem reasonable?  Let me know what you think.

–Günther

Powered by blists - more mailing lists