On Thu, Nov 02, 2023 at 05:08:13PM -0700, Andrii Nakryiko wrote:
> Add simple sanity checks that validate well-formed ranges (min <= max)
> across u64, s64, u32, and s32 ranges. Also for cases when the value is
> constant (either 64-bit or 32-bit), we validate that ranges and tnums
> are in agreement.
>
> These bounds checks are performed at the end of BPF_ALU/BPF_ALU64
> operations, on conditional jumps, and for LDX instructions (where subreg
> zero/sign extension is probably the most important to check). This
> covers most of the interesting cases.
>
> Also, we validate the sanity of the return register when manually
> adjusting it for some special helpers.
>
> By default, sanity violation will trigger a warning in verifier log and
> resetting register bounds to "unbounded" ones. But to aid development
> and debugging, BPF_F_TEST_SANITY_STRICT flag is added, which will
> trigger hard failure of verification with -EFAULT on register bounds
> violations. This allows selftests to catch such issues. veristat will
> also gain a CLI option to enable this behavior.
>
> Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> ---
> include/linux/bpf_verifier.h | 1 +
> include/uapi/linux/bpf.h | 3 +
> kernel/bpf/syscall.c | 3 +-
> kernel/bpf/verifier.c | 117 ++++++++++++++++++++++++++-------
> tools/include/uapi/linux/bpf.h | 3 +
> 5 files changed, 101 insertions(+), 26 deletions(-)
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8691cacd3ad3..af4e2fecbef2 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -2615,6 +2615,56 @@ static void reg_bounds_sync(struct bpf_reg_state *reg)
> __update_reg_bounds(reg);
> }
>
> +static int reg_bounds_sanity_check(struct bpf_verifier_env *env,
> + struct bpf_reg_state *reg, const char *ctx)
> +{
> + const char *msg;
> +
> + if (reg->umin_value > reg->umax_value ||
> + reg->smin_value > reg->smax_value ||
> + reg->u32_min_value > reg->u32_max_value ||
> + reg->s32_min_value > reg->s32_max_value) {
> + msg = "range bounds violation";
> + goto out;
> + }
Maybe Check tnum validity before comparing it with min/max? The mask bit
and value bit at the same position can not both be set.
if (reg->var_off.mask & reg->var_off.value) {
msg = "tnum invalid";
goto out;
}
Unfortunately doing tnum_intersect() on two non-overlapping tnum still
gives a valid tnum; so we can't readily detect such cases like how we do
for ranges above.
> + if (tnum_is_const(reg->var_off)) {
> + u64 uval = reg->var_off.value;
> + s64 sval = (s64)uval;
> +
> + if (reg->umin_value != uval || reg->umax_value != uval ||
> + reg->smin_value != sval || reg->smax_value != sval) {
> + msg = "const tnum out of sync with range bounds";
> + goto out;
> + }
> + }
> +
> + if (tnum_subreg_is_const(reg->var_off)) {
> + u32 uval32 = tnum_subreg(reg->var_off).value;
> + s32 sval32 = (s32)uval32;
> +
> + if (reg->u32_min_value != uval32 || reg->u32_max_value != uval32 ||
> + reg->s32_min_value != sval32 || reg->s32_max_value != sval32) {
> + msg = "const subreg tnum out of sync with range bounds";
> + goto out;
> + }
> + }
> +
> + return 0;
> +out:
> + verbose(env, "REG SANITY VIOLATION (%s): %s u64=[%#llx, %#llx] "
> + "s64=[%#llx, %#llx] u32=[%#x, %#x] s32=[%#x, %#x] var_off=(%#llx, %#llx)\n",
> + ctx, msg, reg->umin_value, reg->umax_value,
> + reg->smin_value, reg->smax_value,
> + reg->u32_min_value, reg->u32_max_value,
> + reg->s32_min_value, reg->s32_max_value,
> + reg->var_off.value, reg->var_off.mask);
> + if (env->test_sanity_strict)
> + return -EFAULT;
> + __mark_reg_unbounded(reg);
> + return 0;
> +}
[...]
