[PATCH mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access

[PATCH mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access

[Gregory Detal]

The current behavior allows to write to mptcp_sock at offset that is
defined in mptcp_subflow_context and vice versa.

This fixes this by splitting the checks for each struct type.

Signed-off-by: Gregory Detal <gregory.detal@gmail.com>
---
 net/mptcp/bpf.c | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
index 208e5d3f066f..57c47bb430b1 100644
--- a/net/mptcp/bpf.c
+++ b/net/mptcp/bpf.c
@@ -47,24 +47,32 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log,
 	size_t end;
 
 	t = btf_type_by_id(reg->btf, reg->btf_id);
-	if (t != mptcp_sock_type && t != mptcp_subflow_type) {
-		bpf_log(log, "only access to mptcp sock or subflow is supported\n");
-		return -EACCES;
-	}
 
-	switch (off) {
-	case offsetof(struct mptcp_sock, snd_burst):
-		end = offsetofend(struct mptcp_sock, snd_burst);
-		break;
-	case offsetof(struct mptcp_subflow_context, scheduled):
-		end = offsetofend(struct mptcp_subflow_context, scheduled);
-		break;
-	case offsetof(struct mptcp_subflow_context, avg_pacing_rate):
-		end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate);
-		break;
-	default:
-		bpf_log(log, "no write support to %s at off %d\n",
-			t == mptcp_sock_type ? "mptcp_sock" : "mptcp_subflow_context", off);
+	if (t == mptcp_sock_type) {
+		switch (off) {
+		case offsetof(struct mptcp_sock, snd_burst):
+			end = offsetofend(struct mptcp_sock, snd_burst);
+			break;
+		default:
+			bpf_log(log, "no write support to mptcp_sock at off %d\n",
+				off);
+			return -EACCES;
+		}
+	} else if (t == mptcp_subflow_type) {
+		switch (off) {
+		case offsetof(struct mptcp_subflow_context, scheduled):
+			end = offsetofend(struct mptcp_subflow_context, scheduled);
+			break;
+		case offsetof(struct mptcp_subflow_context, avg_pacing_rate):
+			end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate);
+			break;
+		default:
+			bpf_log(log, "no write support to mptcp_subflow_context at off %d\n",
+				off);
+			return -EACCES;
+		}
+	} else {
+		bpf_log(log, "only access to mptcp sock or subflow is supported\n");
 		return -EACCES;
 	}
 

---
base-commit: 56030f9d3812071365435354c0eb5ffb3504e58a
change-id: 20240503-bpf_fix_access-a360b88c1534

Best regards,
-- 
Gregory Detal <gregory.detal@gmail.com>

Re: [PATCH mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access

[MPTCP CI]

Hi Gregory,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Unstable: 1 failed test(s): selftest_simult_flows 🔴
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8944103225

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


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)

Re: [PATCH mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access

[Geliang Tang]

Hi Gregory,

On Fri, May 03, 2024 at 07:33:25PM +0000, Gregory Detal wrote:
> The current behavior allows to write to mptcp_sock at offset that is
> defined in mptcp_subflow_context and vice versa.
> 
> This fixes this by splitting the checks for each struct type.
> 
> Signed-off-by: Gregory Detal <gregory.detal@gmail.com>

Thanks for this fix. Looks good to me.

Reviewed-by: Geliang Tang <geliang@kernel.org>

-Geliang

> ---
>  net/mptcp/bpf.c | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/bpf.c b/net/mptcp/bpf.c
> index 208e5d3f066f..57c47bb430b1 100644
> --- a/net/mptcp/bpf.c
> +++ b/net/mptcp/bpf.c
> @@ -47,24 +47,32 @@ static int bpf_mptcp_sched_btf_struct_access(struct bpf_verifier_log *log,
>  	size_t end;
>  
>  	t = btf_type_by_id(reg->btf, reg->btf_id);
> -	if (t != mptcp_sock_type && t != mptcp_subflow_type) {
> -		bpf_log(log, "only access to mptcp sock or subflow is supported\n");
> -		return -EACCES;
> -	}
>  
> -	switch (off) {
> -	case offsetof(struct mptcp_sock, snd_burst):
> -		end = offsetofend(struct mptcp_sock, snd_burst);
> -		break;
> -	case offsetof(struct mptcp_subflow_context, scheduled):
> -		end = offsetofend(struct mptcp_subflow_context, scheduled);
> -		break;
> -	case offsetof(struct mptcp_subflow_context, avg_pacing_rate):
> -		end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate);
> -		break;
> -	default:
> -		bpf_log(log, "no write support to %s at off %d\n",
> -			t == mptcp_sock_type ? "mptcp_sock" : "mptcp_subflow_context", off);
> +	if (t == mptcp_sock_type) {
> +		switch (off) {
> +		case offsetof(struct mptcp_sock, snd_burst):
> +			end = offsetofend(struct mptcp_sock, snd_burst);
> +			break;
> +		default:
> +			bpf_log(log, "no write support to mptcp_sock at off %d\n",
> +				off);
> +			return -EACCES;
> +		}
> +	} else if (t == mptcp_subflow_type) {
> +		switch (off) {
> +		case offsetof(struct mptcp_subflow_context, scheduled):
> +			end = offsetofend(struct mptcp_subflow_context, scheduled);
> +			break;
> +		case offsetof(struct mptcp_subflow_context, avg_pacing_rate):
> +			end = offsetofend(struct mptcp_subflow_context, avg_pacing_rate);
> +			break;
> +		default:
> +			bpf_log(log, "no write support to mptcp_subflow_context at off %d\n",
> +				off);
> +			return -EACCES;
> +		}
> +	} else {
> +		bpf_log(log, "only access to mptcp sock or subflow is supported\n");
>  		return -EACCES;
>  	}
>  
> 
> ---
> base-commit: 56030f9d3812071365435354c0eb5ffb3504e58a
> change-id: 20240503-bpf_fix_access-a360b88c1534
> 
> Best regards,
> -- 
> Gregory Detal <gregory.detal@gmail.com>
> 

Re: [PATCH mptcp-next] Squash to "mptcp: add bpf_mptcp_sched_ops" -- fix bpf access

[Matthieu Baerts]

Hi Gregory, Geliang,

On 03/05/2024 21:33, Gregory Detal wrote:
> The current behavior allows to write to mptcp_sock at offset that is
> defined in mptcp_subflow_context and vice versa.
> 
> This fixes this by splitting the checks for each struct type.

Thank you for the fix and the review!

Now in our tree:

New patches for t/upstream:
- 17783ae38851: "squashed" in "bpf: Add bpf_mptcp_sched_ops"
- c0f6d508db13: "Signed-off-by" + "Co-developed-by"
- Results: 77024827f43c..f16f6f211e69 (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/26924223a2c1354a5444e3e70b286fee1a3f1c67/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.