net: revert "align SO_RCVMARK required privileges with SO_MARK"

PATCH Superseded Maciej Żenczykowski 6 participants 8 messages Jun 5, 2023 – Jun 22, 2023

Subsystems (3)

Networking Network drivers Sockets

lore

Share & tools

lore

Maciej Żenczykowski

author

2023-06-05 08:12 UTC #0 lore raw

PATCH 1/1 net: revert "align SO_RCVMARK required privileges with SO_MARK"

This reverts:
commit 1f86123b97491cc2b5071d7f9933f0e91890c976
net: align SO_RCVMARK required privileges with SO_MARK

The commit referenced in the "Fixes" tag added the SO_RCVMARK socket option for receiving the skb mark in the ancillary data.

Since this is a new capability, and exposes admin configured details regarding the underlying network setup to sockets, let's align the needed capabilities with those of SO_MARK.

This reasoning 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 e42c7beee71d0d84a6193357e3525d0cf2a3e168
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 ("align SO_RCVMARK required privileges with SO_MARK")

Cc Eyal Birger <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Patrick Rohr <[email protected]>

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)

net/core/sock.c +0 -6

index 24f2761bdb1d..6e5662ca00fe 100644
@@ -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;

Signature · 1 line

2.41.0.rc0.172.g3f132b7071-goog

Larysa Zaremba

2023-06-05 15:30 UTC #1 lore raw

Quoted 11 lines · Maciej Żenczykowski

This reverts:
commit 1f86123b97491cc2b5071d7f9933f0e91890c976
net: align SO_RCVMARK required privileges with SO_MARK
The commit referenced in the "Fixes" tag added the SO_RCVMARK socket option for receiving the skb mark in the ancillary data.
Since this is a new capability, and exposes admin configured details regarding the underlying network setup to sockets, let's align the needed capabilities with those of SO_MARK.

No need to copy-paste reverted commit in full. Others are supposed to look it up in the log. The proper way to reference another commit is [0]:

Commit e21d2170f36602ae2708 ("video: remove unnecessary platform_set_drvdata()") removed the unnecessary platform_set_drvdata(), but left the variable "dev" unused, delete it.

Have you checked your patch with checkpatch? I am quite sure it would not allow copy-pasted commit message.

[0] kernel.org/doc/html/v4.17/process/submitting-patches.html

Also, please add patch prefix with tree name specified (net/net-next).

Quoted 24 lines

This reasoning 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...

Quoted 6 lines

Note: This is a non-trivial revert, due to later merged:
commit e42c7beee71d0d84a6193357e3525d0cf2a3e168
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 ("align SO_RCVMARK required privileges with SO_MARK")

I have never seen a reverted commit referenced with a "Fixes: " tag.

Quoted 16 lines

Cc Eyal Birger <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Patrick Rohr <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>
---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)
net/core/sock.c +0 -0
index 24f2761bdb1d..6e5662ca00fe 100644
@@ -1362,12 +1362,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
 		__sock_set_mark(sk, val);
 		break;

net/core/sock.c int sk_setsockopt(struct sock *sk, int level, int optname, · line 1367

Quoted 10 lines

net/core/sock.c +0 -6

 	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;

Both code and your reasoning seem fine.

Quoted 4 lines

Signature · 1 line
2.41.0.rc0.172.g3f132b7071-goog

Simon Horman

2023-06-05 17:30 UTC #2 lore raw

Quoted 21 lines · Larysa Zaremba

Quoted 11 lines · Maciej Żenczykowski
This reverts:
commit 1f86123b97491cc2b5071d7f9933f0e91890c976
net: align SO_RCVMARK required privileges with SO_MARK
The commit referenced in the "Fixes" tag added the SO_RCVMARK socket option for receiving the skb mark in the ancillary data.
Since this is a new capability, and exposes admin configured details regarding the underlying network setup to sockets, let's align the needed capabilities with those of SO_MARK.
No need to copy-paste reverted commit in full. Others are supposed to look it up in the log. The proper way to reference another commit is [0]:
Commit e21d2170f36602ae2708 ("video: remove unnecessary platform_set_drvdata()") removed the unnecessary platform_set_drvdata(), but left the variable "dev" unused, delete it.

Quoted 6 lines

Have you checked your patch with checkpatch? I am quite sure it would not allow copy-pasted commit message.
[0] kernel.org/doc/html/v4.17/process/submitting-patches.html
Also, please add patch prefix with tree name specified (net/net-next).

To add some colour to that, assuming 'net', and with a slightly fixed-up subject:

[PATCH net]: Revert "net: align SO_RCVMARK required privileges with SO_MARK"

Quoted 25 lines

Quoted 25 lines
This reasoning 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...

Quoted 8 lines

Quoted 6 lines
Note: This is a non-trivial revert, due to later merged:
commit e42c7beee71d0d84a6193357e3525d0cf2a3e168
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 ("align SO_RCVMARK required privileges with SO_MARK")
I have never seen a reverted commit referenced with a "Fixes: " tag.

Yes, maybe. Though an example seems to be:

	e7480a44d7c4 ("Revert "net: Remove low_thresh in ip defrag"")

If we do want a fixes tag, then I think it should be:

Fixes 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK")

Quoted 28 lines

Quoted 16 lines
Cc Eyal Birger <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Patrick Rohr <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>
---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)
net/core/sock.c +0 -0
index 24f2761bdb1d..6e5662ca00fe 100644
@@ -1362,12 +1362,6 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
 		__sock_set_mark(sk, val);
 		break;
net/core/sock.c int sk_setsockopt(struct sock *sk, int level, int optname, · line 1367
Quoted 10 lines
net/core/sock.c +0 -6
 	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;
 
Both code and your reasoning seem fine.

Signature · 1 line

pw-bot: cr

Paolo Abeni

maintainer

2023-06-06 11:37 UTC #3 lore raw

In reply to Maciej Żenczykowski

Quoted 30 lines · Maciej Żenczykowski

This reverts:
commit 1f86123b97491cc2b5071d7f9933f0e91890c976
net: align SO_RCVMARK required privileges with SO_MARK
The commit referenced in the "Fixes" tag added the SO_RCVMARK socket option for receiving the skb mark in the ancillary data.
Since this is a new capability, and exposes admin configured details regarding the underlying network setup to sockets, let's align the needed capabilities with those of SO_MARK.
This reasoning 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])...

Quoted 8 lines

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 e42c7beee71d0d84a6193357e3525d0cf2a3e168
bpf: net: Consider has_current_bpf_ctx() when testing capable() in sk_setsockopt()

When you repost, please additionally change the above with the usual commit reference, e.g. commit <12# hash> ("<title>")

Quoted 29 lines

which changed both 'ns_capable' into 'sockopt_ns_capable' calls.
Fixes 1f86123b9749 ("align SO_RCVMARK required privileges with SO_MARK")
Cc Eyal Birger <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Patrick Rohr <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>
---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)
net/core/sock.c +0 -6
index 24f2761bdb1d..6e5662ca00fe 100644
@@ -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;
 

Maciej Żenczykowski

author

2023-06-18 10:31 UTC #4 lore raw

PATCH v2 1/1 revert "net: align SO_RCVMARK required privileges with SO_MARK"

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.

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 <[email protected]>

Cc Simon Horman <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Eyal Birger <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Patrick Rohr <[email protected]>

Signed-off-by Maciej Żenczykowski <[email protected]>

---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)

net/core/sock.c +0 -6

index 24f2761bdb1d..6e5662ca00fe 100644
@@ -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;

Signature · 1 line

2.41.0.162.gfafddb0af9-goog

Simon Horman

Reviewed-by on v2 1/1

2023-06-19 14:12 UTC #5 lore raw

Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"

Quoted 30 lines · Maciej Żenczykowski

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.

Quoted 9 lines

Fixes 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK")
Cc Larysa Zaremba <[email protected]>
Cc Simon Horman <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Eyal Birger <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Patrick Rohr <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>

Reviewed-by Simon Horman <[email protected]>

Kuniyuki Iwashima

Reviewed-by on v2 1/1

2023-06-20 00:18 UTC #6 lore raw

Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"

In reply to Maciej Żenczykowski

From: Maciej Żenczykowski <[email protected]>
Date: Sun, 18 Jun 2023 03:31:30 -0700

Quoted 30 lines

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.

Quoted 9 lines

Fixes 1f86123b9749 ("net: align SO_RCVMARK required privileges with SO_MARK")
Cc Larysa Zaremba <[email protected]>
Cc Simon Horman <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Eyal Birger <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Patrick Rohr <[email protected]>
Signed-off-by Maciej Żenczykowski <[email protected]>

Reviewed-by Kuniyuki Iwashima <[email protected]>

Quoted 23 lines

---
 net/core/sock.c | 6 ------
 1 file changed, 6 deletions(-)

net/core/sock.c +0 -6

index 24f2761bdb1d..6e5662ca00fe 100644
@@ -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;

Signature · 1 line

2.41.0.162.gfafddb0af9-goog

[email protected]

CI Applied

2023-06-22 10:00 UTC #7 lore raw

Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"

In reply to Maciej Żenczykowski

This patch was applied to netdev/net.git (main) by Paolo Abeni <[email protected]>:

Hello:

This patch was applied to netdev/net.git (main)
by Paolo Abeni <[email protected]>:

Quoted 9 lines · 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!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html