Re: [RFC PATCH net-next 4/5] bpf/verifier: track signed and unsigned min/max values
From: Alexei Starovoitov via iovisor-dev <hidden>
Date: 2017-06-08 02:40:44
Also in:
lkml
On Wed, Jun 07, 2017 at 03:59:25PM +0100, Edward Cree wrote:
quoted hunk ↗ jump to hunk
Allows us to, sometimes, combine information from a signed check of one bound and an unsigned check of the other. We now track the full range of possible values, rather than restricting ourselves to [0, 1<<30) and considering anything beyond that as unknown. While this is probably not necessary, it makes the code more straightforward and symmetrical between signed and unsigned bounds. Signed-off-by: Edward Cree <ecree-s/n/eUQHGBpZroRs9YW3xA@public.gmane.org> --- include/linux/bpf_verifier.h | 22 +- kernel/bpf/verifier.c | 661 +++++++++++++++++++++++++------------------ 2 files changed, 395 insertions(+), 288 deletions(-)diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index e341469..10a5944 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h@@ -11,11 +11,15 @@ #include <linux/filter.h> /* for MAX_BPF_STACK */ #include <linux/tnum.h> - /* Just some arbitrary values so we can safely do math without overflowing and - * are obviously wrong for any sort of memory access. - */ -#define BPF_REGISTER_MAX_RANGE (1024 * 1024 * 1024) -#define BPF_REGISTER_MIN_RANGE -1 +/* Maximum variable offset umax_value permitted when resolving memory accesses. + * In practice this is far bigger than any realistic pointer offset; this limit + * ensures that umax_value + (int)off + (int)size cannot overflow a u64. + */ +#define BPF_MAX_VAR_OFF (1ULL << 31) +/* Maximum variable size permitted for ARG_CONST_SIZE[_OR_ZERO]. This ensures + * that converting umax_value to int cannot overflow. + */ +#define BPF_MAX_VAR_SIZ INT_MAX struct bpf_reg_state { enum bpf_reg_type type;@@ -38,7 +42,7 @@ struct bpf_reg_state { * PTR_TO_MAP_VALUE_OR_NULL, we have to NULL-check it _first_. */ u32 id; - /* These three fields must be last. See states_equal() */ + /* These five fields must be last. See states_equal() */ /* For scalar types (SCALAR_VALUE), this represents our knowledge of * the actual value. * For pointer types, this represents the variable part of the offset@@ -51,8 +55,10 @@ struct bpf_reg_state { * These refer to the same value as align, not necessarily the actual * contents of the register. */ - s64 min_value; /* minimum possible (s64)value */ - u64 max_value; /* maximum possible (u64)value */ + s64 smin_value; /* minimum possible (s64)value */ + s64 smax_value; /* maximum possible (s64)value */ + u64 umin_value; /* minimum possible (u64)value */ + u64 umax_value; /* maximum possible (u64)value */
have uneasy feeling about this one. It's 16 extra bytes to be stored in every reg_state and memcmp later while we didn't have cases where people wanted negative values in ptr+var cases. Why bother than?
unknown. While this is probably not necessary, it makes the code more straightforward and symmetrical between signed and unsigned bounds.
it's hard for me to see the 'straightforward' part yet.