Re: [PATCH v7 2/7] um: use execveat to create userspace MMs

[Johannes Berg <johannes@sipsolutions.net>]

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