Re: [PATCH 1/2] af_unix: Do not wait for garbage collector in sendmsg()

[Nam Cao <namcao@linutronix.de>]

Kuniyuki Iwashima <kuniyu@google.com> writes:
> your patch makes it much easier to abuse.
> UNIX_INFLIGHT_SANE_USER is usually much smaller than
> RLIMIT_NOFILE.
>
> unix_schedule_gc() in sendmsg() is to self-regulate malicious users,
> otherwise GC relies on unrelated AF_UNIX socket's close() and could
> be triggered too late since GC is system-wide.

About the abuse, the scenario where inflight sockets bypass
UNIX_INFLIGHT_SANE_USER and delay GC until an unrelated AF_UNIX socket
closes actually exists today.

For example, the following program creates far more than
UNIX_INFLIGHT_SANE_USER inflight sockets, which persists indefinitely
until another unrelated AF_UNIX close().

#include <sys/mount.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <sys/wait.h>

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>

static int send_fd(int unix_fd, int fd)
{
        struct msghdr msgh;
        struct cmsghdr *cmsg;
        char buf[CMSG_SPACE(sizeof(fd))];

        memset(&msgh, 0, sizeof(msgh));

        memset(buf, 0, sizeof(buf));
        msgh.msg_control = buf;
        msgh.msg_controllen = sizeof(buf);

        cmsg = CMSG_FIRSTHDR(&msgh);
        cmsg->cmsg_len = CMSG_LEN(sizeof(fd));
        cmsg->cmsg_level = SOL_SOCKET;
        cmsg->cmsg_type = SCM_RIGHTS;

        msgh.msg_controllen = cmsg->cmsg_len;

        memcpy(CMSG_DATA(cmsg), &fd, sizeof(fd));
        return sendmsg(unix_fd, &msgh, 0);
}

int main(int argc, char *argv[])
{
	int fd[2];
	int i;

	for (int n = 0; n < 100; ++n) {
		if (socketpair(PF_UNIX, SOCK_SEQPACKET, 0, fd) == -1)
			goto out_error;

		for (i = 0; i < 100; ++i) {
			if (send_fd(fd[0], fd[0]) == -1)
				goto out_error;

			if (send_fd(fd[1], fd[1]) == -1)
				goto out_error;
		}
	}

	return 0;

out_error:
	fprintf(stderr, "error: %s\n", strerror(errno));
}

To address this properly, we can schedule the GC at task exit. I can
include that patch in my series, if that sounds good to you.

> Previously every sendmsg() had to wait for GC, and now it's only when
> there is a circular reference AND user has too many inflight sockets.
>
> Please fix the root cause; the former condition on your system.

Can you clarify what you mean by fixing the former condition on my
system. Do you mean ensuring that no application creates a circular
reference?

I am afraid we cannot rely on all users and applications to behave. We
do not want a buggy program or a malicious program to harm another
time-critical task.

Nam