i386/189955: makecontext(3) argument validation seems broken on i386 and powerpc

[Bruce Evans]

The following reply was made to PR i386/189955; it has been noted by GNATS.

From: Bruce Evans <brde at optusnet.com.au>
To: Peter Holm <pho at freebsd.org>
Cc: freebsd-gnats-submit at freebsd.org, freebsd-i386 at freebsd.org
Subject: Re: i386/189955: makecontext(3) argument validation seems broken on
 i386 and powerpc
Date: Tue, 20 May 2014 21:38:50 +1000 (EST)

 On Mon, 19 May 2014, Peter Holm wrote:
 
 >> Description:
 > The argc validation seems broken:
 
 Why should it do any validation at all?  Its man page documents (much more
 clearly than most man pages) that the behaviour for invalid args is
 undefined.  It can't possibly do any useful validation, since ucp points
 to mostly-opaque data.  All it can do is check that ucp is not a null pointer
 and that a bit of data pointed to it is reasonable, and that argc is
 reasonable.  It also cannot check that the extra args given by argc
 are reasonable.
 
 > $ grep -rw NCARGS /usr/include
 > /usr/include/sys/param.h:#define        NCARGS          ARG_MAX         /* max bytes for an exec function */
 
 Bogus.  makecontext() isn't an exec function.  A random integer would work
 almost as well here.  An arbitrary limit gratuitously restricts
 makecontext().  Negative and small positive values for the limit would
 just restrict it enough to break it.  It mallocs storage related to
 argc.  A large argc can cause the size calculations for the malloc() to
 fail, but that is just a special case of undefined behaviour from
 invalid args.  The behaviour is undefined unless argc matches the actual
 number of args.  argc is just a helper for the variadic args.  The limit
 on the number of args for makecontext() is equally poorly documented to
 the limit on the number of args for a C function.  The C standard requires
 some minimum limit that is too small, and IIRC it requires the actual
 limit to be documented, but documentation about this is hard to find.
 In practice, the limit is unreachable except on 16-bit systems (and
 when someone uses ulimit(1) to demonstrate brokenness).
 
 > $ egrep "argc < 0.*argc" src/lib/libc/*/gen/makecontext.c
 > src/lib/libc/amd64/gen/makecontext.c:   else if ((argc < 0) || (argc > 6) || (ucp->uc_stack.ss_sp == NULL) ||
 
 6 is an even more gratuitously restrictive limit.
 
 > src/lib/libc/i386/gen/makecontext.c:    else if ((argc < 0) || (argc > NCARGS)) {
 > src/lib/libc/ia64/gen/makecontext.c:    if (argc < 0 || argc > 8 || ucp == NULL ||
 > src/lib/libc/mips/gen/makecontext.c:    if (argc < 0 || argc > 6 || ucp == NULL ||
 > src/lib/libc/powerpc/gen/makecontext.c: if ((ucp == NULL) || (argc < 0) || (argc > NCARGS)
 > src/lib/libc/powerpc64/gen/makecontext.c:       if ((ucp == NULL) || (argc < 0) || (argc > NCARGS)
 > src/lib/libc/sparc64/gen/makecontext.c: if ((argc < 0) || (argc > 6) ||
 
 The only bugs I see here are:
 - the existence of these checks
 - the style bugs in them:
    - excessive parentheses for amd64, i386, powerpc and sparc64
    - other gratuitous differences, like having the ucp == NULL check first
      for powerpc and elsewhere for amd64, i386 and sparc64.  The complete
      file differences are even larger due to different birthplaces and not
      much convergence.
 
 Bruce