Re: [PATCH] Linux: Add execveat system call wrapper

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 30/04/2020 09:55, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>>> So I think we have to do this:
>>>
>>> * If there are more flags than just the two, fail with EINVAL.
>>>
>>> * To handle AT_EMPTY_PATH, do not open a new file descriptor (using
>>>   openat) if AT_EMPTY_PATH is specified *and* the file name is "".
>>>
>>> * To handle AT_SYMLINK_NOFOLLOW, openat needs to be called with
>>>   O_NOFOLLOW in that case (in addition to O_CLOEXEC).
>>
>> These will surely need to be on the testcase.
> 
> Yes, these permutations need to be tested.
> 
>>> The behavior with AT_EMPTY_PATH/"" and AT_SYMLINK_NOFOLLOW at the same
>>> time is not immedately obvious from the kernel code, so I wrote a small
>>> test program (/bin/sh is a symbolic link to /bin/bash on this system):
>>>
>>> #include <err.h>
>>> #include <fcntl.h>
>>> #include <sys/syscall.h>
>>> #include <unistd.h>
>>>
>>> int
>>> main (void)
>>> {
>>>   int fd = open ("/bin/sh", O_PATH | O_NOFOLLOW);
>>>   if (fd < 0)
>>>     err (1, "open");
>>>   static char *const argv[] = { "sh", "-c", "exit 0", NULL };
>>>   static char *const envp[] = { NULL };
>>>   syscall (SYS_execveat, fd, "", argv, envp,
>>>            AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);
>>>   err (1, "execveat");
>>> }
>>>
>>> This fails:
>>>
>>> openat(AT_FDCWD, "/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
>>> execveat(3, "", ["sh", "-c", "exit 0"], 0x402040 /* 0 vars */, AT_SYMLINK_NOFOLLOW|AT_EMPTY_PATH) = -1 ELOOP (Too many levels of symbolic links)
>>> […]
>>> execveat-opath-symlink: execveat: Too many levels of symbolic links
>>>
>>> So I think for the AT_EMPTY_PATH/"" and AT_SYMLINK_NOFOLLOW case, we
>>> need to call fstatat64 with AT_EMPTY_PATH and see if st_mode indicates
>>> that the descriptor refers to a symbolic link.  If it does, the function
>>> needs to fail with ELOOP.
>>
>> I think execve would handle it:
>>
>> openat(AT_FDCWD, "/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
>> execve("/proc/self/fd/3", ["sh", "-c", "echo test"], 0x556815e580a8 /* 0 vars */) = -1 ELOOP (Too many levels of symbolic   links)
> 
> And execveat fails even without AT_SYMLINK_NOFOLLOW:
> 
> openat(AT_FDCWD, "/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
> execveat(3, "", ["sh", "-c", "exit 0"], 0x402040 /* 0 vars */, AT_EMPTY_PATH) = -1 ELOOP (Too many levels of symbolic links)
> 
> Does this mean we do not need a special case for an O_PATH|O_NOFOLLOW
> open of a symbolic link?

I think current proposal has some issues regarding handling the
path and flags, with the following code for fallback:

---
  int fd;
  if (path[0] == '\0' && flags & AT_EMPTY_PATH)
    fd = dirfd;
  else
    {
      int oflags = O_CLOEXEC;
      if (flags & AT_SYMLINK_NOFOLLOW)
        oflags |= O_NOFOLLOW;
      fd = openat (dirfd, path, oflags);
    }

  if (fd < 0)
    return -1;

  struct fd_to_filename fdfilename;
  const char *gfilename = __fd_to_filename (fd, &fdfilename);

  /* We do not need the return value.  */
  execve (gfilename, argv, envp);
---

It seems to follow execveat syscall semantic.  For instance:

  int fd = open ("/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH);
  [...]
  execveat (fd, "", argv, envp, AT_EMPTY_PATH);

produces:

openat(AT_FDCWD, "/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
execve("/proc/self/fd/3", ["sh", "-c", "echo test"], 0x5624523db098 /* 0 vars */) = -1 ELOOP 

And:

  int fd = open ("/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH);
  [...]
  execveat (fd, "", argv, envp, AT_EMPTY_PATH | AT_SYMLINK_NOFOLLOW);

also produces:

  openat(AT_FDCWD, "/bin/sh", O_RDONLY|O_NOFOLLOW|O_PATH) = 3
  execve("/proc/self/fd/3", ["sh", "-c", "echo test"], 0x563e8f1d0098 /* 0 vars */) = -1 ELOOP