svn commit: r354741 - in head/sys: amd64/amd64 arm/arm arm64/arm64 compat/freebsd32 compat/ia32 i386/i386 kern mips/mips powerpc/powerpc riscv/riscv sparc64/sparc64 sys

[Konstantin Belousov]

On Fri, Nov 15, 2019 at 01:47:24PM -0800, John Baldwin wrote:
> On 11/15/19 12:01 PM, Konstantin Belousov wrote:
> > On Fri, Nov 15, 2019 at 06:42:13PM +0000, John Baldwin wrote:
> >> Author: jhb
> >> Date: Fri Nov 15 18:42:13 2019
> >> New Revision: 354741
> >> URL: https://svnweb.freebsd.org/changeset/base/354741
> >>
> >> Log:
> >>   Add a sv_copyout_auxargs() hook in sysentvec.
> >>   
> >>   Change the FreeBSD ELF ABIs to use this new hook to copyout ELF auxv
> >>   instead of doing it in the sv_fixup hook.  In particular, this new
> >>   hook allows the stack space to be allocated at the same time the auxv
> >>   values are copied out to userland.  This allows us to avoid wasting
> >>   space for unused auxv entries as well as not having to recalculate
> >>   where the auxv vector is by walking back up over the argv and
> >>   environment vectors.
> >>   
> >>   Reviewed by:	brooks, emaste
> >>   Tested on:	amd64 (amd64 and i386 binaries), i386, mips, mips64
> >>   Sponsored by:	DARPA
> >>   Differential Revision:	https://reviews.freebsd.org/D22355
> >>
> >> @@ -1376,11 +1373,18 @@ __elfN(freebsd_fixup)(register_t **stack_base, struct 
> >>  	imgp->auxargs = NULL;
> >>  	KASSERT(pos - argarray <= AT_COUNT, ("Too many auxargs"));
> >>  
> >> -	error = copyout(argarray, auxbase, sizeof(*argarray) * AT_COUNT);
> >> +	auxlen = sizeof(*argarray) * (pos - argarray);
> >> +	*base -= auxlen;
> >> +	copyout(argarray, (void *)*base, auxlen);
> >>  	free(argarray, M_TEMP);
> >> -	if (error != 0)
> >> -		return (error);
> >> +}
> > So you are ignoring copyout errors ?
> 
> All of exec_copyout_strings() ignores copyout errors.  Previously we would ignore
> errors for the argv and envv arrays and strings, but just happened to check for
> the error during fixup (but a.out binaries don't do a fixup, so none of their
> copyout calls are checked).  We could change sv_copyout_strings to return an error
> value and return the stack pointer via a pointer arg instead and then start
> checking for errors in all the copyout_strings implementations if we wanted to
> start doing checks.
> 
We should start checking for errors there, sometime. One of the reason
is that user can cause situation where the copyouts fail due to the
(mis)configured stack limit and stack gap size.