[mptcp-next] tcp: ulp: diag: remove net admin restriction

Commit Message

Matthieu Baerts Feb. 26, 2025, 11:26 a.m. UTC

Since its introduction in commit 61723b393292 ("tcp: ulp: add functions
to dump ulp-specific information"), the ULP diag info have been exported
only if the requester had CAP_NET_ADMIN.

It looks like there is nothing sensitive being exported here by the
MPTCP and KTLS layers. So it seems safe to remove this restriction in
order to ease the debugging from the userspace side without requiring
additional capabilities.

Signed-off-by: Matthieu Baerts (NGI0) <[e-mail]>
---
 net/ipv4/tcp_diag.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)


---
base-commit: 1238896935ea03f333a183a32fab666cc0c20e3b
change-id: 20250226-mptcp-tcp-ulp-diag-cap-a4d9b7cd91ec

Best regards,

Comments

Davide Caratti Feb. 26, 2025, 11:39 a.m. UTC | #1

hi Matthieu, thanks for the patch!

On Wed, Feb 26, 2025 at 12:26:36PM +0100, Matthieu Baerts (NGI0) wrote:
> Since its introduction in commit 61723b393292 ("tcp: ulp: add functions
> to dump ulp-specific information"), the ULP diag info have been exported
> only if the requester had CAP_NET_ADMIN.
> 
> It looks like there is nothing sensitive being exported here by the
> MPTCP and KTLS layers. 

if I well remember, at the beginning we have been "cautious" about dumping
local/remote token value to unprivileged users (via 'get_info()' function).
But on a second thought this scrutiny makes no sense for values that travel
in cleartext inside MP_OPTIONS, so...

> So it seems safe to remove this restriction in
> order to ease the debugging from the userspace side without requiring
> additional capabilities.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <[e-mail]>

Reviewed-by: Davide Caratti <[e-mail]>

MPTCP CI Feb. 26, 2025, 12:35 p.m. UTC | #2

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/13543107873

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/39f106995494
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=937982


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Mat Martineau Feb. 28, 2025, 9:39 p.m. UTC | #3

On Wed, 26 Feb 2025, Matthieu Baerts (NGI0) wrote:

> Since its introduction in commit 61723b393292 ("tcp: ulp: add functions
> to dump ulp-specific information"), the ULP diag info have been exported
> only if the requester had CAP_NET_ADMIN.
>
> It looks like there is nothing sensitive being exported here by the
> MPTCP and KTLS layers. So it seems safe to remove this restriction in
> order to ease the debugging from the userspace side without requiring
> additional capabilities.
>

Hi Matthieu -

I agree the token values aren't sensitive (we definitely wouldn't want to 
expose the keys exchanged with MP_CAPABLE, for example). Token values are 
like port numbers in terms of what's exposed.

The DSS mapping and ssn_offset do give all users on the system access to 
narrow ranges of values for the subflow TCP sequence numbers and 
MPTCP-level DSNs. The diag interface doesn't expose TCP sequence numbers 
for TCP sockets, it doesn't seem like a good idea to leak those through 
the mapping values either. WDYT?

- Mat

> Signed-off-by: Matthieu Baerts (NGI0) <[e-mail]>
> ---
> net/ipv4/tcp_diag.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
> index f428ecf9120f2f596e1d67db2b2a0d0d0e211905..8257bd85e067ee862034c95745216c443113bdb0 100644
> --- a/net/ipv4/tcp_diag.c
> +++ b/net/ipv4/tcp_diag.c
> @@ -113,6 +113,7 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
> 			    struct sk_buff *skb)
> {
> 	struct inet_connection_sock *icsk = inet_csk(sk);
> +	const struct tcp_ulp_ops *ulp_ops;
> 	int err = 0;
>
> #ifdef CONFIG_TCP_MD5SIG
> @@ -129,15 +130,13 @@ static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
> 	}
> #endif
>
> -	if (net_admin) {
> -		const struct tcp_ulp_ops *ulp_ops;
> -
> -		ulp_ops = icsk->icsk_ulp_ops;
> -		if (ulp_ops)
> -			err = tcp_diag_put_ulp(skb, sk, ulp_ops);
> -		if (err)
> +	ulp_ops = icsk->icsk_ulp_ops;
> +	if (ulp_ops) {
> +		err = tcp_diag_put_ulp(skb, sk, ulp_ops);
> +		if (err < 0)
> 			return err;
> 	}
> +
> 	return 0;
> }
>
> @@ -164,7 +163,7 @@ static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
> 	}
> #endif
>
> -	if (net_admin && sk_fullsock(sk)) {
> +	if (sk_fullsock(sk)) {
> 		const struct tcp_ulp_ops *ulp_ops;
>
> 		ulp_ops = icsk->icsk_ulp_ops;
>
> ---
> base-commit: 1238896935ea03f333a183a32fab666cc0c20e3b
> change-id: 20250226-mptcp-tcp-ulp-diag-cap-a4d9b7cd91ec
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <[e-mail]>
>
>
>

Matthieu Baerts March 1, 2025, 10:26 a.m. UTC | #4

Hi Mat,

On 28/02/2025 22:39, Mat Martineau wrote:
> On Wed, 26 Feb 2025, Matthieu Baerts (NGI0) wrote:
> 
>> Since its introduction in commit 61723b393292 ("tcp: ulp: add functions
>> to dump ulp-specific information"), the ULP diag info have been exported
>> only if the requester had CAP_NET_ADMIN.
>>
>> It looks like there is nothing sensitive being exported here by the
>> MPTCP and KTLS layers. So it seems safe to remove this restriction in
>> order to ease the debugging from the userspace side without requiring
>> additional capabilities.
>>
> 
> Hi Matthieu -
> 
> I agree the token values aren't sensitive (we definitely wouldn't want
> to expose the keys exchanged with MP_CAPABLE, for example). Token values
> are like port numbers in terms of what's exposed.
> 
> The DSS mapping and ssn_offset do give all users on the system access to
> narrow ranges of values for the subflow TCP sequence numbers and MPTCP-
> level DSNs. The diag interface doesn't expose TCP sequence numbers for
> TCP sockets, it doesn't seem like a good idea to leak those through the
> mapping values either. WDYT?

Indeed, I was still thinking about that, and that's why I didn't send
this patch to netdev.

It sounds safer to let the different layers decide on what can be
exposed. In MPTCP case, it sounds indeed better not to expose sequence
numbers, only the token, IDs, and flags.

I will resurrect my previous version where I passed "net_admin" to the
two ULP diag callbacks.

Cheers,
Matt

Patch

diff --git a/net/ipv4/tcp_diag.c b/net/ipv4/tcp_diag.c
index f428ecf9120f2f596e1d67db2b2a0d0d0e211905..8257bd85e067ee862034c95745216c443113bdb0 100644
--- a/net/ipv4/tcp_diag.c
+++ b/net/ipv4/tcp_diag.c
@@ -113,6 +113,7 @@  static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 			    struct sk_buff *skb)
 {
 	struct inet_connection_sock *icsk = inet_csk(sk);
+	const struct tcp_ulp_ops *ulp_ops;
 	int err = 0;
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -129,15 +130,13 @@  static int tcp_diag_get_aux(struct sock *sk, bool net_admin,
 	}
 #endif
 
-	if (net_admin) {
-		const struct tcp_ulp_ops *ulp_ops;
-
-		ulp_ops = icsk->icsk_ulp_ops;
-		if (ulp_ops)
-			err = tcp_diag_put_ulp(skb, sk, ulp_ops);
-		if (err)
+	ulp_ops = icsk->icsk_ulp_ops;
+	if (ulp_ops) {
+		err = tcp_diag_put_ulp(skb, sk, ulp_ops);
+		if (err < 0)
 			return err;
 	}
+
 	return 0;
 }
 
@@ -164,7 +163,7 @@  static size_t tcp_diag_get_aux_size(struct sock *sk, bool net_admin)
 	}
 #endif
 
-	if (net_admin && sk_fullsock(sk)) {
+	if (sk_fullsock(sk)) {
 		const struct tcp_ulp_ops *ulp_ops;
 
 		ulp_ops = icsk->icsk_ulp_ops;