pkg/csource: Force promotion of 64-bit constant values

[markjdb]

Constant 64-bit arguments to the variadic syscall(2) must have their
width specified explicitly. In practice this is not necessary most of
the time, but on amd64/freebsd with clang the compiler can and does
store the constant 32-bit value to the stack, leaving garbage in the
upper 32 bits.

This makes C reproducers somewhat uglier, but I see no other solution.

---

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

---

[codecov-io]

Codecov Report

Merging #1493 into master will decrease coverage by <.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1493      +/-   ##
==========================================
- Coverage   56.19%   56.19%   -0.01%     
==========================================
  Files         140      140              
  Lines       26409    26414       +5     
==========================================
+ Hits        14841    14843       +2     
- Misses      10863    10864       +1     
- Partials      705      707       +2

Impacted Files	Coverage Δ
pkg/csource/csource.go	74.71% <66.66%> (-0.51%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f39aff9...0d54bd6. Read the comment docs.

[dvyukov]

Interesting. I tested clang/gcc -O0-/-02. clang with -O0 uses movl to move to stack, the rest use pushq.
I wonder if we consistently use size == 8. E.g. I see all fd's are 4. What's the convention? Shouldn't we cast all of them.
Also won't we over-cast some values that are incorrectly marked as size=8 to ull on 32-bit arch?...
Let's also check native condition in emitCall to do this. We don't use syscall on all arches. Even on linux/freebsd we don't always use it.

[markjdb]

Right. So C reproducers work fine if they are compiled with gcc. :)

I am really assuming that the data model is one of ILP32 or LP64. These are the only two used on FreeBSD, but that may well not be true across all platforms supported by syzkaller.

Arguments of size 4 pose no problem, the compiler handles them as we want. Yes, this change will cause problems if the OS expects a 4-byte argument and we widen it to 8. Do you know of any examples where this would happen? I note that fileoff is 4 bytes on freebsd/386, which is incorrect, but that's the opposite problem.

I will add a check for the use of syscall().

[dvyukov]

Arguments of size 4 pose no problem, the compiler handles them as we want. Yes, this change will cause problems if the OS expects a 4-byte argument and we widen it to 8. Do you know of any examples where this would happen?

I mainly don't understand what OSes expect.
But the change will affect lots of arguments. Most syscall arguments get the default base type - intptr (e.g. any const[X], or flags[foo]). Currently if the value is <2^32, we pass it as int. With this change we start passing them as long. Generally, we did not pay lots of attention to base type of syscall args.
However, afaiu on linux syscalls don't have more than 6 args (?), so all are passed in regs, and this change effectively becomes no-op. But I afraid that we (at least me) don't really understand what happens here.

[dvyukov]

Yes, on linux we currently only have pseudo syscalls with >6 args: syz_kvm_setup_cpu and syz_mount_image. But for these native=false and we should not add suffixes at all.

[markjdb]

Indeed, I believe you are right about the limit on syscall arguments on Linux. FreeBSD has a larger limit, and syscall() is itself a system call there.

On amd64 I believe the change will have no effect for the first 6 arguments of syscall(), but note that this is not the same as the first 6 syscall arguments, since syscall() takes an extra argument. Beyond the first 6 syscall() arguments, the change will cause undesired behaviour if syzkaller specifies an 8 byte type but the callee expects only 4 bytes on the stack. I can't think of any place in syzkaller+FreeBSD where that will happen, but as you noted syzkaller's system call argument representations already do not match the OS ABIs, e.g., flags arguments have size 8 but in most cases they are 4 bytes in the ABI. So it is probably just a matter of time.

So I can see three approaches:

1. Commit the change and hope that it does not break anything.
2. Introduce explicit-width types, e.g., flags32/flags64, and go through and convert syscall descriptions so that syzkaller's notion of argument sizes always matches the OS ABI. This will fix some existing problems, like the one I noted above. (off_t is always 8 bytes on FreeBSD, but syskaller's fileoff is 4 bytes wide on freebsd/386.)
3. Hard-code handling for known problematic cases, like the "offset" argument to mmap on freebsd/amd64.

I can try and implement 3), as it's not clear to me that the mismatches between syzkaller and OS ABIs are causing enough pain to warrant 2).

[dvyukov]

arch_prctl$ARCH_SET_GS and CDROM ioctls need to be changed to use intptr I think.
The lookup_dcookie thing is more interesting:

SYSCALL_DEFINE3(lookup_dcookie, u64, cookie64, char __user *, buf, size_t, len)
{
	return do_lookup_dcookie(cookie64, buf, len);
}

#ifdef CONFIG_COMPAT
COMPAT_SYSCALL_DEFINE4(lookup_dcookie, u32, w0, u32, w1, char __user *, buf, compat_size_t, len)
{
#ifdef __BIG_ENDIAN
	return do_lookup_dcookie(((u64)w0 << 32) | w1, buf, len);
#else
	return do_lookup_dcookie(((u64)w1 << 32) | w0, buf, len);
#endif
}
#endif

I will take a closer look tomorrow if it's equivalent.

[dvyukov]

If ctx.target.PtrSize == 8, shouldn't we always add ul?
If argument slots are 8 bytes, there is little point in looking at the actual argument size and leaving garbage in high 4 bytes if we think the argument is int32.
If we always add ul, we just clear the unused part (or maybe used part).

[markjdb]

I believe it is fine to do so on amd64. On other 64-bit architectures, I'm not sure, but I would be surprised if it breaks anything.

[markjdb]

... that said, I don't see why we should append ul for arguments like file descriptors, which are 4 bytes wide according to both syzkaller and common OS ABIs. It would look strange and increase the amount of clutter in the C reproducers.

[dvyukov]

If you can reliably distinguish between these and the ones where we have a wrong type, then, yes, there is no point ;)
But we never tried to ensure that sizes of syscalls arguments are correct. My view on this was always "whatever, it does not matter, it will pass arguments correctly regardless of size". I don't know if we have wrong types, how many are there, etc...
So the idea is to actually implement that "it will pass arguments correctly regardless of size" assumption that was always there.

[dvyukov]

Good point. So this does not affect 64 bits at all. But this will affect the following arguments on 32-bits:

        prog_test.go:434: linux/386: arch_prctl$ARCH_SET_GS(arg)
        prog_test.go:434: linux/386: ioctl$CDROM_MEDIA_CHANGED(slot)
        prog_test.go:434: linux/386: ioctl$CDROM_SELECT_DISK(disk)
        prog_test.go:434: linux/386: ioctl$CDROM_SELECT_SPEED(speed)
        prog_test.go:434: linux/386: lookup_dcookie(cookie)
        prog_test.go:434: linux/arm: ioctl$CDROM_MEDIA_CHANGED(slot)
        prog_test.go:434: linux/arm: ioctl$CDROM_SELECT_DISK(disk)
        prog_test.go:434: linux/arm: ioctl$CDROM_SELECT_SPEED(speed)
        prog_test.go:434: linux/arm: lookup_dcookie(cookie)
        prog_test.go:434: freebsd/386: mknodat(dev)
        prog_test.go:434: freebsd/386: sendfile(nbytes)

Checking if it's fine.

Either way, taking into account amount of discussion here, I think we need to at least extend the comment to capture main points mentioned in this discussion.