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/[email protected]/
Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
Cc: Daniel Borkmann <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: Stanislav Fomichev <[email protected]>
Cc: Neil Horman <[email protected]>
Cc: Marcelo Ricardo Leitner <[email protected]>
Cc: Xin Long <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Suggested-by: Stanislav Fomichev <[email protected]>
Acked-by: Stanislav Fomichev <[email protected]>
Signed-off-by: Alexander Mikhalitsyn <[email protected]>
---
net/sctp/socket.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index cda8c2874691..a211a203003c 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -8281,6 +8281,35 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
return retval;
}
+static bool sctp_bpf_bypass_getsockopt(int level, int optname)
+{
+ if (level == SOL_SCTP) {
+ switch (optname) {
+ /*
+ * 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.
+ *
+ * Let's prevent bpf cgroup hook from running on them.
+ */
+ case SCTP_SOCKOPT_PEELOFF:
+ case SCTP_SOCKOPT_PEELOFF_FLAGS:
+ /*
+ * As pointed by Marcelo Ricardo Leitner it seems reasonable to skip
+ * bpf getsockopt hook for this sockopt too. Because internaly, it
+ * triggers connect() and if error will be masked userspace can be confused.
+ */
+ case SCTP_SOCKOPT_CONNECTX3:
+ return true;
+ default:
+ return false;
+ }
+ }
+
+ return false;
+}
+
static int sctp_hash(struct sock *sk)
{
/* STUB */
@@ -9650,6 +9679,7 @@ struct proto sctp_prot = {
.shutdown = sctp_shutdown,
.setsockopt = sctp_setsockopt,
.getsockopt = sctp_getsockopt,
+ .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
.sendmsg = sctp_sendmsg,
.recvmsg = sctp_recvmsg,
.bind = sctp_bind,
@@ -9705,6 +9735,7 @@ struct proto sctpv6_prot = {
.shutdown = sctp_shutdown,
.setsockopt = sctp_setsockopt,
.getsockopt = sctp_getsockopt,
+ .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
.sendmsg = sctp_sendmsg,
.recvmsg = sctp_recvmsg,
.bind = sctp_bind,
--
2.34.1
Hi,
Two things:
On Thu, May 11, 2023 at 02:31:48PM +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/[email protected]/
Cool, but the description is mentioning the CONNECTX3 sockopt.
>
> Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> Cc: Daniel Borkmann <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: Stanislav Fomichev <[email protected]>
> Cc: Neil Horman <[email protected]>
> Cc: Marcelo Ricardo Leitner <[email protected]>
> Cc: Xin Long <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Stanislav Fomichev <[email protected]>
> Acked-by: Stanislav Fomichev <[email protected]>
> Signed-off-by: Alexander Mikhalitsyn <[email protected]>
> ---
> net/sctp/socket.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index cda8c2874691..a211a203003c 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -8281,6 +8281,35 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
> return retval;
> }
>
> +static bool sctp_bpf_bypass_getsockopt(int level, int optname)
> +{
> + if (level == SOL_SCTP) {
> + switch (optname) {
> + /*
> + * 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.
> + *
> + * Let's prevent bpf cgroup hook from running on them.
> + */
This and..
> + case SCTP_SOCKOPT_PEELOFF:
> + case SCTP_SOCKOPT_PEELOFF_FLAGS:
> + /*
> + * As pointed by Marcelo Ricardo Leitner it seems reasonable to skip
> + * bpf getsockopt hook for this sockopt too. Because internaly, it
> + * triggers connect() and if error will be masked userspace can be confused.
> + */
..this comments can be removed, as they are easily visible on the
description later on for who is interested on why such lines were
added.
Thanks,
Marcelo
> + case SCTP_SOCKOPT_CONNECTX3:
> + return true;
> + default:
> + return false;
> + }
> + }
> +
> + return false;
> +}
> +
> static int sctp_hash(struct sock *sk)
> {
> /* STUB */
> @@ -9650,6 +9679,7 @@ struct proto sctp_prot = {
> .shutdown = sctp_shutdown,
> .setsockopt = sctp_setsockopt,
> .getsockopt = sctp_getsockopt,
> + .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
> .sendmsg = sctp_sendmsg,
> .recvmsg = sctp_recvmsg,
> .bind = sctp_bind,
> @@ -9705,6 +9735,7 @@ struct proto sctpv6_prot = {
> .shutdown = sctp_shutdown,
> .setsockopt = sctp_setsockopt,
> .getsockopt = sctp_getsockopt,
> + .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
> .sendmsg = sctp_sendmsg,
> .recvmsg = sctp_recvmsg,
> .bind = sctp_bind,
> --
> 2.34.1
>
On Thu, May 11, 2023 at 3:12 PM Marcelo Ricardo Leitner
<[email protected]> wrote:
>
> Hi,
Hi Marcelo,
thanks! Fixed in -v3.
Kind regards,
Alex
>
> Two things:
>
> On Thu, May 11, 2023 at 02:31:48PM +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/[email protected]/
>
> Cool, but the description is mentioning the CONNECTX3 sockopt.
>
> >
> > Fixes: 0d01da6afc54 ("bpf: implement getsockopt and setsockopt hooks")
> > Cc: Daniel Borkmann <[email protected]>
> > Cc: Christian Brauner <[email protected]>
> > Cc: Stanislav Fomichev <[email protected]>
> > Cc: Neil Horman <[email protected]>
> > Cc: Marcelo Ricardo Leitner <[email protected]>
> > Cc: Xin Long <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: Stanislav Fomichev <[email protected]>
> > Acked-by: Stanislav Fomichev <[email protected]>
> > Signed-off-by: Alexander Mikhalitsyn <[email protected]>
> > ---
> > net/sctp/socket.c | 31 +++++++++++++++++++++++++++++++
> > 1 file changed, 31 insertions(+)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index cda8c2874691..a211a203003c 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -8281,6 +8281,35 @@ static int sctp_getsockopt(struct sock *sk, int level, int optname,
> > return retval;
> > }
> >
> > +static bool sctp_bpf_bypass_getsockopt(int level, int optname)
> > +{
> > + if (level == SOL_SCTP) {
> > + switch (optname) {
> > + /*
> > + * 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.
> > + *
> > + * Let's prevent bpf cgroup hook from running on them.
> > + */
>
> This and..
>
> > + case SCTP_SOCKOPT_PEELOFF:
> > + case SCTP_SOCKOPT_PEELOFF_FLAGS:
> > + /*
> > + * As pointed by Marcelo Ricardo Leitner it seems reasonable to skip
> > + * bpf getsockopt hook for this sockopt too. Because internaly, it
> > + * triggers connect() and if error will be masked userspace can be confused.
> > + */
>
> ..this comments can be removed, as they are easily visible on the
> description later on for who is interested on why such lines were
> added.
>
> Thanks,
> Marcelo
>
> > + case SCTP_SOCKOPT_CONNECTX3:
> > + return true;
> > + default:
> > + return false;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
> > static int sctp_hash(struct sock *sk)
> > {
> > /* STUB */
> > @@ -9650,6 +9679,7 @@ struct proto sctp_prot = {
> > .shutdown = sctp_shutdown,
> > .setsockopt = sctp_setsockopt,
> > .getsockopt = sctp_getsockopt,
> > + .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
> > .sendmsg = sctp_sendmsg,
> > .recvmsg = sctp_recvmsg,
> > .bind = sctp_bind,
> > @@ -9705,6 +9735,7 @@ struct proto sctpv6_prot = {
> > .shutdown = sctp_shutdown,
> > .setsockopt = sctp_setsockopt,
> > .getsockopt = sctp_getsockopt,
> > + .bpf_bypass_getsockopt = sctp_bpf_bypass_getsockopt,
> > .sendmsg = sctp_sendmsg,
> > .recvmsg = sctp_recvmsg,
> > .bind = sctp_bind,
> > --
> > 2.34.1
> >