On Thu, 2024-07-04 at 18:27 +0200, Benjamin Berg wrote:
>
> + /* set a nice name */
> + stub_syscall2(__NR_prctl, PR_SET_NAME, (unsigned long)"uml-userspace");
Is that even needed when you're passing it as argv[0] in execve()? But
whatever, it's fine, just wondering.
> + /* setup signal stack inside stub data */
> + stack.ss_flags = 0;
> + stack.ss_size = STUB_DATA_PAGES * UM_KERN_PAGE_SIZE;
> + stack.ss_sp = (void *)init_data.stub_start + UM_KERN_PAGE_SIZE;
> + stub_syscall2(__NR_sigaltstack, (unsigned long)&stack, 0);
> +
> + /* register SIGSEGV handler (SA_RESTORER, the handler never returns) */
> + sa.sa_flags = SA_ONSTACK | SA_NODEFER | SA_SIGINFO | 0x04000000;
> + sa.sa_handler_ = (void *) init_data.segv_handler;
> + sa.sa_restorer = NULL;
> + sa.sa_mask = 0L; /* No need to mask anything */
most of that init could be in the initializer, except the dynamic ones
of course; the NULL/0 is also unnecessary I guess (though might want the
sa_mask for the comment)
> + struct stub_init_data init_data = {
> + .stub_start = STUB_START,
> + .segv_handler = STUB_CODE +
> + (unsigned long) stub_segv_handler -
> + (unsigned long) __syscall_stub_start,
> + };
> + struct iomem_region *iomem;
> + int ret;
> +
> + init_data.stub_code_fd = phys_mapping(uml_to_phys(__syscall_stub_start),
> + &offset);
> + init_data.stub_code_offset = MMAP_OFFSET(offset);
> +
> + init_data.stub_data_fd = phys_mapping(uml_to_phys(stack), &offset);
> + init_data.stub_data_offset = MMAP_OFFSET(offset);
also could move more here into the initializer
> +static int __init init_stub_exe_fd(void)
> +{
> + size_t len = 0;
maybe that should be called 'written'?
> + int res;
and I technically that should be ssize_t for the write() return value,
not that it'll be big enough to matter
> + while (len < stub_exe_end - stub_exe_start) {
> + res = write(stub_exe_fd, stub_exe_start + len,
> + stub_exe_end - stub_exe_start - len);
> + if (res < 0) {
> + if (errno == EINTR)
> + continue;
> +
> + if (tmpfile)
> + unlink(tmpfile);
> + panic("%s: Failed write to memfd: %d", __func__, errno);
nit: not always memfd now
> + if (!tmpfile) {
> + fcntl(stub_exe_fd, F_ADD_SEALS,
> + F_SEAL_WRITE | F_SEAL_SHRINK | F_SEAL_GROW | F_SEAL_SEAL);
> + } else {
> + /* Only executable by us */
> + if (fchmod(stub_exe_fd, 00500) < 0) {
now it's also readable, so comment doesn't seem right? maybe just remove
it?
> + unlink(tmpfile);
> + panic("Could not make stub binary excutable: %d",
> + errno);
perhaps print -errno?
> + }
> +
> + close(stub_exe_fd);
> + stub_exe_fd = open(tmpfile, O_RDONLY | O_CLOEXEC | O_NOFOLLOW);
> + if (stub_exe_fd < 0) {
> + unlink(tmpfile);
> + panic("Could not reopen stub binary: %d", errno);
same here
johannes
