From a7605f298f8d823429b8cfd264bd28c3bd345eb5 Mon Sep 17 00:00:00 2001 From: Serhei Makarov Date: Fri, 9 Nov 2018 14:36:19 -0500 Subject: [PATCH 26/32] PR23860: additional ugly stack/clobber protection for strings In addition to prior commit, emit_string_copy() does not work for overlapping source/destination. So, make sure strings are not allocated in a way which overlaps map key/value arguments. This increases space pressure, inducing a couple of bpf-asm.exp testcase failures. * bpf-internal.h (value::format_str): New flag. (value::value): Take format_str flag. (value::mk_str): Take format_str flag. (program::format_map): New field, caches format_str separately. (program::new_str): Take format_str flag. * bpf-base.cxx (program::new_str): Cache format_str separately. * bpf-opt.cxx (alloc_literal_str): Store non-format str in lower half. * bpf-translate.cxx (emit_string_copy): Comment -- doesn't support overlap. (emit_string_copy): DEBUG_CODEGEN -- identify if zero-padding was done. (emit_print_format): Set format_str flag. --- bpf-base.cxx | 13 ++++++++----- bpf-internal.h | 15 +++++++++++---- bpf-opt.cxx | 35 ++++++++++++++++++++++++----------- bpf-translate.cxx | 7 +++++-- 4 files changed, 48 insertions(+), 22 deletions(-) diff --git a/bpf-base.cxx b/bpf-base.cxx index 5d132bcd1..b19b69133 100644 --- a/bpf-base.cxx +++ b/bpf-base.cxx @@ -657,14 +657,17 @@ program::new_imm(int64_t i) } value * -program::new_str(std::string str) +program::new_str(std::string str, bool format_str) { - auto old = str_map.find(str); - if (old != str_map.end()) + std::unordered_map& m = str_map; + if (format_str) m = format_map; + + auto old = m.find(str); + if (old != m.end()) return old->second; - value *v = new value(value::mk_str(str)); - auto ok = str_map.insert(std::pair(str, v)); + value *v = new value(value::mk_str(str, format_str)); + auto ok = m.insert(std::pair(str, v)); assert(ok.second); return v; } diff --git a/bpf-internal.h b/bpf-internal.h index 75fefb769..f97501d7d 100644 --- a/bpf-internal.h +++ b/bpf-internal.h @@ -78,18 +78,24 @@ struct value int64_t imm_val; std::string str_val; - value(value_type t = UNINIT, regno r = noreg, int64_t c = 0, std::string s = "") - : type(t), reg_val(r), imm_val(c), str_val(s) + bool format_str; // for str_val + + value(value_type t = UNINIT, regno r = noreg, int64_t c = 0, + std::string s = "", bool format_str = false) + : type(t), reg_val(r), imm_val(c), str_val(s), format_str(format_str) { } static value mk_imm(int64_t i) { return value(IMM, noreg, i); } - static value mk_str(std::string s) { return value(STR, noreg, 0, s); } + static value mk_str(std::string s, bool format_str = false) { + return value(STR, noreg, 0, s, format_str); + } static value mk_reg(regno r) { return value(TMPREG, r); } static value mk_hardreg(regno r) { return value(HARDREG, r); } bool is_reg() const { return type >= HARDREG; } bool is_imm() const { return type == IMM; } bool is_str() const { return type == STR; } + bool is_format() const { assert(is_str()); return format_str; } regno reg() const { assert(is_reg()); return reg_val; } int64_t imm() const { assert(is_imm()); return imm_val; } @@ -262,12 +268,13 @@ struct program // Store at most one of each IMM and STR value: std::unordered_map imm_map; std::unordered_map str_map; + std::unordered_map format_map; regno max_reg() const { return reg_vals.size() + MAX_BPF_REG; } value *lookup_reg(regno r); value *new_reg(); value *new_imm(int64_t); - value *new_str(std::string); + value *new_str(std::string, bool format_str = false); // The BPF local stack is [0, -512] indexed off BPF_REG_10. // The translator has dibs on the low bytes, [0, -max_tmp_space], diff --git a/bpf-opt.cxx b/bpf-opt.cxx index 089dcde55..f3aa5c462 100644 --- a/bpf-opt.cxx +++ b/bpf-opt.cxx @@ -19,38 +19,51 @@ namespace bpf { // Allocate space on the stack and store a string literal in that space: static value * -alloc_literal_str(program &p, insn_inserter &ins, std::string &str) +alloc_literal_str(program &p, insn_inserter &ins, value *s) { + std::string str = s->str(); + + size_t str_bytes = str.size() + 1; + str_bytes += 4 - str_bytes % 4; // write aligned words to avoid garbage data + + int ofs; size_t tmp_space; + // Append the string to existing temporary data. // // TODO: This could produce significant space limitations. // A better solution would be to integrate with the // register allocator and reclaim the space after // the string literal is no longer live. - size_t tmp_space = p.max_tmp_space; + tmp_space = p.max_tmp_space; tmp_space += 4 - tmp_space % 4; // write aligned words to avoid verifier error p.use_tmp_space(tmp_space); - size_t str_bytes = str.size() + 1; - str_bytes += 4 - str_bytes % 4; // write aligned words to avoid garbage data if (tmp_space + str_bytes > MAX_BPF_STACK) throw std::runtime_error("string allocation failed due to lack of room on stack"); tmp_space += str_bytes; #if 1 + // The following aren't ideal because an unlucky ordering of + // allocation requests will waste additional space. + // XXX PR23860: Passing a short (non-padded) string constant can fail // the verifier, which is not smart enough to determine that accesses // past the end of the string will never occur. To fix this, make sure // the string offset is at least -BPF_MAXSTRINGLEN. - // - // Not ideal because an unlucky ordering of allocations may waste space. - if (tmp_space < BPF_MAXSTRINGLEN) - tmp_space = BPF_MAXSTRINGLEN; + //if (!s->is_format() && tmp_space < BPF_MAXSTRINGLEN) + // tmp_space = BPF_MAXSTRINGLEN; + + // TODO PR23860: An even uglier workaround for emit_string_copy() + // overlapping source and destination regions. Only do this for + // non-format strings, as format strings are not manipulated by the + // eBPF program. + if (!s->is_format() && tmp_space < BPF_MAXSTRINGLEN * 2 + str_bytes) + tmp_space = BPF_MAXSTRINGLEN * 2 + str_bytes; #endif p.use_tmp_space(tmp_space); - int ofs = -tmp_space; + ofs = -tmp_space; value *frame = p.lookup_reg(BPF_REG_10); value *out = emit_simple_literal_str(p, ins, frame, ofs, str, false /* don't zero pad */); @@ -73,7 +86,7 @@ lower_str_values(program &p) { insn_before_inserter ins(b, j, "str"); std::string str0 = s0->str(); - value *new_s0 = alloc_literal_str(p, ins, str0); + value *new_s0 = alloc_literal_str(p, ins, s0); j->src0 = new_s0; } @@ -82,7 +95,7 @@ lower_str_values(program &p) { insn_before_inserter ins(b, j, "str"); std::string str1 = s1->str(); - value *new_s1 = alloc_literal_str(p, ins, str1); + value *new_s1 = alloc_literal_str(p, ins, s1); j->src1 = new_s1; } } diff --git a/bpf-translate.cxx b/bpf-translate.cxx index 0181380b7..20cd47032 100644 --- a/bpf-translate.cxx +++ b/bpf-translate.cxx @@ -2568,6 +2568,9 @@ emit_simple_literal_str(program &this_prog, insn_inserter &this_ins, // dest[+ofs] in 4-byte chunks, with optional zero-padding up to // BPF_MAXSTRINGLEN. // +// TODO (PR23860): This code does not work when the source and target +// regions overlap. +// // ??? Could use 8-byte chunks if we're starved for instruction count. // ??? Endianness of the target may come into play here. value * @@ -2583,7 +2586,7 @@ bpf_unparser::emit_string_copy(value *dest, int ofs, value *src, bool zero_pad) } #ifdef DEBUG_CODEGEN - this_ins.notes.push("strcpy"); + this_ins.notes.push(zero_pad ? "strcpy_zero_pad" : "strcpy"); #endif size_t str_bytes = BPF_MAXSTRINGLEN; @@ -2866,7 +2869,7 @@ bpf_unparser::emit_print_format (const std::string& format, // bpf program stack. This is handled by bpf-opt.cxx lowering STR values. size_t format_bytes = format.size() + 1; this_prog.mk_mov(this_ins, this_prog.lookup_reg(BPF_REG_1), - this_prog.new_str(format)); + this_prog.new_str(format, true /*format_str*/)); emit_mov(this_prog.lookup_reg(BPF_REG_2), this_prog.new_imm(format_bytes)); for (size_t i = 0; i < nargs; ++i) emit_mov(this_prog.lookup_reg(BPF_REG_3 + i), actual[i]); -- 2.14.5