From a7605f298f8d823429b8cfd264bd28c3bd345eb5 Mon Sep 17 00:00:00 2001
From: Serhei Makarov <smakarov@redhat.com>
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<std::string, value *>& 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<std::string, value *>(str, v));
+ value *v = new value(value::mk_str(str, format_str));
+ auto ok = m.insert(std::pair<std::string, value *>(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<int64_t, value *> imm_map;
std::unordered_map<std::string, value *> str_map;
+ std::unordered_map<std::string, value *> 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