DragonFly BSD
DragonFly bugs List (threaded) for 2005-01
[Date Prev][Date Next]  [Thread Prev][Thread Next]  [Date Index][Thread Index]

Re: Looks like split of execve(2) syscall created bugs

From: Maxim Sobolev <sobomax@xxxxxxxxxxxx>
Date: Sat, 29 Jan 2005 21:40:30 +0200

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.

    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:

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.


[Date Prev][Date Next]  [Thread Prev][Thread Next]  [Date Index][Thread Index]