Skip to contentSkip to conversation

bpf: Fix same-register dst/src OOB read and pointer leak in sock_ops

PATCH Superseded Jay Wang 1 participant 2 messages Jul 28

Subsystems (2)
Stable branch BPF
lore
Share & tools
lore

Jay Wang

author
#0 lore raw

PATCH 1/1 bpf: Fix same-register dst/src OOB read and pointer leak in sock_ops

From: Jiayuan Chen <[email protected]>

[ Upstream commit 10f86a2a5c91fc4c4d001960f1c21abe52545ef6 ]

When a BPF sock_ops program accesses ctx fields with dst_reg == src_reg, the SOCK_OPS_GET_SK() and SOCK_OPS_GET_FIELD() macros fail to zero the destination register in the !fullsock / !locked_tcp_sock path.

Both macros borrow a temporary register to check is_fullsock / is_locked_tcp_sock when dst_reg == src_reg, because dst_reg holds the ctx pointer. When the check is false (e.g., TCP_NEW_SYN_RECV state with a request_sock), dst_reg should be zeroed but is not, leaving the stale ctx pointer:

- SOCK_OPS_GET_SK: dst_reg retains the ctx pointer, passes NULL checks as PTR_TO_SOCKET_OR_NULL, and can be used as a bogus socket pointer, leading to stack-out-of-bounds access in helpers like bpf_skc_to_tcp6_sock().

- SOCK_OPS_GET_FIELD: dst_reg retains the ctx pointer which the verifier believes is a SCALAR_VALUE, leaking a kernel pointer.

Fix both macros by:
- Changing JMP_A(1) to JMP_A(2) in the fullsock path to skip the added instruction.
- Adding BPF_MOV64_IMM(si->dst_reg, 0) after the temp register restore in the !fullsock path, placed after the restore because dst_reg == src_reg means we need src_reg intact to read ctx->temp.

Fixes fd09af010788 ("bpf: sock_ops ctx access may stomp registers in corner case")
Fixes 84f44df664e9 ("bpf: sock_ops sk access may stomp registers when dst_reg = src_reg")
Reported-by Quan Sun <[email protected]>
Reported-by Yinhao Hu <[email protected]>
Reported-by Kaiyan Mei <[email protected]>
Reported-by Dongliang Mu <[email protected]>
Reviewed-by Emil Tsalapatis <[email protected]>
Closes https://lore.kernel.org/bpf/[email protected]/T/#u open here
Signed-off-by Jiayuan Chen <[email protected]>
Acked-by Martin KaFai Lau <[email protected]>
Link https://patch.msgid.link/[email protected] open here
Signed-off-by Jakub Kicinski <[email protected]>
Signed-off-by Sasha Levin <[email protected]>
Signed-off-by Jay Wang <[email protected]>
---
 net/core/filter.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
net/core/filter.c +4 -2
index b03621405d062..609a97237b424 100644
@@ -10467,10 +10467,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 				      si->dst_reg, si->dst_reg,		      \
 				      offsetof(OBJ, OBJ_FIELD));	      \
 		if (si->dst_reg == si->src_reg)	{			      \
-			*insn++ = BPF_JMP_A(1);				      \
+			*insn++ = BPF_JMP_A(2);				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
 				      temp));				      \
+			*insn++ = BPF_MOV64_IMM(si->dst_reg, 0);	      \
 		}							      \
 	} while (0)
 

@@ -10504,10 +10505,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 				      si->dst_reg, si->src_reg,		      \
 				      offsetof(struct bpf_sock_ops_kern, sk));\
 		if (si->dst_reg == si->src_reg)	{			      \
-			*insn++ = BPF_JMP_A(1);				      \
+			*insn++ = BPF_JMP_A(2);				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
 				      temp));				      \
+			*insn++ = BPF_MOV64_IMM(si->dst_reg, 0);	      \
 		}							      \
 	} while (0)
Signature · 1 line
2.47.3
author
#1 lore raw

PATCH v2 1/1 bpf: Fix same-register dst/src OOB read and pointer leak in sock_ops

From: Jiayuan Chen <[email protected]>

[ Upstream commit 10f86a2a5c91fc4c4d001960f1c21abe52545ef6 ]

When a BPF sock_ops program accesses ctx fields with dst_reg == src_reg, the SOCK_OPS_GET_SK() and SOCK_OPS_GET_FIELD() macros fail to zero the destination register in the !fullsock / !locked_tcp_sock path.

Both macros borrow a temporary register to check is_fullsock / is_locked_tcp_sock when dst_reg == src_reg, because dst_reg holds the ctx pointer. When the check is false (e.g., TCP_NEW_SYN_RECV state with a request_sock), dst_reg should be zeroed but is not, leaving the stale ctx pointer:

- SOCK_OPS_GET_SK: dst_reg retains the ctx pointer, passes NULL checks as PTR_TO_SOCKET_OR_NULL, and can be used as a bogus socket pointer, leading to stack-out-of-bounds access in helpers like bpf_skc_to_tcp6_sock().

- SOCK_OPS_GET_FIELD: dst_reg retains the ctx pointer which the verifier believes is a SCALAR_VALUE, leaking a kernel pointer.

Fix both macros by:
- Changing JMP_A(1) to JMP_A(2) in the fullsock path to skip the added instruction.
- Adding BPF_MOV64_IMM(si->dst_reg, 0) after the temp register restore in the !fullsock path, placed after the restore because dst_reg == src_reg means we need src_reg intact to read ctx->temp.

Fixes fd09af010788 ("bpf: sock_ops ctx access may stomp registers in corner case")
Fixes 84f44df664e9 ("bpf: sock_ops sk access may stomp registers when dst_reg = src_reg")
Reported-by Quan Sun <[email protected]>
Reported-by Yinhao Hu <[email protected]>
Reported-by Kaiyan Mei <[email protected]>
Reported-by Dongliang Mu <[email protected]>
Reviewed-by Emil Tsalapatis <[email protected]>
Closes https://lore.kernel.org/bpf/[email protected]/T/#u open here
Signed-off-by Jiayuan Chen <[email protected]>
Acked-by Martin KaFai Lau <[email protected]>
Link https://patch.msgid.link/[email protected] open here
Signed-off-by Jakub Kicinski <[email protected]>
Signed-off-by Sasha Levin <[email protected]>
Signed-off-by Jay Wang <[email protected]>

---
v2: rebased onto v6.12.98 to resolve context conflict reported by CI

 net/core/filter.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
net/core/filter.c +4 -2
index f54e7152d6879..90e396423eb7a 100644
@@ -10483,10 +10483,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 				      si->dst_reg, si->dst_reg,		      \
 				      offsetof(OBJ, OBJ_FIELD));	      \
 		if (si->dst_reg == si->src_reg)	{			      \
-			*insn++ = BPF_JMP_A(1);				      \
+			*insn++ = BPF_JMP_A(2);				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
 				      temp));				      \
+			*insn++ = BPF_MOV64_IMM(si->dst_reg, 0);	      \
 		}							      \
 	} while (0)
 

@@ -10520,10 +10521,11 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
 				      si->dst_reg, si->src_reg,		      \
 				      offsetof(struct bpf_sock_ops_kern, sk));\
 		if (si->dst_reg == si->src_reg)	{			      \
-			*insn++ = BPF_JMP_A(1);				      \
+			*insn++ = BPF_JMP_A(2);				      \
 			*insn++ = BPF_LDX_MEM(BPF_DW, reg, si->src_reg,	      \
 				      offsetof(struct bpf_sock_ops_kern,      \
 				      temp));				      \
+			*insn++ = BPF_MOV64_IMM(si->dst_reg, 0);	      \
 		}							      \
 	} while (0)
Signature · 1 line
2.47.3