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