Looks like split of execve(2) syscall created bugs

[Maxim Sobolev]

Matthew Dillon wrote:
:Hi DF'ers!
:
:I am working on eliminating stackgap in FreeBSD's linuxlator following 
:DF's footsteps and fond that there is potential bug has been introduced 
:into execve(). The problem is that DF now first checks if argv[0] is 
:NULL, then replaces it with pathname and then proceeds with scanning 
:other arguments instead of stopping there. According to the comment in 
:the code, such behaviour has been introduced to make shell scripts 
:working. However there are two problems with this approach:
:
:1. According to the POSIX, execve() should pass arguments list 
:unmodified to the newly created process. This means that if I invoke 
:execve with argv[0] being NULL, the new image should see argc == 0 and 
:argv[0] = NULL. DF in this case will copy the new image path as argv[0] 
:and new image will see see it as argv[0]/argc == 1.
:
:2. In some cases, the new logic will result in bogus arguments passed to 
:the new image or EFAULT when first argument is NULL. This will happen 
:due to the bug in routine which copies arguments from the user space 
:into the kernel space. It assumes that both argv[0] and argv[1] are 
:NULL, while only former is required to be to stop processing.
:
:The proper fix is to move special handling of argv[0] == NULL case into 
:imgact_shell.c where it belongs.
:
:-Maxim

    It's unclear whether passing a NULL argv[0] for any case is legal.
    I think to be strictly conforming, argv[0] must always be non-NULL.
Latest POSIX explicitly mentions that NULL argv[0] is permitted, 
although discouraged for compatibility reasons:

----
RATIONALE
Early proposals required that the value of argc passed to main() be "one 
or greater". This was driven by the same requirement in drafts of the 
ISO C standard. In fact, historical implementations have passed a value 
of zero when no arguments are supplied to the caller of the exec 
functions. This requirement was removed from the ISO C standard and 
subsequently removed from this volume of IEEE Std 1003.1-2001 as well. 
The wording, in particular the use of the word should, requires a 
Strictly Conforming POSIX Application to pass at least one argument to 
the exec function, thus guaranteeing that argc be one or greater when 
invoked by such an application. In fact, this is good practice, since 
many existing applications reference argv[0] without first checking the 
value of argc.
----

    You'll have to be more specific about case (2).  What in the codebase
    are you refering to, file and line ?
Trunk as of several hours ago, sys/kern/kern_exec.c function 
exec_copyin_args() around line 700. The code there fetches pointer to 
argv[0] from userspace, checks if it's NULL and puts first argument 
instead of it. Then it increases userspace pointer by one and fetches 
the next pointer *unconditionally*, so that in the case when argv[0] is 
NULL you may get some invalid (e.g. junk but non-NULL pointer) and get 
EFAULT for no reason. The same code ignores argv being NULL - see my 
follow-up. FreeBSD code in this case explicitly returns EFAULT.

-Maxim