ipc: fix endianness issues #1854

eaibmz · 2020-06-22T14:53:54Z

Use native byte-order for IPC and program serialization.
This way we will be able to support both little- and big-endian
architectures.

Signed-off-by: Alexander Egorenkov Alexander.Egorenkov@ibm.com

Before sending a pull request, please review Contribution Guidelines:
https://github.com/google/syzkaller/blob/master/docs/contributing.md

codecov · 2020-06-22T15:07:51Z

Codecov Report

Merging #1854 into master will increase coverage by 0.0%.
The diff coverage is 83.3%.

Impacted Files	Coverage Δ
pkg/ipc/ipc.go	`49.5% <50.0%> (ø)`
prog/decodeexec.go	`77.6% <100.0%> (-0.4%)`	⬇️
prog/encodingexec.go	`86.2% <100.0%> (-0.4%)`	⬇️
prog/target.go	`61.5% <0.0%> (-5.8%)`	⬇️
prog/prog.go	`80.1% <0.0%> (+0.9%)`	⬆️
prog/mutation.go	`90.8% <0.0%> (+2.2%)`	⬆️

dvyukov · 2020-06-22T15:26:00Z

executor/executor.cc

+	if (output_pos < output_data || (char*)output_pos >= (char*)output_data + kMaxOutput)
+		fail("output overflow: pos=%p region=[%p:%p]",
+		     output_pos, output_data, (char*)output_data + kMaxOutput);
+	*output_pos = v;


in pkg/ipc/ipc.go we do:

func readUint64(outp *[]byte) (uint64, bool) { out := *outp if len(out) < 8 { return 0, false } v := binary.LittleEndian.Uint64(out) *outp = out[8:] return v, true }

that's native endianess, right.
Or, do you plan more changes to pkg/ipc as well?

added more ipc fixes to the commit, i wanted first to separate them but ok

dvyukov · 2020-06-22T15:26:19Z

executor/executor_linux.h

@@ -167,8 +167,11 @@ static void cover_reset(cover_t* cov)

 static void cover_collect(cover_t* cov)
 {
-	// Note: this assumes little-endian kernel.
-	cov->size = *(uint32*)cov->data;
+	if (is_kernel_64_bit) {


Please no braces {} around single-statement blocks.

dvyukov · 2020-06-22T15:26:58Z

A meta comment to this and previous changes:
how can we make this tested on CI?

eaibmz · 2020-06-22T15:37:31Z

A meta comment to this and previous changes:
how can we make this tested on CI?

Later i will provide support for IBM's s390x 64-bit big-endian architecture which can be run in QEMU on x86 w/o KVM or with on IBM/Z.
But first i need to fix all the issues with endianness and there are many :)

eaibmz · 2020-06-22T15:48:43Z

executor/executor.cc

@@ -95,6 +95,7 @@ const int kOutFd = 4;
 static uint32* output_data;
 static uint32* output_pos;
 static uint32* write_output(uint32 v);
+static uint32* write_output_64(uint64 v);


I'm not sure about the name. Maybe we should use function overloading here but i decided against because
it is easy to misuse and pass uint32 instead of uint64 and compiler will not warn about it. What do you think ?

Let's go with this version for now. I don't see significant reasons to switch to something else now.

eaibmz · 2020-06-22T21:13:21Z

NB: Tests like sys/test/test/align0 are going to be the next block on the road to the first big-endian architecture. It also assumes little-endian architecture, sigh. Any ideas how to better address that ?

But otherwise, all unit tests run now in my s390x syzkaller setup.

Thank you for feedback

dvyukov · 2020-06-23T08:45:41Z

executor/executor.cc

@@ -95,6 +95,7 @@ const int kOutFd = 4;
 static uint32* output_data;
 static uint32* output_pos;
 static uint32* write_output(uint32 v);
+static uint32* write_output_64(uint64 v);


Let's go with this version for now. I don't see significant reasons to switch to something else now.

dvyukov · 2020-06-23T08:46:59Z

executor/executor.cc

@@ -1308,6 +1309,15 @@ uint32* write_output(uint32 v)
 	return output_pos++;
 }

+uint32* write_output_64(uint64 v)
+{
+	if (output_pos < output_data || (char*)output_pos >= (char*)output_data + kMaxOutput)


We now need some adjustment to the check I think. It assumes we write a single element.

good finding, thanks

dvyukov · 2020-06-23T08:50:42Z

prog/encodingexec_test.go

@@ -204,7 +210,8 @@ func TestSerializeForExec(t *testing.T) {
 			"test$array1(&(0x7f0000000000)={0x42, \"0102030405\"})",
 			[]uint64{
 				execInstrCopyin, dataOffset + 0, execArgConst, 1, 0x42,
-				execInstrCopyin, dataOffset + 1, execArgData, 5, 0x0504030201,
+				execInstrCopyin, dataOffset + 1, execArgData, 5,
+				convDataToUint64([]byte{0x01, 0x02, 0x03, 0x04, 0x05}),


This is quite verbose and convoluted way to represent numbers. I will need some additional brain cycles to decode this while reading. Will it work if we add letoh64 and then do letoh64(0x0504030201)?

Hmm, i must disagree here because i find it more readable if i do not have to swap bytes every time i look at this code :) But i do not mind changing it, no big deal.

dvyukov · 2020-06-23T08:59:11Z

prog/encodingexec_test.go

+			for _, v := range test.serialized {
+				tmp := make([]byte, 8)
+				*(*uint64)(unsafe.Pointer(&tmp[0])) = v
+				w.Write(tmp)


All these changes... here, in prog package and in ipc package... it's not that I am opposed to every single use of unsafe... but we are adding lots of them throughout and new imports of "unsafe". Since this is now something we need to care about throughout the codebase, I am trying to consider alternatives.
For example, if we would have binary.NativeEndian (or HostEndian), it seems that it would allow to quite nice solution. Say, here, we just change s/LittleEndian/HostEndian. What do you think?
I am surprised binary package does not have it... is there something I am missing?.. whatever... we can add it ourselves.
We need to add it in some low-level enough package to avoid circular dependencies. That's probably sys/targets for now.
I think we can even avoid any additional overhead and indirection by doing (for each arch:

// little_endian.go // +build amd64 386 arm ... package targets import "encdoing/binary" var HostEndian = binary.LittleEndian

This is a great idea, i like it :) I also find it abhorrent to add all those casting statements, it feels dirty. I also was surprised as i discovered that Go doesn't offer something like binary.NativeEndian.

Let me rework it then.

Re Go, that's:
golang/go#35398
golang/go#36040
and:
golang/go#37658
So it's probably not going to happen.

A problem. targets package uses prog package already but i need HostEndian in the prog package, cycle. Any suggestion ?

What if we make Endianness a field of Target ? I will need it anyways later for extractFromELF ?

type Target struct { ... ByteOrder binary.ByteOrder ... }

You are right re cycle. Then I guess we need to put it into prog package.

I would prefer both standalone prog.HostEndianess and optionally as field of target. The reason for standalone is that it will allow inlining and not forcing arguments to escape.

dvyukov · 2020-06-23T09:07:34Z

NB: Tests like sys/test/test/align0 are going to be the next block on the road to the first big-endian architecture. It also assumes little-endian architecture, sigh. Any ideas how to better address that ?

Oh, you are running these tests. That's good.
We can start by adding littleendian requirement, see these "# requires:" just to disable the known failing tests.
If you are asking how to actually run these tests... well, an obvious thing is to add few additional tests for !littleendian (for some basic cases and for bugs/regressions).
I don't know how to magically run all tests. They are very specifically hardcode exact layout in memory without any additional complexity (like splitting fields and marking them as le/be).
We could add some additional syz_* function that would, say, convert one of arguments from le to host or something and write some tests using such function, if it will be useful.

dvyukov · 2020-06-23T09:12:05Z

But otherwise, all unit tests run now in my s390x syzkaller setup.

Cool!
Would it be possible to run some small subset of tests on CI using qemu tcg? IIRC it is able to somehow run e.g. arm binary on x86 magically. If we install some additional qemu packages, can we run something?

We would also like to run some non-x86 arches on syzbot using qemu tcg.
The major problem is that it's super slow. Know problems: #1552 and #1679 (to not spend half an hour booting full distro image). But there may be some other problems as well. We could start by ignoring all "kernel stall/hang" bugs on these instances.
Just in case you are interested in helping :)

eaibmz · 2020-06-23T10:28:11Z

NB: Tests like sys/test/test/align0 are going to be the next block on the road to the first big-endian architecture. It also assumes little-endian architecture, sigh. Any ideas how to better address that ?

Oh, you are running these tests. That's good.
We can start by adding littleendian requirement, see these "# requires:" just to disable the known failing tests.
If you are asking how to actually run these tests... well, an obvious thing is to add few additional tests for !littleendian (for some basic cases and for bugs/regressions).
I don't know how to magically run all tests. They are very specifically hardcode exact layout in memory without any additional complexity (like splitting fields and marking them as le/be).
We could add some additional syz_* function that would, say, convert one of arguments from le to host or something and write some tests using such function, if it will be useful.

I run all unit tests but those which hardcode data in little-endian format fail of course on s390x big-endian arch.

I must say i find the idea of adding littleendian flag on the one side very good because it will allow me to disable failing little-endian tests quickly and proceed with my port. But on the other side i find it's not good disabling tests because they fail. Which leaves me the option of duplicating the same tests and fixing byte-order, which is better but not quite because duplication is bad. What do you think ?

Could you elaborate your idea with new 'syz_*' functions please ?

dvyukov · 2020-06-23T10:32:20Z

Which leaves me the option of duplicating the same tests and fixing byte-order, which is better but not quite because duplication is bad. What do you think ?

That's the only option that I see now. I am fine adding few basic tests and tests for bugs/regressions.

Could you elaborate your idea with new 'syz_*' functions please ?

No. Because I don't have any details :)
It may help to write some tests maybe b/c it can swap order, while the syzkaller program notation does not allow that... or maybe it does with int64be type.

eaibmz · 2020-06-23T10:49:13Z

But otherwise, all unit tests run now in my s390x syzkaller setup.

Cool!
Would it be possible to run some small subset of tests on CI using qemu tcg? IIRC it is able to somehow run e.g. arm binary on x86 magically. If we install some additional qemu packages, can we run something?

We would also like to run some non-x86 arches on syzbot using qemu tcg.
The major problem is that it's super slow. Know problems: #1552 and #1679 (to not spend half an hour booting full distro image). But there may be some other problems as well. We could start by ignoring all "kernel stall/hang" bugs on these instances.
Just in case you are interested in helping :)

You probably mean the binfmt_misc file system and /proc/sys/fs/binfmt_misc.
I actually used it on my x86 as i started working on the s390x port of syzkaller, boostraping a debian rootfs with qemu-system-s390x.

What you mean (correct me if i'm wrong) is cross-compiling Go unit tests for s390x arch on x86 arch and run the produced binary with qemu-system-s390x. Let me test it and then report back, right now i cannot say anything about it. But otherwise qemu tcg s390x was fine on my x86 machine. Slow but it worked :)

BTW, i also ported s390x to buildroot and will upstream it soon. I use it now for syzkaller testing.
It is much faster than a debian with QEMU TCG on my x86 machine.

Hmm, half hour booting, that's very long. I need maybe a couple of minutes to boot my buildroot kernel + rootfs prepared for syzakller.

dvyukov · 2020-06-23T11:05:10Z

You probably mean the binfmt_misc file system and /proc/sys/fs/binfmt_misc.

You are right.
But we could run qemu explicitly as well. I did not think about integration into testing system yet.

BTW, i also ported s390x to buildroot and will upstream it soon. I use it now for syzkaller testing.

Good. Yes, buildroot should produce something much more thin than a modern Debian with systemd.

eaibmz · 2020-06-23T13:46:04Z

Which leaves me the option of duplicating the same tests and fixing byte-order, which is better but not quite because duplication is bad. What do you think ?

That's the only option that I see now. I am fine adding few basic tests and tests for bugs/regressions.

I will address !littleendian in the next PR.

Use native byte-order for IPC and program serialization. This way we will be able to support both little- and big-endian architectures. Signed-off-by: Alexander Egorenkov <Alexander.Egorenkov@ibm.com>

dvyukov · 2020-06-23T14:18:39Z

Nice!

dvyukov · 2020-06-23T14:23:09Z

Small followup: 8e0c064

eaibmz force-pushed the upstream-next branch from 1718f60 to b944dbd Compare June 22, 2020 14:54

eaibmz changed the title ~~executor: fix endianness issues for KCOV IPC~~ executor: fix endianness issues in KCOV IPC Jun 22, 2020

eaibmz force-pushed the upstream-next branch from b944dbd to e326286 Compare June 22, 2020 15:22

eaibmz changed the title ~~executor: fix endianness issues in KCOV IPC~~ executor: use native endianness for KCOV data Jun 22, 2020

dvyukov reviewed Jun 22, 2020

View reviewed changes

eaibmz force-pushed the upstream-next branch from e326286 to 0c68293 Compare June 22, 2020 15:33

eaibmz changed the title ~~executor: use native endianness for KCOV data~~ ipc: fix endianness issues Jun 22, 2020

eaibmz commented Jun 22, 2020

View reviewed changes

eaibmz force-pushed the upstream-next branch 4 times, most recently from 37bfd94 to ebdc38a Compare June 22, 2020 20:31

eaibmz force-pushed the upstream-next branch 2 times, most recently from 34bfe4a to 457ef1b Compare June 23, 2020 07:01

dvyukov reviewed Jun 23, 2020

View reviewed changes

eaibmz force-pushed the upstream-next branch from 457ef1b to 76a6a27 Compare June 23, 2020 13:42

ipc: fix endianness issues

c97313b

Use native byte-order for IPC and program serialization. This way we will be able to support both little- and big-endian architectures. Signed-off-by: Alexander Egorenkov <Alexander.Egorenkov@ibm.com>

eaibmz force-pushed the upstream-next branch from 76a6a27 to c97313b Compare June 23, 2020 13:47

dvyukov merged commit e5d10a4 into google:master Jun 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ipc: fix endianness issues #1854

ipc: fix endianness issues #1854

eaibmz commented Jun 22, 2020 •

edited

Loading

codecov bot commented Jun 22, 2020 •

edited

Loading

dvyukov Jun 22, 2020

eaibmz Jun 22, 2020 •

edited

Loading

dvyukov Jun 22, 2020

eaibmz Jun 22, 2020

dvyukov commented Jun 22, 2020

eaibmz commented Jun 22, 2020 •

edited

Loading

eaibmz Jun 22, 2020 •

edited

Loading

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

eaibmz commented Jun 22, 2020 •

edited

Loading

dvyukov Jun 23, 2020

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

eaibmz Jun 23, 2020

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

eaibmz Jun 23, 2020

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

eaibmz Jun 23, 2020 •

edited

Loading

dvyukov Jun 23, 2020

eaibmz Jun 23, 2020

dvyukov commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

dvyukov commented Jun 23, 2020

ipc: fix endianness issues #1854

ipc: fix endianness issues #1854

Conversation

eaibmz commented Jun 22, 2020 • edited Loading

codecov bot commented Jun 22, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

eaibmz Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvyukov commented Jun 22, 2020

eaibmz commented Jun 22, 2020 • edited Loading

eaibmz Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eaibmz commented Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eaibmz Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvyukov commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 23, 2020

dvyukov commented Jun 23, 2020

dvyukov commented Jun 23, 2020

eaibmz commented Jun 22, 2020 •

edited

Loading

codecov bot commented Jun 22, 2020 •

edited

Loading

eaibmz Jun 22, 2020 •

edited

Loading

eaibmz commented Jun 22, 2020 •

edited

Loading

eaibmz Jun 22, 2020 •

edited

Loading

eaibmz commented Jun 22, 2020 •

edited

Loading

eaibmz Jun 23, 2020 •

edited

Loading