cmd/compile, runtime: use __msan_memmove for moving data, split msanread to fields

Currently, for data moving, we generate an msanread of the source,
followed by an msanwrite of the destination. msanread checks
the source is initialized.

This has a problem: if the source is an aggregate type containing
alignment paddings, the padding bytes may not be thought as
initialized by MSAN. If we copy the aggregate type by value, if
it counts as a read, MSAN reports using uninitialized data. This
CL changes it to use __msan_memmove for data copying, which tells
MSAN to propagate initialized-ness but not check for it.

Caveat: technically __msan_memmove is not a public API of MSAN,
although the C compiler does generate direct calls to it.

Also, when instrumenting a load of a struct, split the
instrumentation to fields, instead of generating an msanread for
the whole struct. This skips padding bytes, which may not be
considered initialized in MSAN.

Fixes #42820.

Change-Id: Id861c8bbfd94cfcccefcc58eaf9e4eb43b4d85c6
Reviewed-on: https://go-review.googlesource.com/c/go/+/270859
Trust: Cherry Zhang <cherryyz@google.com>
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

diff --git a/misc/cgo/testsanitizers/msan_test.go b/misc/cgo/testsanitizers/msan_test.go
index 88b90d3d70d7961608c2118b9bb2572c6a3d50e1..5e2f9759baef49c1f92b9feed27f0de24e6c3000 100644 (file)
--- a/misc/cgo/testsanitizers/msan_test.go
+++ b/misc/cgo/testsanitizers/msan_test.go
@@ -28,6 +28,7 @@ func TestMSAN(t *testing.T) {
                {src: "msan4.go"},
                {src: "msan5.go"},
                {src: "msan6.go"},
+               {src: "msan7.go"},
                {src: "msan_fail.go", wantErr: true},
        }
        for _, tc := range cases {

diff --git a/misc/cgo/testsanitizers/testdata/msan7.go b/misc/cgo/testsanitizers/testdata/msan7.go
new file mode 100644 (file)
index 0000000..2f29fd2
--- /dev/null
+++ b/misc/cgo/testsanitizers/testdata/msan7.go
@@ -0,0 +1,38 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+// Test passing C struct to exported Go function.
+
+/*
+#include <stdint.h>
+#include <stdlib.h>
+
+// T is a C struct with alignment padding after b.
+// The padding bytes are not considered initialized by MSAN.
+// It is big enough to be passed on stack in C ABI (and least
+// on AMD64).
+typedef struct { char b; uintptr_t x, y; } T;
+
+extern void F(T);
+
+// Use weak as a hack to permit defining a function even though we use export.
+void CF(int x) __attribute__ ((weak));
+void CF(int x) {
+       T *t = malloc(sizeof(T));
+       t->b = (char)x;
+       t->x = x;
+       t->y = x;
+       F(*t);
+}
+*/
+import "C"
+
+//export F
+func F(t C.T) { println(t.b, t.x, t.y) }
+
+func main() {
+       C.CF(C.int(0))
+}

diff --git a/src/cmd/compile/internal/gc/builtin.go b/src/cmd/compile/internal/gc/builtin.go
index fd95b657b26621504abd70bd4ddcf0c7d984fafd..e04f23e2294ed6bfbd06681db61169e09f9e069e 100644 (file)
--- a/src/cmd/compile/internal/gc/builtin.go
+++ b/src/cmd/compile/internal/gc/builtin.go
@@ -184,16 +184,17 @@ var runtimeDecls = [...]struct {
        {"racewriterange", funcTag, 121},
        {"msanread", funcTag, 121},
        {"msanwrite", funcTag, 121},
-       {"checkptrAlignment", funcTag, 122},
-       {"checkptrArithmetic", funcTag, 124},
-       {"libfuzzerTraceCmp1", funcTag, 126},
-       {"libfuzzerTraceCmp2", funcTag, 128},
-       {"libfuzzerTraceCmp4", funcTag, 129},
-       {"libfuzzerTraceCmp8", funcTag, 130},
-       {"libfuzzerTraceConstCmp1", funcTag, 126},
-       {"libfuzzerTraceConstCmp2", funcTag, 128},
-       {"libfuzzerTraceConstCmp4", funcTag, 129},
-       {"libfuzzerTraceConstCmp8", funcTag, 130},
+       {"msanmove", funcTag, 122},
+       {"checkptrAlignment", funcTag, 123},
+       {"checkptrArithmetic", funcTag, 125},
+       {"libfuzzerTraceCmp1", funcTag, 127},
+       {"libfuzzerTraceCmp2", funcTag, 129},
+       {"libfuzzerTraceCmp4", funcTag, 130},
+       {"libfuzzerTraceCmp8", funcTag, 131},
+       {"libfuzzerTraceConstCmp1", funcTag, 127},
+       {"libfuzzerTraceConstCmp2", funcTag, 129},
+       {"libfuzzerTraceConstCmp4", funcTag, 130},
+       {"libfuzzerTraceConstCmp8", funcTag, 131},
        {"x86HasPOPCNT", varTag, 6},
        {"x86HasSSE41", varTag, 6},
        {"x86HasFMA", varTag, 6},
@@ -202,7 +203,7 @@ var runtimeDecls = [...]struct {
 }
 
 func runtimeTypes() []*types.Type {
-       var typs [131]*types.Type
+       var typs [132]*types.Type
        typs[0] = types.Bytetype
        typs[1] = types.NewPtr(typs[0])
        typs[2] = types.Types[TANY]
@@ -325,14 +326,15 @@ func runtimeTypes() []*types.Type {
        typs[119] = functype(nil, []*Node{anonfield(typs[65])}, []*Node{anonfield(typs[20])})
        typs[120] = functype(nil, []*Node{anonfield(typs[26]), anonfield(typs[26])}, []*Node{anonfield(typs[26])})
        typs[121] = functype(nil, []*Node{anonfield(typs[5]), anonfield(typs[5])}, nil)
-       typs[122] = functype(nil, []*Node{anonfield(typs[7]), anonfield(typs[1]), anonfield(typs[5])}, nil)
-       typs[123] = types.NewSlice(typs[7])
-       typs[124] = functype(nil, []*Node{anonfield(typs[7]), anonfield(typs[123])}, nil)
-       typs[125] = types.Types[TUINT8]
-       typs[126] = functype(nil, []*Node{anonfield(typs[125]), anonfield(typs[125])}, nil)
-       typs[127] = types.Types[TUINT16]
-       typs[128] = functype(nil, []*Node{anonfield(typs[127]), anonfield(typs[127])}, nil)
-       typs[129] = functype(nil, []*Node{anonfield(typs[65]), anonfield(typs[65])}, nil)
-       typs[130] = functype(nil, []*Node{anonfield(typs[24]), anonfield(typs[24])}, nil)
+       typs[122] = functype(nil, []*Node{anonfield(typs[5]), anonfield(typs[5]), anonfield(typs[5])}, nil)
+       typs[123] = functype(nil, []*Node{anonfield(typs[7]), anonfield(typs[1]), anonfield(typs[5])}, nil)
+       typs[124] = types.NewSlice(typs[7])
+       typs[125] = functype(nil, []*Node{anonfield(typs[7]), anonfield(typs[124])}, nil)
+       typs[126] = types.Types[TUINT8]
+       typs[127] = functype(nil, []*Node{anonfield(typs[126]), anonfield(typs[126])}, nil)
+       typs[128] = types.Types[TUINT16]
+       typs[129] = functype(nil, []*Node{anonfield(typs[128]), anonfield(typs[128])}, nil)
+       typs[130] = functype(nil, []*Node{anonfield(typs[65]), anonfield(typs[65])}, nil)
+       typs[131] = functype(nil, []*Node{anonfield(typs[24]), anonfield(typs[24])}, nil)
        return typs[:]
 }

diff --git a/src/cmd/compile/internal/gc/builtin/runtime.go b/src/cmd/compile/internal/gc/builtin/runtime.go
index aac2de38c699522b06683ac9c8c66915417a541f..acb69c7b282e13f88802dd393bad143350902d37 100644 (file)
--- a/src/cmd/compile/internal/gc/builtin/runtime.go
+++ b/src/cmd/compile/internal/gc/builtin/runtime.go
@@ -237,6 +237,7 @@ func racewriterange(addr, size uintptr)
 // memory sanitizer
 func msanread(addr, size uintptr)
 func msanwrite(addr, size uintptr)
+func msanmove(dst, src, size uintptr)
 
 func checkptrAlignment(unsafe.Pointer, *byte, uintptr)
 func checkptrArithmetic(unsafe.Pointer, []unsafe.Pointer)

diff --git a/src/cmd/compile/internal/gc/go.go b/src/cmd/compile/internal/gc/go.go
index da6b6d6e7274e2a6310ae9ddfefdaa89e31fdcc8..274930bd156f89e709e480361c24409eded4ad88 100644 (file)
--- a/src/cmd/compile/internal/gc/go.go
+++ b/src/cmd/compile/internal/gc/go.go
@@ -309,6 +309,7 @@ var (
        growslice,
        msanread,
        msanwrite,
+       msanmove,
        newobject,
        newproc,
        panicdivide,

diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go
index 0b38e70cd2a95562f7bf6f1a6ab1b5356d610fb0..65b9291b764d0237bc1f33fe6cd4ac76dd1c8406 100644 (file)
--- a/src/cmd/compile/internal/gc/ssa.go
+++ b/src/cmd/compile/internal/gc/ssa.go
@@ -79,6 +79,7 @@ func initssaconfig() {
        growslice = sysfunc("growslice")
        msanread = sysfunc("msanread")
        msanwrite = sysfunc("msanwrite")
+       msanmove = sysfunc("msanmove")
        newobject = sysfunc("newobject")
        newproc = sysfunc("newproc")
        panicdivide = sysfunc("panicdivide")
@@ -966,7 +967,45 @@ func (s *state) newValueOrSfCall2(op ssa.Op, t *types.Type, arg0, arg1 *ssa.Valu
        return s.newValue2(op, t, arg0, arg1)
 }
 
-func (s *state) instrument(t *types.Type, addr *ssa.Value, wr bool) {
+type instrumentKind uint8
+
+const (
+       instrumentRead = iota
+       instrumentWrite
+       instrumentMove
+)
+
+func (s *state) instrument(t *types.Type, addr *ssa.Value, kind instrumentKind) {
+       s.instrument2(t, addr, nil, kind)
+}
+
+// instrumentFields instruments a read/write operation on addr.
+// If it is instrumenting for MSAN and t is a struct type, it instruments
+// operation for each field, instead of for the whole struct.
+func (s *state) instrumentFields(t *types.Type, addr *ssa.Value, kind instrumentKind) {
+       if !flag_msan || !t.IsStruct() {
+               s.instrument(t, addr, kind)
+               return
+       }
+       for _, f := range t.Fields().Slice() {
+               if f.Sym.IsBlank() {
+                       continue
+               }
+               offptr := s.newValue1I(ssa.OpOffPtr, types.NewPtr(f.Type), f.Offset, addr)
+               s.instrumentFields(f.Type, offptr, kind)
+       }
+}
+
+func (s *state) instrumentMove(t *types.Type, dst, src *ssa.Value) {
+       if flag_msan {
+               s.instrument2(t, dst, src, instrumentMove)
+       } else {
+               s.instrument(t, src, instrumentRead)
+               s.instrument(t, dst, instrumentWrite)
+       }
+}
+
+func (s *state) instrument2(t *types.Type, addr, addr2 *ssa.Value, kind instrumentKind) {
        if !s.curfn.Func.InstrumentBody() {
                return
        }
@@ -983,33 +1022,54 @@ func (s *state) instrument(t *types.Type, addr *ssa.Value, wr bool) {
        var fn *obj.LSym
        needWidth := false
 
+       if addr2 != nil && kind != instrumentMove {
+               panic("instrument2: non-nil addr2 for non-move instrumentation")
+       }
+
        if flag_msan {
-               fn = msanread
-               if wr {
+               switch kind {
+               case instrumentRead:
+                       fn = msanread
+               case instrumentWrite:
                        fn = msanwrite
+               case instrumentMove:
+                       fn = msanmove
+               default:
+                       panic("unreachable")
                }
                needWidth = true
        } else if flag_race && t.NumComponents(types.CountBlankFields) > 1 {
                // for composite objects we have to write every address
                // because a write might happen to any subobject.
                // composites with only one element don't have subobjects, though.
-               fn = racereadrange
-               if wr {
+               switch kind {
+               case instrumentRead:
+                       fn = racereadrange
+               case instrumentWrite:
                        fn = racewriterange
+               default:
+                       panic("unreachable")
                }
                needWidth = true
        } else if flag_race {
                // for non-composite objects we can write just the start
                // address, as any write must write the first byte.
-               fn = raceread
-               if wr {
+               switch kind {
+               case instrumentRead:
+                       fn = raceread
+               case instrumentWrite:
                        fn = racewrite
+               default:
+                       panic("unreachable")
                }
        } else {
                panic("unreachable")
        }
 
        args := []*ssa.Value{addr}
+       if addr2 != nil {
+               args = append(args, addr2)
+       }
        if needWidth {
                args = append(args, s.constInt(types.Types[TUINTPTR], w))
        }
@@ -1017,7 +1077,7 @@ func (s *state) instrument(t *types.Type, addr *ssa.Value, wr bool) {
 }
 
 func (s *state) load(t *types.Type, src *ssa.Value) *ssa.Value {
-       s.instrument(t, src, false)
+       s.instrumentFields(t, src, instrumentRead)
        return s.rawLoad(t, src)
 }
 
@@ -1030,15 +1090,14 @@ func (s *state) store(t *types.Type, dst, val *ssa.Value) {
 }
 
 func (s *state) zero(t *types.Type, dst *ssa.Value) {
-       s.instrument(t, dst, true)
+       s.instrument(t, dst, instrumentWrite)
        store := s.newValue2I(ssa.OpZero, types.TypeMem, t.Size(), dst, s.mem())
        store.Aux = t
        s.vars[&memVar] = store
 }
 
 func (s *state) move(t *types.Type, dst, src *ssa.Value) {
-       s.instrument(t, src, false)
-       s.instrument(t, dst, true)
+       s.instrumentMove(t, dst, src)
        store := s.newValue3I(ssa.OpMove, types.TypeMem, t.Size(), dst, src, s.mem())
        store.Aux = t
        s.vars[&memVar] = store
@@ -5248,7 +5307,7 @@ func (s *state) rtcall(fn *obj.LSym, returns bool, results []*types.Type, args .
 
 // do *left = right for type t.
 func (s *state) storeType(t *types.Type, left, right *ssa.Value, skip skipMask, leftIsStmt bool) {
-       s.instrument(t, left, true)
+       s.instrument(t, left, instrumentWrite)
 
        if skip == 0 && (!t.HasPointers() || ssa.IsStackAddr(left)) {
                // Known to not have write barrier. Store the whole type.

diff --git a/src/runtime/msan.go b/src/runtime/msan.go
index c0f3957e2891e3dadb514ef5c673bd15f64ead8c..6a5960b0a8e0b703a848c847dfefff32dcf43b89 100644 (file)
--- a/src/runtime/msan.go
+++ b/src/runtime/msan.go
@@ -50,8 +50,12 @@ func msanmalloc(addr unsafe.Pointer, sz uintptr)
 //go:noescape
 func msanfree(addr unsafe.Pointer, sz uintptr)
 
-// These are called from msan_amd64.s
+//go:noescape
+func msanmove(dst, src unsafe.Pointer, sz uintptr)
+
+// These are called from msan_GOARCH.s
 //go:cgo_import_static __msan_read_go
 //go:cgo_import_static __msan_write_go
 //go:cgo_import_static __msan_malloc_go
 //go:cgo_import_static __msan_free_go
+//go:cgo_import_static __msan_memmove

diff --git a/src/runtime/msan_amd64.s b/src/runtime/msan_amd64.s
index cbe739df53a66d6e29f97c55958ac2f0c988ed28..669e9ca73f803babbfda2abc92d299c76bbb4487 100644 (file)
--- a/src/runtime/msan_amd64.s
+++ b/src/runtime/msan_amd64.s
@@ -58,6 +58,15 @@ TEXT runtime·msanfree(SB), NOSPLIT, $0-16
        MOVQ    $__msan_free_go(SB), AX
        JMP     msancall<>(SB)
 
+// func runtime·msanmove(dst, src unsafe.Pointer, sz uintptr)
+TEXT   runtime·msanmove(SB), NOSPLIT, $0-24
+       MOVQ    dst+0(FP), RARG0
+       MOVQ    src+8(FP), RARG1
+       MOVQ    size+16(FP), RARG2
+       // void __msan_memmove(void *dst, void *src, uintptr_t sz);
+       MOVQ    $__msan_memmove(SB), AX
+       JMP     msancall<>(SB)
+
 // Switches SP to g0 stack and calls (AX). Arguments already set.
 TEXT   msancall<>(SB), NOSPLIT, $0-0
        get_tls(R12)

diff --git a/src/runtime/msan_arm64.s b/src/runtime/msan_arm64.s
index 5e29f1aefbb8b3981fce5b746c2e8b2cfbeed9b1..f19906cfc838f8b1f7bf25d19d6cd7b353aaaad3 100644 (file)
--- a/src/runtime/msan_arm64.s
+++ b/src/runtime/msan_arm64.s
@@ -9,6 +9,7 @@
 
 #define RARG0 R0
 #define RARG1 R1
+#define RARG2 R2
 #define FARG R3
 
 // func runtime·domsanread(addr unsafe.Pointer, sz uintptr)
@@ -45,6 +46,15 @@ TEXT runtime·msanfree(SB), NOSPLIT, $0-16
        MOVD    $__msan_free_go(SB), FARG
        JMP     msancall<>(SB)
 
+// func runtime·msanmove(dst, src unsafe.Pointer, sz uintptr)
+TEXT   runtime·msanmove(SB), NOSPLIT, $0-24
+       MOVD    dst+0(FP), RARG0
+       MOVD    src+8(FP), RARG1
+       MOVD    size+16(FP), RARG2
+       // void __msan_memmove(void *dst, void *src, uintptr_t sz);
+       MOVD    $__msan_memmove(SB), FARG
+       JMP     msancall<>(SB)
+
 // Switches SP to g0 stack and calls (FARG). Arguments already set.
 TEXT   msancall<>(SB), NOSPLIT, $0-0
        MOVD    RSP, R19                  // callee-saved