corbinlc
commented
2 years ago I wonder if the SIGSYS needs to be intercepted and then PR_clone used instead. I have a feeling that PR_fork is on its way out.
Closed corbinlc closed 1 year ago
corbinlc
commented
2 years ago I wonder if the SIGSYS needs to be intercepted and then PR_clone used instead. I have a feeling that PR_fork is on its way out.
corbinlc
commented
2 years ago This is now some glibc implementations of fork are handled. With the last 5 arguments listed here, clone will act like fork.
INLINE_SYSCALL (clone, 5, \
CLONE_CHILD_SETTID | CLONE_CHILD_CLEARTID | SIGCHLD, 0, \
NULL, NULL, &THREAD_SELF->tid)```
michalbednarski
commented
2 years ago Seems that you're right.
Something to test that under Termux would be nice (so far most programs didn't use fork syscall and instead used clone; vfork also seems to be allowed (and also vfork is another clone with preset arguments))
Note that fork sets flags to just SIGCHLD
clone has different argument orders on different architectures, but it seems to not be problem in this case as across architectures supported by proot only last two arguments may be swapped and we want set both to zero anyway (You can find architectures which swap arguments by running grep -r 'select CLONE_BACKWARDS' arch in Linux source tree)
So looks like answer would be (I haven't tested this code) :
case PR_fork:
set_sysnum(tracee, PR_clone);
poke_reg(tracee, SYSARG_1, SIGCHLD);
poke_reg(tracee, SYSARG_2, 0);
poke_reg(tracee, SYSARG_3, 0);
poke_reg(tracee, SYSARG_4, 0);
poke_reg(tracee, SYSARG_5, 0);
restart_syscall_after_seccomp(tracee);
break;Thanks. That works well. I came to the same conclusion that you don't need to worry about the arch dependent swapping of variables and that you can set args 2-5 to 0, but I am glad you have the same thoughts. It is interesting that the code in glibc is more complex and set additional arguments when fork is called. The trickier one is alarm is also not allowed anymore. This will probably require setitimer. That is a little more complex as the result at sysexit will have to be modified. This is what glibc does:
unsigned int
alarm (unsigned int seconds)
{
struct itimerval old, new;
unsigned int retval;
new.it_interval.tv_usec = 0;
new.it_interval.tv_sec = 0;
new.it_value.tv_usec = 0;
new.it_value.tv_sec = (long int) seconds;
if (__setitimer (ITIMER_REAL, &new, &old) < 0)
return 0;
retval = old.it_value.tv_sec;
/* Round to the nearest second, but never report zero seconds when the alarm is still set. */
if (old.it_value.tv_usec >= 500000
(retval == 0 && old.it_value.tv_usec > 0))
++retval;
return retval;
}So a solution for this will have to allocate memory for the struct being used and intercept at sysexit as well.
corbinlc
commented
1 year ago This is what I put in seccomp.c:
case PR_alarm:
{
unsigned int seconds;
struct itimerval old, new;
word_t old_arg = 0;
word_t new_arg = 0;
seconds = peek_reg(tracee, CURRENT, SYSARG_1);
old.it_interval.tv_usec = 0;
old.it_interval.tv_sec = 0;
old.it_value.tv_usec = 0;
old.it_value.tv_sec = 0;
old_arg = alloc_mem(tracee, sizeof(old));
if(write_data(tracee, old_arg, &old, sizeof(old))) {
set_result_after_seccomp(tracee, -EFAULT);
break;
}
new.it_interval.tv_usec = 0;
new.it_interval.tv_sec = 0;
new.it_value.tv_usec = 0;
new.it_value.tv_sec = (long int) seconds;
new_arg = alloc_mem(tracee, sizeof(new));
if(write_data(tracee, new_arg, &new, sizeof(new))) {
set_result_after_seccomp(tracee, -EFAULT);
break;
}
set_sysnum(tracee, PR_setitimer);
poke_reg(tracee, SYSARG_1, ITIMER_REAL);
poke_reg(tracee, SYSARG_2, new_arg);
poke_reg(tracee, SYSARG_3, old_arg);
restart_syscall_after_seccomp(tracee);
break;
}@michalbednarski Where is the best place to handled modifying the return value for things restarted by seccomp.c?
michalbednarski
commented
1 year ago I'm afraid there is no dedicated place for that, so you'd have to put it in translate_syscall_exit (Note that switch (syscall_number) is on rewritten syscall, so add case PR_setitimer: and inside that add condition on e.g. tracee->restore_original_regs_after_seccomp_event and/or custom field in typedef struct { ... } Tracee)
Another issue here is that struct itimerval varies between 32 and 64-bit
corbinlc
commented
1 year ago @michalbednarski
Does this look like a good first attempt for the translate_syscall_exit modification?
case PR_setitimer: {
struct itimerval old;
if (!tracee->restore_original_regs_after_seccomp_event)
goto end;
if (syscall_result < 0) {
status = 0;
break;
}
status = read_data(tracee, &old, peek_reg(tracee, ORIGINAL, SYSARG_3), sizeof(old));
if (status < 0)
break;
status = old.it_value.tv_sec;
/* Round to the nearest second, but never report zero seconds when the alarm is still set. */
if (old.it_value.tv_usec >= 500000 ||
(status == 0 && old.it_value.tv_usec > 0))
++status;
break;
}Note, status gets written into the SYSARG_RESULT at the end of the case statement.
Also, what is your concern about the different between 32 and 64 bit usage of the struct itimerval? I didn't see any issue after look at the definition.
Thanks.
michalbednarski
commented
1 year ago In my test this worked okay (except 32-on-64 mode, I'm including patched version below)
Differences between 32 and 64 bit itimerval, as gathered by using gdb scripting on AOSP build
$ gdb --batch -x itimerval_info.py out/target/product/emulator_x86_64/symbols/apex/com.android.runtime/lib/bionic/libc.so
struct itimerval { // sizeof=16
timeval it_interval; // pos=0.0 size=8
timeval it_value; // pos=8.0 size=8
}
struct timeval { // sizeof=8
__kernel_old_time_t tv_sec; // pos=0.0 size=4
__kernel_suseconds_t tv_usec; // pos=4.0 size=4
}
$ gdb --batch -x itimerval_info.py out/target/product/emulator_x86_64/symbols/apex/com.android.runtime/lib64/bionic/libc.so
struct itimerval { // sizeof=32
timeval it_interval; // pos=0.0 size=16
timeval it_value; // pos=16.0 size=16
}
struct timeval { // sizeof=16
__kernel_old_time_t tv_sec; // pos=0.0 size=8
__kernel_suseconds_t tv_usec; // pos=8.0 size=8
}
$ cat itimerval_info.py
def print_type_info(name):
typ = gdb.lookup_type(name)
print(name + " { // sizeof=" + str(typ.sizeof))
for field in typ.fields():
print(" " + field.type.name + " " + field.name + "; // pos=" + str(field.bitpos/8) + " size=" + str(field.type.sizeof))
print("}")
print_type_info("struct itimerval")
print_type_info("struct timeval")Version supporting 32-on-64 (mostly based on your patches):
diff --git a/src/syscall/exit.c b/src/syscall/exit.c
index b3c88b2..d487332 100644
--- a/src/syscall/exit.c
+++ b/src/syscall/exit.c
@@ -536,6 +536,36 @@ void translate_syscall_exit(Tracee *tracee)
status = handle_statx_syscall(tracee, false);
break;
+ case PR_setitimer: {
+ struct itimerval old;
+
+ if (!tracee->restore_original_regs_after_seccomp_event)
+ goto end;
+
+ if (syscall_result < 0) {
+ status = 0;
+ break;
+ }
+
+ status = read_data(tracee, &old, peek_reg(tracee, ORIGINAL, SYSARG_3), sizeof(old));
+ if (status < 0)
+ break;
+
+ if (is_32on64_mode(tracee)) {
+ uint32_t sec = ((uint32_t*) &old)[2];
+ uint32_t usec = ((uint32_t*) &old)[3];
+ old.it_value.tv_sec = sec;
+ old.it_value.tv_usec = usec;
+ }
+
+ status = old.it_value.tv_sec;
+ /* Round to the nearest second, but never report zero seconds when the alarm is still set. */
+ if (old.it_value.tv_usec >= 500000 ||
+ (status == 0 && old.it_value.tv_usec > 0))
+ ++status;
+ break;
+ }
+
default:
goto end;
}
diff --git a/src/tracee/seccomp.c b/src/tracee/seccomp.c
index 715cfe6..6c758c5 100644
--- a/src/tracee/seccomp.c
+++ b/src/tracee/seccomp.c
@@ -408,6 +408,60 @@ static int handle_seccomp_event_common(Tracee *tracee)
restart_syscall_after_seccomp(tracee);
break;
+ case PR_fork:
+ set_sysnum(tracee, PR_clone);
+ poke_reg(tracee, SYSARG_1, SIGCHLD);
+ poke_reg(tracee, SYSARG_2, 0);
+ poke_reg(tracee, SYSARG_3, 0);
+ poke_reg(tracee, SYSARG_4, 0);
+ poke_reg(tracee, SYSARG_5, 0);
+ restart_syscall_after_seccomp(tracee);
+ break;
+
+ case PR_alarm:
+ {
+ unsigned int seconds;
+ struct itimerval old, new;
+ word_t old_arg = 0;
+ word_t new_arg = 0;
+
+ seconds = peek_reg(tracee, CURRENT, SYSARG_1);
+
+ old.it_interval.tv_usec = 0;
+ old.it_interval.tv_sec = 0;
+ old.it_value.tv_usec = 0;
+ old.it_value.tv_sec = 0;
+
+ old_arg = alloc_mem(tracee, sizeof(old));
+ if(write_data(tracee, old_arg, &old, sizeof(old))) {
+ set_result_after_seccomp(tracee, -EFAULT);
+ break;
+ }
+
+ new.it_interval.tv_usec = 0;
+ new.it_interval.tv_sec = 0;
+ new.it_value.tv_usec = 0;
+ new.it_value.tv_sec = (long int) seconds;
+
+ if (is_32on64_mode(tracee)) {
+ ((uint32_t*) &new)[2] = seconds;
+ new.it_value.tv_sec = 0;
+ }
+
+ new_arg = alloc_mem(tracee, sizeof(new));
+ if(write_data(tracee, new_arg, &new, sizeof(new))) {
+ set_result_after_seccomp(tracee, -EFAULT);
+ break;
+ }
+
+ set_sysnum(tracee, PR_setitimer);
+ poke_reg(tracee, SYSARG_1, ITIMER_REAL);
+ poke_reg(tracee, SYSARG_2, new_arg);
+ poke_reg(tracee, SYSARG_3, old_arg);
+ restart_syscall_after_seccomp(tracee);
+ break;
+ }
+
case PR_access:
set_sysnum(tracee, PR_faccessat);
poke_reg(tracee, SYSARG_4, 0);(In theory my fix violates strict aliasing, but currently appears to work)
corbinlc
commented
1 year ago This all seems to work. I see one more issue, but I don't think it is related.
Btw... I didn't see an easy way to try this with Termux (had to keep using UserLAnd for testing) because F-droid is not available for Chromebooks. I probably could side load it, but haven't tried working around it yet.
corbinlc
commented
1 year ago I found no more issues with this.
It looks like running PRoot on HP Chromebooks is running into a unique problem. Specifically, it is getting an unrecoverable SIGSYS after calling the
forksystem call. The devices effected are x86_64 devices. Note, this is occurring in UserLAnd, but should equally effect Termux. I will see if I can repeat it there later. @michalbednarski do you have any thoughts on this?Here is a short PRoot log. The only interesting part is right at the end: