From 21d737fbf9bad3f2a9190bc31212f29edbfcaeb3 Mon Sep 17 00:00:00 2001 From: Aleksandr Nogikh Date: Wed, 28 Sep 2022 13:17:51 +0000 Subject: sys: control structural changes during neutralization Ideally, we should properly support the already existing fix flag to distinguish between fixing and checking, but for now at least let it control whether structural changes are to be made. Otherwise we get into trouble while hint-mutating syz_mount_image calls, because we iterate over all call arguments and (possibly) remove them at the same time. It leads to `bad group arg size %v, should be <= %v for %#v type %#v` errors. --- prog/target.go | 13 ++++++--- sys/akaros/init.go | 3 +- sys/linux/init.go | 10 +++++-- sys/linux/init_images.go | 56 +++++++++++++++++++++++++----------- sys/linux/init_images_test.go | 8 ++++-- sys/linux/testdata/fs_images/0.in | 2 +- sys/linux/testdata/fs_images/0.out0 | Bin 8736 -> 8736 bytes sys/openbsd/init.go | 5 ++-- sys/targets/common.go | 7 +++-- 9 files changed, 70 insertions(+), 34 deletions(-) diff --git a/prog/target.go b/prog/target.go index bc619d5b6..d50f1bf8f 100644 --- a/prog/target.go +++ b/prog/target.go @@ -33,7 +33,10 @@ type Target struct { // Neutralize neutralizes harmful calls by transforming them into non-harmful ones // (e.g. an ioctl that turns off console output is turned into ioctl that turns on output). - Neutralize func(c *Call) + // fixStructure determines whether it's allowed to make structural changes (e.g. add or + // remove arguments). It is helpful e.g. when we do neutralization while iterating over the + // arguments. + Neutralize func(c *Call, fixStructure bool) error // AnnotateCall annotates a syscall invocation in C reproducers. // The returned string will be placed inside a comment except for the @@ -128,7 +131,7 @@ func AllTargets() []*Target { } func (target *Target) lazyInit() { - target.Neutralize = func(c *Call) {} + target.Neutralize = func(c *Call, fixStructure bool) error { return nil } target.AnnotateCall = func(c ExecCall) string { return "" } target.initTarget() target.initArch(target) @@ -190,8 +193,10 @@ func (target *Target) GetConst(name string) uint64 { } func (target *Target) sanitize(c *Call, fix bool) error { - target.Neutralize(c) - return nil + // For now, even though we accept the fix argument, it does not have the full effect. + // It de facto only denies structural changes, e.g. deletions of arguments. + // TODO: rewrite the corresponding sys/*/init.go code. + return target.Neutralize(c, fix) } func RestoreLinks(syscalls []*Syscall, resources []*ResourceDesc, types []Type) { diff --git a/sys/akaros/init.go b/sys/akaros/init.go index fa987eb7a..7f8f1dc16 100644 --- a/sys/akaros/init.go +++ b/sys/akaros/init.go @@ -20,7 +20,7 @@ func InitTarget(target *prog.Target) { target.Neutralize = arch.Neutralize } -func (arch *arch) Neutralize(c *prog.Call) { +func (arch *arch) Neutralize(c *prog.Call, fixStructure bool) error { switch c.Meta.CallName { case "mmap": c.Args[3].(*prog.ConstArg).Val |= arch.MAP_FIXED @@ -30,4 +30,5 @@ func (arch *arch) Neutralize(c *prog.Call) { pid.Val = 0 } } + return nil } diff --git a/sys/linux/init.go b/sys/linux/init.go index 09b70f092..7d537075c 100644 --- a/sys/linux/init.go +++ b/sys/linux/init.go @@ -173,8 +173,11 @@ type arch struct { TIOCGSERIAL uint64 } -func (arch *arch) neutralize(c *prog.Call) { - arch.unix.Neutralize(c) +func (arch *arch) neutralize(c *prog.Call, fixStructure bool) error { + err := arch.unix.Neutralize(c, fixStructure) + if err != nil { + return err + } switch c.Meta.CallName { case "mremap": // Add MREMAP_FIXED flag, otherwise it produces non-deterministic results. @@ -243,13 +246,14 @@ func (arch *arch) neutralize(c *prog.Call) { // Enabling a SCHED_FIFO or a SCHED_RR policy may lead to false positive stall-related crashes. neutralizeSchedAttr(c.Args[1]) case "syz_mount_image": - arch.fixUpSyzMountImage(c) + return arch.fixUpSyzMountImage(c, fixStructure) } switch c.Meta.Name { case "setsockopt$EBT_SO_SET_ENTRIES": arch.neutralizeEbtables(c) } + return nil } func neutralizeSchedAttr(a prog.Arg) { diff --git a/sys/linux/init_images.go b/sys/linux/init_images.go index eb1d7c7ad..e160234d9 100644 --- a/sys/linux/init_images.go +++ b/sys/linux/init_images.go @@ -64,9 +64,9 @@ func (zr *zeroReader) Read(p []byte) (n int, err error) { const imageMaxSize = 129 << 20 -func fixUpImageSegments(parsed *mountImageArgs) { - const maxImageSegments = 4096 - parsed.filterSegments(func(i int, segment *mountImageSegment) bool { +func fixUpImageSegments(parsed *mountImageArgs, fixStructure bool) error { + const maxImageSegments = 16784 + err := parsed.filterSegments(func(i int, segment *mountImageSegment) bool { if i >= maxImageSegments { // We don't want more than maxImageSegments segments. return false @@ -76,13 +76,16 @@ func fixUpImageSegments(parsed *mountImageArgs) { return false } return segment.offset.Val < imageMaxSize && segment.size.Val < imageMaxSize - }) + }, !fixStructure) + if err != nil { + return err + } // Overwriting of the image multiple times is not efficient and complicates image extraction in Go. // So let's make segments non-overlapping. - sort.Stable(parsed) - resizeSegment := func(s *mountImageSegment, size uint64) { - s.size.Val = size - s.data.SetData(s.data.Data()[0:size]) + if fixStructure { + sort.Stable(parsed) + } else if !sort.IsSorted(parsed) { + return fmt.Errorf("segments are not sorted") } newSize := parsed.size.Val @@ -99,11 +102,19 @@ func fixUpImageSegments(parsed *mountImageArgs) { // Adjust the end of the previous segment. prevSegment := parsed.segments[idx-1] if prevSegment.offset.Val+prevSegment.size.Val > segment.offset.Val { - resizeSegment(prevSegment, segment.offset.Val-prevSegment.offset.Val) + if fixStructure { + prevSegment.resize(segment.offset.Val - prevSegment.offset.Val) + } else { + return fmt.Errorf("segment %d has invalid size", idx-1) + } } } if segment.offset.Val+segment.size.Val > imageMaxSize { - resizeSegment(segment, imageMaxSize-segment.offset.Val) + if fixStructure { + segment.resize(imageMaxSize - segment.offset.Val) + } else { + return fmt.Errorf("segment %d has invalid size", idx) + } } if segment.offset.Val+segment.size.Val > newSize { newSize = segment.offset.Val + segment.size.Val @@ -116,12 +127,12 @@ func fixUpImageSegments(parsed *mountImageArgs) { parsed.size.Val = newSize // Drop 0-size segments. - parsed.filterSegments(func(i int, segment *mountImageSegment) bool { + return parsed.filterSegments(func(i int, segment *mountImageSegment) bool { return segment.size.Val > 0 - }) + }, !fixStructure) } -func (arch *arch) fixUpSyzMountImage(c *prog.Call) { +func (arch *arch) fixUpSyzMountImage(c *prog.Call, fixStructure bool) error { // Previously we did such a sanitization right in the common_linux.h, but this was problematic // for two reasons: // 1) It further complicates the already complicated executor code. @@ -129,10 +140,13 @@ func (arch *arch) fixUpSyzMountImage(c *prog.Call) { // So now we do all the initialization in Go and let the C code only interpret the commands. ret, err := parseSyzMountImage(c) if err != nil { - deactivateSyzMountImage(c) - return + if fixStructure { + deactivateSyzMountImage(c) + return nil + } + return err } - fixUpImageSegments(ret) + return fixUpImageSegments(ret, fixStructure) } type mountImageArgs struct { @@ -142,18 +156,21 @@ type mountImageArgs struct { segments []*mountImageSegment } -func (m *mountImageArgs) filterSegments(filter func(int, *mountImageSegment) bool) { +func (m *mountImageArgs) filterSegments(filter func(int, *mountImageSegment) bool, failOnRemove bool) error { newArgs := []prog.Arg{} newSegments := []*mountImageSegment{} for i, segment := range m.segments { if filter(i, segment) { newSegments = append(newSegments, segment) newArgs = append(newArgs, m.segmentsGroup.Inner[i]) + } else if failOnRemove { + return fmt.Errorf("segment #%d got filtered out", i) } } m.segments = newSegments m.segmentsGroup.Inner = newArgs m.segmentsCount.Val = uint64(len(newArgs)) + return nil } // Methods for segment sorting. @@ -177,6 +194,11 @@ type mountImageSegment struct { parseError error } +func (s *mountImageSegment) resize(newSize uint64) { + s.size.Val = newSize + s.data.SetData(s.data.Data()[0:newSize]) +} + func parseImageSegment(segmentArg prog.Arg) *mountImageSegment { ret := &mountImageSegment{} segmentFields, ok := segmentArg.(*prog.GroupArg) diff --git a/sys/linux/init_images_test.go b/sys/linux/init_images_test.go index 0bfb68cc4..1da41276e 100644 --- a/sys/linux/init_images_test.go +++ b/sys/linux/init_images_test.go @@ -38,12 +38,14 @@ func TestSyzMountImageNeutralize(t *testing.T) { // Invalid offset. In: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file1\x00', 0x20, 0x2, &(0x7f0000000200)=[{&(0x7f0000010000)="cefaad1bc0210000ff0f0000ffffffffffffffffffffffffffffffff73797a6b616c73797a6b616c00"/64, 0x40, 0x0}, {&(0x7f0000010040)="0200000011000000140000001f22000002000000ed4100000000000001000000020000005ffb19635ffb19635ffb196300"/64, 0x40, 0x9100000}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, // The segment is deleted. - Out: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file1\x00', 0x40, 0x1, &(0x7f0000000200)=[{&(0x7f0000010000)="cefaad1bc0210000ff0f0000ffffffffffffffffffffffffffffffff73797a6b616c73797a6b616c00"/64, 0x40, 0x0}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, + Out: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file1\x00', 0x40, 0x1, &(0x7f0000000200)=[{&(0x7f0000010000)="cefaad1bc0210000ff0f0000ffffffffffffffffffffffffffffffff73797a6b616c73797a6b616c00"/64, 0x40, 0x0}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, + StrictErr: `got filtered out`, }, { // Overlapping and unsorted segments. - In: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x3, &(0x7f0000000200)=[{&(0x7f0000010000)="cafef00d"/64, 0x50, 0x20}, {&(0x7f0000010040)="deadbeef"/64, 0x30, 0x10}, {&(0x7f0000010080)="abcdef"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, - Out: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x2, &(0x7f0000000200)=[{&(0x7f0000010040)="deadbeef00"/16, 0x10, 0x10}, {&(0x7f0000010000)="cafef00d00"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, + In: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x3, &(0x7f0000000200)=[{&(0x7f0000010000)="cafef00d"/64, 0x50, 0x20}, {&(0x7f0000010040)="deadbeef"/64, 0x30, 0x10}, {&(0x7f0000010080)="abcdef"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, + Out: `syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x2, &(0x7f0000000200)=[{&(0x7f0000010040)="deadbeef00"/16, 0x10, 0x10}, {&(0x7f0000010000)="cafef00d00"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0)`, + StrictErr: `segments are not sorted`, }, }) } diff --git a/sys/linux/testdata/fs_images/0.in b/sys/linux/testdata/fs_images/0.in index 3eed397dd..7080226a0 100644 --- a/sys/linux/testdata/fs_images/0.in +++ b/sys/linux/testdata/fs_images/0.in @@ -1 +1 @@ -syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x3, &(0x7f0000000200)=[{&(0x7f0000010040)="deadbeef00"/64, 0x10, 0x10}, {&(0x7f0000010000)="cafef00d00"/64, 0x0, 0x20}, {&(0x7f0000010080)="abcdef00"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}) \ No newline at end of file +syz_mount_image$bfs(&(0x7f0000000000)='bfs\x00', &(0x7f0000000100)='./file0\x00', 0x2220, 0x2, &(0x7f0000000200)=[{&(0x7f0000010040)="deadbeef00"/16, 0x10, 0x10}, {&(0x7f0000010000)="cafef00d00"/64, 0x40, 0x20}], 0x0, &(0x7f00000100a0)={[], [], 0x0}, 0x0) \ No newline at end of file diff --git a/sys/linux/testdata/fs_images/0.out0 b/sys/linux/testdata/fs_images/0.out0 index 424d24bd8..4eb5233c9 100644 Binary files a/sys/linux/testdata/fs_images/0.out0 and b/sys/linux/testdata/fs_images/0.out0 differ diff --git a/sys/openbsd/init.go b/sys/openbsd/init.go index 497291244..2a2aeae4c 100644 --- a/sys/openbsd/init.go +++ b/sys/openbsd/init.go @@ -82,7 +82,7 @@ func isExecutorFd(dev uint64) bool { return major == devFdMajor && minor >= 200 } -func (arch *arch) neutralize(c *prog.Call) { +func (arch *arch) neutralize(c *prog.Call, fixStructure bool) error { argStart := 1 switch c.Meta.CallName { case "chflagsat": @@ -150,8 +150,9 @@ func (arch *arch) neutralize(c *prog.Call) { case "sysctl": arch.neutralizeSysctl(c) default: - arch.unix.Neutralize(c) + return arch.unix.Neutralize(c, fixStructure) } + return nil } func (arch *arch) neutralizeClockSettime(c *prog.Call) { diff --git a/sys/targets/common.go b/sys/targets/common.go index f55493256..a29365ce3 100644 --- a/sys/targets/common.go +++ b/sys/targets/common.go @@ -82,12 +82,12 @@ func MakeUnixNeutralizer(target *prog.Target) *UnixNeutralizer { } } -func (arch *UnixNeutralizer) Neutralize(c *prog.Call) { +func (arch *UnixNeutralizer) Neutralize(c *prog.Call, fixStructure bool) error { switch c.Meta.CallName { case "mmap": if c.Meta.Name == "mmap$bifrost" { // Mali bifrost mmap doesn't support MAP_FIXED. - return + return nil } // Add MAP_FIXED flag, otherwise it produces non-deterministic results. c.Args[3].(*prog.ConstArg).Val |= arch.MAP_FIXED @@ -98,7 +98,7 @@ func (arch *UnixNeutralizer) Neutralize(c *prog.Call) { } switch c.Args[pos+1].Type().(type) { case *prog.ProcType, *prog.ResourceType: - return + return nil } mode := c.Args[pos].(*prog.ConstArg) dev := c.Args[pos+1].(*prog.ConstArg) @@ -128,4 +128,5 @@ func (arch *UnixNeutralizer) Neutralize(c *prog.Call) { code.Val = 1 } } + return nil } -- cgit mrf-deployment