Page MenuHomeFreeBSD

truss: improved support for decoding compat32 arguments
ClosedPublic

Authored by arichardson on Dec 15 2020, 6:59 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 5:47 PM
Unknown Object (File)
Fri, Mar 22, 5:47 PM
Unknown Object (File)
Fri, Mar 22, 5:47 PM
Unknown Object (File)
Fri, Mar 22, 5:47 PM
Unknown Object (File)
Fri, Mar 22, 5:46 PM
Unknown Object (File)
Fri, Mar 22, 5:46 PM
Unknown Object (File)
Tue, Mar 19, 2:04 AM
Unknown Object (File)
Thu, Feb 29, 11:54 PM
Subscribers

Details

Summary

Currently running truss -a -e does not decode any
argument values for freebsd32_* syscalls (open/readlink/etc.)

This change checks whether a syscall starts with freebsd{32,64}_ and if
so strips that prefix when looking up the syscall information. To ensure
that the truss logs include the real syscall name we create a copy of
the syscall information struct with the updated.

The other problem is that when reading string array values, truss
naively iterates over an array of char* and fetches the pointer value.
This will result in arguments not being loaded if the pointer is not
aligned to sizeof(void*), which can happens in the compat32 case. If it
happens to be aligned, we would end up printing every other value.
To fix this problem, this changes adds a pointer_size member to the
procabi struct and uses that to correctly read indirect arguments
as 64/32 bit addresses in the the compat32 case (and also compat64 on
CheriBSD).

The motivating use-case for this change is using truss for 64-bit
programs on a CHERI system, but most of the diff also applies to 32-bit
compat on a 64-bit system, so I'm upstreaming this instead of keeping it
as a local CheriBSD patch.

Output of truss -aef ldd32 /usr/bin/ldd32 before:
39113: freebsd32_mmap(0x0,0x1000,0x3,0x1002,0xffffffff,0x0,0x0) = 543440896 (0x20644000)
39113: freebsd32_ioctl(0x1,0x402c7413,0xffffd2a0) = 0 (0x0)
/usr/bin/ldd32:
39113: write(1,"/usr/bin/ldd32:\n",16) = 16 (0x10)
39113: fork() = 39114 (0x98ca)
39114: <new process>
39114: freebsd32_execve(0xffffd97e,0xffffd680,0x20634000) EJUSTRETURN
39114: freebsd32_mmap(0x0,0x20000,0x3,0x1002,0xffffffff,0x0,0x0) = 541237248 (0x2042a000)
39114: freebsd32_mprotect(0x20427000,0x1000,0x1) = 0 (0x0)
39114: issetugid() = 0 (0x0)
39114: openat(AT_FDCWD,"/etc/libmap32.conf",O_RDONLY|O_CLOEXEC,00) ERR#2 'No such file or directory'
39114: openat(AT_FDCWD,"/var/run/ld-elf32.so.hints",O_RDONLY|O_CLOEXEC,00) = 3 (0x3)
39114: read(3,"Ehnt\^A\0\0\0\M^@\0\0\0#\0\0\0\0"...,128) = 128 (0x80)
39114: freebsd32_fstat(0x3,0xffffbd98) = 0 (0x0)
39114: freebsd32_pread(0x3,0x2042f000,0x23,0x80,0x0) = 35 (0x23)
39114: close(3) = 0 (0x0)
39114: openat(AT_FDCWD,"/usr/lib32/libc.so.7",O_RDONLY|O_CLOEXEC|O_VERIFY,00) = 3 (0x3)
39114: freebsd32_fstat(0x3,0xffffc7d0) = 0 (0x0)
39114: freebsd32_mmap(0x0,0x1000,0x1,0x40002,0x3,0x0,0x0) = 541368320 (0x2044a000)

After:

783: freebsd32_mmap(0x0,4096,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANON|MAP_ALIGNED(12),-1,0x0) = 543543296 (0x2065d000)
783: freebsd32_ioctl(1,TIOCGETA,0xffffd7b0)    = 0 (0x0)

/usr/bin/ldd32:

783: write(1,"/usr/bin/ldd32:\n",16)           = 16 (0x10)
784: <new process>
783: fork()                                    = 784 (0x310)
784: freebsd32_execve("/usr/bin/ldd32",[ "(null)" ],[ "LD_32_TRACE_LOADED_OBJECTS_PROGNAME=/usr/bin/ldd32", "LD_TRACE_LOADED_OBJECTS_PROGNAME=/usr/bin/ldd32", "LD_32_TRACE_LOADED_OBJECTS=yes", "LD_TRACE_LOADED_OBJECTS=yes", "USER=root", "LOGNAME=root", "HOME=/root", "SHELL=/bin/csh", "BLOCKSIZE=K", "MAIL=/var/mail/root", "MM_CHARSET=UTF-8", "LANG=C.UTF-8", "PATH=/sbin:/bin:/usr/sbin:/usr/bin:/usr/local/sbin:/usr/local/bin:/root/bin", "TERM=vt100", "HOSTTYPE=FreeBSD", "VENDOR=amd", "OSTYPE=FreeBSD", "MACHTYPE=x86_64", "SHLVL=1", "PWD=/root", "GROUP=wheel", "HOST=freebsd-amd64", "EDITOR=vi", "PAGER=less" ]) EJUSTRETURN
784: freebsd32_mmap(0x0,135168,PROT_READ|PROT_WRITE,MAP_PRIVATE|MAP_ANON,-1,0x0) = 541212672 (0x20424000)
784: freebsd32_mprotect(0x20421000,4096,PROT_READ) = 0 (0x0)
784: issetugid()                               = 0 (0x0)
784: sigfastblock(0x1,0x204234fc)              = 0 (0x0)
784: open("/etc/libmap32.conf",O_RDONLY|O_CLOEXEC,00) ERR#2 'No such file or directory'
784: open("/var/run/ld-elf32.so.hints",O_RDONLY|O_CLOEXEC,00) = 3 (0x3)
784: read(3,"Ehnt\^A\0\0\0\M^@\0\0\0\v\0\0\0"...,128) = 128 (0x80)
784: freebsd32_fstat(3,{ mode=-r--r--r-- ,inode=18680,size=32768,blksize=0 }) = 0 (0x0)
784: freebsd32_pread(3,"/usr/lib32\0",11,0x80) = 11 (0xb)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 35461
Build 32371: arc lint + arc unit

Event Timeline

usr.bin/truss/setup.c
95

This works just fine as void * even downstream in CheriBSD.

117

Meh, should be void * upstream and we can make it be Linux64 downstream.

135–142

Is more like we have downstream. It decouples the native ABI from what other ABIs you might know about so is easier to work out what it's doing at a glance about and easier to extend (otherwise you get a bunch of duplication once you add a third possible native ABI).

usr.bin/truss/syscalls.c
1061–1071

I still don't like this messing with stailq's and would be fine printing the non-prefixed syscall name (and we still have the ABI floating around if we wanted to reconstruct the prefixed name later).

1064–1065

freebsd64 should stay downstream

1820–1822

and keep strarray64 (which only makes sense alongside the __capability annotation for strarray) downstream

usr.bin/truss/syscalls.c
1061–1071

I agree this is rather ugly. Let me have another look to see if I can simplify this while keeping the original names.

1064–1065

yeah will remove that before commiting. Missed it while cleaning up CHERI stuff.

1820–1822

I prefer int64_t here because a) it makes the diff to cheribsd a bit smaller and b) we only ever care about the address so using a pointer could suggest that it's dereferenceable.

usr.bin/truss/setup.c
95

Well, sizeof(void* __capability) in order to support a hybrid truss, but I guess that's simpler.

usr.bin/truss/setup.c
95

Sure, though your proposed patch didn't actually do that :)

usr.bin/truss/syscalls.c
1820–1822

uintptr_t strarray then? (Which can be uintcap_t for us instead)

usr.bin/truss/setup.c
95

It did (https://github.com/CTSRD-CHERI/cheribsd/pull/692/files), not sure why I changed it here. I think to match the linux case...

usr.bin/truss/syscalls.c
1820–1822

That would work although I think it the code below a bit less readable.
It would then need to be something like

			if (pointer_size == sizeof(u.strarray[0])) {
				if (u.strarray[i] == 0)
					break;
				straddr = u.strarray[i];
			} else if (pointer_size == 4) {
				if (u.strarray32[i] == 0)
					break;
				/* sign-extend 32-bit pointers */
				straddr = (intptr_t)u.strarray32[i];
			}

I think I prefer the explicit size version.

usr.bin/truss/syscalls.c
1820–1822

What's wrong with that? Though I'd make it more like the kernel COMPAT_FREEBSD32 code and add #ifs like:

#if __SIZEOF_POINTER__ > 4
			if (pointer_size == 4) {
				if (u.strarray32[i] == 0)
					break;
				/* sign-extend 32-bit pointers */
				straddr = (intptr_t)u.strarray32[i];
			} else
#endif
			{
				if (u.strarray[i] == 0)
					break;
				straddr = u.strarray[i];
			}

possibly with a pointer_size check and errx in the else branch if you want.

arichardson marked 2 inline comments as done.
  • Rebase on top of D27636
  • fix ubsan array bounds error

Hm. Perhaps this should be word_size (or similar) and __SIZEOF_LONG__ rather than pointer_size and __SIZEOF_POINTER__ so there's less churn for CHERI?

I had long-standing todo list to add freebsd32, but I was going to do it manually. This is much nicer. I have a test program I use that exercises most syscalls (or least, different argument values to exercise decoding) that I typically run under truss and kdump when making changes and then compare the before after with kompare to ensure the changes are all desired changes. If you have a git branch these changes sit in I can run that and see how it compares before/after for freebsd32.

usr.bin/truss/syscalls.c
1026

Yes, it's needed for all 32-bit FreeBSD ABIs. One way to handle this might be to have a function pointer in 'abi' for "syscallfixup" and use quad_fixup as that for the FreeBSD ABI (on ILP32) and the FreeBSD32 ABIs (on LP64). This would also permitting adding different behavior if needed for Linux32 in the future.

1070

Rather than hardcoding this here, let's add a new member to abi that is something like 'compat_prefix'. You would define it to "freebsd32_" over in main.c for the FreeBSD 32 compat ABIs. Then this code would be something like:

lookup_name = name;
if (strncmp(abi->compat_prefix, name, strlen(compat_prefix)) == 0)
    lookup_name += strlen(compat_prefix);

This would let you handle freebsd64_ easily in CheriBSD by just adding it to the FreeBSD64 ABI over in main.c. It would also permit handling Linux32 if someone really wanted to. It also avoids potential false positive matches.

1863

Is what style(9) prefers for infinite loops.

arichardson marked 6 inline comments as done.
  • Rebase and add the compat_prefix member
In D27625#637995, @jhb wrote:

I had long-standing todo list to add freebsd32, but I was going to do it manually. This is much nicer. I have a test program I use that exercises most syscalls (or least, different argument values to exercise decoding) that I typically run under truss and kdump when making changes and then compare the before after with kompare to ensure the changes are all desired changes. If you have a git branch these changes sit in I can run that and see how it compares before/after for freebsd32.

I've pushed the changes to https://github.com/arichardson/freebsd/tree/truss-cleanups

usr.bin/truss/syscalls.c
1026

I had a quick look at adding the function pointer, since that seems like the cleaner solution. However, that makes the changes more invasive since I need to move lots of stuff around. I think it makes more sense as a follow-up commit especially if all 32-bit ABIs (including Linux32) need it.

arichardson marked an inline comment as done.

rebase

It's fine with me modulo the nits.

usr.bin/truss/syscalls.c
1060

This seems to be a duplicate line now?

1071

It's too bad there's no 'strstartswith()' or the like.

This revision is now accepted and ready to land.Mar 22 2021, 7:13 PM
This revision was automatically updated to reflect the committed changes.
arichardson marked 2 inline comments as done.