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.

diff --git a/bpf-base.cxx b/bpf-base.cxx
index 8d2cd6cdab176efaf7a642ca8e824a6409058f2a..62ca8de5728d0b1cddd3dd0b316bc544c51d3c3d 100644 (file)
--- 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 75fefb769106d9a1e67a5d76303e4e5406c2a903..f97501d7d9734d8033137dfabfc56b32f40dc8cd 100644 (file)
--- 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 089dcde552b34cd3f237ff6dd58e39c5ed79b240..f3aa5c46285dcaf852bcca1c7b79f45d5a6ac9c5 100644 (file)
--- 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 689070ff9386b51fd24a2183c81876f9df3c130d..eb1b531172ac03560f8b767b8e606686e4c08eb4 100644 (file)
--- 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]);