File efivar-bsc1192344-fix-open-dbx.patch of Package efivar

Overview Repositories Revisions Requests Users Attributes Meta

File efivar-bsc1192344-fix-open-dbx.patch of Package efivar

From 651cf8a100e03b1a4fd13355c831a8ee34d8e712 Mon Sep 17 00:00:00 2001
From: Gabriel Majeri <gabriel.majeri6@gmail.com>
Date: Sun, 24 Sep 2017 14:59:13 +0300
Subject: [PATCH] Use `__typeof__` instead of `typeof`

---
 src/dp-acpi.c  |  2 +-
 src/efivarfs.c | 12 ++++++------
 src/generics.h |  2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/dp-acpi.c b/src/dp-acpi.c
index 844ce49..ce9b989 100644
--- a/src/dp-acpi.c
+++ b/src/dp-acpi.c
@@ -34,7 +34,7 @@ _format_acpi_adr(char *buf, size_t size,
 	ssize_t off = 0;
 	format(buf, size, off, "AcpiAdr", "AcpiAdr(");
 	format_array(buf, size, off, "AcpiAdr", "0x%"PRIx32,
-		     typeof(dp->acpi_adr.adr[0]), dp->acpi_adr.adr,
+		     __typeof__(dp->acpi_adr.adr[0]), dp->acpi_adr.adr,
 		     (efidp_node_size(dp)-4) / sizeof (dp->acpi_adr.adr[0]));
 	format(buf, size, off, "AcpiAdr", ")");
 	return off;
diff --git a/src/efivarfs.c b/src/efivarfs.c
index 426090f..954d40b 100644
--- a/src/efivarfs.c
+++ b/src/efivarfs.c
@@ -69,7 +69,7 @@ efivarfs_probe(void)
 		rc = statfs(path, &buf);
 		if (rc == 0) {
 			char *tmp;
-			typeof(buf.f_type) magic = EFIVARFS_MAGIC;
+			__typeof__(buf.f_type) magic = EFIVARFS_MAGIC;
 			if (!memcmp(&buf.f_type, &magic, sizeof (magic)))
 				return 1;
 			else
@@ -128,7 +128,7 @@ efivarfs_set_fd_immutable(int fd, int immutable)
 static int
 efivarfs_set_immutable(char *path, int immutable)
 {
-	typeof(errno) error = 0;
+	__typeof__(errno) error = 0;
 	int fd;
 	int rc = 0;
 
@@ -158,7 +158,7 @@ efivarfs_get_variable_size(efi_guid_t guid, const char *name, size_t *size)
 	char *path = NULL;
 	int rc = 0;
 	int ret = -1;
-	typeof(errno) errno_value;
+	__typeof__(errno) errno_value;
 
 	rc = make_efivarfs_path(&path, guid, name);
 	if (rc < 0) {
@@ -212,7 +212,7 @@ static int
 efivarfs_get_variable(efi_guid_t guid, const char *name, uint8_t **data,
 		  size_t *data_size, uint32_t *attributes)
 {
-	typeof(errno) errno_value;
+	__typeof__(errno) errno_value;
 	int ret = -1;
 	size_t size = 0;
 	uint32_t ret_attributes = 0;
@@ -276,7 +276,7 @@ efivarfs_del_variable(efi_guid_t guid, const char *name)
 	if (rc < 0)
 		efi_error("unlink failed");
 
-	typeof(errno) errno_value = errno;
+	__typeof__(errno) errno_value = errno;
 	free(path);
 	errno = errno_value;
 
@@ -288,7 +288,7 @@ 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;
+	__typeof__(errno) errno_value;
 	int ret = -1;
 
 	if (strlen(name) > 1024) {
diff --git a/src/generics.h b/src/generics.h
index 22ae266..fdd9f85 100644
--- a/src/generics.h
+++ b/src/generics.h
@@ -65,7 +65,7 @@ generic_get_next_variable_name(const char *path, efi_guid_t **guid, char **name)
 
 		int fd = dirfd(dir);
 		if (fd < 0) {
-			typeof(errno) errno_value = errno;
+			__typeof__(errno) errno_value = errno;
 			efi_error("dirfd failed");
 			closedir(dir);
 			errno = errno_value;
-- 
2.12.3

From eb76603bb4fa44c01a2a31dab804075eb8df8ebf Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Tue, 26 Sep 2017 14:05:02 -0400
Subject: [PATCH] efivarfs_set_variable(): don't test access before creating
 variables.

Coverity, possibly correctly (though it's hard to see what the resulting
problem would be in this case), believes checking access(path, F_OK)
before doing open(path, ...) is a TOCTOU error.  And it arguably is,
except you have to be root to do this and we're operating entirely in
sysfs, so... hard to see how you could race it or what you could gain.
Maybe something at a higher level can be convinced to race stupidly if
you're calling libefivar.  I dunno.

Anyway, attempt to fix it.

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 src/efivarfs.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

Index: efivar/src/efivarfs.c
===================================================================
--- efivar.orig/src/efivarfs.c
+++ efivar/src/efivarfs.c
@@ -290,6 +290,11 @@ efivarfs_set_variable(efi_guid_t guid, c
 	uint8_t buf[sizeof (attributes) + data_size];
 	__typeof__(errno) errno_value;
 	int ret = -1;
+	char *path = NULL;
+	int fd = -1;
+	int flags = 0;
+	char *flagstr;
+	int rc;
 
 	if (strlen(name) > 1024) {
 		efi_error("name too long (%zd of 1024)", strlen(name));
@@ -297,37 +302,45 @@ efivarfs_set_variable(efi_guid_t guid, c
 		return -1;
 	}
 
-	char *path;
-	int rc = make_efivarfs_path(&path, guid, name);
+	rc = make_efivarfs_path(&path, guid, name);
 	if (rc < 0) {
 		efi_error("make_efivarfs_path failed");
-		return -1;
+		goto err;
 	}
 
-	int fd = -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";
+	}
 
-	if (!access(path, F_OK) && !(attributes & EFI_VARIABLE_APPEND_WRITE)) {
-		rc = efi_del_variable(guid, name);
-		if (rc < 0) {
-			efi_error("efi_del_variable failed");
-			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);
 		}
 	}
-
-	fd = open(path, O_WRONLY|O_CREAT, mode);
 	if (fd < 0) {
-		efi_error("open(%s, O_WRONLY|O_CREAT, mode) failed", path);
+		efi_error("open(%s, %s, %0o) failed", path, flagstr, mode);
 		goto err;
 	}
+
 	efivarfs_set_fd_immutable(fd, 0);
 
 	memcpy(buf, &attributes, sizeof (attributes));
 	memcpy(buf + sizeof (attributes), data, data_size);
 #if 0
-		errno = ENOSPC;
-		rc = -1;
+	errno = ENOSPC;
+	rc = -1;
 #else
-		rc = write(fd, buf, sizeof (attributes) + data_size);
+	rc = write(fd, buf, sizeof (attributes) + data_size);
 #endif
 	if (rc >= 0) {
 		ret = 0;
From 9c51123abebd05d3c62af5da9737bdf8dc0b12a1 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 16 May 2018 19:21:27 +0200
Subject: [PATCH] rewrite efivarfs_set_variable() [9896c26c7e68-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 | 174 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 126 insertions(+), 48 deletions(-)

Index: efivar/src/efivarfs.c
===================================================================
--- efivar.orig/src/efivarfs.c
+++ efivar/src/efivarfs.c
@@ -126,6 +126,21 @@ efivarfs_set_fd_immutable(int fd, int im
 }
 
 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)
 {
 	__typeof__(errno) error = 0;
@@ -287,79 +302,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 (fd < 0) {
-		efi_error("open(%s, %s, %0o) failed", path, flagstr, mode);
+
+		/* 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;
+	}
+
+	/*
+	 * 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;
 	}
-err:
-	errno_value = errno;
 
-	if (path)
-		free(path);
+	/* we're done */
+	ret = 0;
 
-	if (fd >= 0)
-		close(fd);
+err:
+	save_errno = errno;
 
-	errno = errno_value;
+	/* 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);
+
+	ioctl(restore_immutable_fd, FS_IOC_SETFLAGS, &orig_attrs);
+	close(wfd);
+	close(rfd);
+	free(buf);
+	free(path);
+
+	errno = save_errno;
 	return ret;
 }
 
From aa2a584e614fec3a30a30a39c6cea718913c09f4 Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Thu, 7 Jun 2018 16:18:09 -0400
Subject: [PATCH] efivarfs: fix some minor immutability bugs

Signed-off-by: Peter Jones <pjones@redhat.com>
---
 src/efivarfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Places

File efivar-bsc1192344-fix-open-dbx.patch of Package efivar

Places