Skip to content

Commit 2399463

Browse files
yonghong-songdavem330
yonghong-song
authored and
davem330
committed
bpf: possibly avoid extra masking for narrower load in verifier
Commit 31fd858 ("bpf: permits narrower load from bpf program context fields") permits narrower load for certain ctx fields. The commit however will already generate a masking even if the prog-specific ctx conversion produces the result with narrower size. For example, for __sk_buff->protocol, the ctx conversion loads the data into register with 2-byte load. A narrower 2-byte load should not generate masking. For __sk_buff->vlan_present, the conversion function set the result as either 0 or 1, essentially a byte. The narrower 2-byte or 1-byte load should not generate masking. To avoid unnecessary masking, prog-specific *_is_valid_access now passes converted_op_size back to verifier, which indicates the valid data width after perceived future conversion. Based on this information, verifier is able to avoid unnecessary marking. Since we want more information back from prog-specific *_is_valid_access checking, all of them are packed into one data structure for more clarity. Acked-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Yonghong Song <yhs@fb.com> Signed-off-by: David S. Miller <davem@davemloft.net>
1 parent 72de465 commit 2399463

File tree

5 files changed

+97
-55
lines changed

5 files changed

+97
-55
lines changed

‎include/linux/bpf.h‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,15 @@ enum bpf_reg_type {
149149

150150
struct bpf_prog;
151151

152+
/* The information passed from prog-specific *_is_valid_access
153+
* back to the verifier.
154+
*/
155+
struct bpf_insn_access_aux {
156+
enum bpf_reg_type reg_type;
157+
int ctx_field_size;
158+
int converted_op_size;
159+
};
160+
152161
struct bpf_verifier_ops {
153162
/* return eBPF function prototype for verification */
154163
const struct bpf_func_proto *(*get_func_proto)(enum bpf_func_id func_id);
@@ -157,7 +166,7 @@ struct bpf_verifier_ops {
157166
* with 'type' (read or write) is allowed
158167
*/
159168
bool (*is_valid_access)(int off, int size, enum bpf_access_type type,
160-
enum bpf_reg_type *reg_type, int *ctx_field_size);
169+
struct bpf_insn_access_aux *info);
161170
int (*gen_prologue)(struct bpf_insn *insn, bool direct_write,
162171
const struct bpf_prog *prog);
163172
u32 (*convert_ctx_access)(enum bpf_access_type type,

‎include/linux/bpf_verifier.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,8 @@ struct bpf_insn_aux_data {
7373
enum bpf_reg_type ptr_type; /* pointer type for load/store insns */
7474
struct bpf_map *map_ptr; /* pointer for call insn into lookup_elem */
7575
};
76-
int ctx_field_size; /* the ctx field size for load/store insns, maybe 0 */
76+
int ctx_field_size; /* the ctx field size for load insn, maybe 0 */
77+
int converted_op_size; /* the valid value width after perceived conversion */
7778
};
7879

7980
#define MAX_USED_MAPS 64 /* max number of maps accessed by one eBPF program */

‎kernel/bpf/verifier.c‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -761,22 +761,34 @@ static int check_packet_access(struct bpf_verifier_env *env, u32 regno, int off,
761761
static int check_ctx_access(struct bpf_verifier_env *env, int insn_idx, int off, int size,
762762
enum bpf_access_type t, enum bpf_reg_type *reg_type)
763763
{
764-
int ctx_field_size = 0;
764+
struct bpf_insn_access_aux info = { .reg_type = *reg_type };
765765

766766
/* for analyzer ctx accesses are already validated and converted */
767767
if (env->analyzer_ops)
768768
return 0;
769769

770770
if (env->prog->aux->ops->is_valid_access &&
771-
env->prog->aux->ops->is_valid_access(off, size, t, reg_type, &ctx_field_size)) {
772-
/* a non zero ctx_field_size indicates:
771+
env->prog->aux->ops->is_valid_access(off, size, t, &info)) {
772+
/* a non zero info.ctx_field_size indicates:
773773
* . For this field, the prog type specific ctx conversion algorithm
774774
* only supports whole field access.
775775
* . This ctx access is a candiate for later verifier transformation
776776
* to load the whole field and then apply a mask to get correct result.
777+
* a non zero info.converted_op_size indicates perceived actual converted
778+
* value width in convert_ctx_access.
777779
*/
778-
if (ctx_field_size)
779-
env->insn_aux_data[insn_idx].ctx_field_size = ctx_field_size;
780+
if ((info.ctx_field_size && !info.converted_op_size) ||
781+
(!info.ctx_field_size && info.converted_op_size)) {
782+
verbose("verifier bug in is_valid_access prog type=%u off=%d size=%d\n",
783+
env->prog->type, off, size);
784+
return -EACCES;
785+
}
786+
787+
if (info.ctx_field_size) {
788+
env->insn_aux_data[insn_idx].ctx_field_size = info.ctx_field_size;
789+
env->insn_aux_data[insn_idx].converted_op_size = info.converted_op_size;
790+
}
791+
*reg_type = info.reg_type;
780792

781793
/* remember the offset of last byte accessed in ctx */
782794
if (env->prog->aux->max_ctx_offset < off + size)
@@ -3388,7 +3400,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
33883400
struct bpf_insn insn_buf[16], *insn;
33893401
struct bpf_prog *new_prog;
33903402
enum bpf_access_type type;
3391-
int i, cnt, off, size, ctx_field_size, is_narrower_load, delta = 0;
3403+
int i, cnt, off, size, ctx_field_size, converted_op_size, is_narrower_load, delta = 0;
33923404

33933405
if (ops->gen_prologue) {
33943406
cnt = ops->gen_prologue(insn_buf, env->seen_direct_write,
@@ -3431,7 +3443,8 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
34313443
off = insn->off;
34323444
size = bpf_size_to_bytes(BPF_SIZE(insn->code));
34333445
ctx_field_size = env->insn_aux_data[i + delta].ctx_field_size;
3434-
is_narrower_load = (type == BPF_READ && size < ctx_field_size);
3446+
converted_op_size = env->insn_aux_data[i + delta].converted_op_size;
3447+
is_narrower_load = type == BPF_READ && size < ctx_field_size;
34353448

34363449
/* If the read access is a narrower load of the field,
34373450
* convert to a 4/8-byte load, to minimum program type specific
@@ -3453,7 +3466,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
34533466
verbose("bpf verifier is misconfigured\n");
34543467
return -EINVAL;
34553468
}
3456-
if (is_narrower_load) {
3469+
if (is_narrower_load && size < converted_op_size) {
34573470
if (ctx_field_size <= 4)
34583471
insn_buf[cnt++] = BPF_ALU32_IMM(BPF_AND, insn->dst_reg,
34593472
(1 << size * 8) - 1);

‎kernel/trace/bpf_trace.c‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ static const struct bpf_func_proto *kprobe_prog_func_proto(enum bpf_func_id func
479479

480480
/* bpf+kprobe programs can access fields of 'struct pt_regs' */
481481
static bool kprobe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
482-
enum bpf_reg_type *reg_type, int *ctx_field_size)
482+
struct bpf_insn_access_aux *info)
483483
{
484484
if (off < 0 || off >= sizeof(struct pt_regs))
485485
return false;
@@ -562,7 +562,7 @@ static const struct bpf_func_proto *tp_prog_func_proto(enum bpf_func_id func_id)
562562
}
563563

564564
static bool tp_prog_is_valid_access(int off, int size, enum bpf_access_type type,
565-
enum bpf_reg_type *reg_type, int *ctx_field_size)
565+
struct bpf_insn_access_aux *info)
566566
{
567567
if (off < sizeof(void *) || off >= PERF_MAX_TRACE_SIZE)
568568
return false;
@@ -581,7 +581,7 @@ const struct bpf_verifier_ops tracepoint_prog_ops = {
581581
};
582582

583583
static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type,
584-
enum bpf_reg_type *reg_type, int *ctx_field_size)
584+
struct bpf_insn_access_aux *info)
585585
{
586586
int sample_period_off;
587587

@@ -595,12 +595,17 @@ static bool pe_prog_is_valid_access(int off, int size, enum bpf_access_type type
595595
/* permit 1, 2, 4 byte narrower and 8 normal read access to sample_period */
596596
sample_period_off = offsetof(struct bpf_perf_event_data, sample_period);
597597
if (off >= sample_period_off && off < sample_period_off + sizeof(__u64)) {
598-
*ctx_field_size = 8;
598+
int allowed;
599+
599600
#ifdef __LITTLE_ENDIAN
600-
return (off & 0x7) == 0 && size <= 8 && (size & (size - 1)) == 0;
601+
allowed = (off & 0x7) == 0 && size <= 8 && (size & (size - 1)) == 0;
601602
#else
602-
return ((off & 0x7) + size) == 8 && size <= 8 && (size & (size - 1)) == 0;
603+
allowed = ((off & 0x7) + size) == 8 && size <= 8 && (size & (size - 1)) == 0;
603604
#endif
605+
if (!allowed)
606+
return false;
607+
info->ctx_field_size = 8;
608+
info->converted_op_size = 8;
604609
} else {
605610
if (size != sizeof(long))
606611
return false;

‎net/core/filter.c‎

Lines changed: 53 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -2856,8 +2856,37 @@ lwt_xmit_func_proto(enum bpf_func_id func_id)
28562856
}
28572857
}
28582858

2859+
static void __set_access_aux_info(int off, struct bpf_insn_access_aux *info)
2860+
{
2861+
info->ctx_field_size = 4;
2862+
switch (off) {
2863+
case offsetof(struct __sk_buff, pkt_type) ...
2864+
offsetof(struct __sk_buff, pkt_type) + sizeof(__u32) - 1:
2865+
case offsetof(struct __sk_buff, vlan_present) ...
2866+
offsetof(struct __sk_buff, vlan_present) + sizeof(__u32) - 1:
2867+
info->converted_op_size = 1;
2868+
break;
2869+
case offsetof(struct __sk_buff, queue_mapping) ...
2870+
offsetof(struct __sk_buff, queue_mapping) + sizeof(__u32) - 1:
2871+
case offsetof(struct __sk_buff, protocol) ...
2872+
offsetof(struct __sk_buff, protocol) + sizeof(__u32) - 1:
2873+
case offsetof(struct __sk_buff, vlan_tci) ...
2874+
offsetof(struct __sk_buff, vlan_tci) + sizeof(__u32) - 1:
2875+
case offsetof(struct __sk_buff, vlan_proto) ...
2876+
offsetof(struct __sk_buff, vlan_proto) + sizeof(__u32) - 1:
2877+
case offsetof(struct __sk_buff, tc_index) ...
2878+
offsetof(struct __sk_buff, tc_index) + sizeof(__u32) - 1:
2879+
case offsetof(struct __sk_buff, tc_classid) ...
2880+
offsetof(struct __sk_buff, tc_classid) + sizeof(__u32) - 1:
2881+
info->converted_op_size = 2;
2882+
break;
2883+
default:
2884+
info->converted_op_size = 4;
2885+
}
2886+
}
2887+
28592888
static bool __is_valid_access(int off, int size, enum bpf_access_type type,
2860-
int *ctx_field_size)
2889+
struct bpf_insn_access_aux *info)
28612890
{
28622891
if (off < 0 || off >= sizeof(struct __sk_buff))
28632892
return false;
@@ -2875,24 +2904,32 @@ static bool __is_valid_access(int off, int size, enum bpf_access_type type,
28752904
break;
28762905
case offsetof(struct __sk_buff, data) ...
28772906
offsetof(struct __sk_buff, data) + sizeof(__u32) - 1:
2907+
if (size != sizeof(__u32))
2908+
return false;
2909+
info->reg_type = PTR_TO_PACKET;
2910+
break;
28782911
case offsetof(struct __sk_buff, data_end) ...
28792912
offsetof(struct __sk_buff, data_end) + sizeof(__u32) - 1:
28802913
if (size != sizeof(__u32))
28812914
return false;
2915+
info->reg_type = PTR_TO_PACKET_END;
28822916
break;
28832917
default:
2884-
/* permit narrower load for not cb/data/data_end fields */
2885-
*ctx_field_size = 4;
28862918
if (type == BPF_WRITE) {
28872919
if (size != sizeof(__u32))
28882920
return false;
28892921
} else {
2890-
if (size != sizeof(__u32))
2922+
int allowed;
2923+
2924+
/* permit narrower load for not cb/data/data_end fields */
28912925
#ifdef __LITTLE_ENDIAN
2892-
return (off & 0x3) == 0 && (size == 1 || size == 2);
2926+
allowed = (off & 0x3) == 0 && size <= 4 && (size & (size - 1)) == 0;
28932927
#else
2894-
return (off & 0x3) + size == 4 && (size == 1 || size == 2);
2928+
allowed = (off & 0x3) + size == 4 && size <= 4 && (size & (size - 1)) == 0;
28952929
#endif
2930+
if (!allowed)
2931+
return false;
2932+
__set_access_aux_info(off, info);
28962933
}
28972934
}
28982935

@@ -2901,8 +2938,7 @@ static bool __is_valid_access(int off, int size, enum bpf_access_type type,
29012938

29022939
static bool sk_filter_is_valid_access(int off, int size,
29032940
enum bpf_access_type type,
2904-
enum bpf_reg_type *reg_type,
2905-
int *ctx_field_size)
2941+
struct bpf_insn_access_aux *info)
29062942
{
29072943
switch (off) {
29082944
case offsetof(struct __sk_buff, tc_classid) ...
@@ -2924,13 +2960,12 @@ static bool sk_filter_is_valid_access(int off, int size,
29242960
}
29252961
}
29262962

2927-
return __is_valid_access(off, size, type, ctx_field_size);
2963+
return __is_valid_access(off, size, type, info);
29282964
}
29292965

29302966
static bool lwt_is_valid_access(int off, int size,
29312967
enum bpf_access_type type,
2932-
enum bpf_reg_type *reg_type,
2933-
int *ctx_field_size)
2968+
struct bpf_insn_access_aux *info)
29342969
{
29352970
switch (off) {
29362971
case offsetof(struct __sk_buff, tc_classid) ...
@@ -2950,22 +2985,12 @@ static bool lwt_is_valid_access(int off, int size,
29502985
}
29512986
}
29522987

2953-
switch (off) {
2954-
case offsetof(struct __sk_buff, data):
2955-
*reg_type = PTR_TO_PACKET;
2956-
break;
2957-
case offsetof(struct __sk_buff, data_end):
2958-
*reg_type = PTR_TO_PACKET_END;
2959-
break;
2960-
}
2961-
2962-
return __is_valid_access(off, size, type, ctx_field_size);
2988+
return __is_valid_access(off, size, type, info);
29632989
}
29642990

29652991
static bool sock_filter_is_valid_access(int off, int size,
29662992
enum bpf_access_type type,
2967-
enum bpf_reg_type *reg_type,
2968-
int *ctx_field_size)
2993+
struct bpf_insn_access_aux *info)
29692994
{
29702995
if (type == BPF_WRITE) {
29712996
switch (off) {
@@ -3028,8 +3053,7 @@ static int tc_cls_act_prologue(struct bpf_insn *insn_buf, bool direct_write,
30283053

30293054
static bool tc_cls_act_is_valid_access(int off, int size,
30303055
enum bpf_access_type type,
3031-
enum bpf_reg_type *reg_type,
3032-
int *ctx_field_size)
3056+
struct bpf_insn_access_aux *info)
30333057
{
30343058
if (type == BPF_WRITE) {
30353059
switch (off) {
@@ -3045,16 +3069,7 @@ static bool tc_cls_act_is_valid_access(int off, int size,
30453069
}
30463070
}
30473071

3048-
switch (off) {
3049-
case offsetof(struct __sk_buff, data):
3050-
*reg_type = PTR_TO_PACKET;
3051-
break;
3052-
case offsetof(struct __sk_buff, data_end):
3053-
*reg_type = PTR_TO_PACKET_END;
3054-
break;
3055-
}
3056-
3057-
return __is_valid_access(off, size, type, ctx_field_size);
3072+
return __is_valid_access(off, size, type, info);
30583073
}
30593074

30603075
static bool __is_valid_xdp_access(int off, int size)
@@ -3071,18 +3086,17 @@ static bool __is_valid_xdp_access(int off, int size)
30713086

30723087
static bool xdp_is_valid_access(int off, int size,
30733088
enum bpf_access_type type,
3074-
enum bpf_reg_type *reg_type,
3075-
int *ctx_field_size)
3089+
struct bpf_insn_access_aux *info)
30763090
{
30773091
if (type == BPF_WRITE)
30783092
return false;
30793093

30803094
switch (off) {
30813095
case offsetof(struct xdp_md, data):
3082-
*reg_type = PTR_TO_PACKET;
3096+
info->reg_type = PTR_TO_PACKET;
30833097
break;
30843098
case offsetof(struct xdp_md, data_end):
3085-
*reg_type = PTR_TO_PACKET_END;
3099+
info->reg_type = PTR_TO_PACKET_END;
30863100
break;
30873101
}
30883102

0 commit comments

Comments
 (0)