On Sun, Jun 21, 2020 at 05:42:55PM -0600, Theo de Raadt wrote:
> Paul Irofti <[email protected]> wrote:
>
> >
> >
> >
> > ??n 22 iunie 2020 01:26:16 EEST, Christian Weisgerber <[email protected]>
> > a scris:
> > >Christian Weisgerber:
> > >
> > >> I tweaked the patch locally to make _timekeep a visible global
> > >> symbol in libc.
> > >>
> > >> Printing its value has revealed two issues:
> > >>
> > >> * The timekeep page is mapped to the same address for every process.
> > >> It changes across reboots, but once running, it's always the same.
> > >> kettenis suggested
> > >> - vaddr_t va;
> > >> + vaddr_t va = 0;
> > >> in exec_timekeep_map(), but that doesn't make a difference.
> > >
> > >But that's the kernel mapping, and my observation concerns the
> > >userland mapping. So based on this, I moved ps_timekeep up into
> > >the fields of struct process that are zeroed on creation.
> > >With that, _timekeep is always 0 for all processes. :-/
> >
> >
> > I don't understand what problem you are trying to solve. Is it that
> > timekeep is the same? That's because we create only one page and the
> > address gets copied on fork. The diff was not designed to have timekeep
> > zero'd on every process so it doesn't account for it.
>
>
> And I think you aren't listening.
>
> He is saying it is at the same VA in *every* userland process. Since most
> processes do use this little system call execve, it is implausible for it
> to be at the same place, just like it is implausible for the signal tramp
> to be same place, or ld.so, or libc.
The code we are talking about is only called from inside this little
system call execve by exec_timekeep_map() and fixup().
683 /* setup new registers and do misc. setup. */
684 if (pack.ep_emul->e_fixup != NULL) {
685 if ((*pack.ep_emul->e_fixup)(p, &pack) != 0)
686 goto free_pack_abort;
687 }
...
694 /* map the process's signal trampoline code */
695 if (exec_sigcode_map(pr, pack.ep_emul))
696 goto free_pack_abort;
697 /* map the process's timekeep page */
698 if (exec_timekeep_map(pr))
699 goto free_pack_abort;
The timekeep map code is doing the same thing as the sigcode map.
880 int
881 exec_timekeep_map(struct process *pr)
882 {
883 size_t timekeep_sz = sizeof(struct timekeep);
884
885 /*
886 * Similar to the sigcode object, except that there is a single
887 * timekeep object, and not one per emulation.
888 */
889 if (timekeep_object == NULL) {
The timekeep_object is checked if allocated and if not it does a
uvm_map(kernel_map).
The timekeep_object is global so the if condition is only true once.
Then a second call to uvm_map() sends it to the process space.
Fixup is called before this, which I think is wrong now.
863 a->au_id = AUX_openbsd_timekeep;
864 a->au_v = p->p_p->ps_timekeep;
865 a++;
It should be map-fixup rather than fixup-map, right? But even reversing the
order leads to the same va address.
683 /* map the process's timekeep page */
684 if (exec_timekeep_map(pr))
685 goto free_pack_abort;
686 /* setup new registers and do misc. setup. */
687 if (pack.ep_emul->e_fixup != NULL) {
688 if ((*pack.ep_emul->e_fixup)(p, &pack) != 0)
689 goto free_pack_abort;
690 }
So I don't know why the address is not randomized, but I bet if I print
pr->ps_sigcode somehow from userland, it will be the same.
Paul
