switch-root: NACK invalid requests instead of false success

initctl switch-root always exits 0, no matter what happens on the
finit side.  do_switch_root_api() sends the ACK before switch_root()
has validated anything, so any failure after that point never reaches
the client -- the error only shows up in the log.  The runlevel guard
in api_cb() has the same problem: it rejects the request but still
sends an ACK.

Split the validation out of switch_root() into switch_root_precheck()
and run it before the ACK.  A failed check now sends a NACK with the
error message, which initctl prints before exiting 1:

    initctl switch-root /mnt
    switch-root: /mnt is not a mount point

The ACK is only sent once the precheck passes, since after that point
we are committed.  On success finit execs the new init and the
connection dies with no reply at all, so initctl treats only an
explicit NACK as failure.

Signed-off-by: Paweł Sobczak <github@fixeq.qzz.io>

src/api.c

@@ -236,14 +236,16 @@ static void bypass_shutdown(void *unused)
 /*
  * Handle switch_root API command.
  * Parses data: "newroot\0newinit\0"
- * Sends ACK before attempting switch_root since it doesn't return on success.
- * Returns: result from switch_root() on failure, doesn't return on success.
+ *
+ * On precheck failure, returns 1 with a message in rq->data for
+ * api_cb() to NACK.  Does not return on success, and a post-ACK
+ * failure has nobody left to reply to, so returns 0 either way.
  */
 static int do_switch_root_api(int sd, struct init_request *rq)
 {
+	char errbuf[128];
 	char *newroot, *newinit = NULL;
 	char *ptr;
-	int result;
 
 	dbg("switch-root %s", rq->data);
 	strterm(rq->data, sizeof(rq->data));
@@ -256,8 +258,13 @@ static int do_switch_root_api(int sd, struct init_request *rq)
 			newinit = ptr;
 	}
 
+	if (switch_root_precheck(newroot, newinit, errbuf, sizeof(errbuf))) {
+		snprintf(rq->data, sizeof(rq->data), "switch-root: %s", errbuf);
+		return 1;
+	}
+
 	/*
-	 * Send ACK first, since we won't return from
+	 * Send ACK now, since we won't return from
 	 * switch_root() on success.
 	 */
 	rq->cmd = INIT_CMD_ACK;
@@ -265,12 +272,10 @@ static int do_switch_root_api(int sd, struct init_request *rq)
 		dbg("Failed sending ACK to client");
 	close(sd);
 
-	/* This does not return on success */
-	result = switch_root(newroot, newinit);
-	if (result)
-		logit(LOG_ERR, "switch_root failed: %s", strerror(errno));
+	/* Does not return on success, logs its own failures */
+	switch_root(newroot, newinit);
 
-	return result;
+	return 0;
 }
 
 static int do_reboot(int cmd, int timeout, char *buf, size_t len)
@@ -435,7 +440,10 @@ static void api_cb(uev_t *w, void *arg, int events)
 
 		case INIT_CMD_SWITCH_ROOT:
 			if (runlevel != INIT_LEVEL && runlevel != 1) {
-				warnx("switch-root only allowed in runlevel S or 1");
+				strlcpy(rq.data, "switch-root: only allowed in runlevel S or 1",
+					sizeof(rq.data));
+				warnx("%s", rq.data);
+				result = 1;
 				goto done;
 			}
 			break;
@@ -541,8 +549,10 @@ static void api_cb(uev_t *w, void *arg, int events)
 			break;
 
 		case INIT_CMD_SWITCH_ROOT:
-			do_switch_root_api(sd, &rq);
-			goto leave;
+			result = do_switch_root_api(sd, &rq);
+			if (result)
+				break;	/* precheck failed before ACK, done: sends the NACK */
+			goto leave;	/* ACK sent and sd closed by do_switch_root_api() */
 
 		case INIT_CMD_ACK:
 			dbg("Client failed reading ACK");

src/initctl.c

@@ -1161,10 +1161,14 @@ int do_switch_root(int argc, char *argv[])
 	printf(" ...\n");
 
 	/*
-	 * On success, finit exec's new init and we lose connection.
-	 * A "failure" to read reply is actually expected on success.
+	 * Unlike do_cmd(), a failing client_send() is expected here: on
+	 * success Finit execs the new init and the connection dies with
+	 * no reply.  Only an explicit NACK means the request was denied.
 	 */
-	client_send(&rq, sizeof(rq));
+	if (client_send(&rq, sizeof(rq)) && rq.cmd == INIT_CMD_NACK) {
+		puts(rq.data);
+		return 1;
+	}
 
 	return 0;
 }

src/initramfs.c

@@ -27,6 +27,7 @@
 #include <fcntl.h>
 #include <ftw.h>
 #include <limits.h>
+#include <stdarg.h>
 #include <string.h>
 #include <sys/mount.h>
 #include <sys/stat.h>
@@ -136,6 +137,96 @@ static int kill_cb(int pid, void *data)
 	return 0;
 }
 
+/*
+ * Log a precheck failure, optionally handing the message back to the
+ * caller in errbuf for relaying to the client.
+ */
+static int __attribute__ ((format (printf, 4, 5)))
+switch_root_fail(char *errbuf, size_t errbuflen, int err, const char *fmt, ...)
+{
+	char msg[128];
+	va_list ap;
+
+	va_start(ap, fmt);
+	vsnprintf(msg, sizeof(msg), fmt, ap);
+	va_end(ap);
+
+	logit(LOG_ERR, "switch_root: %s", msg);
+	if (errbuf)
+		strlcpy(errbuf, msg, errbuflen);
+
+	errno = err;
+	return -1;
+}
+
+/*
+ * Validate a switch_root request without side effects, so the caller
+ * can reject a bad request before committing to teardown.
+ *
+ * On failure, returns -1 with errno set.  If errbuf is non-NULL it
+ * also gets a text reason, better suited for a client reply than
+ * strerror(errno).
+ */
+int switch_root_precheck(const char *newroot, const char *newinit,
+			 char *errbuf, size_t errbuflen)
+{
+	struct stat newroot_st, oldroot_st;
+	char init_path[PATH_MAX];
+	int fd;
+
+	if (!newroot || !newroot[0])
+		return switch_root_fail(errbuf, errbuflen, EINVAL, "no new root given");
+
+	/* Default to /sbin/init if not specified */
+	if (!newinit || !newinit[0])
+		newinit = "/sbin/init";
+
+	/* Verify we're PID 1 */
+	if (getpid() != 1)
+		return switch_root_fail(errbuf, errbuflen, EPERM, "must be run as PID 1");
+
+	/* Verify newroot exists and is a directory */
+	fd = open(newroot, O_RDONLY | O_DIRECTORY);
+	if (fd < 0)
+		return switch_root_fail(errbuf, errbuflen, ENOTDIR,
+					 "%s is not a directory", newroot);
+
+	if (fstat(fd, &newroot_st)) {
+		int saved_errno = errno;
+
+		close(fd);
+		return switch_root_fail(errbuf, errbuflen, saved_errno,
+					 "cannot stat %s: %s", newroot, strerror(saved_errno));
+	}
+	close(fd);
+
+	/* Verify newroot is a mount point (different device than parent) */
+	fd = open("/", O_RDONLY | O_DIRECTORY);
+	if (fd < 0)
+		return switch_root_fail(errbuf, errbuflen, errno,
+					"cannot open /: %s", strerror(errno));
+	if (fstat(fd, &oldroot_st)) {
+		int saved_errno = errno;
+
+		close(fd);
+		return switch_root_fail(errbuf, errbuflen, saved_errno,
+					 "cannot stat /: %s", strerror(saved_errno));
+	}
+	close(fd);
+
+	if (newroot_st.st_dev == oldroot_st.st_dev)
+		return switch_root_fail(errbuf, errbuflen, EINVAL,
+					 "%s is not a mount point", newroot);
+
+	/* Verify init exists in new root */
+	snprintf(init_path, sizeof(init_path), "%s%s", newroot, newinit);
+	if (access(init_path, X_OK))
+		return switch_root_fail(errbuf, errbuflen, ENOENT,
+					 "%s not found or not executable", init_path);
+
+	return 0;
+}
+
 /*
  * Perform switch_root to a new root filesystem
  *
@@ -144,69 +235,23 @@ static int kill_cb(int pid, void *data)
  */
 int switch_root(const char *newroot, const char *newinit)
 {
-	struct stat newroot_st, oldroot_st;
-	char init_path[PATH_MAX];
+	struct stat oldroot_st;
 	int console_fd;
-	int fd;
 	dev_t rootdev;
 	int signo;
 
-	if (!newroot || !newroot[0]) {
-		errno = EINVAL;
+	/* No client left to relay a message to */
+	if (switch_root_precheck(newroot, newinit, NULL, 0))
 		return -1;
-	}
 
-	/* Default to /sbin/init if not specified */
+	/* Default to /sbin/init, as in switch_root_precheck() */
 	if (!newinit || !newinit[0])
 		newinit = "/sbin/init";
 
-	/* Verify we're PID 1 */
-	if (getpid() != 1) {
-		logit(LOG_ERR, "switch_root must be run as PID 1");
-		errno = EPERM;
-		return -1;
-	}
-
-	/* Verify newroot exists and is a directory */
-	fd = open(newroot, O_RDONLY | O_DIRECTORY);
-	if (fd < 0) {
-		logit(LOG_ERR, "switch_root: %s is not a directory", newroot);
-		errno = ENOTDIR;
-		return -1;
-	}
-	if (fstat(fd, &newroot_st)) {
-		close(fd);
-		logit(LOG_ERR, "switch_root: cannot stat %s", newroot);
-		return -1;
-	}
-	close(fd);
-
-	/* Verify newroot is a mount point (different device than parent) */
-	fd = open("/", O_RDONLY | O_DIRECTORY);
-	if (fd < 0) {
-		logit(LOG_ERR, "switch_root: cannot open /");
-		return -1;
-	}
-	if (fstat(fd, &oldroot_st)) {
-		close(fd);
-		logit(LOG_ERR, "switch_root: cannot stat /");
-		return -1;
-	}
-	close(fd);
-
-	if (newroot_st.st_dev == oldroot_st.st_dev) {
-		logit(LOG_ERR, "switch_root: %s is not a mount point", newroot);
-		errno = EINVAL;
-		return -1;
-	}
-
-	/* Verify init exists in new root */
-	snprintf(init_path, sizeof(init_path), "%s%s", newroot, newinit);
-	if (access(init_path, X_OK)) {
-		logit(LOG_ERR, "switch_root: %s not found or not executable", init_path);
-		errno = ENOENT;
-		return -1;
-	}
+	/* Needed below for the initramfs cleanup */
+	if (stat("/", &oldroot_st))
+		return switch_root_fail(NULL, 0, errno,
+					"cannot stat /: %s", strerror(errno));
 
 	logit(LOG_NOTICE, "Performing switch_root to %s, init %s", newroot, newinit);
 

src/private.h

@@ -70,6 +70,8 @@ int          plugin_init      (uev_ctx_t *ctx);
 void         plugin_exit      (void);
 
 void         iterate_proc     (int (*cb)(int, void *), void *data);
+int          switch_root_precheck(const char *newroot, const char *newinit,
+				  char *errbuf, size_t errbuflen);
 int          switch_root      (const char *newroot, const char *newinit);
 
 #endif /* FINIT_PRIVATE_H_ */