[PATCH 0/7] firmware validation

[PATCH 0/7] firmware validation

[Kees Cook]

This creates a new LSM hook to validate the contents (and origin) of
component firmware being provided to the kernel from userspace via the
request_firmware() interface.

Additionally creates a test module and test cases for all of the existing
interfaces as well as the new "fd" interface.

-Kees

[PATCH 1/7] doc: fix minor typos in firmware_class README

[Kees Cook]

This is a tiny clean up for typos in the firmware_class README.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/firmware_class/README |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
index 43fada989e65..71f86859d7d8 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -64,7 +64,7 @@
 
 	 if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
 	 	copy_fw_to_device(fw_entry->data, fw_entry->size);
-	 release(fw_entry);
+	 release_firmware(fw_entry);
 
  Sample/simple hotplug script:
  ============================
@@ -74,7 +74,7 @@
 	HOTPLUG_FW_DIR=/usr/lib/hotplug/firmware/
 
 	echo 1 > /sys/$DEVPATH/loading
-	cat $HOTPLUG_FW_DIR/$FIRMWARE > /sysfs/$DEVPATH/data
+	cat $HOTPLUG_FW_DIR/$FIRMWARE > /sys/$DEVPATH/data
 	echo 0 > /sys/$DEVPATH/loading
 
  Random notes:
@@ -123,6 +123,6 @@
  --------------------
  After firmware cache mechanism is introduced during system sleep,
  request_firmware can be called safely inside device's suspend and
- resume callback, and callers need't cache the firmware by
+ resume callback, and callers needn't cache the firmware by
  themselves any more for dealing with firmware loss during system
  resume.
-- 
1.7.9.5

Re: [PATCH 1/7] doc: fix minor typos in firmware_class README

[Ming Lei]

On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
> This is a tiny clean up for typos in the firmware_class README.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,

[PATCH 2/7] test: add firmware_class loader test

[Kees Cook]

This provides a simple interface to trigger the firmware_class loader
to test built-in, filesystem, and user helper modes. Additionally adds
tests via the new interface to the selftests tree.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.debug                                 |   13 +++
 lib/Makefile                                      |    1 +
 lib/test_firmware.c                               |  117 +++++++++++++++++++++
 tools/testing/selftests/Makefile                  |    1 +
 tools/testing/selftests/firmware/Makefile         |   27 +++++
 tools/testing/selftests/firmware/fw_filesystem.sh |   62 +++++++++++
 tools/testing/selftests/firmware/fw_userhelper.sh |   89 ++++++++++++++++
 7 files changed, 310 insertions(+)
 create mode 100644 lib/test_firmware.c
 create mode 100644 tools/testing/selftests/firmware/Makefile
 create mode 100644 tools/testing/selftests/firmware/fw_filesystem.sh
 create mode 100644 tools/testing/selftests/firmware/fw_userhelper.sh

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 7a638aa3545b..213cd9f7e957 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1649,6 +1649,19 @@ config TEST_BPF
 
 	  If unsure, say N.
 
+config TEST_FIRMWARE
+	tristate "Test firmware loading via userspace interface"
+	default n
+	depends on FW_LOADER
+	help
+	  This builds the "test_firmware" module that creates a userspace
+	  interface for testing firmware loading. This can be used to
+	  control the triggering of firmware loading without needing an
+	  actual firmware-using device. The contents can be rechecked by
+	  userspace.
+
+	  If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
diff --git a/lib/Makefile b/lib/Makefile
index ba967a19edba..230b4b1456d6 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -34,6 +34,7 @@ obj-$(CONFIG_TEST_KSTRTOX) += test-kstrtox.o
 obj-$(CONFIG_TEST_MODULE) += test_module.o
 obj-$(CONFIG_TEST_USER_COPY) += test_user_copy.o
 obj-$(CONFIG_TEST_BPF) += test_bpf.o
+obj-$(CONFIG_TEST_FIRMWARE) += test_firmware.o
 
 ifeq ($(CONFIG_DEBUG_KOBJECT),y)
 CFLAGS_kobject.o += -DDEBUG
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
new file mode 100644
index 000000000000..86374c1c49a4
--- /dev/null
+++ b/lib/test_firmware.c
@@ -0,0 +1,117 @@
+/*
+ * This module provides an interface to trigger and test firmware loading.
+ *
+ * It is designed to be used for basic evaluation of the firmware loading
+ * subsystem (for example when validating firmware verification). It lacks
+ * any extra dependencies, and will not normally be loaded by the system
+ * unless explicitly requested by name.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/firmware.h>
+#include <linux/device.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+
+static DEFINE_MUTEX(test_fw_mutex);
+static const struct firmware *test_firmware;
+
+static ssize_t test_fw_misc_read(struct file *f, char __user *buf,
+				 size_t size, loff_t *offset)
+{
+	ssize_t rc = 0;
+
+	mutex_lock(&test_fw_mutex);
+	if (test_firmware)
+		rc = simple_read_from_buffer(buf, size, offset,
+					     test_firmware->data,
+					     test_firmware->size);
+	mutex_unlock(&test_fw_mutex);
+	return rc;
+}
+
+static const struct file_operations test_fw_fops = {
+	.owner          = THIS_MODULE,
+	.read           = test_fw_misc_read,
+};
+
+static struct miscdevice test_fw_misc_device = {
+	.minor          = MISC_DYNAMIC_MINOR,
+	.name           = "test_firmware",
+	.fops           = &test_fw_fops,
+};
+
+static ssize_t trigger_request_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	int rc;
+	char *name;
+
+	name = kzalloc(count + 1, GFP_KERNEL);
+	if (!name)
+		return -ENOSPC;
+	memcpy(name, buf, count);
+
+	pr_info("loading '%s'\n", name);
+
+	mutex_lock(&test_fw_mutex);
+	release_firmware(test_firmware);
+	test_firmware = NULL;
+	rc = request_firmware(&test_firmware, name, dev);
+	if (rc)
+		pr_info("load of '%s' failed: %d\n", name, rc);
+	pr_info("loaded: %zu\n", test_firmware ? test_firmware->size : 0);
+	mutex_unlock(&test_fw_mutex);
+
+	kfree(name);
+
+	return count;
+}
+static DEVICE_ATTR_WO(trigger_request);
+
+static int __init test_firmware_init(void)
+{
+	int rc;
+
+	rc = misc_register(&test_fw_misc_device);
+	if (rc) {
+		pr_err("could not register misc device: %d\n", rc);
+		return rc;
+	}
+	rc = device_create_file(test_fw_misc_device.this_device,
+				&dev_attr_trigger_request);
+	if (rc) {
+		pr_err("could not create sysfs interface: %d\n", rc);
+		goto dereg;
+	}
+
+	pr_warn("interface ready\n");
+
+	return 0;
+dereg:
+	misc_deregister(&test_fw_misc_device);
+	return rc;
+}
+
+module_init(test_firmware_init);
+
+static void __exit test_firmware_exit(void)
+{
+	release_firmware(test_firmware);
+	device_remove_file(test_fw_misc_device.this_device,
+			   &dev_attr_trigger_request);
+	misc_deregister(&test_fw_misc_device);
+	pr_warn("removed interface\n");
+}
+
+module_exit(test_firmware_exit);
+
+MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index e66e710cc595..5c2bf8ec18f3 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -11,6 +11,7 @@ TARGETS += vm
 TARGETS += powerpc
 TARGETS += user
 TARGETS += sysctl
+TARGETS += firmware
 
 all:
 	for TARGET in $(TARGETS); do \
diff --git a/tools/testing/selftests/firmware/Makefile b/tools/testing/selftests/firmware/Makefile
new file mode 100644
index 000000000000..e23cce0bbc3a
--- /dev/null
+++ b/tools/testing/selftests/firmware/Makefile
@@ -0,0 +1,27 @@
+# Makefile for firmware loading selftests
+
+# No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
+all:
+
+fw_filesystem:
+	@if /bin/sh ./fw_filesystem.sh ; then \
+                echo "fw_filesystem: ok"; \
+        else \
+                echo "fw_filesystem: [FAIL]"; \
+                exit 1; \
+        fi
+
+fw_userhelper:
+	@if /bin/sh ./fw_userhelper.sh ; then \
+                echo "fw_userhelper: ok"; \
+        else \
+                echo "fw_userhelper: [FAIL]"; \
+                exit 1; \
+        fi
+
+run_tests: all fw_filesystem fw_userhelper
+
+# Nothing to clean up.
+clean:
+
+.PHONY: all clean run_tests fw_filesystem fw_userhelper
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
new file mode 100644
index 000000000000..3fc6c10c2479
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+# This validates that the kernel will load firmware out of its list of
+# firmware locations on disk. Since the user helper does similar work,
+# we reset the custom load directory to a location the user helper doesn't
+# know so we can be sure we're not accidentally testing the user helper.
+set -e
+
+modprobe test_firmware
+
+DIR=/sys/devices/virtual/misc/test_firmware
+
+OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
+OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
+
+FWPATH=$(mktemp -d)
+FW="$FWPATH/test-firmware.bin"
+
+test_finish()
+{
+	echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
+	echo -n "$OLD_PATH" >/sys/module/firmware_class/parameters/path
+	rm -f "$FW"
+	rmdir "$FWPATH"
+}
+
+trap "test_finish" EXIT
+
+# Turn down the timeout so failures don't take so long.
+echo 1 >/sys/class/firmware/timeout
+# Set the kernel search path.
+echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
+
+# This is an unlikely real-world firmware content. :)
+echo "ABCD0123" >"$FW"
+
+NAME=$(basename "$FW")
+
+# Request a firmware that doesn't exist, it should fail.
+echo -n "nope-$NAME" >"$DIR"/trigger_request
+if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not expected to match" >&2
+	exit 1
+else
+	echo "$0: timeout works"
+fi
+
+# This should succeed via kernel load or will fail after 1 second after
+# being handed over to the user helper, which won't find the fw either.
+if ! echo -n "$NAME" >"$DIR"/trigger_request ; then
+	echo "$0: could not trigger request" >&2
+	exit 1
+fi
+
+# Verify the contents are what we expect.
+if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not loaded" >&2
+	exit 1
+else
+	echo "$0: filesystem loading works"
+fi
+
+exit 0
diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh b/tools/testing/selftests/firmware/fw_userhelper.sh
new file mode 100644
index 000000000000..6efbade12139
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_userhelper.sh
@@ -0,0 +1,89 @@
+#!/bin/sh
+# This validates that the kernel will fall back to using the user helper
+# to load firmware it can't find on disk itself. We must request a firmware
+# that the kernel won't find, and any installed helper (e.g. udev) also
+# won't find so that we can do the load ourself manually.
+set -e
+
+modprobe test_firmware
+
+DIR=/sys/devices/virtual/misc/test_firmware
+
+OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
+
+FWPATH=$(mktemp -d)
+FW="$FWPATH/test-firmware.bin"
+
+test_finish()
+{
+	echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
+	rm -f "$FW"
+	rmdir "$FWPATH"
+}
+
+load_fw()
+{
+	local name="$1"
+	local file="$2"
+
+	# This will block until our load (below) has finished.
+	echo -n "$name" >"$DIR"/trigger_request &
+
+	# Give kernel a chance to react.
+	local timeout=10
+	while [ ! -e "$DIR"/"$name"/loading ]; do
+		sleep 0.1
+		timeout=$(( $timeout - 1 ))
+		if [ "$timeout" -eq 0 ]; then
+			echo "$0: firmware interface never appeared" >&2
+			exit 1
+		fi
+	done
+
+	echo 1 >"$DIR"/"$name"/loading
+	cat "$file" >"$DIR"/"$name"/data
+	echo 0 >"$DIR"/"$name"/loading
+
+	# Wait for request to finish.
+	wait
+}
+
+trap "test_finish" EXIT
+
+# This is an unlikely real-world firmware content. :)
+echo "ABCD0123" >"$FW"
+NAME=$(basename "$FW")
+
+# Test failure when doing nothing (timeout works).
+echo 1 >/sys/class/firmware/timeout
+echo -n "$NAME" >"$DIR"/trigger_request
+if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not expected to match" >&2
+	exit 1
+else
+	echo "$0: timeout works"
+fi
+
+# Put timeout high enough for us to do work but not so long that failures
+# slow down this test too much.
+echo 4 >/sys/class/firmware/timeout
+
+# Load this script instead of the desired firmware.
+load_fw "$NAME" "$0"
+if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not expected to match" >&2
+	exit 1
+else
+	echo "$0: firmware comparison works"
+fi
+
+# Do a proper load, which should work correctly.
+load_fw "$NAME" "$FW"
+if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not loaded" >&2
+	exit 1
+else
+	echo "$0: user helper firmware loading works"
+fi
+
+exit 0
-- 
1.7.9.5

Re: [PATCH 2/7] test: add firmware_class loader test

[Greg Kroah-Hartman]

On Mon, Jul 14, 2014 at 02:38:12PM -0700, Kees Cook wrote:
> This provides a simple interface to trigger the firmware_class loader
> to test built-in, filesystem, and user helper modes. Additionally adds
> tests via the new interface to the selftests tree.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  lib/Kconfig.debug                                 |   13 +++
>  lib/Makefile                                      |    1 +
>  lib/test_firmware.c                               |  117 +++++++++++++++++++++

Side note, I find it odd that in-kernel test modules are in lib/.

But whatever, you aren't the one that did this first, so I'm not going
to object here, I'll apply this, nice work.

thanks,

greg k-h

Re: [PATCH 2/7] test: add firmware_class loader test

[Kees Cook]

On Thu, Jul 17, 2014 at 6:47 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 14, 2014 at 02:38:12PM -0700, Kees Cook wrote:
>> This provides a simple interface to trigger the firmware_class loader
>> to test built-in, filesystem, and user helper modes. Additionally adds
>> tests via the new interface to the selftests tree.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  lib/Kconfig.debug                                 |   13 +++
>>  lib/Makefile                                      |    1 +
>>  lib/test_firmware.c                               |  117 +++++++++++++++++++++
>
> Side note, I find it odd that in-kernel test modules are in lib/.
>
> But whatever, you aren't the one that did this first, so I'm not going
> to object here, I'll apply this, nice work.

Yeah, with each new selftest, I become less and less happy with the
lack of an intentional layout and execution framework. I've been
pondering some ideas on cleaning it up, but I have nothing solid yet.

The main reason for things living lib/ is because of how the
Kconfig.debug stuff is layed out. I think separating the concept of
"debug" from "test" is needed, and then the selftests make targets
need to be sorted out so that there's some common reporting method of
success vs failure, and that failures don't abort the whole thing,
etc, etc.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 2/7] test: add firmware_class loader test

[Sasha Levin]

On 07/14/2014 05:38 PM, Kees Cook wrote:
> This provides a simple interface to trigger the firmware_class loader
> to test built-in, filesystem, and user helper modes. Additionally adds
> tests via the new interface to the selftests tree.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

Hi folks,

While fuzzing with trinity inside a KVM tools guest running the latest -next
kernel, I've stumbled on the following spew:

[  413.999779] misc test_firmware: Falling back to user helper
[  414.003728] WARNING: CPU: 25 PID: 9860 at lib/kobject.c:209 kobject_add_internal+0x3a3/0x450()
[  414.006815] kobject: (ffff8807b4ef5dc8): attempted to be registered with empty name!
[  414.009482] Modules linked in:
[  414.010818] CPU: 25 PID: 9860 Comm: trinity-c662 Tainted: G        W      3.17.0-rc5-next-20140917-sasha-00041-gd01267b #1198
[  414.014626]  0000000000000009 ffff880236547a68 ffffffff9d56d655 0000000000000000
[  414.017585]  ffff880236547ab8 ffff880236547aa8 ffffffff9a15f3d1 00000000001e3540
[  414.020733]  ffff8807b4ef5dc8 00000000ffffffea ffff88043d114108 ffff8807b4ef5db8
[  414.023345] Call Trace:
[  414.024254] dump_stack (lib/dump_stack.c:52)
[  414.026232] warn_slowpath_common (kernel/panic.c:432)
[  414.028355] warn_slowpath_fmt (kernel/panic.c:446)
[  414.030366] ? __raw_spin_lock_init (kernel/locking/spinlock_debug.c:24)
[  414.032369] kobject_add_internal (lib/kobject.c:211 (discriminator 1))
[  414.034313] ? __dynamic_pr_debug (lib/dynamic_debug.c:561)
[  414.036254] kobject_add (lib/kobject.c:403)
[  414.038017] ? device_private_init (drivers/base/core.c:929)
[  414.040340] ? klist_init (lib/klist.c:90)
[  414.044729] device_add (drivers/base/core.c:1010)
[  414.049854] ? dev_set_name (drivers/base/core.c:876)
[  414.060790] _request_firmware (drivers/base/firmware_class.c:892 drivers/base/firmware_class.c:957 drivers/base/firmware_class.c:1139)
[  414.062814] request_firmware (drivers/base/firmware_class.c:1189)
[  414.064796] trigger_request_store (lib/test_firmware.c:68)
[  414.066774] dev_attr_store (drivers/base/core.c:138)
[  414.068531] sysfs_kf_write (fs/sysfs/file.c:115)
[  414.070826] kernfs_fop_write (fs/kernfs/file.c:308)
[  414.074648] ? kernfs_vma_page_mkwrite (fs/kernfs/file.c:267)
[  414.076864] do_loop_readv_writev (fs/read_write.c:710)
[  414.078947] ? kernfs_vma_page_mkwrite (fs/kernfs/file.c:267)
[  414.081269] do_readv_writev (fs/read_write.c:842)
[  414.083281] ? preempt_count_sub (kernel/sched/core.c:2634)
[  414.085380] ? mutex_lock_nested (./arch/x86/include/asm/preempt.h:98 kernel/locking/mutex.c:599 kernel/locking/mutex.c:616)
[  414.087401] ? __fdget_pos (fs/file.c:714)
[  414.089286] vfs_writev (fs/read_write.c:881)
[  414.091243] SyS_writev (fs/read_write.c:914 fs/read_write.c:905)
[  414.093053] tracesys_phase2 (arch/x86/kernel/entry_64.S:529)


Thanks,
Sasha

Re: [PATCH 2/7] test: add firmware_class loader test

[Kees Cook]

On Wed, Sep 17, 2014 at 6:51 PM, Sasha Levin <sasha.levin@oracle.com> wrote:
> On 07/14/2014 05:38 PM, Kees Cook wrote:
>> This provides a simple interface to trigger the firmware_class loader
>> to test built-in, filesystem, and user helper modes. Additionally adds
>> tests via the new interface to the selftests tree.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Hi folks,
>
> While fuzzing with trinity inside a KVM tools guest running the latest -next
> kernel, I've stumbled on the following spew:
>
> [  413.999779] misc test_firmware: Falling back to user helper

Do you have the log above this? It looks like trinity wrote to
/dev/test_firmware that the test_firmware.ko module creates. I'm
assuming it wrote 0 bytes based on the trace below...

> [  414.003728] WARNING: CPU: 25 PID: 9860 at lib/kobject.c:209 kobject_add_internal+0x3a3/0x450()
> [  414.006815] kobject: (ffff8807b4ef5dc8): attempted to be registered with empty name!
> [  414.009482] Modules linked in:
> [  414.010818] CPU: 25 PID: 9860 Comm: trinity-c662 Tainted: G        W      3.17.0-rc5-next-20140917-sasha-00041-gd01267b #1198
> [  414.014626]  0000000000000009 ffff880236547a68 ffffffff9d56d655 0000000000000000
> [  414.017585]  ffff880236547ab8 ffff880236547aa8 ffffffff9a15f3d1 00000000001e3540
> [  414.020733]  ffff8807b4ef5dc8 00000000ffffffea ffff88043d114108 ffff8807b4ef5db8
> [  414.023345] Call Trace:
> [  414.024254] dump_stack (lib/dump_stack.c:52)
> [  414.026232] warn_slowpath_common (kernel/panic.c:432)
> [  414.028355] warn_slowpath_fmt (kernel/panic.c:446)
> [  414.030366] ? __raw_spin_lock_init (kernel/locking/spinlock_debug.c:24)
> [  414.032369] kobject_add_internal (lib/kobject.c:211 (discriminator 1))
> [  414.034313] ? __dynamic_pr_debug (lib/dynamic_debug.c:561)
> [  414.036254] kobject_add (lib/kobject.c:403)
> [  414.038017] ? device_private_init (drivers/base/core.c:929)
> [  414.040340] ? klist_init (lib/klist.c:90)
> [  414.044729] device_add (drivers/base/core.c:1010)
> [  414.049854] ? dev_set_name (drivers/base/core.c:876)
> [  414.060790] _request_firmware (drivers/base/firmware_class.c:892 drivers/base/firmware_class.c:957 drivers/base/firmware_class.c:1139)

dev_set_name with an empty string should only be possible via an empty
firmware name via the test module interface. I think _request_firmware
should grow a check for !name or name[0] == '\0' in addition to the
!firmware_p check it already does.

Thoughts?

-Kees

> [  414.062814] request_firmware (drivers/base/firmware_class.c:1189)
> [  414.064796] trigger_request_store (lib/test_firmware.c:68)
> [  414.066774] dev_attr_store (drivers/base/core.c:138)
> [  414.068531] sysfs_kf_write (fs/sysfs/file.c:115)
> [  414.070826] kernfs_fop_write (fs/kernfs/file.c:308)
> [  414.074648] ? kernfs_vma_page_mkwrite (fs/kernfs/file.c:267)
> [  414.076864] do_loop_readv_writev (fs/read_write.c:710)
> [  414.078947] ? kernfs_vma_page_mkwrite (fs/kernfs/file.c:267)
> [  414.081269] do_readv_writev (fs/read_write.c:842)
> [  414.083281] ? preempt_count_sub (kernel/sched/core.c:2634)
> [  414.085380] ? mutex_lock_nested (./arch/x86/include/asm/preempt.h:98 kernel/locking/mutex.c:599 kernel/locking/mutex.c:616)
> [  414.087401] ? __fdget_pos (fs/file.c:714)
> [  414.089286] vfs_writev (fs/read_write.c:881)
> [  414.091243] SyS_writev (fs/read_write.c:914 fs/read_write.c:905)
> [  414.093053] tracesys_phase2 (arch/x86/kernel/entry_64.S:529)
>
>
> Thanks,
> Sasha



-- 
Kees Cook
Chrome OS Security

[PATCH 3/7] security: introduce kernel_fw_from_file hook

[Kees Cook]

In order to validate the contents of firmware being loaded, there must be
a hook to evaluate any loaded firmware that wasn't built into the kernel
itself. Without this, there is a risk that a root user could load malicious
firmware designed to mount an attack against kernel memory (e.g. via DMA).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/security.h |   16 ++++++++++++++++
 security/capability.c    |    6 ++++++
 security/security.c      |    6 ++++++
 3 files changed, 28 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index 9c6b9722ff48..dbb80b3e99d7 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -702,6 +702,14 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@inode points to the inode to use as a reference.
  *	The current task must be the one that nominated @inode.
  *	Return 0 if successful.
+ * @kernel_fw_from_file:
+ *	Load firmware from userspace.
+ *	@file contains the file structure pointing to the file containing
+ *	the firmware to load. If the module is being loaded from a blob,
+ *	this argument will be NULL.
+ *	@buf pointer to buffer containing firmware contents.
+ *	@size length of the firmware contents.
+ *	Return 0 if permission is granted.
  * @kernel_module_request:
  *	Ability to trigger the kernel to automatically upcall to userspace for
  *	userspace to load a kernel module with the given name.
@@ -1565,6 +1573,7 @@ struct security_operations {
 	void (*cred_transfer)(struct cred *new, const struct cred *old);
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
+	int (*kernel_fw_from_file)(struct file *file, char *buf, size_t size);
 	int (*kernel_module_request)(char *kmod_name);
 	int (*kernel_module_from_file)(struct file *file);
 	int (*task_fix_setuid) (struct cred *new, const struct cred *old,
@@ -1837,6 +1846,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
 void security_transfer_creds(struct cred *new, const struct cred *old);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
+int security_kernel_fw_from_file(struct file *file, char *buf, size_t size);
 int security_kernel_module_request(char *kmod_name);
 int security_kernel_module_from_file(struct file *file);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
@@ -2363,6 +2373,12 @@ static inline int security_kernel_create_files_as(struct cred *cred,
 	return 0;
 }
 
+static inline int security_kernel_fw_from_file(struct file *file,
+					       char *buf, size_t size)
+{
+	return 0;
+}
+
 static inline int security_kernel_module_request(char *kmod_name)
 {
 	return 0;
diff --git a/security/capability.c b/security/capability.c
index e76373de3129..a74fde6a7468 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -401,6 +401,11 @@ static int cap_kernel_create_files_as(struct cred *new, struct inode *inode)
 	return 0;
 }
 
+static int cap_kernel_fw_from_file(struct file *file, char *buf, size_t size)
+{
+	return 0;
+}
+
 static int cap_kernel_module_request(char *kmod_name)
 {
 	return 0;
@@ -1015,6 +1020,7 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, cred_transfer);
 	set_to_cap_if_null(ops, kernel_act_as);
 	set_to_cap_if_null(ops, kernel_create_files_as);
+	set_to_cap_if_null(ops, kernel_fw_from_file);
 	set_to_cap_if_null(ops, kernel_module_request);
 	set_to_cap_if_null(ops, kernel_module_from_file);
 	set_to_cap_if_null(ops, task_fix_setuid);
diff --git a/security/security.c b/security/security.c
index 31614e9e96e5..35d37d0f0d49 100644
--- a/security/security.c
+++ b/security/security.c
@@ -845,6 +845,12 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
 	return security_ops->kernel_create_files_as(new, inode);
 }
 
+int security_kernel_fw_from_file(struct file *file, char *buf, size_t size)
+{
+	return security_ops->kernel_fw_from_file(file, buf, size);
+}
+EXPORT_SYMBOL_GPL(security_kernel_fw_from_file);
+
 int security_kernel_module_request(char *kmod_name)
 {
 	return security_ops->kernel_module_request(kmod_name);
-- 
1.7.9.5

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Luis R. Rodriguez]

On Mon, Jul 14, 2014 at 02:38:13PM -0700, Kees Cook wrote:
> +int security_kernel_fw_from_file(struct file *file, char *buf, size_t size)
> +{
> +	return security_ops->kernel_fw_from_file(file, buf, size);
> +}
> +EXPORT_SYMBOL_GPL(security_kernel_fw_from_file);

I have a use case in mind already (CRDA) whereby a non-driver but a core part of
the kernel would like to use the request_firmware_direct() API and leave
it optional to that part of the kernel if or not a digital signature check
is required since this is already an optional feature. Since LSM would be
used for the digital signatures it it'd be good if we allowed drivers and
the kernel to specify whether or not a the LSM signature hook does indeed
need to be used.

Additionally there may be different signature requirements on the file, one
might be willing to *require* a digital signature check even if say dm-verity
was used, one use case here is wanting the to require as part of the
specification a one to one mapping of file --> signature. That may be a
bit paranoid... but its certainly a possibility. This could be addressed by
allowing a user to express whether one security mechanism is sufficient
for an expected one, or if one is definitely required. One example here
could be the ability for a driver / kernel to express that dm-verity would
not suffice for a request_firmware_direct() call. I'm not saying that I
*know* this use case exists, am just saying I can expect it and do consider
it subjective to assume we'd agree on possible security intersections.

I see this also adds a hook for fw itself, meanwhile we already had one for
modules. The request_firmware() API is already used on other areas of the
kernel for non-firmware files, it can be used to load optional CPU microcode
upgrade, EEPROM optional override files, and in the wireless case I'd like to
see if we could replace CRDA by using the same APIs / mechanisms as well to
avoid having to deal with complexities on parsers with CFG80211_INTERNAL_REGDB.
Besides nomenclature fuzz as usage of request_firmware() API grows it occurs to
me that perhaps these hooks could be generalized at the kobject granularity
level. That should make it easier to scale and consider these LSM hooks for any
type of other object in the kernel in the future. kobjects may be overkill
though, unless we could enable the option to have have these hooks only
optional for certain type of kobjects.

  Luis

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Kees Cook]

On Mon, Jul 14, 2014 at 3:24 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Mon, Jul 14, 2014 at 02:38:13PM -0700, Kees Cook wrote:
>> +int security_kernel_fw_from_file(struct file *file, char *buf, size_t size)
>> +{
>> +     return security_ops->kernel_fw_from_file(file, buf, size);
>> +}
>> +EXPORT_SYMBOL_GPL(security_kernel_fw_from_file);
>
> I have a use case in mind already (CRDA) whereby a non-driver but a core part of
> the kernel would like to use the request_firmware_direct() API and leave
> it optional to that part of the kernel if or not a digital signature check
> is required since this is already an optional feature. Since LSM would be
> used for the digital signatures it it'd be good if we allowed drivers and
> the kernel to specify whether or not a the LSM signature hook does indeed
> need to be used.
>
> Additionally there may be different signature requirements on the file, one
> might be willing to *require* a digital signature check even if say dm-verity
> was used, one use case here is wanting the to require as part of the
> specification a one to one mapping of file --> signature. That may be a
> bit paranoid... but its certainly a possibility. This could be addressed by
> allowing a user to express whether one security mechanism is sufficient
> for an expected one, or if one is definitely required. One example here
> could be the ability for a driver / kernel to express that dm-verity would
> not suffice for a request_firmware_direct() call. I'm not saying that I
> *know* this use case exists, am just saying I can expect it and do consider
> it subjective to assume we'd agree on possible security intersections.

The policy of accepting the firmware or not is up to the loaded LSM
(so it can choose how to either check that the fd is tied to
dm-verity, or that a signature maps to the right key or key, or if IMA
thinks the file is unchanged, etc).

And example of LSM policy for kernel modules is my proposed kernel
module restriction LSM:
https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=lsm-mod-pin

With fw-restriction in place, I would likely extend this LSM to add a
check to the fw hook too.

> I see this also adds a hook for fw itself, meanwhile we already had one for
> modules. The request_firmware() API is already used on other areas of the
> kernel for non-firmware files, it can be used to load optional CPU microcode
> upgrade, EEPROM optional override files, and in the wireless case I'd like to
> see if we could replace CRDA by using the same APIs / mechanisms as well to
> avoid having to deal with complexities on parsers with CFG80211_INTERNAL_REGDB.
> Besides nomenclature fuzz as usage of request_firmware() API grows it occurs to
> me that perhaps these hooks could be generalized at the kobject granularity
> level. That should make it easier to scale and consider these LSM hooks for any
> type of other object in the kernel in the future. kobjects may be overkill
> though, unless we could enable the option to have have these hooks only
> optional for certain type of kobjects.

Yup, with this and the module hook, adding a similar hook for kexec
makes sense as well. A paranoid kernel doesn't want to trust anything
it's loading from userspace. :)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Luis R. Rodriguez]

On Mon, Jul 14, 2014 at 3:31 PM, Kees Cook <keescook@chromium.org> wrote:
> On Mon, Jul 14, 2014 at 3:24 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Mon, Jul 14, 2014 at 02:38:13PM -0700, Kees Cook wrote:
>>> +int security_kernel_fw_from_file(struct file *file, char *buf, size_t size)
>>> +{
>>> +     return security_ops->kernel_fw_from_file(file, buf, size);
>>> +}
>>> +EXPORT_SYMBOL_GPL(security_kernel_fw_from_file);
>>
>> I have a use case in mind already (CRDA) whereby a non-driver but a core part of
>> the kernel would like to use the request_firmware_direct() API and leave
>> it optional to that part of the kernel if or not a digital signature check
>> is required since this is already an optional feature. Since LSM would be
>> used for the digital signatures it it'd be good if we allowed drivers and
>> the kernel to specify whether or not a the LSM signature hook does indeed
>> need to be used.
>>
>> Additionally there may be different signature requirements on the file, one
>> might be willing to *require* a digital signature check even if say dm-verity
>> was used, one use case here is wanting the to require as part of the
>> specification a one to one mapping of file --> signature. That may be a
>> bit paranoid... but its certainly a possibility. This could be addressed by
>> allowing a user to express whether one security mechanism is sufficient
>> for an expected one, or if one is definitely required. One example here
>> could be the ability for a driver / kernel to express that dm-verity would
>> not suffice for a request_firmware_direct() call. I'm not saying that I
>> *know* this use case exists, am just saying I can expect it and do consider
>> it subjective to assume we'd agree on possible security intersections.
>
> The policy of accepting the firmware or not is up to the loaded LSM
> (so it can choose how to either check that the fd is tied to
> dm-verity, or that a signature maps to the right key or key, or if IMA
> thinks the file is unchanged, etc).
>
> And example of LSM policy for kernel modules is my proposed kernel
> module restriction LSM:
> https://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=lsm-mod-pin
>
> With fw-restriction in place, I would likely extend this LSM to add a
> check to the fw hook too.

That would be great. I'll go review that code next.

>> I see this also adds a hook for fw itself, meanwhile we already had one for
>> modules. The request_firmware() API is already used on other areas of the
>> kernel for non-firmware files, it can be used to load optional CPU microcode
>> upgrade, EEPROM optional override files, and in the wireless case I'd like to
>> see if we could replace CRDA by using the same APIs / mechanisms as well to
>> avoid having to deal with complexities on parsers with CFG80211_INTERNAL_REGDB.
>> Besides nomenclature fuzz as usage of request_firmware() API grows it occurs to
>> me that perhaps these hooks could be generalized at the kobject granularity
>> level. That should make it easier to scale and consider these LSM hooks for any
>> type of other object in the kernel in the future. kobjects may be overkill
>> though, unless we could enable the option to have have these hooks only
>> optional for certain type of kobjects.
>
> Yup, with this and the module hook, adding a similar hook for kexec
> makes sense as well. A paranoid kernel doesn't want to trust anything
> it's loading from userspace. :)

Well I'm actually wondering if we could generalize requiring hooks or
not for LSM as part of the general kobject definition. Then we
wouldn't need to keep growing hooks for modules, firmware, kexec
images, etc, but instead using the interfaces for kobjects and who
depend on them. How that would actually look -- I'm not sure, but just
a thought.

  Luis

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Kees Cook]

On Tue, Jul 22, 2014 at 12:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Mon, Jul 14, 2014 at 3:31 PM, Kees Cook <keescook@chromium.org> wrote:
>> Yup, with this and the module hook, adding a similar hook for kexec
>> makes sense as well. A paranoid kernel doesn't want to trust anything
>> it's loading from userspace. :)
>
> Well I'm actually wondering if we could generalize requiring hooks or
> not for LSM as part of the general kobject definition. Then we
> wouldn't need to keep growing hooks for modules, firmware, kexec
> images, etc, but instead using the interfaces for kobjects and who
> depend on them. How that would actually look -- I'm not sure, but just
> a thought.

Yeah, there does seem to be a repeated "get a thing from userspace"
method here, but the interfaces have been rather scattered so far. I
haven't seen an obvious way to consolidate them yet.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Luis R. Rodriguez]

On Tue, Jul 22, 2014 at 1:55 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jul 22, 2014 at 12:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Mon, Jul 14, 2014 at 3:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>> Yup, with this and the module hook, adding a similar hook for kexec
>>> makes sense as well. A paranoid kernel doesn't want to trust anything
>>> it's loading from userspace. :)
>>
>> Well I'm actually wondering if we could generalize requiring hooks or
>> not for LSM as part of the general kobject definition. Then we
>> wouldn't need to keep growing hooks for modules, firmware, kexec
>> images, etc, but instead using the interfaces for kobjects and who
>> depend on them. How that would actually look -- I'm not sure, but just
>> a thought.
>
> Yeah, there does seem to be a repeated "get a thing from userspace"
> method here, but the interfaces have been rather scattered so far. I
> haven't seen an obvious way to consolidate them yet.

If we are going to be adding a new system call for each type of
userspace object to help LSMs with requirements I wonder if its a
worthy endeavor to review. This series didn't add one but you had
mentioned finit_module() for example, are we going to want one for
finit_firmware() finit_kexec_image(), etc?

 Luis

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Kees Cook]

On Tue, Jul 22, 2014 at 3:11 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Tue, Jul 22, 2014 at 1:55 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Tue, Jul 22, 2014 at 12:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>> On Mon, Jul 14, 2014 at 3:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>>> Yup, with this and the module hook, adding a similar hook for kexec
>>>> makes sense as well. A paranoid kernel doesn't want to trust anything
>>>> it's loading from userspace. :)
>>>
>>> Well I'm actually wondering if we could generalize requiring hooks or
>>> not for LSM as part of the general kobject definition. Then we
>>> wouldn't need to keep growing hooks for modules, firmware, kexec
>>> images, etc, but instead using the interfaces for kobjects and who
>>> depend on them. How that would actually look -- I'm not sure, but just
>>> a thought.
>>
>> Yeah, there does seem to be a repeated "get a thing from userspace"
>> method here, but the interfaces have been rather scattered so far. I
>> haven't seen an obvious way to consolidate them yet.
>
> If we are going to be adding a new system call for each type of
> userspace object to help LSMs with requirements I wonder if its a
> worthy endeavor to review. This series didn't add one but you had
> mentioned finit_module() for example, are we going to want one for
> finit_firmware() finit_kexec_image(), etc?

The kernel pulls in firmware directly from the filesystem, so no
syscall there. :) kexec just had kexec_load_file added as a syscall
(it takes 2 fds: kernel and initrd).

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Luis R. Rodriguez]

On Tue, Jul 22, 2014 at 3:20 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Jul 22, 2014 at 3:11 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> On Tue, Jul 22, 2014 at 1:55 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Tue, Jul 22, 2014 at 12:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>>>> On Mon, Jul 14, 2014 at 3:31 PM, Kees Cook <keescook@chromium.org> wrote:
>>>>> Yup, with this and the module hook, adding a similar hook for kexec
>>>>> makes sense as well. A paranoid kernel doesn't want to trust anything
>>>>> it's loading from userspace. :)
>>>>
>>>> Well I'm actually wondering if we could generalize requiring hooks or
>>>> not for LSM as part of the general kobject definition. Then we
>>>> wouldn't need to keep growing hooks for modules, firmware, kexec
>>>> images, etc, but instead using the interfaces for kobjects and who
>>>> depend on them. How that would actually look -- I'm not sure, but just
>>>> a thought.
>>>
>>> Yeah, there does seem to be a repeated "get a thing from userspace"
>>> method here, but the interfaces have been rather scattered so far. I
>>> haven't seen an obvious way to consolidate them yet.
>>
>> If we are going to be adding a new system call for each type of
>> userspace object to help LSMs with requirements I wonder if its a
>> worthy endeavor to review. This series didn't add one but you had
>> mentioned finit_module() for example, are we going to want one for
>> finit_firmware() finit_kexec_image(), etc?
>
> The kernel pulls in firmware directly from the filesystem, so no
> syscall there. :) kexec just had kexec_load_file added as a syscall
> (it takes 2 fds: kernel and initrd).

OK, too late then now. Look forward to the rest of the stuff then.

 Luis

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[One Thousand Gnomes]

> Yeah, there does seem to be a repeated "get a thing from userspace"
> method here, but the interfaces have been rather scattered so far. I
> haven't seen an obvious way to consolidate them yet.

Sign them all with the kernel key just as we do now with modules.

Alan

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Greg Kroah-Hartman]

On Mon, Jul 14, 2014 at 02:38:13PM -0700, Kees Cook wrote:
> In order to validate the contents of firmware being loaded, there must be
> a hook to evaluate any loaded firmware that wasn't built into the kernel
> itself. Without this, there is a risk that a root user could load malicious
> firmware designed to mount an attack against kernel memory (e.g. via DMA).
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/security.h |   16 ++++++++++++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    6 ++++++
>  3 files changed, 28 insertions(+)

I would like an ack from a security developer/maintainer before applying
this patch...

thanks,

greg k-h

Re: [PATCH 3/7] security: introduce kernel_fw_from_file hook

[Mimi Zohar]

On Mon, 2014-07-14 at 14:38 -0700, Kees Cook wrote: 
> In order to validate the contents of firmware being loaded, there must be
> a hook to evaluate any loaded firmware that wasn't built into the kernel
> itself. Without this, there is a risk that a root user could load malicious
> firmware designed to mount an attack against kernel memory (e.g. via DMA).
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/security.h |   16 ++++++++++++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    6 ++++++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 9c6b9722ff48..dbb80b3e99d7 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -702,6 +702,14 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	@inode points to the inode to use as a reference.
>   *	The current task must be the one that nominated @inode.
>   *	Return 0 if successful.
> + * @kernel_fw_from_file:
> + *	Load firmware from userspace.
> + *	@file contains the file structure pointing to the file containing
> + *	the firmware to load. If the module is being loaded from a blob,
> + *	this argument will be NULL.
> + *	@buf pointer to buffer containing firmware contents.
> + *	@size length of the firmware contents.
> + *	Return 0 if permission is granted.
>   * @kernel_module_request:
>   *	Ability to trigger the kernel to automatically upcall to userspace for
>   *	userspace to load a kernel module with the given name.
> @@ -1565,6 +1573,7 @@ struct security_operations {
>  	void (*cred_transfer)(struct cred *new, const struct cred *old);
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> +	int (*kernel_fw_from_file)(struct file *file, char *buf, size_t size);
>  	int (*kernel_module_request)(char *kmod_name);
>  	int (*kernel_module_from_file)(struct file *file);
>  	int (*task_fix_setuid) (struct cred *new, const struct cred *old,
> @@ -1837,6 +1846,7 @@ int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp);
>  void security_transfer_creds(struct cred *new, const struct cred *old);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> +int security_kernel_fw_from_file(struct file *file, char *buf, size_t size);
>  int security_kernel_module_request(char *kmod_name);
>  int security_kernel_module_from_file(struct file *file);
>  int security_task_fix_setuid(struct cred *new, const struct cred *old,
> @@ -2363,6 +2373,12 @@ static inline int security_kernel_create_files_as(struct cred *cred,
>  	return 0;
>  }
> 
> +static inline int security_kernel_fw_from_file(struct file *file,
> +					       char *buf, size_t size)
> +{
> +	return 0;
> +}
> +
>  static inline int security_kernel_module_request(char *kmod_name)
>  {
>  	return 0;
> diff --git a/security/capability.c b/security/capability.c
> index e76373de3129..a74fde6a7468 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -401,6 +401,11 @@ static int cap_kernel_create_files_as(struct cred *new, struct inode *inode)
>  	return 0;
>  }
> 
> +static int cap_kernel_fw_from_file(struct file *file, char *buf, size_t size)
> +{
> +	return 0;
> +}
> +
>  static int cap_kernel_module_request(char *kmod_name)
>  {
>  	return 0;
> @@ -1015,6 +1020,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, cred_transfer);
>  	set_to_cap_if_null(ops, kernel_act_as);
>  	set_to_cap_if_null(ops, kernel_create_files_as);
> +	set_to_cap_if_null(ops, kernel_fw_from_file);
>  	set_to_cap_if_null(ops, kernel_module_request);
>  	set_to_cap_if_null(ops, kernel_module_from_file);
>  	set_to_cap_if_null(ops, task_fix_setuid);
> diff --git a/security/security.c b/security/security.c
> index 31614e9e96e5..35d37d0f0d49 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -845,6 +845,12 @@ int security_kernel_create_files_as(struct cred *new, struct inode *inode)
>  	return security_ops->kernel_create_files_as(new, inode);
>  }
> 
> +int security_kernel_fw_from_file(struct file *file, char *buf, size_t size)
> +{
> +	return security_ops->kernel_fw_from_file(file, buf, size);
> +}
> +EXPORT_SYMBOL_GPL(security_kernel_fw_from_file);
> +

Nice set of patches!  I'll need to send you a patch to call IMA.

Mimi

>  int security_kernel_module_request(char *kmod_name)
>  {
>  	return security_ops->kernel_module_request(kmod_name);

[PATCH 4/7] firmware_class: perform new LSM checks

[Kees Cook]

This attaches LSM hooks to the existing firmware loading interfaces:
filesystem-found firmware and demand-loaded blobs.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_class.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index d276e33880be..7399bab71ced 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -28,6 +28,7 @@
 #include <linux/suspend.h>
 #include <linux/syscore_ops.h>
 #include <linux/reboot.h>
+#include <linux/security.h>
 
 #include <generated/utsrelease.h>
 
@@ -308,12 +309,17 @@ static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
 	if (rc != size) {
 		if (rc > 0)
 			rc = -EIO;
-		vfree(buf);
-		return rc;
+		goto fail;
 	}
+	rc = security_kernel_fw_from_file(file, buf, size);
+	if (rc)
+		goto fail;
 	fw_buf->data = buf;
 	fw_buf->size = size;
 	return 0;
+fail:
+	vfree(buf);
+	return rc;
 }
 
 static int fw_get_filesystem_firmware(struct device *device,
@@ -640,6 +646,12 @@ static ssize_t firmware_loading_store(struct device *dev,
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
+			if (security_kernel_fw_from_file(NULL, fw_buf->data,
+							 fw_buf->size)) {
+				fw_load_abort(fw_priv);
+				break;
+			}
+
 			set_bit(FW_STATUS_DONE, &fw_buf->status);
 			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
 
-- 
1.7.9.5

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[James Morris]

On Mon, 14 Jul 2014, Kees Cook wrote:

> This attaches LSM hooks to the existing firmware loading interfaces:
> filesystem-found firmware and demand-loaded blobs.

>  static int fw_get_filesystem_firmware(struct device *device,
> @@ -640,6 +646,12 @@ static ssize_t firmware_loading_store(struct device *dev,
>  		break;
>  	case 0:
>  		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> +			if (security_kernel_fw_from_file(NULL, fw_buf->data,
> +							 fw_buf->size)) {
> +				fw_load_abort(fw_priv);
> +				break;
> +			}
> +
>  			set_bit(FW_STATUS_DONE, &fw_buf->status);
>  			clear_bit(FW_STATUS_LOADING, &fw_buf->status);
>  
> 

Can you explain the loading store, and what the semantics are for an LSM 
when a NULL is passed as the file?


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[Kees Cook]

On Thu, Jul 17, 2014 at 8:41 PM, James Morris <jmorris@namei.org> wrote:
> On Mon, 14 Jul 2014, Kees Cook wrote:
>
>> This attaches LSM hooks to the existing firmware loading interfaces:
>> filesystem-found firmware and demand-loaded blobs.
>
>>  static int fw_get_filesystem_firmware(struct device *device,
>> @@ -640,6 +646,12 @@ static ssize_t firmware_loading_store(struct device *dev,
>>               break;
>>       case 0:
>>               if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
>> +                     if (security_kernel_fw_from_file(NULL, fw_buf->data,
>> +                                                      fw_buf->size)) {
>> +                             fw_load_abort(fw_priv);
>> +                             break;
>> +                     }
>> +
>>                       set_bit(FW_STATUS_DONE, &fw_buf->status);
>>                       clear_bit(FW_STATUS_LOADING, &fw_buf->status);
>>
>>
>
> Can you explain the loading store, and what the semantics are for an LSM
> when a NULL is passed as the file?

I'm not sure what you mean by "loading store"?

When NULL is passed as the file, it means that the firmware was passes
a blob, and there is no file backing it:

+ * @kernel_fw_from_file:
+ *     Load firmware from userspace.
+ *     @file contains the file structure pointing to the file containing
+ *     the firmware to load. If the module is being loaded from a blob,
+ *     this argument will be NULL.
+ *     @buf pointer to buffer containing firmware contents.
+ *     @size length of the firmware contents.
+ *     Return 0 if permission is granted.

An LSM that has a policy to require a file to back the firmware would
reject such loads.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[James Morris]

On Fri, 18 Jul 2014, Kees Cook wrote:

> On Thu, Jul 17, 2014 at 8:41 PM, James Morris <jmorris@namei.org> wrote:
> > On Mon, 14 Jul 2014, Kees Cook wrote:
> >
> >> This attaches LSM hooks to the existing firmware loading interfaces:
> >> filesystem-found firmware and demand-loaded blobs.
> >
> >>  static int fw_get_filesystem_firmware(struct device *device,
> >> @@ -640,6 +646,12 @@ static ssize_t firmware_loading_store(struct device *dev,
> >>               break;
> >>       case 0:
> >>               if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> >> +                     if (security_kernel_fw_from_file(NULL, fw_buf->data,
> >> +                                                      fw_buf->size)) {
> >> +                             fw_load_abort(fw_priv);
> >> +                             break;
> >> +                     }
> >> +
> >>                       set_bit(FW_STATUS_DONE, &fw_buf->status);
> >>                       clear_bit(FW_STATUS_LOADING, &fw_buf->status);
> >>
> >>
> >
> > Can you explain the loading store, and what the semantics are for an LSM
> > when a NULL is passed as the file?
> 
> I'm not sure what you mean by "loading store"?

The caller: firmware_loading_store()

> When NULL is passed as the file, it means that the firmware was passes
> a blob, and there is no file backing it:

Where does this blob come from, is cached, built into the kernel, or what?


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[Kees Cook]

On Sat, Jul 19, 2014 at 12:22 AM, James Morris <jmorris@namei.org> wrote:
> On Fri, 18 Jul 2014, Kees Cook wrote:
>
>> On Thu, Jul 17, 2014 at 8:41 PM, James Morris <jmorris@namei.org> wrote:
>> > On Mon, 14 Jul 2014, Kees Cook wrote:
>> >
>> >> This attaches LSM hooks to the existing firmware loading interfaces:
>> >> filesystem-found firmware and demand-loaded blobs.
>> >
>> >>  static int fw_get_filesystem_firmware(struct device *device,
>> >> @@ -640,6 +646,12 @@ static ssize_t firmware_loading_store(struct device *dev,
>> >>               break;
>> >>       case 0:
>> >>               if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
>> >> +                     if (security_kernel_fw_from_file(NULL, fw_buf->data,
>> >> +                                                      fw_buf->size)) {
>> >> +                             fw_load_abort(fw_priv);
>> >> +                             break;
>> >> +                     }
>> >> +
>> >>                       set_bit(FW_STATUS_DONE, &fw_buf->status);
>> >>                       clear_bit(FW_STATUS_LOADING, &fw_buf->status);
>> >>
>> >>
>> >
>> > Can you explain the loading store, and what the semantics are for an LSM
>> > when a NULL is passed as the file?
>>
>> I'm not sure what you mean by "loading store"?
>
> The caller: firmware_loading_store()

Ah, sure! This is part of the existing userspace interface to firmware
loading. It uses a very strange sysfs interface. The existing
interface creates 2 files in /sys: "loading" and "data", and then
issues a uevent. The kernel uevent listener (generally udev) sees the
event, and launches the userspace firmware loader handler. That
handler locates the requested firmware, writes "1" to "loading", then
writes firmware bytes into "data", and when finished, writes "0" to
"loading".

>> When NULL is passed as the file, it means that the firmware was passes
>> a blob, and there is no file backing it:
>
> Where does this blob come from, is cached, built into the kernel, or what?

The firmware_class interface (before this series) uses three
mechanisms to resolve firmware requests. The first is to find a
firmware that is built into the kernel image itself, as part of a
static array of available firmwares. If this isn't found, the second
is to search the filesystem from within kernel space, trying to find a
matching firmware filename. If found, it will open and load it. If
this mechanism fails, it will call out to userspace (using the
uevent-triggered interface I described above).

In all cases, it is up to the caller of request_firmware to decide if
it wants to cache the loaded firmware or not. Once it's done with it,
it releases the memory.

The documentation for the firmware_class interfaces is in
Documentation/firmware_class/README.

With the patch series, the LSM hook sees the userspace-touching loads:
- from kernel built-in: no LSM hook (nonsense to check the static list)
- direct from filesystem: called with file struct
- via uevent /sys "loading"/"data" interface: called with NULL file struct
- via uevent /sys "fd" interface: called with file struct

The reason the "fd" interface was added was because otherwise there's
no way for systems that use the uevent handler to communicate to the
kernel where the bytes being shoved into the "data" interface are
coming from.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[James Morris]

On Sat, 19 Jul 2014, Kees Cook wrote:

[...]

> With the patch series, the LSM hook sees the userspace-touching loads:
> - from kernel built-in: no LSM hook (nonsense to check the static list)
> - direct from filesystem: called with file struct
> - via uevent /sys "loading"/"data" interface: called with NULL file struct
> - via uevent /sys "fd" interface: called with file struct

Thanks for the overview.  Can we get this documented in the LSM code?

> The reason the "fd" interface was added was because otherwise there's
> no way for systems that use the uevent handler to communicate to the
> kernel where the bytes being shoved into the "data" interface are
> coming from.

Ok.

I gather folks have also thought about signing firmware?


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[Mimi Zohar]

On Mon, 2014-07-21 at 09:43 +1000, James Morris wrote: 
> On Sat, 19 Jul 2014, Kees Cook wrote:
> 
> [...]
> 
> > With the patch series, the LSM hook sees the userspace-touching loads:
> > - from kernel built-in: no LSM hook (nonsense to check the static list)
> > - direct from filesystem: called with file struct
> > - via uevent /sys "loading"/"data" interface: called with NULL file struct
> > - via uevent /sys "fd" interface: called with file struct
> 
> Thanks for the overview.  Can we get this documented in the LSM code?
> 
> > The reason the "fd" interface was added was because otherwise there's
> > no way for systems that use the uevent handler to communicate to the
> > kernel where the bytes being shoved into the "data" interface are
> > coming from.
> 
> Ok.
> 
> I gather folks have also thought about signing firmware?

>From an IMA perspective, this would be the same as for any other file,
just a new hook.

Mimi 

Re: [PATCH 4/7] firmware_class: perform new LSM checks

[Greg Kroah-Hartman]

On Mon, Jul 14, 2014 at 02:38:14PM -0700, Kees Cook wrote:
> This attaches LSM hooks to the existing firmware loading interfaces:
> filesystem-found firmware and demand-loaded blobs.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[PATCH 5/7] firmware_class: extract start loading logic

[Kees Cook]

Extract the logic performed when starting a new firmware load.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/base/firmware_class.c |   29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 7399bab71ced..b38cbcd6ebb1 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -604,6 +604,23 @@ static int fw_map_pages_buf(struct firmware_buf *buf)
 	return 0;
 }
 
+/* fw_lock must be held */
+static void fw_load_start(struct firmware_buf *fw_buf)
+{
+	/* discarding any previous partial load */
+	if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		int i;
+
+		for (i = 0; i < fw_buf->nr_pages; i++)
+			__free_page(fw_buf->pages[i]);
+		kfree(fw_buf->pages);
+		fw_buf->pages = NULL;
+		fw_buf->page_array_size = 0;
+		fw_buf->nr_pages = 0;
+		set_bit(FW_STATUS_LOADING, &fw_buf->status);
+	}
+}
+
 /**
  * firmware_loading_store - set value in the 'loading' control file
  * @dev: device pointer
@@ -624,7 +641,6 @@ static ssize_t firmware_loading_store(struct device *dev,
 	struct firmware_priv *fw_priv = to_firmware_priv(dev);
 	struct firmware_buf *fw_buf;
 	int loading = simple_strtol(buf, NULL, 10);
-	int i;
 
 	mutex_lock(&fw_lock);
 	fw_buf = fw_priv->buf;
@@ -633,16 +649,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 
 	switch (loading) {
 	case 1:
-		/* discarding any previous partial load */
-		if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
-			for (i = 0; i < fw_buf->nr_pages; i++)
-				__free_page(fw_buf->pages[i]);
-			kfree(fw_buf->pages);
-			fw_buf->pages = NULL;
-			fw_buf->page_array_size = 0;
-			fw_buf->nr_pages = 0;
-			set_bit(FW_STATUS_LOADING, &fw_buf->status);
-		}
+		fw_load_start(fw_buf);
 		break;
 	case 0:
 		if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
-- 
1.7.9.5

Re: [PATCH 5/7] firmware_class: extract start loading logic

[Ming Lei]

On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
> Extract the logic performed when starting a new firmware load.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Ming Lei <ming.lei@canonical.com>

> ---
>  drivers/base/firmware_class.c |   29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 7399bab71ced..b38cbcd6ebb1 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -604,6 +604,23 @@ static int fw_map_pages_buf(struct firmware_buf *buf)
>         return 0;
>  }
>
> +/* fw_lock must be held */
> +static void fw_load_start(struct firmware_buf *fw_buf)
> +{
> +       /* discarding any previous partial load */
> +       if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +               int i;
> +
> +               for (i = 0; i < fw_buf->nr_pages; i++)
> +                       __free_page(fw_buf->pages[i]);
> +               kfree(fw_buf->pages);
> +               fw_buf->pages = NULL;
> +               fw_buf->page_array_size = 0;
> +               fw_buf->nr_pages = 0;
> +               set_bit(FW_STATUS_LOADING, &fw_buf->status);
> +       }
> +}
> +
>  /**
>   * firmware_loading_store - set value in the 'loading' control file
>   * @dev: device pointer
> @@ -624,7 +641,6 @@ static ssize_t firmware_loading_store(struct device *dev,
>         struct firmware_priv *fw_priv = to_firmware_priv(dev);
>         struct firmware_buf *fw_buf;
>         int loading = simple_strtol(buf, NULL, 10);
> -       int i;
>
>         mutex_lock(&fw_lock);
>         fw_buf = fw_priv->buf;
> @@ -633,16 +649,7 @@ static ssize_t firmware_loading_store(struct device *dev,
>
>         switch (loading) {
>         case 1:
> -               /* discarding any previous partial load */
> -               if (!test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> -                       for (i = 0; i < fw_buf->nr_pages; i++)
> -                               __free_page(fw_buf->pages[i]);
> -                       kfree(fw_buf->pages);
> -                       fw_buf->pages = NULL;
> -                       fw_buf->page_array_size = 0;
> -                       fw_buf->nr_pages = 0;
> -                       set_bit(FW_STATUS_LOADING, &fw_buf->status);
> -               }
> +               fw_load_start(fw_buf);
>                 break;
>         case 0:
>                 if (test_bit(FW_STATUS_LOADING, &fw_buf->status)) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

As an alternative to loading bytes from the "data" blob when reading
firmware, let kernel read from an fd, so that the LSM can reason about
the origin of firmware contents during userspace on-demand loading.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/firmware_class/README |   10 +++++
 drivers/base/firmware_class.c       |   77 ++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
index 71f86859d7d8..d899bdacba1e 100644
--- a/Documentation/firmware_class/README
+++ b/Documentation/firmware_class/README
@@ -59,6 +59,16 @@
  8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
 		 the firmware image and any related resource.
 
+ Alternative loading (via file descriptor):
+ ==========================================
+ Instead of "loading", and "data" above, firmware can be loaded through
+ an open file descriptor as well, via the "fd" file:
+
+ Replacing steps 2 thorugh 6 above with the following userspace actions,
+ performs the same loading:
+	- open firmware image file (e.g. as fd 3).
+	- write fd (e.g. "3") to /sys/class/firmware/xxx/fd.
+
  High level behavior (driver code):
  ==================================
 
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index b38cbcd6ebb1..eac996447d28 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = {
 	.write = firmware_data_write,
 };
 
+/**
+ * fd_store - set value in the 'fd' control file
+ * @dev: device pointer
+ * @attr: device attribute pointer
+ * @buf: buffer to scan for loading fd value
+ * @count: number of bytes in @buf
+ *
+ * Parses an fd number from buf, and then loads firmware bytes from
+ * the current process's matching open fd.
+ *
+ **/
+static ssize_t fd_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct firmware_priv *fw_priv = to_firmware_priv(dev);
+	struct firmware_buf *fw_buf = fw_priv->buf;
+	ssize_t retval;
+	int fd;
+	struct fd f;
+
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
+
+	retval = kstrtoint(buf, 10, &fd);
+	if (retval)
+		return retval;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+
+	mutex_lock(&fw_lock);
+	/* If we raced another loader, we must fail. */
+	if (test_bit(FW_STATUS_DONE, &fw_buf->status)) {
+		retval = -EINVAL;
+		goto unlock;
+	}
+
+	fw_load_start(fw_buf);
+	/* fw_read_file_contents uses vmalloc, not pages. */
+	fw_buf->is_paged_buf = false;
+	retval = fw_read_file_contents(f.file, fw_buf);
+	if (retval) {
+		/* If we called fw_load_start, we must abort on fail. */
+		fw_load_abort(fw_priv);
+		goto unlock;
+	}
+
+	/* Success. */
+	set_bit(FW_STATUS_DONE, &fw_buf->status);
+	list_del_init(&fw_buf->pending_list);
+	complete_all(&fw_buf->completion);
+
+unlock:
+	mutex_unlock(&fw_lock);
+	fdput(f);
+
+	if (retval)
+		return retval;
+	return count;
+}
+static DEVICE_ATTR_WO(fd);
+
 static void firmware_class_timeout_work(struct work_struct *work)
 {
 	struct firmware_priv *fw_priv = container_of(work,
@@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 		mutex_lock(&fw_lock);
 		list_del_init(&buf->pending_list);
 		mutex_unlock(&fw_lock);
-		dev_err(f_dev, "%s: device_create_file failed\n", __func__);
+		dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__);
 		goto err_del_bin_attr;
 	}
 
+	retval = device_create_file(f_dev, &dev_attr_fd);
+	if (retval) {
+		mutex_lock(&fw_lock);
+		list_del_init(&buf->pending_list);
+		mutex_unlock(&fw_lock);
+		dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__);
+		goto err_del_loading;
+	}
+
 	if (opt_flags & FW_OPT_UEVENT) {
 		buf->need_uevent = true;
 		dev_set_uevent_suppress(f_dev, false);
@@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
 	if (!buf->data)
 		retval = -ENOMEM;
 
+	device_remove_file(f_dev, &dev_attr_fd);
+err_del_loading:
 	device_remove_file(f_dev, &dev_attr_loading);
 err_del_bin_attr:
 	device_remove_bin_file(f_dev, &firmware_attr_data);
-- 
1.7.9.5

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Mimi Zohar]

On Mon, 2014-07-14 at 14:38 -0700, Kees Cook wrote: 
> As an alternative to loading bytes from the "data" blob when reading
> firmware, let kernel read from an fd, so that the LSM can reason about
> the origin of firmware contents during userspace on-demand loading.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/firmware_class/README |   10 +++++
>  drivers/base/firmware_class.c       |   77 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 86 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
> index 71f86859d7d8..d899bdacba1e 100644
> --- a/Documentation/firmware_class/README
> +++ b/Documentation/firmware_class/README
> @@ -59,6 +59,16 @@
>   8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
>  		 the firmware image and any related resource.
> 
> + Alternative loading (via file descriptor):
> + ==========================================
> + Instead of "loading", and "data" above, firmware can be loaded through
> + an open file descriptor as well, via the "fd" file:
> +
> + Replacing steps 2 thorugh 6 above with the following userspace actions,
> + performs the same loading:
> +	- open firmware image file (e.g. as fd 3).
> +	- write fd (e.g. "3") to /sys/class/firmware/xxx/fd.
> +
>   High level behavior (driver code):
>   ==================================
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b38cbcd6ebb1..eac996447d28 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = {
>  	.write = firmware_data_write,
>  };
> 
> +/**
> + * fd_store - set value in the 'fd' control file
> + * @dev: device pointer
> + * @attr: device attribute pointer
> + * @buf: buffer to scan for loading fd value
> + * @count: number of bytes in @buf
> + *
> + * Parses an fd number from buf, and then loads firmware bytes from
> + * the current process's matching open fd.
> + *
> + **/
> +static ssize_t fd_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{

I must be missing something.  Here's a static definition, where's the
call to fd_store()?

Mimi

> +	struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +	struct firmware_buf *fw_buf = fw_priv->buf;
> +	ssize_t retval;
> +	int fd;
> +	struct fd f;
> +
> +	if (!capable(CAP_SYS_RAWIO))
> +		return -EPERM;
> +
> +	retval = kstrtoint(buf, 10, &fd);
> +	if (retval)
> +		return retval;
> +
> +	f = fdget(fd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	mutex_lock(&fw_lock);
> +	/* If we raced another loader, we must fail. */
> +	if (test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +		retval = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	fw_load_start(fw_buf);
> +	/* fw_read_file_contents uses vmalloc, not pages. */
> +	fw_buf->is_paged_buf = false;
> +	retval = fw_read_file_contents(f.file, fw_buf);
> +	if (retval) {
> +		/* If we called fw_load_start, we must abort on fail. */
> +		fw_load_abort(fw_priv);
> +		goto unlock;
> +	}
> +
> +	/* Success. */
> +	set_bit(FW_STATUS_DONE, &fw_buf->status);
> +	list_del_init(&fw_buf->pending_list);
> +	complete_all(&fw_buf->completion);
> +
> +unlock:
> +	mutex_unlock(&fw_lock);
> +	fdput(f);
> +
> +	if (retval)
> +		return retval;
> +	return count;
> +}
> +static DEVICE_ATTR_WO(fd);
> +
>  static void firmware_class_timeout_work(struct work_struct *work)
>  {
>  	struct firmware_priv *fw_priv = container_of(work,
> @@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  		mutex_lock(&fw_lock);
>  		list_del_init(&buf->pending_list);
>  		mutex_unlock(&fw_lock);
> -		dev_err(f_dev, "%s: device_create_file failed\n", __func__);
> +		dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__);
>  		goto err_del_bin_attr;
>  	}
> 
> +	retval = device_create_file(f_dev, &dev_attr_fd);
> +	if (retval) {
> +		mutex_lock(&fw_lock);
> +		list_del_init(&buf->pending_list);
> +		mutex_unlock(&fw_lock);
> +		dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__);
> +		goto err_del_loading;
> +	}
> +
>  	if (opt_flags & FW_OPT_UEVENT) {
>  		buf->need_uevent = true;
>  		dev_set_uevent_suppress(f_dev, false);
> @@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>  	if (!buf->data)
>  		retval = -ENOMEM;
> 
> +	device_remove_file(f_dev, &dev_attr_fd);
> +err_del_loading:
>  	device_remove_file(f_dev, &dev_attr_loading);
>  err_del_bin_attr:
>  	device_remove_bin_file(f_dev, &firmware_attr_data);

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Fri, Jul 18, 2014 at 10:11 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2014-07-14 at 14:38 -0700, Kees Cook wrote:
>> As an alternative to loading bytes from the "data" blob when reading
>> firmware, let kernel read from an fd, so that the LSM can reason about
>> the origin of firmware contents during userspace on-demand loading.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  Documentation/firmware_class/README |   10 +++++
>>  drivers/base/firmware_class.c       |   77 ++++++++++++++++++++++++++++++++++-
>>  2 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
>> index 71f86859d7d8..d899bdacba1e 100644
>> --- a/Documentation/firmware_class/README
>> +++ b/Documentation/firmware_class/README
>> @@ -59,6 +59,16 @@
>>   8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
>>                the firmware image and any related resource.
>>
>> + Alternative loading (via file descriptor):
>> + ==========================================
>> + Instead of "loading", and "data" above, firmware can be loaded through
>> + an open file descriptor as well, via the "fd" file:
>> +
>> + Replacing steps 2 thorugh 6 above with the following userspace actions,
>> + performs the same loading:
>> +     - open firmware image file (e.g. as fd 3).
>> +     - write fd (e.g. "3") to /sys/class/firmware/xxx/fd.
>> +
>>   High level behavior (driver code):
>>   ==================================
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index b38cbcd6ebb1..eac996447d28 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = {
>>       .write = firmware_data_write,
>>  };
>>
>> +/**
>> + * fd_store - set value in the 'fd' control file
>> + * @dev: device pointer
>> + * @attr: device attribute pointer
>> + * @buf: buffer to scan for loading fd value
>> + * @count: number of bytes in @buf
>> + *
>> + * Parses an fd number from buf, and then loads firmware bytes from
>> + * the current process's matching open fd.
>> + *
>> + **/
>> +static ssize_t fd_store(struct device *dev,
>> +                              struct device_attribute *attr,
>> +                              const char *buf, size_t count)
>> +{
>
> I must be missing something.  Here's a static definition, where's the
> call to fd_store()?

It's hidden by macro insanity (below).

>
> Mimi
>
>> +     struct firmware_priv *fw_priv = to_firmware_priv(dev);
>> +     struct firmware_buf *fw_buf = fw_priv->buf;
>> +     ssize_t retval;
>> +     int fd;
>> +     struct fd f;
>> +
>> +     if (!capable(CAP_SYS_RAWIO))
>> +             return -EPERM;
>> +
>> +     retval = kstrtoint(buf, 10, &fd);
>> +     if (retval)
>> +             return retval;
>> +
>> +     f = fdget(fd);
>> +     if (!f.file)
>> +             return -EBADF;
>> +
>> +     mutex_lock(&fw_lock);
>> +     /* If we raced another loader, we must fail. */
>> +     if (test_bit(FW_STATUS_DONE, &fw_buf->status)) {
>> +             retval = -EINVAL;
>> +             goto unlock;
>> +     }
>> +
>> +     fw_load_start(fw_buf);
>> +     /* fw_read_file_contents uses vmalloc, not pages. */
>> +     fw_buf->is_paged_buf = false;
>> +     retval = fw_read_file_contents(f.file, fw_buf);
>> +     if (retval) {
>> +             /* If we called fw_load_start, we must abort on fail. */
>> +             fw_load_abort(fw_priv);
>> +             goto unlock;
>> +     }
>> +
>> +     /* Success. */
>> +     set_bit(FW_STATUS_DONE, &fw_buf->status);
>> +     list_del_init(&fw_buf->pending_list);
>> +     complete_all(&fw_buf->completion);
>> +
>> +unlock:
>> +     mutex_unlock(&fw_lock);
>> +     fdput(f);
>> +
>> +     if (retval)
>> +             return retval;
>> +     return count;
>> +}
>> +static DEVICE_ATTR_WO(fd);

DEVICE_ATTR_WO defines the function pointer to fd_store (and
explicitly lacks a function pointer for fd_show).

-Kees

>> +
>>  static void firmware_class_timeout_work(struct work_struct *work)
>>  {
>>       struct firmware_priv *fw_priv = container_of(work,
>> @@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>               mutex_lock(&fw_lock);
>>               list_del_init(&buf->pending_list);
>>               mutex_unlock(&fw_lock);
>> -             dev_err(f_dev, "%s: device_create_file failed\n", __func__);
>> +             dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__);
>>               goto err_del_bin_attr;
>>       }
>>
>> +     retval = device_create_file(f_dev, &dev_attr_fd);
>> +     if (retval) {
>> +             mutex_lock(&fw_lock);
>> +             list_del_init(&buf->pending_list);
>> +             mutex_unlock(&fw_lock);
>> +             dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__);
>> +             goto err_del_loading;
>> +     }
>> +
>>       if (opt_flags & FW_OPT_UEVENT) {
>>               buf->need_uevent = true;
>>               dev_set_uevent_suppress(f_dev, false);
>> @@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>       if (!buf->data)
>>               retval = -ENOMEM;
>>
>> +     device_remove_file(f_dev, &dev_attr_fd);
>> +err_del_loading:
>>       device_remove_file(f_dev, &dev_attr_loading);
>>  err_del_bin_attr:
>>       device_remove_bin_file(f_dev, &firmware_attr_data);
>
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Ming Lei]

On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
> As an alternative to loading bytes from the "data" blob when reading
> firmware, let kernel read from an fd, so that the LSM can reason about
> the origin of firmware contents during userspace on-demand loading.

>From user space view, maybe it is better to keep previous usage and just
check if loading is from 'data' blob or fd in 'echo 0 > loading' of
firmware_loading_store(), then the 'fd' usage becomes very similar with
before.

>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/firmware_class/README |   10 +++++
>  drivers/base/firmware_class.c       |   77 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
> index 71f86859d7d8..d899bdacba1e 100644
> --- a/Documentation/firmware_class/README
> +++ b/Documentation/firmware_class/README
> @@ -59,6 +59,16 @@
>   8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
>                  the firmware image and any related resource.
>
> + Alternative loading (via file descriptor):
> + ==========================================
> + Instead of "loading", and "data" above, firmware can be loaded through
> + an open file descriptor as well, via the "fd" file:
> +
> + Replacing steps 2 thorugh 6 above with the following userspace actions,
> + performs the same loading:
> +       - open firmware image file (e.g. as fd 3).
> +       - write fd (e.g. "3") to /sys/class/firmware/xxx/fd.

Then as above, here should be just to replace step 4, 5 as
          - open image in user space
          - echo fd > /sys/class/firmware/xxx/fd

> +
>   High level behavior (driver code):
>   ==================================
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b38cbcd6ebb1..eac996447d28 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = {
>         .write = firmware_data_write,
>  };
>
> +/**
> + * fd_store - set value in the 'fd' control file
> + * @dev: device pointer
> + * @attr: device attribute pointer
> + * @buf: buffer to scan for loading fd value
> + * @count: number of bytes in @buf
> + *
> + * Parses an fd number from buf, and then loads firmware bytes from
> + * the current process's matching open fd.
> + *
> + **/
> +static ssize_t fd_store(struct device *dev,
> +                                struct device_attribute *attr,
> +                                const char *buf, size_t count)
> +{
> +       struct firmware_priv *fw_priv = to_firmware_priv(dev);
> +       struct firmware_buf *fw_buf = fw_priv->buf;
> +       ssize_t retval;
> +       int fd;
> +       struct fd f;
> +
> +       if (!capable(CAP_SYS_RAWIO))
> +               return -EPERM;
> +
> +       retval = kstrtoint(buf, 10, &fd);
> +       if (retval)
> +               return retval;
> +
> +       f = fdget(fd);
> +       if (!f.file)
> +               return -EBADF;
> +
> +       mutex_lock(&fw_lock);
> +       /* If we raced another loader, we must fail. */
> +       if (test_bit(FW_STATUS_DONE, &fw_buf->status)) {
> +               retval = -EINVAL;
> +               goto unlock;
> +       }
> +
> +       fw_load_start(fw_buf);
> +       /* fw_read_file_contents uses vmalloc, not pages. */
> +       fw_buf->is_paged_buf = false;
> +       retval = fw_read_file_contents(f.file, fw_buf);
> +       if (retval) {
> +               /* If we called fw_load_start, we must abort on fail. */
> +               fw_load_abort(fw_priv);
> +               goto unlock;
> +       }
> +
> +       /* Success. */
> +       set_bit(FW_STATUS_DONE, &fw_buf->status);
> +       list_del_init(&fw_buf->pending_list);
> +       complete_all(&fw_buf->completion);
> +
> +unlock:
> +       mutex_unlock(&fw_lock);
> +       fdput(f);
> +
> +       if (retval)
> +               return retval;
> +       return count;
> +}
> +static DEVICE_ATTR_WO(fd);
> +
>  static void firmware_class_timeout_work(struct work_struct *work)
>  {
>         struct firmware_priv *fw_priv = container_of(work,
> @@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>                 mutex_lock(&fw_lock);
>                 list_del_init(&buf->pending_list);
>                 mutex_unlock(&fw_lock);
> -               dev_err(f_dev, "%s: device_create_file failed\n", __func__);
> +               dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__);
>                 goto err_del_bin_attr;
>         }
>
> +       retval = device_create_file(f_dev, &dev_attr_fd);
> +       if (retval) {
> +               mutex_lock(&fw_lock);
> +               list_del_init(&buf->pending_list);
> +               mutex_unlock(&fw_lock);
> +               dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__);
> +               goto err_del_loading;
> +       }
> +
>         if (opt_flags & FW_OPT_UEVENT) {
>                 buf->need_uevent = true;
>                 dev_set_uevent_suppress(f_dev, false);
> @@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>         if (!buf->data)
>                 retval = -ENOMEM;
>
> +       device_remove_file(f_dev, &dev_attr_fd);
> +err_del_loading:
>         device_remove_file(f_dev, &dev_attr_loading);
>  err_del_bin_attr:
>         device_remove_bin_file(f_dev, &firmware_attr_data);
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Sat, Jul 19, 2014 at 8:04 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
>> As an alternative to loading bytes from the "data" blob when reading
>> firmware, let kernel read from an fd, so that the LSM can reason about
>> the origin of firmware contents during userspace on-demand loading.
>
> From user space view, maybe it is better to keep previous usage and just
> check if loading is from 'data' blob or fd in 'echo 0 > loading' of
> firmware_loading_store(), then the 'fd' usage becomes very similar with
> before.

I don't think this is a good idea because otherwise there isn't a good
way to have an "atomic" check of the firmware contents. What does it
means to write to "fd" several times, then write "data" a little,
before writing "loading", etc? I originally wrote the patch series
requiring the "loading" piece, and it ended up being very complicated
due to needing to switch the memory buffer logic back and forth.
Everything is much much cleaner if "fd" is single-shot, and not part
of the loading/data interface semantics.

And, as it turns out, the userspace changes are much simpler if
"loading" isn't part of the picture for the "fd" interface, too.

-Kees

>
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  Documentation/firmware_class/README |   10 +++++
>>  drivers/base/firmware_class.c       |   77 ++++++++++++++++++++++++++++++++++-
>>  2 files changed, 86 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/firmware_class/README b/Documentation/firmware_class/README
>> index 71f86859d7d8..d899bdacba1e 100644
>> --- a/Documentation/firmware_class/README
>> +++ b/Documentation/firmware_class/README
>> @@ -59,6 +59,16 @@
>>   8), kernel(driver): Driver code calls release_firmware(fw_entry) releasing
>>                  the firmware image and any related resource.
>>
>> + Alternative loading (via file descriptor):
>> + ==========================================
>> + Instead of "loading", and "data" above, firmware can be loaded through
>> + an open file descriptor as well, via the "fd" file:
>> +
>> + Replacing steps 2 thorugh 6 above with the following userspace actions,
>> + performs the same loading:
>> +       - open firmware image file (e.g. as fd 3).
>> +       - write fd (e.g. "3") to /sys/class/firmware/xxx/fd.
>
> Then as above, here should be just to replace step 4, 5 as
>           - open image in user space
>           - echo fd > /sys/class/firmware/xxx/fd
>
>> +
>>   High level behavior (driver code):
>>   ==================================
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index b38cbcd6ebb1..eac996447d28 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -839,6 +839,70 @@ static struct bin_attribute firmware_attr_data = {
>>         .write = firmware_data_write,
>>  };
>>
>> +/**
>> + * fd_store - set value in the 'fd' control file
>> + * @dev: device pointer
>> + * @attr: device attribute pointer
>> + * @buf: buffer to scan for loading fd value
>> + * @count: number of bytes in @buf
>> + *
>> + * Parses an fd number from buf, and then loads firmware bytes from
>> + * the current process's matching open fd.
>> + *
>> + **/
>> +static ssize_t fd_store(struct device *dev,
>> +                                struct device_attribute *attr,
>> +                                const char *buf, size_t count)
>> +{
>> +       struct firmware_priv *fw_priv = to_firmware_priv(dev);
>> +       struct firmware_buf *fw_buf = fw_priv->buf;
>> +       ssize_t retval;
>> +       int fd;
>> +       struct fd f;
>> +
>> +       if (!capable(CAP_SYS_RAWIO))
>> +               return -EPERM;
>> +
>> +       retval = kstrtoint(buf, 10, &fd);
>> +       if (retval)
>> +               return retval;
>> +
>> +       f = fdget(fd);
>> +       if (!f.file)
>> +               return -EBADF;
>> +
>> +       mutex_lock(&fw_lock);
>> +       /* If we raced another loader, we must fail. */
>> +       if (test_bit(FW_STATUS_DONE, &fw_buf->status)) {
>> +               retval = -EINVAL;
>> +               goto unlock;
>> +       }
>> +
>> +       fw_load_start(fw_buf);
>> +       /* fw_read_file_contents uses vmalloc, not pages. */
>> +       fw_buf->is_paged_buf = false;
>> +       retval = fw_read_file_contents(f.file, fw_buf);
>> +       if (retval) {
>> +               /* If we called fw_load_start, we must abort on fail. */
>> +               fw_load_abort(fw_priv);
>> +               goto unlock;
>> +       }
>> +
>> +       /* Success. */
>> +       set_bit(FW_STATUS_DONE, &fw_buf->status);
>> +       list_del_init(&fw_buf->pending_list);
>> +       complete_all(&fw_buf->completion);
>> +
>> +unlock:
>> +       mutex_unlock(&fw_lock);
>> +       fdput(f);
>> +
>> +       if (retval)
>> +               return retval;
>> +       return count;
>> +}
>> +static DEVICE_ATTR_WO(fd);
>> +
>>  static void firmware_class_timeout_work(struct work_struct *work)
>>  {
>>         struct firmware_priv *fw_priv = container_of(work,
>> @@ -912,10 +976,19 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>                 mutex_lock(&fw_lock);
>>                 list_del_init(&buf->pending_list);
>>                 mutex_unlock(&fw_lock);
>> -               dev_err(f_dev, "%s: device_create_file failed\n", __func__);
>> +               dev_err(f_dev, "%s: dev_attr_loading failed\n", __func__);
>>                 goto err_del_bin_attr;
>>         }
>>
>> +       retval = device_create_file(f_dev, &dev_attr_fd);
>> +       if (retval) {
>> +               mutex_lock(&fw_lock);
>> +               list_del_init(&buf->pending_list);
>> +               mutex_unlock(&fw_lock);
>> +               dev_err(f_dev, "%s: &dev_attr_fd failed\n", __func__);
>> +               goto err_del_loading;
>> +       }
>> +
>>         if (opt_flags & FW_OPT_UEVENT) {
>>                 buf->need_uevent = true;
>>                 dev_set_uevent_suppress(f_dev, false);
>> @@ -933,6 +1006,8 @@ static int _request_firmware_load(struct firmware_priv *fw_priv,
>>         if (!buf->data)
>>                 retval = -ENOMEM;
>>
>> +       device_remove_file(f_dev, &dev_attr_fd);
>> +err_del_loading:
>>         device_remove_file(f_dev, &dev_attr_loading);
>>  err_del_bin_attr:
>>         device_remove_bin_file(f_dev, &firmware_attr_data);
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Ming Lei]

On Mon, Jul 21, 2014 at 1:43 AM, Kees Cook <keescook@chromium.org> wrote:
> On Sat, Jul 19, 2014 at 8:04 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
>>> As an alternative to loading bytes from the "data" blob when reading
>>> firmware, let kernel read from an fd, so that the LSM can reason about
>>> the origin of firmware contents during userspace on-demand loading.
>>
>> From user space view, maybe it is better to keep previous usage and just
>> check if loading is from 'data' blob or fd in 'echo 0 > loading' of
>> firmware_loading_store(), then the 'fd' usage becomes very similar with
>> before.
>
> I don't think this is a good idea because otherwise there isn't a good
> way to have an "atomic" check of the firmware contents. What does it

Could you share why 'atomic' check is necessary? As we know, it isn't
real atomic, :-).

> means to write to "fd" several times, then write "data" a little,
> before writing "loading", etc? I originally wrote the patch series

That depends how firmware loader supports these cases, and won't
be difficult to handle them.

For non-fd userspace interface, it is very flexible to be capable of
supporting to load firmware data from multiple images, or in flight.
With single 'fd' interface, it won't be possible any more.

> requiring the "loading" piece, and it ended up being very complicated
> due to needing to switch the memory buffer logic back and forth.
> Everything is much much cleaner if "fd" is single-shot, and not part
> of the loading/data interface semantics.

You might not avoid the 'loading' piece completely, how does
the userspace handle non-exist firmware image? The reasonable
way is to abort the loading from userspace via 'echo -1 > loading'
since userspace already sees that, and you may choose to let kernel
side handle that, but your current patch doesn't support it yet.

Thanks,

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Sun, Jul 20, 2014 at 6:42 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Mon, Jul 21, 2014 at 1:43 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Sat, Jul 19, 2014 at 8:04 PM, Ming Lei <ming.lei@canonical.com> wrote:
>>> On Tue, Jul 15, 2014 at 5:38 AM, Kees Cook <keescook@chromium.org> wrote:
>>>> As an alternative to loading bytes from the "data" blob when reading
>>>> firmware, let kernel read from an fd, so that the LSM can reason about
>>>> the origin of firmware contents during userspace on-demand loading.
>>>
>>> From user space view, maybe it is better to keep previous usage and just
>>> check if loading is from 'data' blob or fd in 'echo 0 > loading' of
>>> firmware_loading_store(), then the 'fd' usage becomes very similar with
>>> before.
>>
>> I don't think this is a good idea because otherwise there isn't a good
>> way to have an "atomic" check of the firmware contents. What does it
>
> Could you share why 'atomic' check is necessary? As we know, it isn't
> real atomic, :-).

Right, I don't really mean actually atomic. I just mean "a single
write operation for loading a single firmware".

>> means to write to "fd" several times, then write "data" a little,
>> before writing "loading", etc? I originally wrote the patch series
>
> That depends how firmware loader supports these cases, and won't
> be difficult to handle them.
>
> For non-fd userspace interface, it is very flexible to be capable of
> supporting to load firmware data from multiple images, or in flight.
> With single 'fd' interface, it won't be possible any more.

That's entirely correct. But I'm not suggesting replacing loading/data
with fd, I'm suggesting adding an additional separate interface. Any
system that wants to generate non-file-backed firmware or do multiple
images at once can continue to use the loading/data interface.

However, since nearly all systems currently use udev and its firmware
helper that reads from single files, this change allows the bulk of
systems to be able to reason via the LSM about the origin of
userspace-delivered firmwares.

>> requiring the "loading" piece, and it ended up being very complicated
>> due to needing to switch the memory buffer logic back and forth.
>> Everything is much much cleaner if "fd" is single-shot, and not part
>> of the loading/data interface semantics.
>
> You might not avoid the 'loading' piece completely, how does
> the userspace handle non-exist firmware image? The reasonable
> way is to abort the loading from userspace via 'echo -1 > loading'
> since userspace already sees that, and you may choose to let kernel
> side handle that, but your current patch doesn't support it yet.

That's not true -- my series doesn't remove or change any of the
existing interface behavior. If userspace doesn't find a firmware
image, it can still write -1 into loading. What I've added is simply a
new way to load the firmware, in line with finit_module and
kexec_file_load: we need to be able to load files from userspace via
fd instead of via blobs.

There is no reason for the fd interface to use "loading" to indicate
it's state. First, it's redundant: loading starts the moment fd_store
is called: from the userspace perspective, the entire load process
happens during that write. Once the write returns, loading has
finished. Adding the requirement for writing "loading" before and
after complicates userspace and complicates kernel-space since now it
must distinguish between fw_buf's is_paged_buf state, and determine
how to interact with intermixed calls to "data". For example, what
would this mean:

echo 1 > loading
write to data
write to fd
echo 0 > loading

With my current patch, writes to "fd" will be rejected if a write to
"loading" has happened, to be able to trivially distinguish between
the two interfaces, and to keep sanity in the state of is_paged_buf
logic.

It seems to me that keeping the interfaces separate makes the most
sense and provides the cleanest approach.

Perhaps it would be easier if I also sent the patch to udev's helper,
so you could see how I propose handling the userspace change to using
the new interface?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Greg Kroah-Hartman]

On Mon, Jul 21, 2014 at 08:08:16AM -0700, Kees Cook wrote:
> Perhaps it would be easier if I also sent the patch to udev's helper,
> so you could see how I propose handling the userspace change to using
> the new interface?

As there is no more "udev firmware helper", I don't know what you would
be patching here.  Firmware should always be loaded by the kernel
directly, udev isn't involved anyore at all.

confused,

greg k-h

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Mon, Jul 21, 2014 at 08:26:35AM -0700, Greg Kroah-Hartman wrote:
> On Mon, Jul 21, 2014 at 08:08:16AM -0700, Kees Cook wrote:
> > Perhaps it would be easier if I also sent the patch to udev's helper,
> > so you could see how I propose handling the userspace change to using
> > the new interface?
> 
> As there is no more "udev firmware helper", I don't know what you would
> be patching here.  Firmware should always be loaded by the kernel
> directly, udev isn't involved anyore at all.
> 
> confused,
> 
> greg k-h

The kernel _can_ load directly (when the paths are configured correctly),
but I'm not sure why you say udev isn't involved any more. It's been like
this for years, and even the latest systemd shows the udev rule is still in
place:
http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-firmware.rules
and that the firmware loader is still in the source tree:
http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c

Here's the patch for the new interface...

-Kees

---
From: Kees Cook <keescook@chromium.org>
Date: Sun, 23 Mar 2014 07:46:07 -0700
Subject: [PATCH] firmware: use fd-based interface if available

The new kernel firmware_class interface file "fd" performs the firmware
loading in a single step, allowing the kernel to validate the firmware
origin and contents.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 src/udev/udev-builtin-firmware.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
index 8cfeed6..893ec13 100644
--- a/src/udev/udev-builtin-firmware.c
+++ b/src/udev/udev-builtin-firmware.c
@@ -80,6 +80,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
         static const char *searchpath[] = { FIRMWARE_PATH };
         char loadpath[UTIL_PATH_SIZE];
         char datapath[UTIL_PATH_SIZE];
+        char fdpath[UTIL_PATH_SIZE];
         char fwpath[UTIL_PATH_SIZE];
         const char *firmware;
         FILE *fwfile = NULL;
@@ -131,6 +132,16 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
                 goto exit;
         }
 
+        /* Use fd-based firmware loading interface. */
+        strscpyl(fdpath, sizeof(fdpath), udev_device_get_syspath(dev), "/fd", NULL);
+        if (stat(fdpath, &statbuf) == 0) {
+                snprintf(datapath, sizeof(datapath), "%d", fileno(fwfile));
+                /* We're done if the we gave the kernel our fw fd. */
+                if (set_loading(udev, fdpath, (const char *)datapath))
+                        goto exit;
+        }
+
+        /* Fallback to legacy blob loading. */
         if (!set_loading(udev, loadpath, "1"))
                 goto exit;
 
-- 
1.7.9.5



-- 
Kees Cook
Chrome OS Security

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Greg Kroah-Hartman]

On Mon, Jul 21, 2014 at 08:43:07AM -0700, Kees Cook wrote:
> On Mon, Jul 21, 2014 at 08:26:35AM -0700, Greg Kroah-Hartman wrote:
> > On Mon, Jul 21, 2014 at 08:08:16AM -0700, Kees Cook wrote:
> > > Perhaps it would be easier if I also sent the patch to udev's helper,
> > > so you could see how I propose handling the userspace change to using
> > > the new interface?
> > 
> > As there is no more "udev firmware helper", I don't know what you would
> > be patching here.  Firmware should always be loaded by the kernel
> > directly, udev isn't involved anyore at all.
> > 
> > confused,
> > 
> > greg k-h
> 
> The kernel _can_ load directly (when the paths are configured correctly),
> but I'm not sure why you say udev isn't involved any more. It's been like
> this for years, and even the latest systemd shows the udev rule is still in
> place:
> http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-firmware.rules
> and that the firmware loader is still in the source tree:
> http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c

Ah, I thought that I had seen patches to delete this code on the systemd
mailing list in the past, I didn't realize they hadn't been accepted
yet.

But, with my current tree, in linux-next, it's really hard to select the
"external firmware loader" on purpose, as we want people to use the
in-kernel one if at all possible, and only fall back to the "legacy"
udev userspace loader if they are running on old userspace systems.

> Here's the patch for the new interface...

I'd really not like to add a new interface for this model when we are
trying to delete it entirely.  Why not just rely on the in-kernel loader
instead for this new feature?

thanks,

greg k-h

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Kees Cook]

On Mon, Jul 21, 2014 at 9:36 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Jul 21, 2014 at 08:43:07AM -0700, Kees Cook wrote:
>> On Mon, Jul 21, 2014 at 08:26:35AM -0700, Greg Kroah-Hartman wrote:
>> > On Mon, Jul 21, 2014 at 08:08:16AM -0700, Kees Cook wrote:
>> > > Perhaps it would be easier if I also sent the patch to udev's helper,
>> > > so you could see how I propose handling the userspace change to using
>> > > the new interface?
>> >
>> > As there is no more "udev firmware helper", I don't know what you would
>> > be patching here.  Firmware should always be loaded by the kernel
>> > directly, udev isn't involved anyore at all.
>> >
>> > confused,
>> >
>> > greg k-h
>>
>> The kernel _can_ load directly (when the paths are configured correctly),
>> but I'm not sure why you say udev isn't involved any more. It's been like
>> this for years, and even the latest systemd shows the udev rule is still in
>> place:
>> http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-firmware.rules
>> and that the firmware loader is still in the source tree:
>> http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c
>
> Ah, I thought that I had seen patches to delete this code on the systemd
> mailing list in the past, I didn't realize they hadn't been accepted
> yet.
>
> But, with my current tree, in linux-next, it's really hard to select the
> "external firmware loader" on purpose, as we want people to use the
> in-kernel one if at all possible, and only fall back to the "legacy"
> udev userspace loader if they are running on old userspace systems.

Heh. Where non-legacy means running a userspace with unaccepted
systemd patches? That's some serious time-travel. :)

>> Here's the patch for the new interface...
>
> I'd really not like to add a new interface for this model when we are
> trying to delete it entirely.  Why not just rely on the in-kernel loader
> instead for this new feature?

Yeah, I see what you're saying. Obviously if the udev loader is going
to vanish entirely, it makes no sense to add the "fd" interface. I'll
keep the last 3 patches in the series in my tree for backporting
purposes, but since the LSM hook is still useful for origin/content
validation, I'd still like to see those go in. Though it sounds like I
should do that through the security-next tree?

applied:
doc: fix minor typos in firmware_class README
test: add firmware_class loader test

hopefully for security-next:
security: introduce kernel_fw_from_file hook
firmware_class: perform new LSM checks

I'll keep these external for backporting to "legacy" kernels/userspace:
firmware_class: extract start loading logic
firmware_class: add "fd" input file
test: add "fd" firmware loading test to selftests

Does that look okay?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 6/7] firmware_class: add "fd" input file

[Greg Kroah-Hartman]

On Mon, Jul 21, 2014 at 10:27:33AM -0700, Kees Cook wrote:
> On Mon, Jul 21, 2014 at 9:36 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Jul 21, 2014 at 08:43:07AM -0700, Kees Cook wrote:
> >> On Mon, Jul 21, 2014 at 08:26:35AM -0700, Greg Kroah-Hartman wrote:
> >> > On Mon, Jul 21, 2014 at 08:08:16AM -0700, Kees Cook wrote:
> >> > > Perhaps it would be easier if I also sent the patch to udev's helper,
> >> > > so you could see how I propose handling the userspace change to using
> >> > > the new interface?
> >> >
> >> > As there is no more "udev firmware helper", I don't know what you would
> >> > be patching here.  Firmware should always be loaded by the kernel
> >> > directly, udev isn't involved anyore at all.
> >> >
> >> > confused,
> >> >
> >> > greg k-h
> >>
> >> The kernel _can_ load directly (when the paths are configured correctly),
> >> but I'm not sure why you say udev isn't involved any more. It's been like
> >> this for years, and even the latest systemd shows the udev rule is still in
> >> place:
> >> http://cgit.freedesktop.org/systemd/systemd/tree/rules/50-firmware.rules
> >> and that the firmware loader is still in the source tree:
> >> http://cgit.freedesktop.org/systemd/systemd/tree/src/udev/udev-builtin-firmware.c
> >
> > Ah, I thought that I had seen patches to delete this code on the systemd
> > mailing list in the past, I didn't realize they hadn't been accepted
> > yet.
> >
> > But, with my current tree, in linux-next, it's really hard to select the
> > "external firmware loader" on purpose, as we want people to use the
> > in-kernel one if at all possible, and only fall back to the "legacy"
> > udev userspace loader if they are running on old userspace systems.
> 
> Heh. Where non-legacy means running a userspace with unaccepted
> systemd patches? That's some serious time-travel. :)

Don't be so sure, I don't think that all distros enable the firmware
option of udev today, so it might be more "real" than you think.

> >> Here's the patch for the new interface...
> >
> > I'd really not like to add a new interface for this model when we are
> > trying to delete it entirely.  Why not just rely on the in-kernel loader
> > instead for this new feature?
> 
> Yeah, I see what you're saying. Obviously if the udev loader is going
> to vanish entirely, it makes no sense to add the "fd" interface. I'll
> keep the last 3 patches in the series in my tree for backporting
> purposes, but since the LSM hook is still useful for origin/content
> validation, I'd still like to see those go in. Though it sounds like I
> should do that through the security-next tree?
> 
> applied:
> doc: fix minor typos in firmware_class README
> test: add firmware_class loader test
> 
> hopefully for security-next:
> security: introduce kernel_fw_from_file hook
> firmware_class: perform new LSM checks
> 
> I'll keep these external for backporting to "legacy" kernels/userspace:
> firmware_class: extract start loading logic
> firmware_class: add "fd" input file
> test: add "fd" firmware loading test to selftests
> 
> Does that look okay?

That's fine with me, or I can take the "security-next" patches through
my tree, if that makes merging with the other firmware patches in my
tree easier (today it's just a line diff, which git can handle easily).

I'll go ack that patch now if James wants to pick it up.

thanks,

greg k-h

[PATCH 7/7] test: add "fd" firmware loading test to selftests

[Kees Cook]

This adds tests for the new "fd" interface to the firmware selftests.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/firmware/fw_userhelper.sh |   34 ++++++++++++++++++---
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_userhelper.sh b/tools/testing/selftests/firmware/fw_userhelper.sh
index 6efbade12139..e2766613e5b5 100644
--- a/tools/testing/selftests/firmware/fw_userhelper.sh
+++ b/tools/testing/selftests/firmware/fw_userhelper.sh
@@ -25,6 +25,7 @@ load_fw()
 {
 	local name="$1"
 	local file="$2"
+	local fd="$3"
 
 	# This will block until our load (below) has finished.
 	echo -n "$name" >"$DIR"/trigger_request &
@@ -40,9 +41,16 @@ load_fw()
 		fi
 	done
 
-	echo 1 >"$DIR"/"$name"/loading
-	cat "$file" >"$DIR"/"$name"/data
-	echo 0 >"$DIR"/"$name"/loading
+	if [ -z "$fd" ]; then
+		echo 1 >"$DIR"/"$name"/loading
+		cat "$file" >"$DIR"/"$name"/data
+		echo 0 >"$DIR"/"$name"/loading
+	else
+		if ! echo "$fd" <"$file" >"$DIR"/"$name"/fd 2>/dev/null ; then
+			# If the echo fails, abort to avoid timeout.
+			echo -1 > "$DIR"/"$name"/loading
+		fi
+	fi
 
 	# Wait for request to finish.
 	wait
@@ -83,7 +91,25 @@ if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
 	echo "$0: firmware was not loaded" >&2
 	exit 1
 else
-	echo "$0: user helper firmware loading works"
+	echo "$0: user helper firmware via 'loading' works"
+fi
+
+# Request a bad file descriptor, expected to fail.
+load_fw "$NAME" "$FW" 100
+if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: firmware was not expected to match" >&2
+	exit 1
+else
+	echo "$0: bad file descriptor correctly fails"
+fi
+
+# Request with the correct file descriptor.
+load_fw "$NAME" "$FW" 0
+if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+	echo "$0: failed to load with good fd" >&2
+	exit 1
+else
+	echo "$0: user helper firmware via 'fd' works"
 fi
 
 exit 0
-- 
1.7.9.5

Re: [PATCH 0/7] firmware validation

[Greg Kroah-Hartman]

On Mon, Jul 14, 2014 at 02:38:10PM -0700, Kees Cook wrote:
> This creates a new LSM hook to validate the contents (and origin) of
> component firmware being provided to the kernel from userspace via the
> request_firmware() interface.
> 
> Additionally creates a test module and test cases for all of the existing
> interfaces as well as the new "fd" interface.

This looks good to me.

I've applied the first 2 patches for now, I'd like to have some other
reviews and acks before applying the rest.

thanks,

greg k-h

Re: [PATCH 0/7] firmware validation

[One Thousand Gnomes]

On Mon, 14 Jul 2014 14:38:10 -0700
Kees Cook <keescook@chromium.org> wrote:

> This creates a new LSM hook to validate the contents (and origin) of
> component firmware being provided to the kernel from userspace via the
> request_firmware() interface.
> 
> Additionally creates a test module and test cases for all of the existing
> interfaces as well as the new "fd" interface.

There is a specific reason that this is a good idea. On any system with
basic capabilities only anyone owning DAC capabilities can replace the
firmware file on disk. So for some devices DAC implies a route to RAWIO
which is not good.

Alan