Skip to content

Commit 34ea973

Browse files
author
Alexei Starovoitov
committed
Merge branch 'bpf-verifier-improve-precision-of-bpf_mul'
Matan Shachnai says: ==================== This patch-set aims to improve precision of BPF_MUL and add testcases to illustrate precision gains using signed and unsigned bounds. Changes from v1: - Fixed typo made in patch. Changes from v2: - Added signed multiplication to BPF_MUL. - Added test cases to exercise BPF_MUL. - Reordered patches in the series. Changes from v3: - Coding style fixes. ==================== Link: https://patch.msgid.link/[email protected] Signed-off-by: Alexei Starovoitov <[email protected]>
2 parents 1846dd8 + 75137d9 commit 34ea973

File tree

2 files changed

+170
-44
lines changed

2 files changed

+170
-44
lines changed

‎kernel/bpf/verifier.c

Lines changed: 36 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -14084,64 +14084,56 @@ static void scalar_min_max_sub(struct bpf_reg_state *dst_reg,
1408414084
static void scalar32_min_max_mul(struct bpf_reg_state *dst_reg,
1408514085
struct bpf_reg_state *src_reg)
1408614086
{
14087-
s32 smin_val = src_reg->s32_min_value;
14088-
u32 umin_val = src_reg->u32_min_value;
14089-
u32 umax_val = src_reg->u32_max_value;
14087+
s32 *dst_smin = &dst_reg->s32_min_value;
14088+
s32 *dst_smax = &dst_reg->s32_max_value;
14089+
u32 *dst_umin = &dst_reg->u32_min_value;
14090+
u32 *dst_umax = &dst_reg->u32_max_value;
14091+
s32 tmp_prod[4];
1409014092

14091-
if (smin_val < 0 || dst_reg->s32_min_value < 0) {
14092-
/* Ain't nobody got time to multiply that sign */
14093-
__mark_reg32_unbounded(dst_reg);
14094-
return;
14095-
}
14096-
/* Both values are positive, so we can work with unsigned and
14097-
* copy the result to signed (unless it exceeds S32_MAX).
14098-
*/
14099-
if (umax_val > U16_MAX || dst_reg->u32_max_value > U16_MAX) {
14100-
/* Potential overflow, we know nothing */
14101-
__mark_reg32_unbounded(dst_reg);
14102-
return;
14093+
if (check_mul_overflow(*dst_umax, src_reg->u32_max_value, dst_umax) ||
14094+
check_mul_overflow(*dst_umin, src_reg->u32_min_value, dst_umin)) {
14095+
/* Overflow possible, we know nothing */
14096+
*dst_umin = 0;
14097+
*dst_umax = U32_MAX;
1410314098
}
14104-
dst_reg->u32_min_value *= umin_val;
14105-
dst_reg->u32_max_value *= umax_val;
14106-
if (dst_reg->u32_max_value > S32_MAX) {
14099+
if (check_mul_overflow(*dst_smin, src_reg->s32_min_value, &tmp_prod[0]) ||
14100+
check_mul_overflow(*dst_smin, src_reg->s32_max_value, &tmp_prod[1]) ||
14101+
check_mul_overflow(*dst_smax, src_reg->s32_min_value, &tmp_prod[2]) ||
14102+
check_mul_overflow(*dst_smax, src_reg->s32_max_value, &tmp_prod[3])) {
1410714103
/* Overflow possible, we know nothing */
14108-
dst_reg->s32_min_value = S32_MIN;
14109-
dst_reg->s32_max_value = S32_MAX;
14104+
*dst_smin = S32_MIN;
14105+
*dst_smax = S32_MAX;
1411014106
} else {
14111-
dst_reg->s32_min_value = dst_reg->u32_min_value;
14112-
dst_reg->s32_max_value = dst_reg->u32_max_value;
14107+
*dst_smin = min_array(tmp_prod, 4);
14108+
*dst_smax = max_array(tmp_prod, 4);
1411314109
}
1411414110
}
1411514111

1411614112
static void scalar_min_max_mul(struct bpf_reg_state *dst_reg,
1411714113
struct bpf_reg_state *src_reg)
1411814114
{
14119-
s64 smin_val = src_reg->smin_value;
14120-
u64 umin_val = src_reg->umin_value;
14121-
u64 umax_val = src_reg->umax_value;
14115+
s64 *dst_smin = &dst_reg->smin_value;
14116+
s64 *dst_smax = &dst_reg->smax_value;
14117+
u64 *dst_umin = &dst_reg->umin_value;
14118+
u64 *dst_umax = &dst_reg->umax_value;
14119+
s64 tmp_prod[4];
1412214120

14123-
if (smin_val < 0 || dst_reg->smin_value < 0) {
14124-
/* Ain't nobody got time to multiply that sign */
14125-
__mark_reg64_unbounded(dst_reg);
14126-
return;
14127-
}
14128-
/* Both values are positive, so we can work with unsigned and
14129-
* copy the result to signed (unless it exceeds S64_MAX).
14130-
*/
14131-
if (umax_val > U32_MAX || dst_reg->umax_value > U32_MAX) {
14132-
/* Potential overflow, we know nothing */
14133-
__mark_reg64_unbounded(dst_reg);
14134-
return;
14121+
if (check_mul_overflow(*dst_umax, src_reg->umax_value, dst_umax) ||
14122+
check_mul_overflow(*dst_umin, src_reg->umin_value, dst_umin)) {
14123+
/* Overflow possible, we know nothing */
14124+
*dst_umin = 0;
14125+
*dst_umax = U64_MAX;
1413514126
}
14136-
dst_reg->umin_value *= umin_val;
14137-
dst_reg->umax_value *= umax_val;
14138-
if (dst_reg->umax_value > S64_MAX) {
14127+
if (check_mul_overflow(*dst_smin, src_reg->smin_value, &tmp_prod[0]) ||
14128+
check_mul_overflow(*dst_smin, src_reg->smax_value, &tmp_prod[1]) ||
14129+
check_mul_overflow(*dst_smax, src_reg->smin_value, &tmp_prod[2]) ||
14130+
check_mul_overflow(*dst_smax, src_reg->smax_value, &tmp_prod[3])) {
1413914131
/* Overflow possible, we know nothing */
14140-
dst_reg->smin_value = S64_MIN;
14141-
dst_reg->smax_value = S64_MAX;
14132+
*dst_smin = S64_MIN;
14133+
*dst_smax = S64_MAX;
1414214134
} else {
14143-
dst_reg->smin_value = dst_reg->umin_value;
14144-
dst_reg->smax_value = dst_reg->umax_value;
14135+
*dst_smin = min_array(tmp_prod, 4);
14136+
*dst_smax = max_array(tmp_prod, 4);
1414514137
}
1414614138
}
1414714139

‎tools/testing/selftests/bpf/progs/verifier_bounds.c

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1200,4 +1200,138 @@ l0_%=: r0 = 0; \
12001200
: __clobber_all);
12011201
}
12021202

1203+
SEC("tc")
1204+
__description("multiply mixed sign bounds. test 1")
1205+
__success __log_level(2)
1206+
__msg("r6 *= r7 {{.*}}; R6_w=scalar(smin=umin=0x1bc16d5cd4927ee1,smax=umax=0x1bc16d674ec80000,smax32=0x7ffffeff,umax32=0xfffffeff,var_off=(0x1bc16d4000000000; 0x3ffffffeff))")
1207+
__naked void mult_mixed0_sign(void)
1208+
{
1209+
asm volatile (
1210+
"call %[bpf_get_prandom_u32];"
1211+
"r6 = r0;"
1212+
"call %[bpf_get_prandom_u32];"
1213+
"r7 = r0;"
1214+
"r6 &= 0xf;"
1215+
"r6 -= 1000000000;"
1216+
"r7 &= 0xf;"
1217+
"r7 -= 2000000000;"
1218+
"r6 *= r7;"
1219+
"exit"
1220+
:
1221+
: __imm(bpf_get_prandom_u32),
1222+
__imm(bpf_skb_store_bytes)
1223+
: __clobber_all);
1224+
}
1225+
1226+
SEC("tc")
1227+
__description("multiply mixed sign bounds. test 2")
1228+
__success __log_level(2)
1229+
__msg("r6 *= r7 {{.*}}; R6_w=scalar(smin=smin32=-100,smax=smax32=200)")
1230+
__naked void mult_mixed1_sign(void)
1231+
{
1232+
asm volatile (
1233+
"call %[bpf_get_prandom_u32];"
1234+
"r6 = r0;"
1235+
"call %[bpf_get_prandom_u32];"
1236+
"r7 = r0;"
1237+
"r6 &= 0xf;"
1238+
"r6 -= 0xa;"
1239+
"r7 &= 0xf;"
1240+
"r7 -= 0x14;"
1241+
"r6 *= r7;"
1242+
"exit"
1243+
:
1244+
: __imm(bpf_get_prandom_u32),
1245+
__imm(bpf_skb_store_bytes)
1246+
: __clobber_all);
1247+
}
1248+
1249+
SEC("tc")
1250+
__description("multiply negative bounds")
1251+
__success __log_level(2)
1252+
__msg("r6 *= r7 {{.*}}; R6_w=scalar(smin=umin=smin32=umin32=0x3ff280b0,smax=umax=smax32=umax32=0x3fff0001,var_off=(0x3ff00000; 0xf81ff))")
1253+
__naked void mult_sign_bounds(void)
1254+
{
1255+
asm volatile (
1256+
"r8 = 0x7fff;"
1257+
"call %[bpf_get_prandom_u32];"
1258+
"r6 = r0;"
1259+
"call %[bpf_get_prandom_u32];"
1260+
"r7 = r0;"
1261+
"r6 &= 0xa;"
1262+
"r6 -= r8;"
1263+
"r7 &= 0xf;"
1264+
"r7 -= r8;"
1265+
"r6 *= r7;"
1266+
"exit"
1267+
:
1268+
: __imm(bpf_get_prandom_u32),
1269+
__imm(bpf_skb_store_bytes)
1270+
: __clobber_all);
1271+
}
1272+
1273+
SEC("tc")
1274+
__description("multiply bounds that don't cross signed boundary")
1275+
__success __log_level(2)
1276+
__msg("r8 *= r6 {{.*}}; R6_w=scalar(smin=smin32=0,smax=umax=smax32=umax32=11,var_off=(0x0; 0xb)) R8_w=scalar(smin=0,smax=umax=0x7b96bb0a94a3a7cd,var_off=(0x0; 0x7fffffffffffffff))")
1277+
__naked void mult_no_sign_crossing(void)
1278+
{
1279+
asm volatile (
1280+
"r6 = 0xb;"
1281+
"r8 = 0xb3c3f8c99262687 ll;"
1282+
"call %[bpf_get_prandom_u32];"
1283+
"r7 = r0;"
1284+
"r6 &= r7;"
1285+
"r8 *= r6;"
1286+
"exit"
1287+
:
1288+
: __imm(bpf_get_prandom_u32),
1289+
__imm(bpf_skb_store_bytes)
1290+
: __clobber_all);
1291+
}
1292+
1293+
SEC("tc")
1294+
__description("multiplication overflow, result in unbounded reg. test 1")
1295+
__success __log_level(2)
1296+
__msg("r6 *= r7 {{.*}}; R6_w=scalar()")
1297+
__naked void mult_unsign_ovf(void)
1298+
{
1299+
asm volatile (
1300+
"r8 = 0x7ffffffffff ll;"
1301+
"call %[bpf_get_prandom_u32];"
1302+
"r6 = r0;"
1303+
"call %[bpf_get_prandom_u32];"
1304+
"r7 = r0;"
1305+
"r6 &= 0x7fffffff;"
1306+
"r7 &= r8;"
1307+
"r6 *= r7;"
1308+
"exit"
1309+
:
1310+
: __imm(bpf_get_prandom_u32),
1311+
__imm(bpf_skb_store_bytes)
1312+
: __clobber_all);
1313+
}
1314+
1315+
SEC("tc")
1316+
__description("multiplication overflow, result in unbounded reg. test 2")
1317+
__success __log_level(2)
1318+
__msg("r6 *= r7 {{.*}}; R6_w=scalar()")
1319+
__naked void mult_sign_ovf(void)
1320+
{
1321+
asm volatile (
1322+
"r8 = 0x7ffffffff ll;"
1323+
"call %[bpf_get_prandom_u32];"
1324+
"r6 = r0;"
1325+
"call %[bpf_get_prandom_u32];"
1326+
"r7 = r0;"
1327+
"r6 &= 0xa;"
1328+
"r6 -= r8;"
1329+
"r7 &= 0x7fffffff;"
1330+
"r6 *= r7;"
1331+
"exit"
1332+
:
1333+
: __imm(bpf_get_prandom_u32),
1334+
__imm(bpf_skb_store_bytes)
1335+
: __clobber_all);
1336+
}
12031337
char _license[] SEC("license") = "GPL";

0 commit comments

Comments
 (0)