> Date: Fri, 15 Aug 2025 15:53:25 +0200
> From: Martin Pieuchot <m...@grenadille.net>
>
> On 15/08/25(Fri) 15:45, Mark Kettenis wrote:
> > > Date: Fri, 15 Aug 2025 14:06:06 +0200
> > > From: Martin Pieuchot <m...@grenadille.net>
> > >
> > > On 15/08/25(Fri) 13:17, Mark Kettenis wrote:
> > > > > Date: Fri, 15 Aug 2025 11:51:18 +0200
> > > > > From: Martin Pieuchot <m...@grenadille.net>
> > > > >
> > > > > On 15/08/25(Fri) 09:39, Miod Vallat wrote:
> > > > > > > Please don't. Keeping that page read-only is important for
> > > > > > > security.
> > > > > > > Maybe if nobody cares about the amd64 and i386 pmaps we should
> > > > > > > just
> > > > > > > delete those architectures?
> > > > > >
> > > > > > But remember, because the end argument was wrong (sz instead of va +
> > > > > > sz), this call did *nothing*.
> > > > > >
> > > > > > At least the commented out code will be correct now.
> > > > >
> > > > > Exactly. Since you committed this code Mark it does nothing. That's
> > > > > what I said in the original report it's dead code.
> > > >
> > > > The original code (before I "broke" it) did an unmap of the memory.
> > > > BTW that means we need to fix the comment.
> > >
> > > Indeed. I'd be glad if you could do that when enable the uvm_map_protect()
> > > call.
> >
> > ok?
>
> Would you please integrate the proper refcount & error handling I fixed in
> my diff?
I'm not sure the refcount and error handling in your diff makes sense.
I'm not sure the current refcount and error handling makes sense
either.
This sigobject is a permanent uvm object that is never going to be
freed. It gets created the first time we do an exec. And if for some
reason we fail to create it you won't be able to exec anything. Not
even init(8)!
So I think it makes more sense to simply panic if that uvm_map() call
fails. Not sure what to do with the uvm_map_protect() call. That
call should never fail. And even if it does fail, the kernel will
work just fine. So I'm inclined to leave it without an error check.
At that point the reference counting should be really simple. The
uao_create() call already gives us a reference. And since we no
longer unmap the object we don't need another one. So we can actually
drop the uao_reference() call that we have right now.
I would rather make those changes in a separate commit though.
> Are we sure this won't explode on other architectures? Should that be
> tested?
We tested a few other architectures while narrowing down the breakage
to amd64 and i386. And the pmap breakage was rather specific to those
architectures. So I don't expect any fallout.
> > Index: kern/kern_exec.c
> > ===================================================================
> > RCS file: /cvs/src/sys/kern/kern_exec.c,v
> > retrieving revision 1.265
> > diff -u -p -r1.265 kern_exec.c
> > --- kern/kern_exec.c 4 Aug 2025 04:59:31 -0000 1.265
> > +++ kern/kern_exec.c 15 Aug 2025 13:43:29 -0000
> > @@ -871,8 +871,10 @@ exec_sigcode_map(struct process *pr)
> > * memory) that we keep a permanent reference to and that we map
> > * in all processes that need this sigcode. The creation is simple,
> > * we create an object, add a permanent reference to it, map it in
> > - * kernel space, copy out the sigcode to it and unmap it. Then we map
> > - * it with PROT_EXEC into the process just the way sys_mmap would map
> > it.
> > + * kernel space, copy out the sigcode to it and map it PROT_READ
> > + * such that the coredump code can write it out into core dumps.
> > + * Then we map it with PROT_EXEC into the process just the way
> > + * sys_mmap would map it.
> > */
> > if (sigobject == NULL) {
> > extern int sigfillsiz;
> > @@ -899,7 +901,7 @@ exec_sigcode_map(struct process *pr)
> > }
> > memcpy((caddr_t)va, sigcode, sz);
> >
> > - (void) uvm_map_protect(kernel_map, va, round_page(sz),
> > + (void) uvm_map_protect(kernel_map, va, round_page(va + sz),
> > PROT_READ, 0, FALSE, FALSE);
> > sigcode_va = va;
> > sigcode_sz = round_page(sz);
>
>
>
