Hi,
Here's my attempt at clearing the path to partial read support in
kernel_read_file(), which fixes a number of issues along the way. I'm
still fighting with the firmware test suite (it doesn't seem to pass
for me even in stock v5.7... ?) But I don't want to block Scott's work[1]
any this week, so here's the series as it is currently.
The primary difference to Scott's approach is to avoid adding a new set of
functions and just adapt the existing APIs to deal with "offset". Also,
the fixes for the enum are first in the series so they can be backported
without the header file relocation.
I'll keep poking at the firmware tests...
-Kees
[1] https://lore.kernel.org/lkml/202007161415.10D015477@keescook/
Kees Cook (12):
firmware_loader: EFI firmware loader must handle pre-allocated buffer
fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
fs/kernel_read_file: Split into separate source file
fs/kernel_read_file: Remove redundant size argument
fs/kernel_read_file: Switch buffer size arg to size_t
fs/kernel_read_file: Add file_size output argument
LSM: Introduce kernel_post_load_data() hook
firmware_loader: Use security_post_load_data()
module: Call security_kernel_post_load_data()
LSM: Add "contents" flag to kernel_read_file hook
fs/kernel_file_read: Add "offset" arg for partial reads
Scott Branden (1):
fs/kernel_read_file: Split into separate include file
drivers/base/firmware_loader/fallback.c | 8 +-
.../base/firmware_loader/fallback_platform.c | 12 +-
drivers/base/firmware_loader/main.c | 13 +-
fs/Makefile | 3 +-
fs/exec.c | 132 +-----------
fs/kernel_read_file.c | 189 ++++++++++++++++++
include/linux/fs.h | 39 ----
include/linux/ima.h | 19 +-
include/linux/kernel_read_file.h | 55 +++++
include/linux/lsm_hook_defs.h | 6 +-
include/linux/lsm_hooks.h | 12 ++
include/linux/security.h | 19 +-
kernel/kexec.c | 2 +-
kernel/kexec_file.c | 18 +-
kernel/module.c | 24 ++-
security/integrity/digsig.c | 8 +-
security/integrity/ima/ima_fs.c | 9 +-
security/integrity/ima/ima_main.c | 58 ++++--
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 17 +-
security/security.c | 26 ++-
security/selinux/hooks.c | 8 +-
22 files changed, 432 insertions(+), 246 deletions(-)
create mode 100644 fs/kernel_read_file.c
create mode 100644 include/linux/kernel_read_file.h
--
2.25.1
From: Scott Branden <[email protected]>
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Kees Cook <[email protected]>
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 38 ---------------------
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h | 51 +++++++++++++++++++++++++++++
include/linux/security.h | 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c | 1 +
15 files changed, 64 insertions(+), 38 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index c2f57cedcd6f..d4a413ea48ce 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -12,6 +12,7 @@
#include <linux/capability.h>
#include <linux/device.h>
+#include <linux/kernel_read_file.h>
#include <linux/module.h>
#include <linux/init.h>
#include <linux/timer.h>
diff --git a/fs/exec.c b/fs/exec.c
index 2bf549757ce7..07a7fe9ac5be 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -23,6 +23,7 @@
* formats.
*/
+#include <linux/kernel_read_file.h>
#include <linux/slab.h>
#include <linux/file.h>
#include <linux/fdtable.h>
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f50a35d54a61..11dd6cc7de58 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,44 +2993,6 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how* nor *where*. */
-#define __kernel_read_file_id(id) \
- id(UNKNOWN, unknown) \
- id(FIRMWARE, firmware) \
- id(MODULE, kernel-module) \
- id(KEXEC_IMAGE, kexec-image) \
- id(KEXEC_INITRAMFS, kexec-initramfs) \
- id(POLICY, security-policy) \
- id(X509_CERTIFICATE, x509-certificate) \
- id(MAX_ID, )
-
-#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
-#define __fid_stringify(dummy, str) #str,
-
-enum kernel_read_file_id {
- __kernel_read_file_id(__fid_enumify)
-};
-
-static const char * const kernel_read_file_str[] = {
- __kernel_read_file_id(__fid_stringify)
-};
-
-static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
-{
- if ((unsigned)id >= READING_MAX_ID)
- return kernel_read_file_str[READING_UNKNOWN];
-
- return kernel_read_file_str[id];
-}
-
-extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..148636bfcc8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -7,6 +7,7 @@
#ifndef _LINUX_IMA_H
#define _LINUX_IMA_H
+#include <linux/kernel_read_file.h>
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/kexec.h>
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
new file mode 100644
index 000000000000..78cf3d7dc835
--- /dev/null
+++ b/include/linux/kernel_read_file.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_READ_FILE_H
+#define _LINUX_KERNEL_READ_FILE_H
+
+#include <linux/file.h>
+#include <linux/types.h>
+
+/* This is a list of *what* is being read, not *how* nor *where*. */
+#define __kernel_read_file_id(id) \
+ id(UNKNOWN, unknown) \
+ id(FIRMWARE, firmware) \
+ id(MODULE, kernel-module) \
+ id(KEXEC_IMAGE, kexec-image) \
+ id(KEXEC_INITRAMFS, kexec-initramfs) \
+ id(POLICY, security-policy) \
+ id(X509_CERTIFICATE, x509-certificate) \
+ id(MAX_ID, )
+
+#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
+#define __fid_stringify(dummy, str) #str,
+
+enum kernel_read_file_id {
+ __kernel_read_file_id(__fid_enumify)
+};
+
+static const char * const kernel_read_file_str[] = {
+ __kernel_read_file_id(__fid_stringify)
+};
+
+static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
+{
+ if ((unsigned int)id >= READING_MAX_ID)
+ return kernel_read_file_str[READING_UNKNOWN];
+
+ return kernel_read_file_str[id];
+}
+
+int kernel_read_file(struct file *file,
+ void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id);
+int kernel_read_file_from_path(const char *path,
+ void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id);
+int kernel_read_file_from_path_initns(const char *path,
+ void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id);
+int kernel_read_file_from_fd(int fd,
+ void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id);
+
+#endif /* _LINUX_KERNEL_READ_FILE_H */
diff --git a/include/linux/security.h b/include/linux/security.h
index 0a0a03b36a3b..42df0d9b4c37 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -23,6 +23,7 @@
#ifndef __LINUX_SECURITY_H
#define __LINUX_SECURITY_H
+#include <linux/kernel_read_file.h>
#include <linux/key.h>
#include <linux/capability.h>
#include <linux/fs.h>
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 09cc78df53c6..1358069ce9e9 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -24,6 +24,7 @@
#include <linux/elf.h>
#include <linux/elfcore.h>
#include <linux/kernel.h>
+#include <linux/kernel_read_file.h>
#include <linux/syscalls.h>
#include <linux/vmalloc.h>
#include "kexec_internal.h"
diff --git a/kernel/module.c b/kernel/module.c
index 26105148f4d2..e9765803601b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -18,6 +18,7 @@
#include <linux/fs.h>
#include <linux/sysfs.h>
#include <linux/kernel.h>
+#include <linux/kernel_read_file.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
#include <linux/elf.h>
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index ac02b7632353..f8869be45d8f 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -10,6 +10,7 @@
#include <linux/sched.h>
#include <linux/slab.h>
#include <linux/cred.h>
+#include <linux/kernel_read_file.h>
#include <linux/key-type.h>
#include <linux/digsig.h>
#include <linux/vmalloc.h>
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 15a44c5022f7..e13ffece3726 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -13,6 +13,7 @@
*/
#include <linux/fcntl.h>
+#include <linux/kernel_read_file.h>
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/seq_file.h>
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f80ee4ce4669..dab4a13221cf 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -18,6 +18,7 @@
#include <linux/module.h>
#include <linux/file.h>
#include <linux/binfmts.h>
+#include <linux/kernel_read_file.h>
#include <linux/mount.h>
#include <linux/mman.h>
#include <linux/slab.h>
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e493063a3c34..f8390f6081f0 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -9,6 +9,7 @@
#include <linux/init.h>
#include <linux/list.h>
+#include <linux/kernel_read_file.h>
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/magic.h>
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index ee5cb944f4ad..81bc95127f92 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -11,6 +11,7 @@
#include <linux/module.h>
#include <linux/fs.h>
+#include <linux/kernel_read_file.h>
#include <linux/lsm_hooks.h>
#include <linux/mount.h>
#include <linux/path.h>
diff --git a/security/security.c b/security/security.c
index 0ce3e73edd42..f5920115a325 100644
--- a/security/security.c
+++ b/security/security.c
@@ -16,6 +16,7 @@
#include <linux/export.h>
#include <linux/init.h>
#include <linux/kernel.h>
+#include <linux/kernel_read_file.h>
#include <linux/lsm_hooks.h>
#include <linux/integrity.h>
#include <linux/ima.h>
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index efa6108b1ce9..5de45010fb1a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -24,6 +24,7 @@
#include <linux/init.h>
#include <linux/kd.h>
#include <linux/kernel.h>
+#include <linux/kernel_read_file.h>
#include <linux/tracehook.h>
#include <linux/errno.h>
#include <linux/sched/signal.h>
--
2.25.1
These routines are used in places outside of exec(2), so in preparation
for refactoring them, move them into a separate source file,
fs/kernel_read_file.c.
Signed-off-by: Kees Cook <[email protected]>
---
fs/Makefile | 3 +-
fs/exec.c | 132 ----------------------------------------
fs/kernel_read_file.c | 138 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 140 insertions(+), 133 deletions(-)
create mode 100644 fs/kernel_read_file.c
diff --git a/fs/Makefile b/fs/Makefile
index 2ce5112b02c8..a05fc247b2a7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
seq_file.o xattr.o libfs.o fs-writeback.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
- fs_types.o fs_context.o fs_parser.o fsopen.o
+ fs_types.o fs_context.o fs_parser.o fsopen.o \
+ kernel_read_file.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/exec.c b/fs/exec.c
index 07a7fe9ac5be..d619b79aab30 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- loff_t i_size, pos;
- ssize_t bytes = 0;
- void *allocated = NULL;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (!*buf)
- *buf = allocated = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
- if (bytes < 0) {
- ret = bytes;
- goto out_free;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (allocated) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct file *file;
- struct path root;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- task_lock(&init_task);
- get_fs_root(init_task.fs, &root);
- task_unlock(&init_task);
-
- file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
- path_put(&root);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
defined(CONFIG_BINFMT_ELF_FDPIC)
ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
new file mode 100644
index 000000000000..54d972d4befc
--- /dev/null
+++ b/fs/kernel_read_file.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/fs.h>
+#include <linux/fs_struct.h>
+#include <linux/kernel_read_file.h>
+#include <linux/security.h>
+#include <linux/vmalloc.h>
+
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
+ loff_t max_size, enum kernel_read_file_id id)
+{
+ loff_t i_size, pos;
+ ssize_t bytes = 0;
+ void *allocated = NULL;
+ int ret;
+
+ if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ return -EINVAL;
+
+ ret = deny_write_access(file);
+ if (ret)
+ return ret;
+
+ ret = security_kernel_read_file(file, id);
+ if (ret)
+ goto out;
+
+ i_size = i_size_read(file_inode(file));
+ if (i_size <= 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ ret = -EFBIG;
+ goto out;
+ }
+
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
+ if (!*buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ pos = 0;
+ while (pos < i_size) {
+ bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+ if (bytes < 0) {
+ ret = bytes;
+ goto out_free;
+ }
+
+ if (bytes == 0)
+ break;
+ }
+
+ if (pos != i_size) {
+ ret = -EIO;
+ goto out_free;
+ }
+
+ ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ if (!ret)
+ *size = pos;
+
+out_free:
+ if (ret < 0) {
+ if (allocated) {
+ vfree(*buf);
+ *buf = NULL;
+ }
+ }
+
+out:
+ allow_write_access(file);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file);
+
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+ loff_t max_size, enum kernel_read_file_id id)
+{
+ struct file *file;
+ int ret;
+
+ if (!path || !*path)
+ return -EINVAL;
+
+ file = filp_open(path, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ ret = kernel_read_file(file, buf, size, max_size, id);
+ fput(file);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
+
+int kernel_read_file_from_path_initns(const char *path, void **buf,
+ loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id)
+{
+ struct file *file;
+ struct path root;
+ int ret;
+
+ if (!path || !*path)
+ return -EINVAL;
+
+ task_lock(&init_task);
+ get_fs_root(init_task.fs, &root);
+ task_unlock(&init_task);
+
+ file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
+ path_put(&root);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ ret = kernel_read_file(file, buf, size, max_size, id);
+ fput(file);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
+
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id)
+{
+ struct fd f = fdget(fd);
+ int ret = -EBADF;
+
+ if (!f.file)
+ goto out;
+
+ ret = kernel_read_file(f.file, buf, size, max_size, id);
+out:
+ fdput(f);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
--
2.25.1
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ++++---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++----
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, &buffer,
- &size, msize, id);
+ &size, msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error %d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown) \
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
loff_t size;
int rc;
key_perm_t perm;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e3fcad871861..15a44c5022f7 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -272,7 +272,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
static ssize_t ima_read_policy(char *path)
{
- void *data;
+ void *data = NULL;
char *datap;
loff_t size;
int rc, pathlen = strlen(path);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..f80ee4ce4669 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
{
/*
- * READING_FIRMWARE_PREALLOC_BUFFER
- *
* Do devices using pre-allocated memory run the risk of the
* firmware being accessible to the device prior to the completion
* of IMA's signature verification any more than when using two
- * buffers?
+ * buffers? It may be desirable to include the buffer address
+ * in this API and walk all the dma_map_single() mappings to check.
*/
return 0;
}
const int read_idmap[READING_MAX_ID] = {
[READING_FIRMWARE] = FIRMWARE_CHECK,
- [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
[READING_MODULE] = MODULE_CHECK,
[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
--
2.25.1
In preparation for further refactoring of kernel_read_file*(), rename
the "max_size" argument to the more accurate "buf_size", and correct
its type to size_t. Add kerndoc to explain the specifics of how the
arguments will be used. Note that with buf_size now size_t, it can no
longer be negative (and was never called with a negative value). Adjust
callers to use it as a "maximum size" when *buf is NULL.
Signed-off-by: Kees Cook <[email protected]>
---
fs/kernel_read_file.c | 34 +++++++++++++++++++++++---------
include/linux/kernel_read_file.h | 8 ++++----
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index dc28a8def597..e21a76001fff 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,15 +5,31 @@
#include <linux/security.h>
#include <linux/vmalloc.h>
+/**
+ * kernel_read_file() - read file contents into a kernel buffer
+ *
+ * @file file to read from
+ * @buf pointer to a "void *" buffer for reading into (if
+ * *@buf is NULL, a buffer will be allocated, and
+ * @buf_size will be ignored)
+ * @buf_size size of buf, if already allocated. If @buf not
+ * allocated, this is the largest size to allocate.
+ * @id the kernel_read_file_id identifying the type of
+ * file contents being read (for LSMs to examine)
+ *
+ * Returns number of bytes read (no single read will be bigger
+ * than INT_MAX), or negative on error.
+ *
+ */
int kernel_read_file(struct file *file, void **buf,
- loff_t max_size, enum kernel_read_file_id id)
+ size_t buf_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
void *allocated = NULL;
int ret;
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
ret = deny_write_access(file);
@@ -29,7 +45,7 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || i_size > buf_size) {
ret = -EFBIG;
goto out;
}
@@ -75,7 +91,7 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- loff_t max_size, enum kernel_read_file_id id)
+ size_t buf_size, enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -87,14 +103,14 @@ int kernel_read_file_from_path(const char *path, void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t max_size,
+ size_t buf_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -113,13 +129,13 @@ int kernel_read_file_from_path_initns(const char *path, void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
+int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -128,7 +144,7 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, max_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 0ca0bdbed1bd..910039e7593e 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -36,16 +36,16 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
}
int kernel_read_file(struct file *file,
- void **buf, loff_t max_size,
+ void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
- void **buf, loff_t max_size,
+ void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path_initns(const char *path,
- void **buf, loff_t max_size,
+ void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_fd(int fd,
- void **buf, loff_t max_size,
+ void **buf, size_t buf_size,
enum kernel_read_file_id id);
#endif /* _LINUX_KERNEL_READ_FILE_H */
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 97661ffabc4e..04f779c4f5ed 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -175,7 +175,7 @@ int __init integrity_load_x509(const unsigned int id, const char *path)
int rc;
key_perm_t perm;
- rc = kernel_read_file_from_path(path, &data, 0,
+ rc = kernel_read_file_from_path(path, &data, INT_MAX,
READING_X509_CERTIFICATE);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 9ba145d3d6d9..8695170d0e5c 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -284,7 +284,7 @@ static ssize_t ima_read_policy(char *path)
datap = path;
strsep(&datap, "\n");
- rc = kernel_read_file_from_path(path, &data, 0, READING_POLICY);
+ rc = kernel_read_file_from_path(path, &data, INT_MAX, READING_POLICY);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
--
2.25.1
The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm ware_request_platform()")
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (rc)
return rc; /* rc == -ENOENT when the fw was not found */
- fw_priv->data = vmalloc(size);
+ if (fw_priv->data && size > fw_priv->allocated_size)
+ return -ENOMEM;
+ if (!fw_priv->data)
+ fw_priv->data = vmalloc(size);
if (!fw_priv->data)
return -ENOMEM;
--
2.25.1
On 2020-07-17 10:42 a.m., Kees Cook wrote:
> The EFI platform firmware fallback would clobber any pre-allocated
> buffers. Instead, correctly refuse to reallocate when too small (as
> already done in the sysfs fallback), or perform allocation normally
> when needed.
>
> Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm ware_request_platform()")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: Scott Branden <[email protected]>
> ---
> To aid in backporting, this change is made before moving
> kernel_read_file() to separate header/source files.
> ---
> drivers/base/firmware_loader/fallback_platform.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
> index cdd2c9a9f38a..685edb7dd05a 100644
> --- a/drivers/base/firmware_loader/fallback_platform.c
> +++ b/drivers/base/firmware_loader/fallback_platform.c
> @@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
> if (rc)
> return rc; /* rc == -ENOENT when the fw was not found */
>
> - fw_priv->data = vmalloc(size);
> + if (fw_priv->data && size > fw_priv->allocated_size)
> + return -ENOMEM;
> + if (!fw_priv->data)
> + fw_priv->data = vmalloc(size);
> if (!fw_priv->data)
> return -ENOMEM;
>
On 2020-07-17 10:42 a.m., Kees Cook wrote:
> FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
> that are interested in filtering between types of things. The "how"
> should be an internal detail made uninteresting to the LSMs.
>
> Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
> Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures (pre-allocated buffer)")
> Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware (pre-allocated buffer)")
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: Scott Branden <[email protected]>
> ---
> To aid in backporting, this change is made before moving
> kernel_read_file() to separate header/source files.
> ---
> drivers/base/firmware_loader/main.c | 5 ++---
> fs/exec.c | 7 ++++---
> include/linux/fs.h | 2 +-
> kernel/module.c | 2 +-
> security/integrity/digsig.c | 2 +-
> security/integrity/ima/ima_fs.c | 2 +-
> security/integrity/ima/ima_main.c | 6 ++----
> 7 files changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
> index ca871b13524e..c2f57cedcd6f 100644
> --- a/drivers/base/firmware_loader/main.c
> +++ b/drivers/base/firmware_loader/main.c
> @@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
> int i, len;
> int rc = -ENOENT;
> char *path;
> - enum kernel_read_file_id id = READING_FIRMWARE;
> size_t msize = INT_MAX;
> void *buffer = NULL;
>
> /* Already populated data member means we're loading into a buffer */
> if (!decompress && fw_priv->data) {
> buffer = fw_priv->data;
> - id = READING_FIRMWARE_PREALLOC_BUFFER;
> msize = fw_priv->allocated_size;
> }
>
> @@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
>
> /* load firmware files from the mount namespace of init */
> rc = kernel_read_file_from_path_initns(path, &buffer,
> - &size, msize, id);
> + &size, msize,
> + READING_FIRMWARE);
> if (rc) {
> if (rc != -ENOENT)
> dev_warn(device, "loading %s failed with error %d\n",
> diff --git a/fs/exec.c b/fs/exec.c
> index e6e8a9a70327..2bf549757ce7 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> {
> loff_t i_size, pos;
> ssize_t bytes = 0;
> + void *allocated = NULL;
> int ret;
>
> if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> @@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> goto out;
> }
>
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> - *buf = vmalloc(i_size);
> + if (!*buf)
> + *buf = allocated = vmalloc(i_size);
> if (!*buf) {
> ret = -ENOMEM;
> goto out;
> @@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>
> out_free:
> if (ret < 0) {
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + if (allocated) {
> vfree(*buf);
> *buf = NULL;
> }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3f881a892ea7..95fc775ed937 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
> #endif
> extern int do_pipe_flags(int *, int);
>
> +/* This is a list of *what* is being read, not *how*. */
> #define __kernel_read_file_id(id) \
> id(UNKNOWN, unknown) \
> id(FIRMWARE, firmware) \
> - id(FIRMWARE_PREALLOC_BUFFER, firmware) \
> id(FIRMWARE_EFI_EMBEDDED, firmware) \
> id(MODULE, kernel-module) \
> id(KEXEC_IMAGE, kexec-image) \
> diff --git a/kernel/module.c b/kernel/module.c
> index 0c6573b98c36..26105148f4d2 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
> {
> struct load_info info = { };
> loff_t size;
> - void *hdr;
> + void *hdr = NULL;
> int err;
>
> err = may_init_module();
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index e9cbadade74b..ac02b7632353 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const void *data,
>
> int __init integrity_load_x509(const unsigned int id, const char *path)
> {
> - void *data;
> + void *data = NULL;
> loff_t size;
> int rc;
> key_perm_t perm;
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index e3fcad871861..15a44c5022f7 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -272,7 +272,7 @@ static const struct file_operations ima_ascii_measurements_ops = {
>
> static ssize_t ima_read_policy(char *path)
> {
> - void *data;
> + void *data = NULL;
> char *datap;
> loff_t size;
> int rc, pathlen = strlen(path);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index c1583d98c5e5..f80ee4ce4669 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -611,19 +611,17 @@ void ima_post_path_mknod(struct dentry *dentry)
> int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> {
> /*
> - * READING_FIRMWARE_PREALLOC_BUFFER
> - *
> * Do devices using pre-allocated memory run the risk of the
> * firmware being accessible to the device prior to the completion
> * of IMA's signature verification any more than when using two
> - * buffers?
> + * buffers? It may be desirable to include the buffer address
> + * in this API and walk all the dma_map_single() mappings to check.
> */
> return 0;
> }
>
> const int read_idmap[READING_MAX_ID] = {
> [READING_FIRMWARE] = FIRMWARE_CHECK,
> - [READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
> [READING_MODULE] = MODULE_CHECK,
> [READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
> [READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
On 2020-07-17 10:43 a.m., Kees Cook wrote:
> These routines are used in places outside of exec(2), so in preparation
> for refactoring them, move them into a separate source file,
> fs/kernel_read_file.c.
>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: Scott Branden <[email protected]>
> ---
> fs/Makefile | 3 +-
> fs/exec.c | 132 ----------------------------------------
> fs/kernel_read_file.c | 138 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 140 insertions(+), 133 deletions(-)
> create mode 100644 fs/kernel_read_file.c
>
> diff --git a/fs/Makefile b/fs/Makefile
> index 2ce5112b02c8..a05fc247b2a7 100644
> --- a/fs/Makefile
> +++ b/fs/Makefile
> @@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
> seq_file.o xattr.o libfs.o fs-writeback.o \
> pnode.o splice.o sync.o utimes.o d_path.o \
> stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
> - fs_types.o fs_context.o fs_parser.o fsopen.o
> + fs_types.o fs_context.o fs_parser.o fsopen.o \
> + kernel_read_file.o
>
> ifeq ($(CONFIG_BLOCK),y)
> obj-y += buffer.o block_dev.o direct-io.o mpage.o
> diff --git a/fs/exec.c b/fs/exec.c
> index 07a7fe9ac5be..d619b79aab30 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
> }
> EXPORT_SYMBOL(open_exec);
>
> -int kernel_read_file(struct file *file, void **buf, loff_t *size,
> - loff_t max_size, enum kernel_read_file_id id)
> -{
> - loff_t i_size, pos;
> - ssize_t bytes = 0;
> - void *allocated = NULL;
> - int ret;
> -
> - if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> - return -EINVAL;
> -
> - ret = deny_write_access(file);
> - if (ret)
> - return ret;
> -
> - ret = security_kernel_read_file(file, id);
> - if (ret)
> - goto out;
> -
> - i_size = i_size_read(file_inode(file));
> - if (i_size <= 0) {
> - ret = -EINVAL;
> - goto out;
> - }
> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> - ret = -EFBIG;
> - goto out;
> - }
> -
> - if (!*buf)
> - *buf = allocated = vmalloc(i_size);
> - if (!*buf) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - pos = 0;
> - while (pos < i_size) {
> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> - if (bytes < 0) {
> - ret = bytes;
> - goto out_free;
> - }
> -
> - if (bytes == 0)
> - break;
> - }
> -
> - if (pos != i_size) {
> - ret = -EIO;
> - goto out_free;
> - }
> -
> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
> - if (!ret)
> - *size = pos;
> -
> -out_free:
> - if (ret < 0) {
> - if (allocated) {
> - vfree(*buf);
> - *buf = NULL;
> - }
> - }
> -
> -out:
> - allow_write_access(file);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file);
> -
> -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
> - loff_t max_size, enum kernel_read_file_id id)
> -{
> - struct file *file;
> - int ret;
> -
> - if (!path || !*path)
> - return -EINVAL;
> -
> - file = filp_open(path, O_RDONLY, 0);
> - if (IS_ERR(file))
> - return PTR_ERR(file);
> -
> - ret = kernel_read_file(file, buf, size, max_size, id);
> - fput(file);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> -
> -int kernel_read_file_from_path_initns(const char *path, void **buf,
> - loff_t *size, loff_t max_size,
> - enum kernel_read_file_id id)
> -{
> - struct file *file;
> - struct path root;
> - int ret;
> -
> - if (!path || !*path)
> - return -EINVAL;
> -
> - task_lock(&init_task);
> - get_fs_root(init_task.fs, &root);
> - task_unlock(&init_task);
> -
> - file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
> - path_put(&root);
> - if (IS_ERR(file))
> - return PTR_ERR(file);
> -
> - ret = kernel_read_file(file, buf, size, max_size, id);
> - fput(file);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
> -
> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
> - enum kernel_read_file_id id)
> -{
> - struct fd f = fdget(fd);
> - int ret = -EBADF;
> -
> - if (!f.file)
> - goto out;
> -
> - ret = kernel_read_file(f.file, buf, size, max_size, id);
> -out:
> - fdput(f);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
> -
> #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
> defined(CONFIG_BINFMT_ELF_FDPIC)
> ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len)
> diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
> new file mode 100644
> index 000000000000..54d972d4befc
> --- /dev/null
> +++ b/fs/kernel_read_file.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/fs.h>
> +#include <linux/fs_struct.h>
> +#include <linux/kernel_read_file.h>
> +#include <linux/security.h>
> +#include <linux/vmalloc.h>
> +
> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> + loff_t max_size, enum kernel_read_file_id id)
> +{
> + loff_t i_size, pos;
> + ssize_t bytes = 0;
> + void *allocated = NULL;
> + int ret;
> +
> + if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> + return -EINVAL;
> +
> + ret = deny_write_access(file);
> + if (ret)
> + return ret;
> +
> + ret = security_kernel_read_file(file, id);
> + if (ret)
> + goto out;
> +
> + i_size = i_size_read(file_inode(file));
> + if (i_size <= 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> + ret = -EFBIG;
> + goto out;
> + }
> +
> + if (!*buf)
> + *buf = allocated = vmalloc(i_size);
> + if (!*buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + pos = 0;
> + while (pos < i_size) {
> + bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> + if (bytes < 0) {
> + ret = bytes;
> + goto out_free;
> + }
> +
> + if (bytes == 0)
> + break;
> + }
> +
> + if (pos != i_size) {
> + ret = -EIO;
> + goto out_free;
> + }
> +
> + ret = security_kernel_post_read_file(file, *buf, i_size, id);
> + if (!ret)
> + *size = pos;
> +
> +out_free:
> + if (ret < 0) {
> + if (allocated) {
> + vfree(*buf);
> + *buf = NULL;
> + }
> + }
> +
> +out:
> + allow_write_access(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file);
> +
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
> + loff_t max_size, enum kernel_read_file_id id)
> +{
> + struct file *file;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + file = filp_open(path, O_RDONLY, 0);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + ret = kernel_read_file(file, buf, size, max_size, id);
> + fput(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> +
> +int kernel_read_file_from_path_initns(const char *path, void **buf,
> + loff_t *size, loff_t max_size,
> + enum kernel_read_file_id id)
> +{
> + struct file *file;
> + struct path root;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + task_lock(&init_task);
> + get_fs_root(init_task.fs, &root);
> + task_unlock(&init_task);
> +
> + file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
> + path_put(&root);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + ret = kernel_read_file(file, buf, size, max_size, id);
> + fput(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
> +
> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
> + enum kernel_read_file_id id)
> +{
> + struct fd f = fdget(fd);
> + int ret = -EBADF;
> +
> + if (!f.file)
> + goto out;
> +
> + ret = kernel_read_file(f.file, buf, size, max_size, id);
> +out:
> + fdput(f);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
Hi Kees,
Thanks for sending out. This looks different than your other patch series.
We should get the first 5 patches accepted now though as they are
simple cleanups and fixes. That will reduce the number of outstanding
patches in the series.
At first glance the issue with the changes after that is the existing
API assumes it has read the whole file and failed if it did not.
Now, if the file is larger than the amount requested there is no indication?
On 2020-07-17 10:42 a.m., Kees Cook wrote:
> Hi,
>
> Here's my attempt at clearing the path to partial read support in
> kernel_read_file(), which fixes a number of issues along the way. I'm
> still fighting with the firmware test suite (it doesn't seem to pass
> for me even in stock v5.7... ?) But I don't want to block Scott's work[1]
> any this week, so here's the series as it is currently.
>
> The primary difference to Scott's approach is to avoid adding a new set of
> functions and just adapt the existing APIs to deal with "offset". Also,
> the fixes for the enum are first in the series so they can be backported
> without the header file relocation.
>
> I'll keep poking at the firmware tests...
>
> -Kees
>
> [1] https://lore.kernel.org/lkml/202007161415.10D015477@keescook/
>
> Kees Cook (12):
> firmware_loader: EFI firmware loader must handle pre-allocated buffer
> fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
> fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
> fs/kernel_read_file: Split into separate source file
> fs/kernel_read_file: Remove redundant size argument
> fs/kernel_read_file: Switch buffer size arg to size_t
> fs/kernel_read_file: Add file_size output argument
> LSM: Introduce kernel_post_load_data() hook
> firmware_loader: Use security_post_load_data()
> module: Call security_kernel_post_load_data()
> LSM: Add "contents" flag to kernel_read_file hook
> fs/kernel_file_read: Add "offset" arg for partial reads
>
> Scott Branden (1):
> fs/kernel_read_file: Split into separate include file
>
> drivers/base/firmware_loader/fallback.c | 8 +-
> .../base/firmware_loader/fallback_platform.c | 12 +-
> drivers/base/firmware_loader/main.c | 13 +-
> fs/Makefile | 3 +-
> fs/exec.c | 132 +-----------
> fs/kernel_read_file.c | 189 ++++++++++++++++++
> include/linux/fs.h | 39 ----
> include/linux/ima.h | 19 +-
> include/linux/kernel_read_file.h | 55 +++++
> include/linux/lsm_hook_defs.h | 6 +-
> include/linux/lsm_hooks.h | 12 ++
> include/linux/security.h | 19 +-
> kernel/kexec.c | 2 +-
> kernel/kexec_file.c | 18 +-
> kernel/module.c | 24 ++-
> security/integrity/digsig.c | 8 +-
> security/integrity/ima/ima_fs.c | 9 +-
> security/integrity/ima/ima_main.c | 58 ++++--
> security/integrity/ima/ima_policy.c | 1 +
> security/loadpin/loadpin.c | 17 +-
> security/security.c | 26 ++-
> security/selinux/hooks.c | 8 +-
> 22 files changed, 432 insertions(+), 246 deletions(-)
> create mode 100644 fs/kernel_read_file.c
> create mode 100644 include/linux/kernel_read_file.h
>
On Fri, Jul 17, 2020 at 12:17:02PM -0700, Scott Branden wrote:
> Thanks for sending out.? This looks different than your other patch series.
Yes, it mutated in my head as I considered how all of this should hang
together, which is why I wanted to get it sent before the weekend. I'm
still trying to figure out why the fireware testsuite fails for me, etc.
> We should get the first 5 patches accepted now though as they are
> simple cleanups and fixes.? That will reduce the number of outstanding
> patches in the series.
Agreed. I'd like to get some more eyes on it, but I can get it ready for
-next.
> At first glance the issue with the changes after that is the existing
> API assumes it has read the whole file and failed if it did not.
> Now, if the file is larger than the amount requested there is no indication?
The intention is to have old API users unchanged and new users can use
a pre-allocated buf (with buf_size) along with file_size to examine
their partial read progress. If I broke the old API, that's a bug and I
need to fix it, but that's why I wanted to start with the firmware test
suite (basic things like module loading work fine after this series, but
I wanted to really exercise the corners that the firmware suite pokes
at).
--
Kees Cook
From: Kees Cook
> Sent: 17 July 2020 18:43
> In preparation for further refactoring of kernel_read_file*(), rename
> the "max_size" argument to the more accurate "buf_size", and correct
> its type to size_t. Add kerndoc to explain the specifics of how the
> arguments will be used. Note that with buf_size now size_t, it can no
> longer be negative (and was never called with a negative value). Adjust
> callers to use it as a "maximum size" when *buf is NULL.
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> fs/kernel_read_file.c | 34 +++++++++++++++++++++++---------
> include/linux/kernel_read_file.h | 8 ++++----
> security/integrity/digsig.c | 2 +-
> security/integrity/ima/ima_fs.c | 2 +-
> 4 files changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
> index dc28a8def597..e21a76001fff 100644
> --- a/fs/kernel_read_file.c
> +++ b/fs/kernel_read_file.c
> @@ -5,15 +5,31 @@
> #include <linux/security.h>
> #include <linux/vmalloc.h>
>
> +/**
> + * kernel_read_file() - read file contents into a kernel buffer
> + *
> + * @file file to read from
> + * @buf pointer to a "void *" buffer for reading into (if
> + * *@buf is NULL, a buffer will be allocated, and
> + * @buf_size will be ignored)
> + * @buf_size size of buf, if already allocated. If @buf not
> + * allocated, this is the largest size to allocate.
> + * @id the kernel_read_file_id identifying the type of
> + * file contents being read (for LSMs to examine)
> + *
> + * Returns number of bytes read (no single read will be bigger
> + * than INT_MAX), or negative on error.
> + *
> + */
That seems to be self-inconsistent.
If '*buf' is NULL is both says that buf_size is ignored and
is treated as a limit.
To make life easier, zero should probably be treated as no-limit.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)