Merge pull request #1046 from pchaigno/check-fmt-printk

Verify format specifiers in bpf_trace_printk in rewriter

src/cc/frontends/clang/b_frontend_action.cc

@@ -488,6 +488,7 @@ bool BTypeVisitor::VisitCallExpr(CallExpr *Call) {
           text += ", ((" + args[3] + " & 0x1) << 4) | sizeof(" + args[2] + "))";
           rewriter_.ReplaceText(expansionRange(Call->getSourceRange()), text);
         } else if (Decl->getName() == "bpf_trace_printk") {
+          checkFormatSpecifiers(args[0], Call->getArg(0)->getLocStart());
           //  #define bpf_trace_printk(fmt, args...)
           //    ({ char _fmt[] = fmt; bpf_trace_printk_(_fmt, sizeof(_fmt), args...); })
           text = "({ char _fmt[] = " + args[0] + "; bpf_trace_printk_(_fmt, sizeof(_fmt)";
@@ -523,6 +524,54 @@ bool BTypeVisitor::VisitCallExpr(CallExpr *Call) {
   return true;
 }
 
+bool BTypeVisitor::checkFormatSpecifiers(const string& fmt, SourceLocation loc) {
+  unsigned nb_specifiers = 0, i, j;
+  bool has_s = false;
+  for (i = 0; i < fmt.length(); i++) {
+    if (!isascii(fmt[i]) || (!isprint(fmt[i]) && !isspace(fmt[i]))) {
+      warning(loc.getLocWithOffset(i), "unrecognized character");
+      return false;
+    }
+    if (fmt[i] != '%')
+      continue;
+    if (nb_specifiers >= 3) {
+      warning(loc.getLocWithOffset(i), "cannot use more than 3 conversion specifiers");
+      return false;
+    }
+    nb_specifiers++;
+    i++;
+    if (fmt[i] == 'l') {
+      i++;
+    } else if (fmt[i] == 'p' || fmt[i] == 's') {
+      i++;
+      if (!isspace(fmt[i]) && !ispunct(fmt[i]) && fmt[i] != 0) {
+        warning(loc.getLocWithOffset(i - 2),
+                "only %%d %%u %%x %%ld %%lu %%lx %%lld %%llu %%llx %%p %%s conversion specifiers allowed");
+        return false;
+      }
+      if (fmt[i - 1] == 's') {
+        if (has_s) {
+          warning(loc.getLocWithOffset(i - 2), "cannot use several %%s conversion specifiers");
+          return false;
+        }
+        has_s = true;
+      }
+      continue;
+    }
+    j = 1;
+    if (fmt[i] == 'l') {
+      i++;
+      j++;
+    }
+    if (fmt[i] != 'd' && fmt[i] != 'u' && fmt[i] != 'x') {
+      warning(loc.getLocWithOffset(i - j),
+              "only %%d %%u %%x %%ld %%lu %%lx %%lld %%llu %%llx %%p %%s conversion specifiers allowed");
+      return false;
+    }
+  }
+  return true;
+}
+
 bool BTypeVisitor::VisitBinaryOperator(BinaryOperator *E) {
   if (!E->isAssignmentOp())
     return true;
@@ -589,6 +638,12 @@ DiagnosticBuilder BTypeVisitor::error(SourceLocation loc, const char (&fmt)[N])
   return C.getDiagnostics().Report(loc, diag_id);
 }
 
+template <unsigned N>
+DiagnosticBuilder BTypeVisitor::warning(SourceLocation loc, const char (&fmt)[N]) {
+  unsigned int diag_id = C.getDiagnostics().getCustomDiagID(DiagnosticsEngine::Warning, fmt);
+  return C.getDiagnostics().Report(loc, diag_id);
+}
+
 // Open table FDs when bpf tables (as denoted by section("maps*") attribute)
 // are declared.
 bool BTypeVisitor::VisitVarDecl(VarDecl *Decl) {

src/cc/frontends/clang/b_frontend_action.h

@@ -74,8 +74,11 @@ class BTypeVisitor : public clang::RecursiveASTVisitor<BTypeVisitor> {
 
  private:
   clang::SourceRange expansionRange(clang::SourceRange range);
+  bool checkFormatSpecifiers(const std::string& fmt, clang::SourceLocation loc);
   template <unsigned N>
   clang::DiagnosticBuilder error(clang::SourceLocation loc, const char (&fmt)[N]);
+  template <unsigned N>
+  clang::DiagnosticBuilder warning(clang::SourceLocation loc, const char (&fmt)[N]);
 
   clang::ASTContext &C;
   clang::DiagnosticsEngine &diag_;

tests/python/test_clang.py

@@ -5,6 +5,21 @@
 from bcc import BPF
 import ctypes
 from unittest import main, TestCase
+import os
+import sys
+from contextlib import contextmanager
+
+@contextmanager
+def redirect_stderr(to):
+    stderr_fd = sys.stderr.fileno()
+    with os.fdopen(os.dup(stderr_fd), 'wb') as copied, os.fdopen(to, 'w') as to:
+        sys.stderr.flush()
+        os.dup2(to.fileno(), stderr_fd)
+        try:
+            yield sys.stderr
+        finally:
+            sys.stderr.flush()
+            os.dup2(copied.fileno(), stderr_fd)
 
 class TestClang(TestCase):
     def test_complex(self):
@@ -419,8 +434,56 @@ int trace_read_entry(struct pt_regs *ctx, struct file *file) {
         b = BPF(text=text)
         b.attach_kprobe(event="__vfs_read", fn_name="trace_read_entry")
 
+    def test_printk_f(self):
+        text = """
+#include <uapi/linux/ptrace.h>
+int trace_entry(struct pt_regs *ctx) {
+  bpf_trace_printk("%0.2f\\n", 1);
+  return 0;
+}
+"""
+        r, w = os.pipe()
+        with redirect_stderr(to=w):
+            BPF(text=text)
+        r = os.fdopen(r)
+        output = r.read()
+        expectedWarn = "warning: only %d %u %x %ld %lu %lx %lld %llu %llx %p %s conversion specifiers allowed"
+        self.assertIn(expectedWarn, output)
+        r.close()
+
+    def test_printk_lf(self):
+        text = """
+#include <uapi/linux/ptrace.h>
+int trace_entry(struct pt_regs *ctx) {
+  bpf_trace_printk("%lf\\n", 1);
+  return 0;
+}
+"""
+        r, w = os.pipe()
+        with redirect_stderr(to=w):
+            BPF(text=text)
+        r = os.fdopen(r)
+        output = r.read()
+        expectedWarn = "warning: only %d %u %x %ld %lu %lx %lld %llu %llx %p %s conversion specifiers allowed"
+        self.assertIn(expectedWarn, output)
+        r.close()
+
+    def test_printk_2s(self):
+        text = """
+#include <uapi/linux/ptrace.h>
+int trace_entry(struct pt_regs *ctx) {
+  bpf_trace_printk("%s %s\\n", "hello", "world");
+  return 0;
+}
+"""
+        r, w = os.pipe()
+        with redirect_stderr(to=w):
+            BPF(text=text)
+        r = os.fdopen(r)
+        output = r.read()
+        expectedWarn = "warning: cannot use several %s conversion specifiers"
+        self.assertIn(expectedWarn, output)
+        r.close()
+
 if __name__ == "__main__":
     main()
-
-
-