Re: [PATCH bpf-next v2 02/15] bpf: Fix sign-extension ctx member accesses

Yonghong Song <yhs@xxxxxxxx> · Tue, 18 Jul 2023 17:40:18 -0700

On 7/16/23 6:40 PM, Eduard Zingerman wrote:
On Wed, 2023-07-12 at 23:07 -0700, Yonghong Song wrote:
In uapi bpf.h, there are two ctx structures which contain
signed members. Without cpu v4, such signed members will
be loaded with unsigned load and the compiler will generate
proper left and right shifts to get the proper final value.

With sign-extension load, however, left/right shifts are gone,
we need to ensure these signed members are properly handled,
with signed loads or other means. The following are list of
signed ctx members and how they are handled.

This is not a generic approach, in theory any field could
be cast as a signed integer. Do we want to support this?
If so, then it should be handled in convert_ctx_access()
by generating additional sign extension instructions.

Good point. I will make change in verifier.c which is more
generic and cover more cases.



(1).
   struct bpf_sock {
      ...
      __s32 rx_queue_mapping;
   }

The corresponding kernel fields are
   struct sock_common {
      ...
      unsigned short          skc_rx_queue_mapping;
      ...
   }

Current ctx rewriter uses unsigned load for the kernel field
which is correct and does not need further handling.

(2).
   struct bpf_sockopt {
      ...
      __s32   level;
      __s32   optname;
      __s32   optlen;
      __s32   retval;
   }
The level/optname/optlen are from struct bpf_sockopt_kern
   struct bpf_sockopt_kern {
      ...
      s32             level;
      s32             optname;
      s32             optlen;
      ...
   }
and the 'retval' is from struct bpf_cg_run_ctx
   struct bpf_cg_run_ctx {
      ...
      int retval;
   }
Current the above four fields are loaded with unsigned load.
Let us modify the read macro for bpf_sockopt which use
the same signedness for the original insn.

Signed-off-by: Yonghong Song <yhs@xxxxxx>
---
  kernel/bpf/cgroup.c | 14 ++++++++------
  1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index 5b2741aa0d9b..29e3606ff6f4 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -2397,9 +2397,10 @@ static bool cg_sockopt_is_valid_access(int off, int size,
  }
  
  #define CG_SOCKOPT_READ_FIELD(F)					\
-	BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F),	\
+	BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) |	\
+		      BPF_MODE(si->code) | BPF_CLASS(si->code)),	\
  		    si->dst_reg, si->src_reg,				\
-		    offsetof(struct bpf_sockopt_kern, F))
+		    offsetof(struct bpf_sockopt_kern, F), si->imm)
  
  #define CG_SOCKOPT_WRITE_FIELD(F)					\
  	BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_sockopt_kern, F) |	\
@@ -2456,7 +2457,7 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type,
  			*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx),
  					      treg, treg,
  					      offsetof(struct task_struct, bpf_ctx));
-			*insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MEM |
+			*insn++ = BPF_RAW_INSN(BPF_CLASS(si->code) | BPF_MODE(si->code) |
  					       BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval),
  					       treg, si->src_reg,
  					       offsetof(struct bpf_cg_run_ctx, retval),
@@ -2470,9 +2471,10 @@ static u32 cg_sockopt_convert_ctx_access(enum bpf_access_type type,
  			*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct task_struct, bpf_ctx),
  					      si->dst_reg, si->dst_reg,
  					      offsetof(struct task_struct, bpf_ctx));
-			*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval),
-					      si->dst_reg, si->dst_reg,
-					      offsetof(struct bpf_cg_run_ctx, retval));
+			*insn++ = BPF_RAW_INSN((BPF_FIELD_SIZEOF(struct bpf_cg_run_ctx, retval) |
+						BPF_MODE(si->code) | BPF_CLASS(si->code)),
+					       si->dst_reg, si->dst_reg,
+					       offsetof(struct bpf_cg_run_ctx, retval), si->imm);
  		}
  		break;
  	case offsetof(struct bpf_sockopt, optval):








