| Message ID | 20230618103130.51628-1-maze@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a9628e88776eb7d045cf46467f1afdd0f7fe72ea |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] revert "net: align SO_RCVMARK required privileges with SO_MARK" | expand |
On Sun, Jun 18, 2023 at 03:31:30AM -0700, Maciej Żenczykowski wrote: > This reverts commit 1f86123b9749 ("net: align SO_RCVMARK required > privileges with SO_MARK") because the reasoning in the commit message > is not really correct: > SO_RCVMARK is used for 'reading' incoming skb mark (via cmsg), as such > it is more equivalent to 'getsockopt(SO_MARK)' which has no priv check > and retrieves the socket mark, rather than 'setsockopt(SO_MARK) which > sets the socket mark and does require privs. > > Additionally incoming skb->mark may already be visible if > sysctl_fwmark_reflect and/or sysctl_tcp_fwmark_accept are enabled. > > Furthermore, it is easier to block the getsockopt via bpf > (either cgroup setsockopt hook, or via syscall filters) > then to unblock it if it requires CAP_NET_RAW/ADMIN. > > On Android the socket mark is (among other things) used to store > the network identifier a socket is bound to. Setting it is privileged, > but retrieving it is not. We'd like unprivileged userspace to be able > to read the network id of incoming packets (where mark is set via > iptables [to be moved to bpf])... > > An alternative would be to add another sysctl to control whether > setting SO_RCVMARK is privilged or not. > (or even a MASK of which bits in the mark can be exposed) > But this seems like over-engineering... > > Note: This is a non-trivial revert, due to later merged commit e42c7beee71d > ("bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt()") > which changed both 'ns_capable' into 'sockopt_ns_capable' calls. > > Fixes: 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK") > Cc: Larysa Zaremba <larysa.zaremba@intel.com> > Cc: Simon Horman <simon.horman@corigine.com> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Eyal Birger <eyal.birger@gmail.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Patrick Rohr <prohr@google.com> > Signed-off-by: Maciej Żenczykowski <maze@google.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
From: Maciej Żenczykowski <maze@google.com> Date: Sun, 18 Jun 2023 03:31:30 -0700 > This reverts commit 1f86123b9749 ("net: align SO_RCVMARK required > privileges with SO_MARK") because the reasoning in the commit message > is not really correct: > SO_RCVMARK is used for 'reading' incoming skb mark (via cmsg), as such > it is more equivalent to 'getsockopt(SO_MARK)' which has no priv check > and retrieves the socket mark, rather than 'setsockopt(SO_MARK) which > sets the socket mark and does require privs. > > Additionally incoming skb->mark may already be visible if > sysctl_fwmark_reflect and/or sysctl_tcp_fwmark_accept are enabled. > > Furthermore, it is easier to block the getsockopt via bpf > (either cgroup setsockopt hook, or via syscall filters) > then to unblock it if it requires CAP_NET_RAW/ADMIN. > > On Android the socket mark is (among other things) used to store > the network identifier a socket is bound to. Setting it is privileged, > but retrieving it is not. We'd like unprivileged userspace to be able > to read the network id of incoming packets (where mark is set via > iptables [to be moved to bpf])... > > An alternative would be to add another sysctl to control whether > setting SO_RCVMARK is privilged or not. > (or even a MASK of which bits in the mark can be exposed) > But this seems like over-engineering... > > Note: This is a non-trivial revert, due to later merged commit e42c7beee71d > ("bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt()") > which changed both 'ns_capable' into 'sockopt_ns_capable' calls. > > Fixes: 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK") > Cc: Larysa Zaremba <larysa.zaremba@intel.com> > Cc: Simon Horman <simon.horman@corigine.com> > Cc: Paolo Abeni <pabeni@redhat.com> > Cc: Eyal Birger <eyal.birger@gmail.com> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Eric Dumazet <edumazet@google.com> > Cc: Patrick Rohr <prohr@google.com> > Signed-off-by: Maciej Żenczykowski <maze@google.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > net/core/sock.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/net/core/sock.c b/net/core/sock.c > index 24f2761bdb1d..6e5662ca00fe 100644 > --- a/net/core/sock.c > +++ b/net/core/sock.c > @@ -1362,12 +1362,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname, > __sock_set_mark(sk, val); > break; > case SO_RCVMARK: > - if (!sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) && > - !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) { > - ret = -EPERM; > - break; > - } > - > sock_valbool_flag(sk, SOCK_RCVMARK, valbool); > break; > > -- > 2.41.0.162.gfafddb0af9-goog
Hello: This patch was applied to netdev/net.git (main) by Paolo Abeni <pabeni@redhat.com>: On Sun, 18 Jun 2023 03:31:30 -0700 you wrote: > This reverts commit 1f86123b9749 ("net: align SO_RCVMARK required > privileges with SO_MARK") because the reasoning in the commit message > is not really correct: > SO_RCVMARK is used for 'reading' incoming skb mark (via cmsg), as such > it is more equivalent to 'getsockopt(SO_MARK)' which has no priv check > and retrieves the socket mark, rather than 'setsockopt(SO_MARK) which > sets the socket mark and does require privs. > > [...] Here is the summary with links: - [net,v2] revert "net: align SO_RCVMARK required privileges with SO_MARK" https://git.kernel.org/netdev/net/c/a9628e88776e You are awesome, thank you!
diff --git a/net/core/sock.c b/net/core/sock.c index 24f2761bdb1d..6e5662ca00fe 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1362,12 +1362,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname, __sock_set_mark(sk, val); break; case SO_RCVMARK: - if (!sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_RAW) && - !sockopt_ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) { - ret = -EPERM; - break; - } - sock_valbool_flag(sk, SOCK_RCVMARK, valbool); break;
This reverts commit 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK") because the reasoning in the commit message is not really correct: SO_RCVMARK is used for 'reading' incoming skb mark (via cmsg), as such it is more equivalent to 'getsockopt(SO_MARK)' which has no priv check and retrieves the socket mark, rather than 'setsockopt(SO_MARK) which sets the socket mark and does require privs. Additionally incoming skb->mark may already be visible if sysctl_fwmark_reflect and/or sysctl_tcp_fwmark_accept are enabled. Furthermore, it is easier to block the getsockopt via bpf (either cgroup setsockopt hook, or via syscall filters) then to unblock it if it requires CAP_NET_RAW/ADMIN. On Android the socket mark is (among other things) used to store the network identifier a socket is bound to. Setting it is privileged, but retrieving it is not. We'd like unprivileged userspace to be able to read the network id of incoming packets (where mark is set via iptables [to be moved to bpf])... An alternative would be to add another sysctl to control whether setting SO_RCVMARK is privilged or not. (or even a MASK of which bits in the mark can be exposed) But this seems like over-engineering... Note: This is a non-trivial revert, due to later merged commit e42c7beee71d ("bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt()") which changed both 'ns_capable' into 'sockopt_ns_capable' calls. Fixes: 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK") Cc: Larysa Zaremba <larysa.zaremba@intel.com> Cc: Simon Horman <simon.horman@corigine.com> Cc: Paolo Abeni <pabeni@redhat.com> Cc: Eyal Birger <eyal.birger@gmail.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Eric Dumazet <edumazet@google.com> Cc: Patrick Rohr <prohr@google.com> Signed-off-by: Maciej Żenczykowski <maze@google.com> --- net/core/sock.c | 6 ------ 1 file changed, 6 deletions(-)