From: Mike Yuan Date: Fri, 6 Jun 2025 19:47:39 +0000 (+0200) Subject: sd-varlink: hook up fd passing control with SO_PASSRIGHTS X-Git-Tag: v258-rc1~301^2~1 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ed6b7b6acea44b2e0233aa0d6d2c0b147ecac968;p=thirdparty%2Fsystemd.git sd-varlink: hook up fd passing control with SO_PASSRIGHTS This is a tricky one, because we effectively turn fd passing input toggle into a tristate: unset, disabled, and enabled; whereas unset and disabled were identical previously. *Unset* state silently ignores SCM_RIGHTS passed by invoking recv() instead of recvmsg(), and for disabled we now disable SO_PASSRIGHTS completely. The plot thickens when it comes to the server, since we want to turn off the SO_PASSRIGHTS already on the listening socket so that there's no race between accept() and recvmsg() wrt SO_PASSRIGHTS state. However, if we do this unconditionally, the existing use case of creating a custom connection callback and enabling fd passing there would be broken. Hence, let's introduce a new flag, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT, which when set ties the enablement of fd passing to SO_PASSRIGHTS in server, and set it for all our varlink servers. Refer to the previous commit for the rationale behind return value change in sd_varlink_set_allow_fd_passing_input(). --- diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index bf764bb97f1..1e70ce41a7c 100644 --- a/src/libsystemd/sd-varlink/sd-varlink.c +++ b/src/libsystemd/sd-varlink/sd-varlink.c @@ -139,12 +139,14 @@ static int varlink_new(sd_varlink **ret) { .ucred = UCRED_INVALID, + .peer_pidfd = -EBADF, + .timestamp = USEC_INFINITY, .timeout = VARLINK_DEFAULT_TIMEOUT_USEC, - .af = -1, + .allow_fd_passing_input = -1, - .peer_pidfd = -EBADF, + .af = -1, }; *ret = v; @@ -849,7 +851,7 @@ static int varlink_read(sd_varlink *v) { p = v->input_buffer + v->input_buffer_index + v->input_buffer_size; rs = MALLOC_SIZEOF_SAFE(v->input_buffer) - (v->input_buffer_index + v->input_buffer_size); - if (v->allow_fd_passing_input) { + if (v->allow_fd_passing_input > 0) { iov = IOVEC_MAKE(p, rs); /* Allocate the fd buffer on the heap, since we need a lot of space potentially */ @@ -893,14 +895,14 @@ static int varlink_read(sd_varlink *v) { return n; if (n == 0) { /* EOF */ - if (v->allow_fd_passing_input) + if (v->allow_fd_passing_input > 0) cmsg_close_all(&mh); v->read_disconnected = true; return 1; } - if (v->allow_fd_passing_input) { + if (v->allow_fd_passing_input > 0) { struct cmsghdr *cmsg; cmsg = cmsg_find(&mh, SOL_SOCKET, SCM_RIGHTS, (socklen_t) -1); @@ -3159,7 +3161,7 @@ _public_ int sd_varlink_peek_fd(sd_varlink *v, size_t i) { /* Returns one of the file descriptors that were received along with the current message. This does * not duplicate the fd nor invalidate it, it hence remains in our possession. */ - if (!v->allow_fd_passing_input) + if (v->allow_fd_passing_input <= 0) return -EPERM; if (i >= v->n_input_fds) @@ -3185,7 +3187,7 @@ _public_ int sd_varlink_take_fd(sd_varlink *v, size_t i) { * we'll invalidate the reference to it under our possession. If called twice in a row will return * -EBADF */ - if (!v->allow_fd_passing_input) + if (v->allow_fd_passing_input <= 0) return -EPERM; if (i >= v->n_input_fds) @@ -3197,7 +3199,7 @@ _public_ int sd_varlink_take_fd(sd_varlink *v, size_t i) { _public_ int sd_varlink_get_n_fds(sd_varlink *v) { assert_return(v, -EINVAL); - if (!v->allow_fd_passing_input) + if (v->allow_fd_passing_input <= 0) return -EPERM; return (int) v->n_input_fds; @@ -3247,20 +3249,29 @@ _public_ int sd_varlink_set_allow_fd_passing_input(sd_varlink *v, int b) { assert_return(v, -EINVAL); - if (v->allow_fd_passing_input == !!b) + if (v->allow_fd_passing_input >= 0 && (v->allow_fd_passing_input > 0) == !!b) return 0; - if (!b) { - v->allow_fd_passing_input = false; - return 1; - } - r = verify_unix_socket(v); - if (r < 0) + if (r < 0) { + assert(v->allow_fd_passing_input <= 0); + + if (!b) { + v->allow_fd_passing_input = false; + return 0; + } + return r; + } - v->allow_fd_passing_input = true; - return 0; + if (!v->server || FLAGS_SET(v->server->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) { + r = setsockopt_int(v->input_fd, SOL_SOCKET, SO_PASSRIGHTS, !!b); + if (r < 0 && !ERRNO_IS_NEG_NOT_SUPPORTED(r)) + log_debug_errno(r, "Failed to set SO_PASSRIGHTS socket option: %m"); + } + + v->allow_fd_passing_input = !!b; + return 1; } _public_ int sd_varlink_set_allow_fd_passing_output(sd_varlink *v, int b) { @@ -3297,7 +3308,8 @@ _public_ int sd_varlink_server_new(sd_varlink_server **ret, sd_varlink_server_fl SD_VARLINK_SERVER_INHERIT_USERDATA| SD_VARLINK_SERVER_INPUT_SENSITIVE| SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT| - SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT)) == 0, -EINVAL); + SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT| + SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) == 0, -EINVAL); s = new(sd_varlink_server, 1); if (!s) @@ -3493,6 +3505,12 @@ _public_ int sd_varlink_server_add_connection_pair( if (r < 0) return r; + /* Link up the server and the connection, and take reference in both directions. Note that the + * reference on the connection is left dangling. It will be dropped when the connection is closed, + * which happens in varlink_close(), including in the event loop quit callback. */ + v->server = sd_varlink_server_ref(server); + sd_varlink_ref(v); + v->input_fd = input_fd; v->output_fd = output_fd; if (server->flags & SD_VARLINK_SERVER_INHERIT_USERDATA) @@ -3510,12 +3528,6 @@ _public_ int sd_varlink_server_add_connection_pair( (void) sd_varlink_set_allow_fd_passing_input(v, FLAGS_SET(server->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT)); (void) sd_varlink_set_allow_fd_passing_output(v, FLAGS_SET(server->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT)); - /* Link up the server and the connection, and take reference in both directions. Note that the - * reference on the connection is left dangling. It will be dropped when the connection is closed, - * which happens in varlink_close(), including in the event loop quit callback. */ - v->server = sd_varlink_server_ref(server); - sd_varlink_ref(v); - varlink_set_state(v, VARLINK_IDLE_SERVER); if (server->event) { @@ -3632,6 +3644,13 @@ _public_ int sd_varlink_server_listen_fd(sd_varlink_server *s, int fd) { if (r < 0) return r; + /* If fd passing is disabled on server, and SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT flag is set, + * turn off SO_PASSRIGHTS immediately on listening socket. The conditionalization behind a flag + * is needed to retain backwards compat, where implementations would register a connection callback + * to enable fd passing after accept(), which might race with clients wrt SO_PASSRIGHTS state. */ + if (FLAGS_SET(s->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) + (void) setsockopt_int(fd, SOL_SOCKET, SO_PASSRIGHTS, FLAGS_SET(s->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT)); + r = varlink_server_create_listen_fd_socket(s, fd, &ss); if (r < 0) return r; @@ -3673,6 +3692,10 @@ _public_ int sd_varlink_server_listen_address(sd_varlink_server *s, const char * fd = fd_move_above_stdio(fd); + /* See the comment in sd_varlink_server_listen_fd() */ + if (FLAGS_SET(s->flags, SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT)) + (void) setsockopt_int(fd, SOL_SOCKET, SO_PASSRIGHTS, FLAGS_SET(s->flags, SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT)); + (void) sockaddr_un_unlink(&sockaddr.un); WITH_UMASK(~m & 0777) diff --git a/src/libsystemd/sd-varlink/varlink-internal.h b/src/libsystemd/sd-varlink/varlink-internal.h index a163f81061d..ef848243738 100644 --- a/src/libsystemd/sd-varlink/varlink-internal.h +++ b/src/libsystemd/sd-varlink/varlink-internal.h @@ -166,12 +166,12 @@ struct sd_varlink { bool prefer_write:1; bool got_pollhup:1; - bool allow_fd_passing_input:1; - bool allow_fd_passing_output:1; - bool output_buffer_sensitive:1; /* whether to erase the output buffer after writing it to the socket */ bool input_sensitive:1; /* Whether incoming messages might be sensitive */ + bool allow_fd_passing_output; + int allow_fd_passing_input; + int af; /* address family if socket; AF_UNSPEC if not socket; negative if not known */ usec_t timestamp; diff --git a/src/libsystemd/sd-varlink/varlink-util.c b/src/libsystemd/sd-varlink/varlink-util.c index cc18656d138..be7df731437 100644 --- a/src/libsystemd/sd-varlink/varlink-util.c +++ b/src/libsystemd/sd-varlink/varlink-util.c @@ -174,7 +174,7 @@ int varlink_server_new( _cleanup_(sd_varlink_server_unrefp) sd_varlink_server *s = NULL; int r; - r = sd_varlink_server_new(&s, flags); + r = sd_varlink_server_new(&s, flags|SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT); if (r < 0) return log_debug_errno(r, "Failed to allocate varlink server object: %m"); diff --git a/src/systemd/sd-varlink.h b/src/systemd/sd-varlink.h index 67d9cc3d948..9d3dc48a444 100644 --- a/src/systemd/sd-varlink.h +++ b/src/systemd/sd-varlink.h @@ -68,6 +68,7 @@ __extension__ typedef enum _SD_ENUM_TYPE_S64(sd_varlink_server_flags_t) { SD_VARLINK_SERVER_INPUT_SENSITIVE = 1 << 4, /* Automatically mark all connection input as sensitive */ SD_VARLINK_SERVER_ALLOW_FD_PASSING_INPUT = 1 << 5, /* Allow receiving fds over all connections */ SD_VARLINK_SERVER_ALLOW_FD_PASSING_OUTPUT = 1 << 6, /* Allow sending fds over all connections */ + SD_VARLINK_SERVER_FD_PASSING_INPUT_STRICT = 1 << 7, /* Reject input messages with fds if fd passing is disabled (needs kernel v6.16+) */ _SD_ENUM_FORCE_S64(SD_VARLINK_SERVER) } sd_varlink_server_flags_t;