rewrite efivarfs_set_variable() [9896c26-based]

This patch rewrites the efivarfs_set_variable() function, to address the
following issues:

- a size_t value is printed with %zd -- size_t is unsigned, so it should
  be printed with %zu or %zx,

- a VLA is used for storing input of basically unbounded size -- we should
  use a range-checked malloc() call instead,

- the efivarfs file is opened for writing while it may be immutable --
  this is the trickiest issue to resolve,

- passing just O_APPEND|O_CREAT to open() is undefined -- O_WRONLY is
  required, and O_APPEND and (O_CREAT | O_EXCL) should both be independent
  of it (and of each other),

- some error branches call efi_error() without setting errno first,

- the variable is removed on any write failure, even if we didn't create
  the variable -- failed writes are expected to be atomic (from the kernel
  side and from the firmware side) and not to leave behind side effects;
  so only delete the variable on error if we created it.

A small helper function efivarfs_make_fd_mutable() is introduced as well.

(It's best to review the new efivarfs_set_variable() function in its
entirety, with the patch applied, rather than comparing old vs. new, hunk
for hunk.)

Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1516599
Signed-off-by: Laszlo Ersek <lersek@redhat.com>

src/efivarfs.c

@@ -127,6 +127,21 @@ efivarfs_set_fd_immutable(int fd, int immutable)
 	return rc;
 }
 
+static int
+efivarfs_make_fd_mutable(int fd, unsigned *orig_attrs)
+{
+	unsigned mutable_attrs;
+
+	if (ioctl(fd, FS_IOC_GETFLAGS, orig_attrs) == -1)
+		return -1;
+	if ((*orig_attrs & FS_IMMUTABLE_FL) == 0)
+		return 0;
+	mutable_attrs = *orig_attrs & ~(unsigned)FS_IMMUTABLE_FL;
+	if (ioctl(fd, FS_IOC_SETFLAGS, &mutable_attrs) == -1)
+		return -1;
+	return 0;
+}
+
 static int
 efivarfs_set_immutable(char *path, int immutable)
 {
@@ -303,79 +318,142 @@ static int
 efivarfs_set_variable(efi_guid_t guid, const char *name, uint8_t *data,
 		      size_t data_size, uint32_t attributes, mode_t mode)
 {
-	uint8_t buf[sizeof (attributes) + data_size];
-	__typeof__(errno) errno_value;
+	char *path;
+	size_t alloc_size;
+	uint8_t *buf;
+	int rfd = -1;
+	struct stat rfd_stat;
+	unsigned orig_attrs = 0;
+	int restore_immutable_fd = -1;
+	int wfd = -1;
+	int open_wflags;
 	int ret = -1;
-	char *path = NULL;
-	int fd = -1;
-	int flags = 0;
-	char *flagstr;
-	int rc;
+	int save_errno;
 
 	if (strlen(name) > 1024) {
-		efi_error("name too long (%zd of 1024)", strlen(name));
 		errno = EINVAL;
+		efi_error("name too long (%zu of 1024)", strlen(name));
 		return -1;
 	}
 
-	rc = make_efivarfs_path(&path, guid, name);
-	if (rc < 0) {
+	if (data_size > (size_t)-1 - sizeof (attributes)) {
+		errno = EOVERFLOW;
+		efi_error("data_size too large (%zu)", data_size);
+		return -1;
+	}
+
+	if (make_efivarfs_path(&path, guid, name) < 0) {
 		efi_error("make_efivarfs_path failed");
-		goto err;
+		return -1;
 	}
 
-	if (attributes & EFI_VARIABLE_APPEND_WRITE) {
-		flags = O_APPEND|O_CREAT;
-		flagstr = "O_APPEND|O_CREAT";
-	} else {
-		flags = O_WRONLY|O_CREAT|O_EXCL;
-		flagstr = "O_WRONLY|O_CREAT|O_EXCL";
+	alloc_size = sizeof (attributes) + data_size;
+	buf = malloc(alloc_size);
+	if (buf == NULL) {
+		efi_error("malloc(%zu) failed\n", alloc_size);
+		goto err;
 	}
 
-	fd = open(path, flags, mode);
-	if (fd < 0) {
-		if (flags == (O_WRONLY|O_CREAT|O_EXCL)) {
-			rc = efi_del_variable(guid, name);
-			if (rc < 0) {
-				efi_error("efi_del_variable failed");
-				goto err;
-			}
-			fd = open(path, flags, mode);
+	/*
+	 * Open the file first in read-only mode. This is necessary when the
+	 * variable exists and it is also protected -- then we first have to
+	 * *attempt* to clear the immutable flag from the file. For clearing
+	 * the flag, we can only open the file read-only. In other cases,
+	 * opening the file for reading is not necessary, but it doesn't hurt
+	 * either.
+	 */
+	rfd = open(path, O_RDONLY);
+	if (rfd != -1) {
+		/* save the containing device and the inode number for later */
+		if (fstat(rfd, &rfd_stat) == -1) {
+			efi_error("fstat() failed on r/o fd %d", rfd);
+			goto err;
 		}
+
+		/* if the file is indeed immutable, clear and remember it */
+		if (efivarfs_make_fd_mutable(rfd, &orig_attrs) == 0 &&
+		    (orig_attrs & FS_IMMUTABLE_FL))
+			restore_immutable_fd = rfd;
 	}
-	if (fd < 0) {
-		efi_error("open(%s, %s, %0o) failed", path, flagstr, mode);
+
+	/*
+	 * Open the variable file for writing now. First, use O_APPEND
+	 * dependent on the input attributes. Second, the file either doesn't
+	 * exist here, or it does and we made an attempt to make it mutable
+	 * above. If the file was created afresh between the two open()s, then
+	 * we catch that with O_EXCL. If the file was removed between the two
+	 * open()s, we catch that with lack of O_CREAT. If the file was
+	 * *replaced* between the two open()s, we'll catch that later with
+	 * fstat() comparison.
+	 */
+	open_wflags = O_WRONLY;
+	if (attributes & EFI_VARIABLE_APPEND_WRITE)
+		open_wflags |= O_APPEND;
+	if (rfd == -1)
+		open_wflags |= O_CREAT | O_EXCL;
+
+	wfd = open(path, open_wflags, mode);
+	if (wfd == -1) {
+		efi_error("failed to %s %s for %s",
+			  rfd == -1 ? "create" : "open",
+			  path,
+			  ((attributes & EFI_VARIABLE_APPEND_WRITE) ?
+			   "appending" : "writing"));
 		goto err;
 	}
 
-	efivarfs_set_fd_immutable(fd, 0);
+	/*
+	 * If we couldn't open the file for reading, then we have to attempt
+	 * making it mutable now -- in case we created a protected file (for
+	 * writing or appending), then the kernel made it immutable
+	 * immediately, and the write() below would fail otherwise.
+	 */
+	if (rfd == -1) {
+		if (efivarfs_make_fd_mutable(wfd, &orig_attrs) == 0 &&
+		    (orig_attrs & FS_IMMUTABLE_FL))
+			restore_immutable_fd = wfd;
+	} else {
+		/* make sure rfd and wfd refer to the same file */
+		struct stat wfd_stat;
+
+		if (fstat(wfd, &wfd_stat) == -1) {
+			efi_error("fstat() failed on w/o fd %d", wfd);
+			goto err;
+		}
+		if (rfd_stat.st_dev != wfd_stat.st_dev ||
+		    rfd_stat.st_ino != wfd_stat.st_ino) {
+			errno = EINVAL;
+			efi_error("r/o fd %d and w/o fd %d refer to different "
+				  "files", rfd, wfd);
+			goto err;
+		}
+	}
 
 	memcpy(buf, &attributes, sizeof (attributes));
 	memcpy(buf + sizeof (attributes), data, data_size);
-#if 0
-	errno = ENOSPC;
-	rc = -1;
-#else
-	rc = write(fd, buf, sizeof (attributes) + data_size);
-#endif
-	if (rc >= 0) {
-		ret = 0;
-		efivarfs_set_fd_immutable(fd, 1);
-	} else {
-		efi_error("write failed");
-		efivarfs_set_fd_immutable(fd, 0);
-		unlink(path);
+
+	if (write(wfd, buf, alloc_size) == -1) {
+		efi_error("writing to fd %d failed", wfd);
+		goto err;
 	}
+
+	/* we're done */
+	ret = 0;
+
 err:
-	errno_value = errno;
+	save_errno = errno;
 
-	if (path)
-		free(path);
+	/* if we're exiting with error and created the file, remove it */
+	if (ret == -1 && rfd == -1 && wfd != -1 && unlink(path) == -1)
+		efi_error("failed to unlink %s", path);
 
-	if (fd >= 0)
-		close(fd);
+	ioctl(restore_immutable_fd, FS_IOC_SETFLAGS, &orig_attrs);
+	close(wfd);
+	close(rfd);
+	free(buf);
+	free(path);
 
-	errno = errno_value;
+	errno = save_errno;
 	return ret;
 }