Re: [PATCH net-next] sctp: add bpf_bypass_getsockopt proto callback

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Wed, May 10, 2023 at 04:55:37PM +0200, Aleksandr Mikhalitsyn wrote:
> On Wed, May 10, 2023 at 4:39 PM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> >
> > On Wed, May 10, 2023 at 03:15:27PM +0200, Alexander Mikhalitsyn wrote:
> > > Add bpf_bypass_getsockopt proto callback and filter out
> > > SCTP_SOCKOPT_PEELOFF and SCTP_SOCKOPT_PEELOFF_FLAGS socket options
> > > from running eBPF hook on them.
> > >
> > > These options do fd_install(), and if BPF_CGROUP_RUN_PROG_GETSOCKOPT
> > > hook returns an error after success of the original handler
> > > sctp_getsockopt(...), userspace will receive an error from getsockopt
> > > syscall and will be not aware that fd was successfully installed into fdtable.
> > >
> > > This patch was born as a result of discussion around a new SCM_PIDFD interface:
> > > https://lore.kernel.org/all/20230413133355.350571-3-aleksandr.mikhalitsyn@canonical.com/
> >
> > I read some of the emails in there but I don't get why the fd leak is
> > special here. I mean, I get that it leaks, but masking the error
> > return like this can lead to several other problems in the application
> > as well.
> >
> > For example, SCTP_SOCKOPT_CONNECTX3 will trigger a connect(). If it
> > failed, and the hook returns success, the user app will at least log a
> > wrong "connection successful".
> >
> > If the hook can't be responsible for cleaning up before returning a
> > different value, then maybe we want to extend the list of sockopts in
> > here. AFAICT these would be the 3 most critical sockopts.
> >
> 
> Dear Marcelo,

Hello!

> 
> Thanks for pointing this out. Initially this problem was discovered by
> Christian Brauner and for SO_PEERPIDFD (a new SOL_SOCKET option that
> we want to add),
> after this I decided to check if we do fd_install in any other socket
> options in the kernel and found that we have 2 cases in SCTP. It was
> an accidental finding. :)
> 
> So, this patch isn't specific to fd_install things and probably we
> should filter out bpf hook from being called for other socket options
> as well.

Understood.

> 
> So, I need to filter out SCTP_SOCKOPT_CONNECTX3 and
> SCTP_SOCKOPT_PEELOFF* for SCTP, right?

Gotta say, it seems weird that it will filter out some of the most
important sockopts. But I'm not acquainted to bpf hooks so I won't
question (much? :) ) that.

Considering that filtering is needed, then yes, on getsock those are
ones I'm seeing that needs filtering. Otherwise they will either
trigger leakage or will confuse the application.

Should we care about setsock as well? We have SCTP_SOCKOPT_CONNECTX
and SCTP_SOCKOPT_CONNECTX_OLD in there, and well, I guess any of those
would misbehave if they failed and yet the hook returns success.

Thanks,
Marcelo