netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK"
@ 2023-06-05  8:12 Maciej Żenczykowski
  2023-06-05 15:27 ` Larysa Zaremba
  2023-06-06 11:37 ` Paolo Abeni
  0 siblings, 2 replies; 8+ messages in thread
From: Maciej Żenczykowski @ 2023-06-05  8:12 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Linux Network Development Mailing List, Maciej Żenczykowski,
	Eyal Birger, Jakub Kicinski, Eric Dumazet, Patrick Rohr

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 <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(-)

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.rc0.172.g3f132b7071-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK"
  2023-06-05  8:12 [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK" Maciej Żenczykowski
@ 2023-06-05 15:27 ` Larysa Zaremba
  2023-06-05 17:30   ` Simon Horman
  2023-06-06 11:37 ` Paolo Abeni
  1 sibling, 1 reply; 8+ messages in thread
From: Larysa Zaremba @ 2023-06-05 15:27 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Maciej Żenczykowski, Linux Network Development Mailing List,
	Eyal Birger, Jakub Kicinski, Eric Dumazet, Patrick Rohr

On Mon, Jun 05, 2023 at 01:12:18AM -0700, Maciej Żenczykowski wrote:
> 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).

> 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")

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

> 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(-)
> 
> 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;
>  

Both code and your reasoning seem fine.

> -- 
> 2.41.0.rc0.172.g3f132b7071-goog
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK"
  2023-06-05 15:27 ` Larysa Zaremba
@ 2023-06-05 17:30   ` Simon Horman
  0 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2023-06-05 17:30 UTC (permalink / raw)
  To: Larysa Zaremba
  Cc: Maciej Żenczykowski, Maciej Żenczykowski,
	Linux Network Development Mailing List, Eyal Birger,
	Jakub Kicinski, Eric Dumazet, Patrick Rohr

On Mon, Jun 05, 2023 at 05:27:32PM +0200, Larysa Zaremba wrote:
> On Mon, Jun 05, 2023 at 01:12:18AM -0700, Maciej Żenczykowski wrote:
> > 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).

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"

> 
> > 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")
> 
> 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")

> > 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(-)
> > 
> > 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;
> >  
> 
> Both code and your reasoning seem fine.

-- 
pw-bot: cr


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK"
  2023-06-05  8:12 [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK" Maciej Żenczykowski
  2023-06-05 15:27 ` Larysa Zaremba
@ 2023-06-06 11:37 ` Paolo Abeni
  2023-06-18 10:31   ` [PATCH net v2] revert "net: align " Maciej Żenczykowski
  1 sibling, 1 reply; 8+ messages in thread
From: Paolo Abeni @ 2023-06-06 11:37 UTC (permalink / raw)
  To: Maciej Żenczykowski, Maciej Żenczykowski
  Cc: Linux Network Development Mailing List, Eyal Birger,
	Jakub Kicinski, Eric Dumazet, Patrick Rohr

On Mon, 2023-06-05 at 01:12 -0700, Maciej Żenczykowski wrote:
> 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()

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

> which changed both 'ns_capable' into 'sockopt_ns_capable' calls.
> 
> Fixes: 1f86123b9749 ("align SO_RCVMARK required privileges with SO_MARK")
> 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(-)
> 
> 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;
>  


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"
  2023-06-06 11:37 ` Paolo Abeni
@ 2023-06-18 10:31   ` Maciej Żenczykowski
  2023-06-19 14:12     ` Simon Horman
                       ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Maciej Żenczykowski @ 2023-06-18 10:31 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Linux Network Development Mailing List, Maciej Żenczykowski,
	Larysa Zaremba, Simon Horman, Paolo Abeni, Eyal Birger,
	Jakub Kicinski, Eric Dumazet, Patrick Rohr

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(-)

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"
  2023-06-18 10:31   ` [PATCH net v2] revert "net: align " Maciej Żenczykowski
@ 2023-06-19 14:12     ` Simon Horman
  2023-06-20  0:17     ` Kuniyuki Iwashima
  2023-06-22 10:00     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 8+ messages in thread
From: Simon Horman @ 2023-06-19 14:12 UTC (permalink / raw)
  To: Maciej Żenczykowski
  Cc: Maciej Żenczykowski, Linux Network Development Mailing List,
	Larysa Zaremba, Paolo Abeni, Eyal Birger, Jakub Kicinski,
	Eric Dumazet, Patrick Rohr

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>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"
  2023-06-18 10:31   ` [PATCH net v2] revert "net: align " Maciej Żenczykowski
  2023-06-19 14:12     ` Simon Horman
@ 2023-06-20  0:17     ` Kuniyuki Iwashima
  2023-06-22 10:00     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 8+ messages in thread
From: Kuniyuki Iwashima @ 2023-06-20  0:17 UTC (permalink / raw)
  To: maze
  Cc: edumazet, eyal.birger, kuba, larysa.zaremba, netdev, pabeni,
	prohr, simon.horman, zenczykowski, kuniyu

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH net v2] revert "net: align SO_RCVMARK required privileges with SO_MARK"
  2023-06-18 10:31   ` [PATCH net v2] revert "net: align " Maciej Żenczykowski
  2023-06-19 14:12     ` Simon Horman
  2023-06-20  0:17     ` Kuniyuki Iwashima
@ 2023-06-22 10:00     ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2023-06-22 10:00 UTC (permalink / raw)
  To: =?utf-8?q?Maciej_=C5=BBenczykowski_=3Cmaze=40google=2Ecom=3E?=
  Cc: zenczykowski, netdev, larysa.zaremba, simon.horman, pabeni,
	eyal.birger, kuba, edumazet, prohr

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2023-06-22 10:00 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-05  8:12 [PATCH] net: revert "align SO_RCVMARK required privileges with SO_MARK" Maciej Żenczykowski
2023-06-05 15:27 ` Larysa Zaremba
2023-06-05 17:30   ` Simon Horman
2023-06-06 11:37 ` Paolo Abeni
2023-06-18 10:31   ` [PATCH net v2] revert "net: align " Maciej Żenczykowski
2023-06-19 14:12     ` Simon Horman
2023-06-20  0:17     ` Kuniyuki Iwashima
2023-06-22 10:00     ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).