On Thu, Jun 19, 2025 at 12:01 AM Kuniyuki Iwashima <kuni1840@xxxxxxxxx> wrote:
> From: Paul Moore <paul@xxxxxxxxxxxxxx>
> Date: Wed, 18 Jun 2025 23:23:31 -0400
> > On Sat, Jun 14, 2025 at 4:40 PM Kuniyuki Iwashima <kuni1840@xxxxxxxxx> wrote:
> > > From: Paul Moore <paul@xxxxxxxxxxxxxx>
> > > Date: Sat, 14 Jun 2025 07:43:46 -0400
> > > > On June 13, 2025 6:24:15 PM Kuniyuki Iwashima <kuni1840@xxxxxxxxx> wrote:
> > > > > From: Kuniyuki Iwashima <kuniyu@xxxxxxxxxx>
> > > > >
> > > > > Since commit 77cbe1a6d873 ("af_unix: Introduce SO_PASSRIGHTS."),
> > > > > we can disable SCM_RIGHTS per socket, but it's not flexible.
> > > > >
> > > > > This series allows us to implement more fine-grained filtering for
> > > > > SCM_RIGHTS with BPF LSM.
> > > >
> > > > My ability to review this over the weekend is limited due to device and
> > > > network access, but I'll take a look next week.
> > > >
> > > > That said, it would be good if you could clarify the "filtering" aspect of
> > > > your comments; it may be obvious when I'm able to look at the full patchset
> > >
> > > I meant to mention that just below the quoted part :)
> > >
> > > ---8<---
> > > Changes:
> > > v2: Remove SCM_RIGHTS fd scrubbing functionality
> > > ---8<---
> >
> > Thanks :)
> >
> > While looking at your patches tonight, I was wondering if you had ever
> > considered adding a new LSM hook to __scm_send() that specifically
> > targets SCM_RIGHTS? I was thinking of something like this:
> >
> > diff --git a/net/core/scm.c b/net/core/scm.c
> > index 0225bd94170f..5fec8abc99f5 100644
> > --- a/net/core/scm.c
> > +++ b/net/core/scm.c
> > @@ -173,6 +173,9 @@ int __scm_send(struct socket *sock, struct msghdr *msg, stru
> > ct scm_cookie *p)
> > case SCM_RIGHTS:
> > if (!ops || ops->family != PF_UNIX)
> > goto error;
> > + err = security_sock_scm_rights(sock);
> > + if (err<0)
> > + goto error;
> > err=scm_fp_copy(cmsg, &p->fp);
> > if (err<0)
> > goto error;
> >
> > ... if I'm correct in my understanding of what you are trying to
> > accomplish, I believe this should allow you to meet your goals with a
> > much simpler and targeted approach. Or am I thinking about this
> > wrong?
>
> As BPF LSM is just a hook point and not tied to a specific socket,
> we cannot know who will receive the message in __scm_send().
Okay, based on your patches, I'm assuming you're okay with just the
socket endpoint, yes? Unfortunately, it's not really possible to get
the receiving task on the send side.
Beyond that, and given the immediate goal of access control based on
SCM_RIGHTS files, I think I'd rather see a unix_skb_parms passed to
the LSM instead of a skb as it will make the individual LSM subsystem
code a bit cleaner. I'd prefer a scm_cookie, but given the
destructive nature of unix_scm_to_skb() and the fact that it is called
before we've determined the socket endpoint (at least in the datagram
case), I don't think that will work.
I'm also not overly excited about converting security_unix_may_send()
into a per-msg/skb hook for both stream and datagram sends, that has
the potential for increasing the overhead for LSMs that really only
care about the datagram sends and the establishment of a stream
connection.
I'm open to suggestions, thoughts, etc. but I think modifying
security_unix_may_send() to take a unix_skb_params pointer, e.g.
'security_unix_may_send(sk, other, UNIXCB(skb))', and moving the
SEQ_PACKET restriction out of unix_dgram_sendmsg() and into the
existing LSM callbacks is okay. However, instead of adding
security_unix_may_send() to unix_stream_sendmsg(), I would suggest the
creation of a new hook, security_unix_send_scm(sk, other, UNIXCB(skb))
(feel free to suggest a different name), that would handle the per-skb
access control for streams.
Of course there is an issue with unix_skb_params not being defined
outside of net/unix, but from a practical perspective that is going to
be a challenge regardless of which, unix_skb_params or the full skb,
is passed to the LSM hook. You'll need to solve this with all the
relevant stakeholders regardless.
As a FYI, we've documented our policy/guidance on both modifying
existing LSM hooks as well as adding new hooks, see the link below.
https://github.com/LinuxSecurityModule/kernel/blob/main/README.md#new-lsm-hooks
> BTW, I was about to send v3, what target tree should be specified in
> subject, bpf-next or something else ?
I wouldn't worry too much about the subject, prefix, etc. as the To/CC
line tends to be more important, at least from a LSM subsystem
perspective. I would ensure that you send it to the LSM list and any
related lists, MAINTAINERS and/or scripts/get_maintainer.pl is your
friend here. Since this is almost surely LSM framework tree material,
my general rule is that I'll handle any merge conflicts so long as
your patch is based on the lsm/dev branch or Linus' default branch.
Of course, you can base your patch against any tree you like, and I'll
grab it, but if there are significant merge conflicts you'll likely
get a nasty email back about that ;)
As far as a v3 is concerned, while I generally don't like to tell
people *not* to post patches, I'll leave that up to you, I do think we
could benefit from some additional discussion here before you post a
v3. Reviewer time is precious and I would hate to see it spent on an
approach that still has open questions, just in case we all decide to
go in a different direction.
--
paul-moore.com
