lkml.org 
[lkml]   [2017]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 net-next 02/12] bpf/verifier: rework value tracking
On 06/27/2017 02:56 PM, Edward Cree wrote:
> Tracks value alignment by means of tracking known & unknown bits.
> Tightens some min/max value checks and fixes a couple of bugs therein.
> If pointer leaks are allowed, and adjust_ptr_min_max_vals returns -EACCES,
> treat the pointer as an unknown scalar and try again, because we might be
> able to conclude something about the result (e.g. pointer & 0x40 is either
> 0 or 0x40).
>
> Signed-off-by: Edward Cree <ecree@solarflare.com>
[...]
> +static int adjust_reg_min_max_vals(struct bpf_verifier_env *env,
> + struct bpf_insn *insn)
> +{
> + struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg, *src_reg;
> + struct bpf_reg_state *ptr_reg = NULL, off_reg = {0};
> + u8 opcode = BPF_OP(insn->code);
> + int rc;
> +
> + dst_reg = &regs[insn->dst_reg];
> + check_reg_overflow(dst_reg);
> + src_reg = NULL;
> + if (dst_reg->type != SCALAR_VALUE)
> + ptr_reg = dst_reg;
> + if (BPF_SRC(insn->code) == BPF_X) {
> + src_reg = &regs[insn->src_reg];
> + check_reg_overflow(src_reg);
> +
> + if (src_reg->type != SCALAR_VALUE) {
> + if (dst_reg->type != SCALAR_VALUE) {
> + /* Combining two pointers by any ALU op yields
> + * an arbitrary scalar.
> + */
> + if (!env->allow_ptr_leaks) {
> + verbose("R%d pointer %s pointer prohibited\n",
> + insn->dst_reg,
> + bpf_alu_string[opcode >> 4]);
> + return -EACCES;
> + }
> + mark_reg_unknown(regs, insn->dst_reg);
> + return 0;
> + } else {
> + /* scalar += pointer
> + * This is legal, but we have to reverse our
> + * src/dest handling in computing the range
> + */
> + rc = adjust_ptr_min_max_vals(env, insn,
> + src_reg, dst_reg);
> + if (rc == -EACCES && env->allow_ptr_leaks) {
> + /* scalar += unknown scalar */
> + __mark_reg_unknown(&off_reg);
> + return adjust_scalar_min_max_vals(
> + env, insn,
> + dst_reg, &off_reg);

Could you elaborate on this one? If I understand it correctly, then
the scalar += pointer case would mean the following: given I have one
of the allowed pointer types in adjust_ptr_min_max_vals() then the
prior scalar type inherits the ptr type/id. I would then 'destroy' the
pointer value so we get a -EACCES on it. We mark the tmp off_reg as
scalar type, but shouldn't also actual dst_reg be marked as such
like in below pointer += scalar case, such that we undo the prior
ptr_type inheritance?

> + }
> + return rc;
> + }
> + } else if (ptr_reg) {
> + /* pointer += scalar */
> + rc = adjust_ptr_min_max_vals(env, insn,
> + dst_reg, src_reg);
> + if (rc == -EACCES && env->allow_ptr_leaks) {
> + /* unknown scalar += scalar */
> + __mark_reg_unknown(dst_reg);
> + return adjust_scalar_min_max_vals(
> + env, insn, dst_reg, src_reg);
> + }
> + return rc;
> + }
> + } else {
[...]

\
 
 \ /
  Last update: 2017-06-28 19:11    [W:0.502 / U:0.848 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site