Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

In D3565#74326, @kib wrote:

Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

Hey Kostik,

Remember that we're not actually applying ASLR in this patch as this is simply a prerequisite patch, so the values copied from the sysentvec will remain constant, as you pointed out above.

You bring up some technical points that I do need to research further. Before making any more changes or comments, I'm going to wait for Robert Watson's review of this patch.

Thank you for your constructive and productive comments. I look forward to working with you in a positive manner as this patch progresses.

Thanks,

Shawn

In D3565#74326, @kib wrote:

Anything like this patch can only be considered if your 'aslr' implementation is ever found acceptable. Until it is not, the patch only adds bloat.

Yes, I'm agreed with this.

That said, and being whatever trivial, the patch has technical issues. The first and most obvious one is the fact that struct proc is the wrong location for the data you put into it. I argue that the locations of objects in the user address space is the property of the vmspace and not the process.

Second issue is less trivial. Right now, the data which you moved is located in the static (as in, not changing) struct sysentvec. In other words, after you have dereferenced sv = p->p_sysent, you know that you have a pointer to the stable and _consistent_ set of values like address of psstrings, address of signal trampoline, base of the shared page etc. With your patch, this property is lost. Now, you must guarantee that the operations which modify the user address space layout, like execve(), do not happen in parallel. I did not inspected your patch closely for the issue, but neither did you.

And, of course, the patch changes the ABI by making e.g. psstrings/sigtrampoline process-private, instead of all ABI-sharing processes sharing the psstrings/sigtramp value. I am certain that this e.g. breaks core dump analysis due to signal trampoline address change.

The signal handling and debugging is fully working after this patch, and even working, when you applied our whole patch with full randomization.

See this small test case:

op@opn /tmp> sysctl hardening.pax.aslr
hardening.pax.aslr.vdso_len: 28
hardening.pax.aslr.exec_len: 30
hardening.pax.aslr.stack_len: 42
hardening.pax.aslr.mmap_len: 30
hardening.pax.aslr.status: 2
op@opn /tmp> cat test.c
#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>
#include <signal.h>

void *
mthr(void *arg)
{
        raise(4);

        return (NULL);
}

int
main(int argc, char **argv)
{
        pthread_t       thread_0;

        pthread_create(&thread_0, NULL, mthr, NULL);

        printf(".\n");

        pthread_join(thread_0, NULL);

        return (0);
}
op@opn /tmp> rm test
op@opn /tmp> make test
cc -O2 -pipe -pipe   -DHARDENEDBSD -lpthread -ggdb  test.c  -o test
op@opn /tmp> gdb ./test 
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "amd64-marcel-freebsd"...
(gdb) r
Starting program: /tmp/test 
[New LWP 100548]
[New Thread 14385215400 (LWP 100551/test)]

Program received signal SIGILL, Illegal instruction.
[Switching to Thread 14385215400 (LWP 100551/test)]
0x0000014384c2bcba in thr_kill () from /lib/libc.so.7
(gdb) thread signal
signal mask:

signal pending:

si_signo 4 si_errno 0
si_code UNKNOWN (65543) si_pid 8375 si_uid 1001 si_status 0 si_addr 0x0
(gdb)

Looking at the members of struct proc and struct vmspace I do think I agree that vmspace is a better place for these things.

I'll or someone else on our team be exploring what happens to coredumps with an alternative layout of sigcode and ps_strings on CHERI so hopefully we'll have some useful insights there.

One thing I have been wondering is if it makes sense to add a syscall to retrieve a ps_strings pointer rather than relying on constructing a pointer from a long from sysctl.