* [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-07-24 20:32 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, linux-audit, keescook, john.johansen, penguin-kernel, paul,
sds, linux-api
In-Reply-To: <20200724203226.16374-1-casey@schaufler-ca.com>
Add an entry /proc/.../attr/context which displays the full
process security "context" in compound format:
lsm1\0value\0lsm2\0value\0...
This entry is not writable.
A security module may decide that its policy does not allow
this information to be displayed. In this case none of the
information will be displayed.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-api@vger.kernel.org
---
Documentation/security/lsm.rst | 28 +++++++++++
fs/proc/base.c | 1 +
include/linux/lsm_hooks.h | 6 +++
security/apparmor/include/procattr.h | 2 +-
security/apparmor/lsm.c | 8 +++-
security/apparmor/procattr.c | 22 +++++----
security/security.c | 70 ++++++++++++++++++++++++++++
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
9 files changed, 126 insertions(+), 15 deletions(-)
diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
index 6a2a2e973080..fd4c87358d54 100644
--- a/Documentation/security/lsm.rst
+++ b/Documentation/security/lsm.rst
@@ -129,3 +129,31 @@ to identify it as the first security module to be registered.
The capabilities security module does not use the general security
blobs, unlike other modules. The reasons are historical and are
based on overhead, complexity and performance concerns.
+
+LSM External Interfaces
+=======================
+
+The LSM infrastructure does not generally provide external interfaces.
+The individual security modules provide what external interfaces they
+require.
+
+The file ``/sys/kernel/security/lsm`` provides a comma
+separated list of the active security modules.
+
+The file ``/proc/pid/attr/display`` contains the name of the security
+module for which the ``/proc/pid/attr/current`` interface will
+apply. This interface can be written to.
+
+The infrastructure does provide an interface for the special
+case where multiple security modules provide a process context.
+This is provided in compound context format.
+
+- `lsm\0value\0lsm\0value\0`
+
+The `lsm` and `value` fields are nul terminated bytestrings.
+Each field may contain whitespace or non-printable characters.
+The nul bytes are included in the size of a compound context.
+The context ``Bell\0Secret\0Biba\0Loose\0`` has a size of 23.
+
+The file ``/proc/pid/attr/context`` provides the security
+context of the identified process.
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 40471a12ced2..ba8b0316e999 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2795,6 +2795,7 @@ static const struct pid_entry attr_dir_stuff[] = {
ATTR(NULL, "keycreate", 0666),
ATTR(NULL, "sockcreate", 0666),
ATTR(NULL, "display", 0666),
+ ATTR(NULL, "context", 0444),
#ifdef CONFIG_SECURITY_SMACK
DIR("smack", 0555,
proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 6908fa03cf31..5be04dacc17a 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1359,6 +1359,12 @@
* @pages contains the number of pages.
* Return 0 if permission is granted.
*
+ * @getprocattr:
+ * Provide the named process attribute for display in special files in
+ * the /proc/.../attr directory. Attribute naming and the data displayed
+ * is at the discretion of the security modules. The exception is the
+ * "context" attribute, which will contain the security context of the
+ * task as a nul terminated text string without trailing whitespace.
* @ismaclabel:
* Check if the extended attribute specified by @name
* represents a MAC label. Returns 1 if name is a MAC
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
#ifndef __AA_PROCATTR_H
#define __AA_PROCATTR_H
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
int aa_setprocattr_changehat(char *args, size_t size, int flags);
#endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 31a6f11890f1..7ce570b0f491 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -593,6 +593,7 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
const struct cred *cred = get_task_cred(task);
struct aa_task_ctx *ctx = task_ctx(current);
struct aa_label *label = NULL;
+ bool newline = true;
if (strcmp(name, "current") == 0)
label = aa_get_newest_label(cred_label(cred));
@@ -600,11 +601,14 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
label = aa_get_newest_label(ctx->previous);
else if (strcmp(name, "exec") == 0 && ctx->onexec)
label = aa_get_newest_label(ctx->onexec);
- else
+ else if (strcmp(name, "context") == 0) {
+ label = aa_get_newest_label(cred_label(cred));
+ newline = false;
+ } else
error = -EINVAL;
if (label)
- error = aa_getprocattr(label, value);
+ error = aa_getprocattr(label, value, newline);
aa_put_label(label);
put_cred(cred);
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index c929bf4a3df1..be3b083d9b74 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
* aa_getprocattr - Return the profile information for @profile
* @profile: the profile to print profile info about (NOT NULL)
* @string: Returns - string containing the profile info (NOT NULL)
+ * @newline: Should a newline be added to @string.
*
* Returns: length of @string on success else error on failure
*
@@ -30,20 +31,21 @@
*
* Returns: size of string placed in @string else error code on failure
*/
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
{
struct aa_ns *ns = labels_ns(label);
struct aa_ns *current_ns = aa_get_current_ns();
+ int flags = FLAG_VIEW_SUBNS | FLAG_HIDDEN_UNCONFINED;
int len;
if (!aa_ns_visible(current_ns, ns, true)) {
aa_put_ns(current_ns);
return -EACCES;
}
+ if (newline)
+ flags |= FLAG_SHOW_MODE;
- len = aa_label_snxprint(NULL, 0, current_ns, label,
- FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
- FLAG_HIDDEN_UNCONFINED);
+ len = aa_label_snxprint(NULL, 0, current_ns, label, flags);
AA_BUG(len < 0);
*string = kmalloc(len + 2, GFP_KERNEL);
@@ -52,19 +54,19 @@ int aa_getprocattr(struct aa_label *label, char **string)
return -ENOMEM;
}
- len = aa_label_snxprint(*string, len + 2, current_ns, label,
- FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
- FLAG_HIDDEN_UNCONFINED);
+ len = aa_label_snxprint(*string, len + 2, current_ns, label, flags);
if (len < 0) {
aa_put_ns(current_ns);
return len;
}
- (*string)[len] = '\n';
- (*string)[len + 1] = 0;
+ if (newline) {
+ (*string)[len] = '\n';
+ (*string)[++len] = 0;
+ }
aa_put_ns(current_ns);
- return len + 1;
+ return len;
}
/**
diff --git a/security/security.c b/security/security.c
index d35e578fa45b..bce6be720401 100644
--- a/security/security.c
+++ b/security/security.c
@@ -754,6 +754,48 @@ static void __init lsm_early_task(struct task_struct *task)
panic("%s: Early task alloc failed.\n", __func__);
}
+/**
+ * append_ctx - append a lsm/context pair to a compound context
+ * @ctx: the existing compound context
+ * @ctxlen: size of the old context, including terminating nul byte
+ * @lsm: new lsm name, nul terminated
+ * @new: new context, possibly nul terminated
+ * @newlen: maximum size of @new
+ *
+ * replace @ctx with a new compound context, appending @newlsm and @new
+ * to @ctx. On exit the new data replaces the old, which is freed.
+ * @ctxlen is set to the new size, which includes a trailing nul byte.
+ *
+ * Returns 0 on success, -ENOMEM if no memory is available.
+ */
+static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
+ int newlen)
+{
+ char *final;
+ size_t llen;
+
+ llen = strlen(lsm) + 1;
+ /*
+ * A security module may or may not provide a trailing nul on
+ * when returning a security context. There is no definition
+ * of which it should be, and there are modules that do it
+ * each way.
+ */
+ newlen = strnlen(new, newlen) + 1;
+
+ final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
+ if (final == NULL)
+ return -ENOMEM;
+ if (*ctxlen)
+ memcpy(final, *ctx, *ctxlen);
+ memcpy(final + *ctxlen, lsm, llen);
+ memcpy(final + *ctxlen + llen, new, newlen);
+ kfree(*ctx);
+ *ctx = final;
+ *ctxlen = *ctxlen + llen + newlen;
+ return 0;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
@@ -2124,6 +2166,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
char **value)
{
struct security_hook_list *hp;
+ char *final = NULL;
+ char *cp;
+ int rc = 0;
+ int finallen = 0;
int display = lsm_task_display(current);
int slot = 0;
@@ -2151,6 +2197,30 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
return -ENOMEM;
}
+ if (!strcmp(name, "context")) {
+ hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
+ list) {
+ rc = hp->hook.getprocattr(p, "context", &cp);
+ if (rc == -EINVAL)
+ continue;
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
+ cp, rc);
+ kfree(cp);
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ }
+ if (final == NULL)
+ return -EINVAL;
+ *value = final;
+ return finallen;
+ }
+
hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
continue;
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c13c207c5da1..43d5c09b9a9e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6288,7 +6288,7 @@ static int selinux_getprocattr(struct task_struct *p,
goto bad;
}
- if (!strcmp(name, "current"))
+ if (!strcmp(name, "current") || !strcmp(name, "context"))
sid = __tsec->sid;
else if (!strcmp(name, "prev"))
sid = __tsec->osid;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6f0cdb40addc..d7bb6442f192 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3463,7 +3463,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
char *cp;
int slen;
- if (strcmp(name, "current") != 0)
+ if (strcmp(name, "current") != 0 && strcmp(name, "context") != 0)
return -EINVAL;
cp = kstrdup(skp->smk_known, GFP_KERNEL);
--
2.24.1
^ permalink raw reply related
* [PATCH v19 23/23] AppArmor: Remove the exclusive flag
From: Casey Schaufler @ 2020-07-24 20:32 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, linux-audit, keescook, john.johansen, penguin-kernel, paul,
sds
In-Reply-To: <20200724203226.16374-1-casey@schaufler-ca.com>
With the inclusion of the "display" process attribute
mechanism AppArmor no longer needs to be treated as an
"exclusive" security module. Remove the flag that indicates
it is exclusive. Remove the stub getpeersec_dgram AppArmor
hook as it has no effect in the single LSM case and
interferes in the multiple LSM case.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/apparmor/lsm.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 7ce570b0f491..4b7cbe9bb1be 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1129,22 +1129,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
return error;
}
-/**
- * apparmor_socket_getpeersec_dgram - get security label of packet
- * @sock: the peer socket
- * @skb: packet data
- * @secid: pointer to where to put the secid of the packet
- *
- * Sets the netlabel socket state on sk from parent
- */
-static int apparmor_socket_getpeersec_dgram(struct socket *sock,
- struct sk_buff *skb, u32 *secid)
-
-{
- /* TODO: requires secid support */
- return -ENOPROTOOPT;
-}
-
/**
* apparmor_sock_graft - Initialize newly created socket
* @sk: child sock
@@ -1248,8 +1232,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
#endif
LSM_HOOK_INIT(socket_getpeersec_stream,
apparmor_socket_getpeersec_stream),
- LSM_HOOK_INIT(socket_getpeersec_dgram,
- apparmor_socket_getpeersec_dgram),
LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
#ifdef CONFIG_NETWORK_SECMARK
LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
@@ -1918,7 +1900,7 @@ static int __init apparmor_init(void)
DEFINE_LSM(apparmor) = {
.name = "apparmor",
- .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
+ .flags = LSM_FLAG_LEGACY_MAJOR,
.enabled = &apparmor_enabled,
.blobs = &apparmor_blob_sizes,
.init = apparmor_init,
--
2.24.1
^ permalink raw reply related
* [PATCH v3 03/19] firmware_loader: EFI firmware loader must handle pre-allocated buffer
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, stable, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
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: stable@vger.kernel.org
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
^ permalink raw reply related
* [PATCH v3 15/19] IMA: Add support for file reads without contents
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
From: Scott Branden <scott.branden@broadcom.com>
When the kernel_read_file LSM hook is called with contents=false, IMA
can appraise the file directly, without requiring a filled buffer. When
such a buffer is available, though, IMA can continue to use it instead
of forcing a double read here.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Link: https://lore.kernel.org/lkml/20200706232309.12010-10-scott.branden@broadcom.com/
Signed-off-by: Kees Cook <keescook@chromium.org>
---
security/integrity/ima/ima_main.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dc4f90660aa6..459e50526a12 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -613,11 +613,8 @@ void ima_post_path_mknod(struct dentry *dentry)
int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
bool contents)
{
- /* Reject all partial reads during appraisal. */
- if (!contents) {
- if (ima_appraise & IMA_APPRAISE_ENFORCE)
- return -EACCES;
- }
+ enum ima_hooks func;
+ u32 secid;
/*
* Do devices using pre-allocated memory run the risk of the
@@ -626,7 +623,20 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
* 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;
+
+ /*
+ * There will be a call made to ima_post_read_file() with
+ * a filled buffer, so we don't need to perform an extra
+ * read early here.
+ */
+ if (contents)
+ return 0;
+
+ /* Read entire file for all partial reads during appraisal. */
+ func = read_idmap[read_id] ?: FILE_CHECK;
+ security_task_getsecid(current, &secid);
+ return process_measurement(file, current_cred(), secid, NULL,
+ 0, MAY_READ, func);
}
const int read_idmap[READING_MAX_ID] = {
--
2.25.1
^ permalink raw reply related
* [PATCH v3 14/19] LSM: Add "contents" flag to kernel_read_file hook
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
As with the kernel_load_data LSM hook, add a "contents" flag to the
kernel_read_file LSM hook that indicates whether the LSM can expect
a matching call to the kernel_post_read_file LSM hook with the full
contents of the file. With the coming addition of partial file read
support for kernel_read_file*() API, the LSM will no longer be able
to always see the entire contents of a file during the read calls.
For cases where the LSM must read examine the complete file contents,
it will need to do so on its own every time the kernel_read_file
hook is called with contents=false (or reject such cases). Adjust all
existing LSMs to retain existing behavior.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
fs/kernel_read_file.c | 2 +-
include/linux/ima.h | 6 ++++--
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 3 +++
include/linux/security.h | 6 ++++--
security/integrity/ima/ima_main.c | 10 +++++++++-
security/loadpin/loadpin.c | 14 ++++++++++++--
security/security.c | 7 ++++---
security/selinux/hooks.c | 5 +++--
9 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 2e29c38eb4df..d73bc3fa710a 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -39,7 +39,7 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id);
+ ret = security_kernel_read_file(file, id, true);
if (ret)
goto out;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 502e36ad7804..259023039dc9 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,7 +23,8 @@ extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id, bool contents);
extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
@@ -91,7 +92,8 @@ static inline int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index aaa2916bbae7..c2ded57c5d9b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -188,7 +188,7 @@ LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_read_file, struct file *file,
- enum kernel_read_file_id id)
+ enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 812d626195fc..b66433b5aa15 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -650,6 +650,7 @@
* @file contains the file structure pointing to the file being read
* by the kernel.
* @id kernel read file identifier
+ * @contents if a subsequent @kernel_post_read_file will be called.
* Return 0 if permission is granted.
* @kernel_post_read_file:
* Read a file specified by userspace.
@@ -658,6 +659,8 @@
* @buf pointer to buffer containing the file contents.
* @size length of the file contents.
* @id kernel read file identifier
+ * This must be paired with a prior @kernel_read_file call that had
+ * @contents set to true.
* Return 0 if permission is granted.
* @task_fix_setuid:
* Update the module's state after setting one or more of the user
diff --git a/include/linux/security.h b/include/linux/security.h
index e748974c707b..a5d66b89cd6c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -390,7 +390,8 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
int security_kernel_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
+int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
int security_task_fix_setuid(struct cred *new, const struct cred *old,
@@ -1028,7 +1029,8 @@ static inline int security_kernel_post_load_data(char *buf, loff_t size,
}
static inline int security_kernel_read_file(struct file *file,
- enum kernel_read_file_id id)
+ enum kernel_read_file_id id,
+ bool contents)
{
return 0;
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 1a7bc4c7437d..dc4f90660aa6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -602,6 +602,7 @@ void ima_post_path_mknod(struct dentry *dentry)
* ima_read_file - pre-measure/appraise hook decision based on policy
* @file: pointer to the file to be measured/appraised/audit
* @read_id: caller identifier
+ * @contents: whether a subsequent call will be made to ima_post_read_file()
*
* Permit reading a file based on policy. The policy rules are written
* in terms of the policy identifier. Appraising the integrity of
@@ -609,8 +610,15 @@ void ima_post_path_mknod(struct dentry *dentry)
*
* For permission return 0, otherwise return -EACCES.
*/
-int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
+int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
+ bool contents)
{
+ /* Reject all partial reads during appraisal. */
+ if (!contents) {
+ if (ima_appraise & IMA_APPRAISE_ENFORCE)
+ return -EACCES;
+ }
+
/*
* Do devices using pre-allocated memory run the risk of the
* firmware being accessible to the device prior to the completion
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index db320a43f42e..a1778ebef137 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -117,11 +117,21 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb)
}
}
-static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
struct super_block *load_root;
const char *origin = kernel_read_file_id_str(id);
+ /*
+ * If we will not know that we'll be seeing the full contents
+ * then we cannot trust a load will be complete and unchanged
+ * off disk. Treat all contents=false hooks as if there were
+ * no associated file struct.
+ */
+ if (!contents)
+ file = NULL;
+
/* If the file id is excluded, ignore the pinning. */
if ((unsigned int)id < ARRAY_SIZE(ignore_read_file_id) &&
ignore_read_file_id[id]) {
@@ -178,7 +188,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
static int loadpin_load_data(enum kernel_load_data_id id, bool contents)
{
- return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
+ return loadpin_read_file(NULL, (enum kernel_read_file_id) id, contents);
}
static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
diff --git a/security/security.c b/security/security.c
index 090674f1197a..800af5403176 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1657,14 +1657,15 @@ int security_kernel_module_request(char *kmod_name)
return integrity_kernel_module_request(kmod_name);
}
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
+int security_kernel_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
int ret;
- ret = call_int_hook(kernel_read_file, 0, file, id);
+ ret = call_int_hook(kernel_read_file, 0, file, id, contents);
if (ret)
return ret;
- return ima_read_file(file, id);
+ return ima_read_file(file, id, contents);
}
EXPORT_SYMBOL_GPL(security_kernel_read_file);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 1a5c68196faf..6d183bbc12a6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4004,13 +4004,14 @@ static int selinux_kernel_module_from_file(struct file *file)
}
static int selinux_kernel_read_file(struct file *file,
- enum kernel_read_file_id id)
+ enum kernel_read_file_id id,
+ bool contents)
{
int rc = 0;
switch (id) {
case READING_MODULE:
- rc = selinux_kernel_module_from_file(file);
+ rc = selinux_kernel_module_from_file(contents ? file : NULL);
break;
default:
break;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 11/19] LSM: Introduce kernel_post_load_data() hook
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
There are a few places in the kernel where LSMs would like to have
visibility into the contents of a kernel buffer that has been loaded or
read. While security_kernel_post_read_file() (which includes the
buffer) exists as a pairing for security_kernel_read_file(), no such
hook exists to pair with security_kernel_load_data().
Earlier proposals for just using security_kernel_post_read_file() with a
NULL file argument were rejected (i.e. "file" should always be valid for
the security_..._file hooks, but it appears at least one case was
left in the kernel during earlier refactoring. (This will be fixed in
a subsequent patch.)
Since not all cases of security_kernel_load_data() can have a single
contiguous buffer made available to the LSM hook (e.g. kexec image
segments are separately loaded), there needs to be a way for the LSM to
reason about its expectations of the hook coverage. In order to handle
this, add a "contents" argument to the "kernel_load_data" hook that
indicates if the newly added "kernel_post_load_data" hook will be called
with the full contents once loaded. That way, LSMs requiring full contents
can choose to unilaterally reject "kernel_load_data" with contents=false
(which is effectively the existing hook coverage), but when contents=true
they can allow it and later evaluate the "kernel_post_load_data" hook
once the buffer is loaded.
With this change, LSMs can gain coverage over non-file-backed data loads
(e.g. init_module(2) and firmware userspace helper), which will happen
in subsequent patches.
Additionally prepare IMA to start processing these cases.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/fallback.c | 2 +-
.../base/firmware_loader/fallback_platform.c | 2 +-
include/linux/ima.h | 12 +++++++++--
include/linux/lsm_hook_defs.h | 4 +++-
include/linux/lsm_hooks.h | 9 ++++++++
include/linux/security.h | 12 +++++++++--
kernel/kexec.c | 2 +-
kernel/module.c | 2 +-
security/integrity/ima/ima_main.c | 21 ++++++++++++++++++-
security/loadpin/loadpin.c | 2 +-
security/security.c | 18 +++++++++++++---
security/selinux/hooks.c | 2 +-
12 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 5327bfc6ba71..a196aacce22c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, false);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index 6958ab1a8059..a12c79d47efc 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, false);
if (rc)
return rc;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 148636bfcc8f..502e36ad7804 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,9 @@ extern void ima_post_create_tmpfile(struct inode *inode);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
-extern int ima_load_data(enum kernel_load_data_id id);
+extern int ima_load_data(enum kernel_load_data_id id, bool contents);
+extern int ima_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
@@ -78,7 +80,13 @@ static inline int ima_file_mprotect(struct vm_area_struct *vma,
return 0;
}
-static inline int ima_load_data(enum kernel_load_data_id id)
+static inline int ima_load_data(enum kernel_load_data_id id, bool contents)
+{
+ return 0;
+}
+
+static inline int ima_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 6791813cd439..aaa2916bbae7 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -184,7 +184,9 @@ LSM_HOOK(void, LSM_RET_VOID, cred_getsecid, const struct cred *c, u32 *secid)
LSM_HOOK(int, 0, kernel_act_as, struct cred *new, u32 secid)
LSM_HOOK(int, 0, kernel_create_files_as, struct cred *new, struct inode *inode)
LSM_HOOK(int, 0, kernel_module_request, char *kmod_name)
-LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id)
+LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id id, bool contents)
+LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
+ enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_read_file, struct file *file,
enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 95b7c1d32062..812d626195fc 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -635,7 +635,16 @@
* @kernel_load_data:
* Load data provided by userspace.
* @id kernel load data identifier
+ * @contents if a subsequent @kernel_post_load_data will be called.
* Return 0 if permission is granted.
+ * @kernel_post_load_data:
+ * Load data provided by a non-file source (usually userspace buffer).
+ * @buf pointer to buffer containing the data contents.
+ * @size length of the data contents.
+ * @id kernel load data identifier
+ * Return 0 if permission is granted.
+ * This must be paired with a prior @kernel_load_data call that had
+ * @contents set to true.
* @kernel_read_file:
* Read a file specified by userspace.
* @file contains the file structure pointing to the file being read
diff --git a/include/linux/security.h b/include/linux/security.h
index 42df0d9b4c37..e748974c707b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -387,7 +387,9 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
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_module_request(char *kmod_name);
-int security_kernel_load_data(enum kernel_load_data_id id);
+int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
+int security_kernel_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id);
int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
enum kernel_read_file_id id);
@@ -1014,7 +1016,13 @@ static inline int security_kernel_module_request(char *kmod_name)
return 0;
}
-static inline int security_kernel_load_data(enum kernel_load_data_id id)
+static inline int security_kernel_load_data(enum kernel_load_data_id id, bool contents)
+{
+ return 0;
+}
+
+static inline int security_kernel_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id)
{
return 0;
}
diff --git a/kernel/kexec.c b/kernel/kexec.c
index f977786fe498..c82c6c06f051 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -205,7 +205,7 @@ static inline int kexec_load_check(unsigned long nr_segments,
return -EPERM;
/* Permit LSMs and IMA to fail the kexec */
- result = security_kernel_load_data(LOADING_KEXEC_IMAGE);
+ result = security_kernel_load_data(LOADING_KEXEC_IMAGE, false);
if (result < 0)
return result;
diff --git a/kernel/module.c b/kernel/module.c
index 860d713dd910..d56cb34d9a2f 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2967,7 +2967,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_load_data(LOADING_MODULE);
+ err = security_kernel_load_data(LOADING_MODULE, false);
if (err)
return err;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dab4a13221cf..85000dc8595c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -676,6 +676,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
/**
* ima_load_data - appraise decision based on policy
* @id: kernel load data caller identifier
+ * @contents: whether the full contents will be available in a later
+ * call to ima_post_load_data().
*
* Callers of this LSM hook can not measure, appraise, or audit the
* data provided by userspace. Enforce policy rules requring a file
@@ -683,7 +685,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
*
* For permission return 0, otherwise return -EACCES.
*/
-int ima_load_data(enum kernel_load_data_id id)
+int ima_load_data(enum kernel_load_data_id id, bool contents)
{
bool ima_enforce, sig_enforce;
@@ -723,6 +725,23 @@ int ima_load_data(enum kernel_load_data_id id)
return 0;
}
+/**
+ * ima_post_load_data - appraise decision based on policy
+ * @buf: pointer to in memory file contents
+ * @size: size of in memory file contents
+ * @id: kernel load data caller identifier
+ *
+ * Measure/appraise/audit in memory buffer based on policy. Policy rules
+ * are written in terms of a policy identifier.
+ *
+ * On success return 0. On integrity appraisal error, assuming the file
+ * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
+ */
+int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id load_id)
+{
+ return 0;
+}
+
/*
* process_buffer_measurement - Measure the buffer to ima log.
* @buf: pointer to the buffer that needs to be added to the log.
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 81bc95127f92..db320a43f42e 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -176,7 +176,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
return 0;
}
-static int loadpin_load_data(enum kernel_load_data_id id)
+static int loadpin_load_data(enum kernel_load_data_id id, bool contents)
{
return loadpin_read_file(NULL, (enum kernel_read_file_id) id);
}
diff --git a/security/security.c b/security/security.c
index f5920115a325..090674f1197a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1680,17 +1680,29 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
}
EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
-int security_kernel_load_data(enum kernel_load_data_id id)
+int security_kernel_load_data(enum kernel_load_data_id id, bool contents)
{
int ret;
- ret = call_int_hook(kernel_load_data, 0, id);
+ ret = call_int_hook(kernel_load_data, 0, id, contents);
if (ret)
return ret;
- return ima_load_data(id);
+ return ima_load_data(id, contents);
}
EXPORT_SYMBOL_GPL(security_kernel_load_data);
+int security_kernel_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id)
+{
+ int ret;
+
+ ret = call_int_hook(kernel_post_load_data, 0, buf, size, id);
+ if (ret)
+ return ret;
+ return ima_post_load_data(buf, size, id);
+}
+EXPORT_SYMBOL_GPL(security_kernel_post_load_data);
+
int security_task_fix_setuid(struct cred *new, const struct cred *old,
int flags)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 5de45010fb1a..1a5c68196faf 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4019,7 +4019,7 @@ static int selinux_kernel_read_file(struct file *file,
return rc;
}
-static int selinux_kernel_load_data(enum kernel_load_data_id id)
+static int selinux_kernel_load_data(enum kernel_load_data_id id, bool contents)
{
int rc = 0;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 13/19] module: Call security_kernel_post_load_data()
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Jessica Yu, Scott Branden, Mimi Zohar,
Luis Chamberlain, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
Now that there is an API for checking loaded contents for modules
loaded without a file, call into the LSM hooks.
Cc: Jessica Yu <jeyu@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
kernel/module.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index d56cb34d9a2f..90a4788dff9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2967,7 +2967,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_load_data(LOADING_MODULE, false);
+ err = security_kernel_load_data(LOADING_MODULE, true);
if (err)
return err;
@@ -2977,11 +2977,17 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
return -ENOMEM;
if (copy_chunked_from_user(info->hdr, umod, info->len) != 0) {
- vfree(info->hdr);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
- return 0;
+ err = security_kernel_post_load_data((char *)info->hdr, info->len,
+ LOADING_MODULE);
+out:
+ if (err)
+ vfree(info->hdr);
+
+ return err;
}
static void free_copy(struct load_info *info)
--
2.25.1
^ permalink raw reply related
* [PATCH v3 12/19] firmware_loader: Use security_post_load_data()
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/fallback.c | 8 ++++----
.../base/firmware_loader/fallback_platform.c | 7 ++++++-
security/integrity/ima/ima_main.c | 20 +++++++++----------
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index a196aacce22c..7cfdfdcb819c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -272,9 +272,9 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: map pages failed\n",
__func__);
else
- rc = security_kernel_post_read_file(NULL,
- fw_priv->data, fw_priv->size,
- READING_FIRMWARE);
+ rc = security_kernel_post_load_data(fw_priv->data,
+ fw_priv->size,
+ LOADING_FIRMWARE);
/*
* Same logic as fw_load_abort, only the DONE bit
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE, false);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, true);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index a12c79d47efc..4d1157af0e86 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE, false);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, true);
if (rc)
return rc;
@@ -27,6 +27,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (fw_priv->data && size > fw_priv->allocated_size)
return -ENOMEM;
+
+ rc = security_kernel_post_load_data((u8 *)data, size, LOADING_FIRMWARE);
+ if (rc)
+ return rc;
+
if (!fw_priv->data)
fw_priv->data = vmalloc(size);
if (!fw_priv->data)
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 85000dc8595c..1a7bc4c7437d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -648,15 +648,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum ima_hooks func;
u32 secid;
- if (!file && read_id == READING_FIRMWARE) {
- if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("Prevent firmware loading_store.\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0;
- }
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
}
break;
case LOADING_FIRMWARE:
- if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
+ if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) && !contents) {
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool contents)
*/
int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id load_id)
{
+ if (load_id == LOADING_FIRMWARE) {
+ if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+ (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+ pr_err("Prevent firmware loading_store.\n");
+ return -EACCES; /* INTEGRITY_UNKNOWN */
+ }
+ return 0;
+ }
+
return 0;
}
--
2.25.1
^ permalink raw reply related
* [PATCH v3 09/19] fs/kernel_read_file: Switch buffer size arg to size_t
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
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.
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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 602f52717757..692b83e82edf 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
^ permalink raw reply related
* [PATCH v3 10/19] fs/kernel_read_file: Add file_size output argument
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
In preparation for adding partial read support, add an optional output
argument to kernel_read_file*() that reports the file size so callers
can reason more easily about their reading progress.
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/main.c | 1 +
fs/kernel_read_file.c | 19 +++++++++++++------
include/linux/kernel_read_file.h | 4 ++++
kernel/kexec_file.c | 4 ++--
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
7 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index f80c0d102be8..bd199404935f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -495,6 +495,7 @@ 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, msize,
+ NULL,
READING_FIRMWARE);
if (rc < 0) {
if (rc != -ENOENT)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index e21a76001fff..2e29c38eb4df 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -14,6 +14,8 @@
* @buf_size will be ignored)
* @buf_size size of buf, if already allocated. If @buf not
* allocated, this is the largest size to allocate.
+ * @file_size if non-NULL, the full size of @file will be
+ * written here.
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
@@ -22,7 +24,8 @@
*
*/
int kernel_read_file(struct file *file, void **buf,
- size_t buf_size, enum kernel_read_file_id id)
+ size_t buf_size, size_t *file_size,
+ enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
@@ -49,6 +52,8 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EFBIG;
goto out;
}
+ if (file_size)
+ *file_size = i_size;
if (!*buf)
*buf = allocated = vmalloc(i_size);
@@ -91,7 +96,8 @@ 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,
- size_t buf_size, enum kernel_read_file_id id)
+ size_t buf_size, size_t *file_size,
+ enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -103,14 +109,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, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_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,
- size_t buf_size,
+ size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -129,13 +135,14 @@ 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, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_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, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -144,7 +151,7 @@ int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, buf_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 910039e7593e..023293eaf948 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -37,15 +37,19 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
int kernel_read_file(struct file *file,
void **buf, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
void **buf, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path_initns(const char *path,
void **buf, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_fd(int fd,
void **buf, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id);
#endif /* _LINUX_KERNEL_READ_FILE_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index eda19ca256a3..878ca684a3a1 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -222,7 +222,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
void *ldata;
ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
- INT_MAX, READING_KEXEC_IMAGE);
+ INT_MAX, NULL, READING_KEXEC_IMAGE);
if (ret < 0)
return ret;
image->kernel_buf_len = ret;
@@ -242,7 +242,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
/* It is possible that there no initramfs is being loaded */
if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
- INT_MAX,
+ INT_MAX, NULL,
READING_KEXEC_INITRAMFS);
if (ret < 0)
goto out;
diff --git a/kernel/module.c b/kernel/module.c
index b6fd4f51cc30..860d713dd910 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4001,7 +4001,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
|MODULE_INIT_IGNORE_VERMAGIC))
return -EINVAL;
- err = kernel_read_file_from_fd(fd, &hdr, INT_MAX,
+ err = kernel_read_file_from_fd(fd, &hdr, INT_MAX, NULL,
READING_MODULE);
if (err < 0)
return err;
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 04f779c4f5ed..8a523dfd7fd7 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, INT_MAX,
+ rc = kernel_read_file_from_path(path, &data, INT_MAX, NULL,
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 692b83e82edf..5fc56ccb6678 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, INT_MAX, READING_POLICY);
+ rc = kernel_read_file_from_path(path, &data, INT_MAX, NULL, READING_POLICY);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 08/19] fs/kernel_read_file: Remove redundant size argument
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/main.c | 10 ++++++----
fs/kernel_read_file.c | 20 +++++++++-----------
include/linux/kernel_read_file.h | 8 ++++----
kernel/kexec_file.c | 14 +++++++-------
kernel/module.c | 7 +++----
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 6 ++++--
7 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..f80c0d102be8 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, &buffer,
- &size, msize,
+ rc = kernel_read_file_from_path_initns(path, &buffer, msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error %d\n",
path, rc);
@@ -506,6 +505,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
+ rc = 0;
+
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include <linux/security.h>
#include <linux/vmalloc.h>
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, 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,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,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, size, max_size, id);
+ ret = kernel_read_file(file, buf, 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,
+int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -130,7 +128,7 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, size, max_size, id);
+ ret = kernel_read_file(f.file, buf, max_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 78cf3d7dc835..0ca0bdbed1bd 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 *size, loff_t max_size,
+ void **buf, 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,
+ void **buf, 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,
+ void **buf, 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,
+ void **buf, loff_t max_size,
enum kernel_read_file_id id);
#endif /* _LINUX_KERNEL_READ_FILE_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 1358069ce9e9..eda19ca256a3 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -220,13 +220,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
{
int ret;
void *ldata;
- loff_t size;
ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
- &size, INT_MAX, READING_KEXEC_IMAGE);
- if (ret)
+ INT_MAX, READING_KEXEC_IMAGE);
+ if (ret < 0)
return ret;
- image->kernel_buf_len = size;
+ image->kernel_buf_len = ret;
/* Call arch image probe handlers */
ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
@@ -243,11 +242,12 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
/* It is possible that there no initramfs is being loaded */
if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
- &size, INT_MAX,
+ INT_MAX,
READING_KEXEC_INITRAMFS);
- if (ret)
+ if (ret < 0)
goto out;
- image->initrd_buf_len = size;
+ image->initrd_buf_len = ret;
+ ret = 0;
}
if (cmdline_len) {
diff --git a/kernel/module.c b/kernel/module.c
index e9765803601b..b6fd4f51cc30 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,6 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
{
struct load_info info = { };
- loff_t size;
void *hdr = NULL;
int err;
@@ -4002,12 +4001,12 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
|MODULE_INIT_IGNORE_VERMAGIC))
return -EINVAL;
- err = kernel_read_file_from_fd(fd, &hdr, &size, INT_MAX,
+ err = kernel_read_file_from_fd(fd, &hdr, INT_MAX,
READING_MODULE);
- if (err)
+ if (err < 0)
return err;
info.hdr = hdr;
- info.len = size;
+ info.len = err;
return load_module(&info, uargs, flags);
}
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index f8869be45d8f..97661ffabc4e 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -171,16 +171,17 @@ 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 = NULL;
- loff_t size;
+ size_t size;
int rc;
key_perm_t perm;
- rc = kernel_read_file_from_path(path, &data, &size, 0,
+ rc = kernel_read_file_from_path(path, &data, 0,
READING_X509_CERTIFICATE);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
}
+ size = rc;
perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW | KEY_USR_READ;
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e13ffece3726..602f52717757 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -275,7 +275,7 @@ static ssize_t ima_read_policy(char *path)
{
void *data = NULL;
char *datap;
- loff_t size;
+ size_t size;
int rc, pathlen = strlen(path);
char *p;
@@ -284,11 +284,13 @@ static ssize_t ima_read_policy(char *path)
datap = path;
strsep(&datap, "\n");
- rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY);
+ rc = kernel_read_file_from_path(path, &data, 0, READING_POLICY);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
}
+ size = rc;
+ rc = 0;
datap = data;
while (size > 0 && (p = strsep(&datap, "\n"))) {
--
2.25.1
^ permalink raw reply related
* [PATCH v3 06/19] fs/kernel_read_file: Split into separate include file
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Christoph Hellwig, Mimi Zohar,
Luis Chamberlain, Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
From: Scott Branden <scott.branden@broadcom.com>
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 <hch@lst.de>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Link: https://lore.kernel.org/r/20200706232309.12010-2-scott.branden@broadcom.com
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
^ permalink raw reply related
* [PATCH v3 07/19] fs/kernel_read_file: Split into separate source file
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
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.
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
^ permalink raw reply related
* [PATCH v3 05/19] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, stable, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firmware_request_platform()")
Cc: stable@vger.kernel.org
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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 | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +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*. */
+/* 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(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image) \
id(KEXEC_INITRAMFS, kexec-initramfs) \
--
2.25.1
^ permalink raw reply related
* [PATCH v3 02/19] selftest/firmware: Add selftest timeout in settings
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, SeongJae Park, Scott Branden, Mimi Zohar,
Luis Chamberlain, Jessica Yu, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
The firmware tests would always time out for me. Add a correct timeout,
including details on how the value was reached. Additionally allow the
test harness to skip comments in settings files and report how long a
given timeout was.
Reviewed-by: SeongJae Park <sjpark@amazon.de>
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
tools/testing/selftests/firmware/settings | 8 ++++++++
tools/testing/selftests/kselftest/runner.sh | 6 +++++-
2 files changed, 13 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/firmware/settings
diff --git a/tools/testing/selftests/firmware/settings b/tools/testing/selftests/firmware/settings
new file mode 100644
index 000000000000..085e664ee093
--- /dev/null
+++ b/tools/testing/selftests/firmware/settings
@@ -0,0 +1,8 @@
+# The async firmware timeout is set to 1 second (but ends up being effectively
+# 2 seconds). There are 3 test configs, each done with and without firmware
+# present, each with 2 "nowait" functions tested 5 times. Expected time for a
+# normal execution should be 2 * 3 * 2 * 2 * 5 = 120 seconds for those alone.
+# Additionally, fw_fallback may take 5 seconds for internal timeouts in each
+# of the 3 configs, so at least another 15 seconds are needed. Add another
+# 10 seconds for each testing config: 120 + 15 + 30
+timeout=165
diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index 676b3a8b114d..cd5ddf979f15 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -53,6 +53,10 @@ run_one()
settings="$BASE_DIR/$DIR/settings"
if [ -r "$settings" ] ; then
while read line ; do
+ # Skip comments.
+ if echo "$line" | grep -q '^#'; then
+ continue
+ fi
field=$(echo "$line" | cut -d= -f1)
value=$(echo "$line" | cut -d= -f2-)
eval "kselftest_$field"="$value"
@@ -80,7 +84,7 @@ run_one()
echo "not ok $test_num $TEST_HDR_MSG # SKIP"
elif [ $rc -eq $timeout_rc ]; then \
echo "#"
- echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT"
+ echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT $kselftest_timeout seconds"
else
echo "not ok $test_num $TEST_HDR_MSG # exit=$rc"
fi)
--
2.25.1
^ permalink raw reply related
* [PATCH v3 01/19] test_firmware: Test platform fw loading on non-EFI systems
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, stable, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
On non-EFI systems, it wasn't possible to test the platform firmware
loader because it will have never set "checked_fw" during __init.
Instead, allow the test code to override this check. Additionally split
the declarations into a private header file so it there is greater
enforcement of the symbol visibility.
Fixes: 548193cba2a7 ("test_firmware: add support for firmware_request_platform")
Cc: stable@vger.kernel.org
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/firmware/efi/embedded-firmware.c | 21 ++++++++++++++++-----
drivers/firmware/efi/embedded-firmware.h | 19 +++++++++++++++++++
include/linux/efi_embedded_fw.h | 13 -------------
lib/test_firmware.c | 5 +++++
4 files changed, 40 insertions(+), 18 deletions(-)
create mode 100644 drivers/firmware/efi/embedded-firmware.h
diff --git a/drivers/firmware/efi/embedded-firmware.c b/drivers/firmware/efi/embedded-firmware.c
index a1b199de9006..0fb03cd0a5a2 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -14,11 +14,22 @@
#include <linux/vmalloc.h>
#include <crypto/sha.h>
+#include "embedded-firmware.h"
+
+#ifdef CONFIG_TEST_FIRMWARE
+# define EFI_EMBEDDED_FW_VISIBILITY
+#else
+# define EFI_EMBEDDED_FW_VISIBILITY static
+#endif
+
+EFI_EMBEDDED_FW_VISIBILITY LIST_HEAD(efi_embedded_fw_list);
+EFI_EMBEDDED_FW_VISIBILITY bool efi_embedded_fw_checked;
+
/* Exported for use by lib/test_firmware.c only */
-LIST_HEAD(efi_embedded_fw_list);
+#ifdef CONFIG_TEST_FIRMWARE
EXPORT_SYMBOL_GPL(efi_embedded_fw_list);
-
-static bool checked_for_fw;
+EXPORT_SYMBOL_GPL(efi_embedded_fw_checked);
+#endif
static const struct dmi_system_id * const embedded_fw_table[] = {
#ifdef CONFIG_TOUCHSCREEN_DMI
@@ -119,14 +130,14 @@ void __init efi_check_for_embedded_firmwares(void)
}
}
- checked_for_fw = true;
+ efi_embedded_fw_checked = true;
}
int efi_get_embedded_fw(const char *name, const u8 **data, size_t *size)
{
struct efi_embedded_fw *iter, *fw = NULL;
- if (!checked_for_fw) {
+ if (!efi_embedded_fw_checked) {
pr_warn("Warning %s called while we did not check for embedded fw\n",
__func__);
return -ENOENT;
diff --git a/drivers/firmware/efi/embedded-firmware.h b/drivers/firmware/efi/embedded-firmware.h
new file mode 100644
index 000000000000..34113316d068
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _EFI_EMBEDDED_FW_INTERNAL_H_
+#define _EFI_EMBEDDED_FW_INTERNAL_H_
+
+/*
+ * This struct and efi_embedded_fw_list are private to the efi-embedded fw
+ * implementation they only in separate header for use by lib/test_firmware.c.
+ */
+struct efi_embedded_fw {
+ struct list_head list;
+ const char *name;
+ const u8 *data;
+ size_t length;
+};
+
+extern struct list_head efi_embedded_fw_list;
+extern bool efi_embedded_fw_checked;
+
+#endif /* _EFI_EMBEDDED_FW_INTERNAL_H_ */
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index 57eac5241303..4ad5db9f5312 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -7,19 +7,6 @@
#define EFI_EMBEDDED_FW_PREFIX_LEN 8
-/*
- * This struct and efi_embedded_fw_list are private to the efi-embedded fw
- * implementation they are in this header for use by lib/test_firmware.c only!
- */
-struct efi_embedded_fw {
- struct list_head list;
- const char *name;
- const u8 *data;
- size_t length;
-};
-
-extern struct list_head efi_embedded_fw_list;
-
/**
* struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
* code to search for embedded firmwares.
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 9fee2b93a8d1..62af792e151c 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -489,6 +489,7 @@ static ssize_t trigger_request_store(struct device *dev,
static DEVICE_ATTR_WO(trigger_request);
#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+#include "../drivers/firmware/efi/embedded-firmware.h"
static ssize_t trigger_request_platform_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -501,6 +502,7 @@ static ssize_t trigger_request_platform_store(struct device *dev,
};
struct efi_embedded_fw efi_embedded_fw;
const struct firmware *firmware = NULL;
+ bool saved_efi_embedded_fw_checked;
char *name;
int rc;
@@ -513,6 +515,8 @@ static ssize_t trigger_request_platform_store(struct device *dev,
efi_embedded_fw.data = (void *)test_data;
efi_embedded_fw.length = sizeof(test_data);
list_add(&efi_embedded_fw.list, &efi_embedded_fw_list);
+ saved_efi_embedded_fw_checked = efi_embedded_fw_checked;
+ efi_embedded_fw_checked = true;
pr_info("loading '%s'\n", name);
rc = firmware_request_platform(&firmware, name, dev);
@@ -530,6 +534,7 @@ static ssize_t trigger_request_platform_store(struct device *dev,
rc = count;
out:
+ efi_embedded_fw_checked = saved_efi_embedded_fw_checked;
release_firmware(firmware);
list_del(&efi_embedded_fw.list);
kfree(name);
--
2.25.1
^ permalink raw reply related
* [PATCH v3 04/19] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, stable, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
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: stable@vger.kernel.org
Acked-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
^ permalink raw reply related
* [PATCH v3 00/19] Introduce partial kernel_read_file() support
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
v3:
- add reviews/acks
- add "IMA: Add support for file reads without contents" patch
- trim CC list, in case that's why vger ignored v2
v2: [missing from lkml archives! (CC list too long?) repeating changes here]
- fix issues in firmware test suite
- add firmware partial read patches
- various bug fixes/cleanups
v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/
Hi,
Here's my tree for adding partial read support in kernel_read_file(),
which fixes a number of issues along the way. It's got Scott's firmware
and IMA patches ported and everything tests cleanly for me (even with
CONFIG_IMA_APPRAISE=y).
I think the intention is for this to go via Greg's tree since Scott's
driver code will depend on it?
Thanks,
-Kees
Kees Cook (15):
test_firmware: Test platform fw loading on non-EFI systems
selftest/firmware: Add selftest timeout in settings
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
firmware: Store opt_flags in fw_priv
Scott Branden (4):
fs/kernel_read_file: Split into separate include file
IMA: Add support for file reads without contents
firmware: Add request_partial_firmware_into_buf()
test_firmware: Test partial read support
drivers/base/firmware_loader/fallback.c | 19 +-
drivers/base/firmware_loader/fallback.h | 5 +-
.../base/firmware_loader/fallback_platform.c | 16 +-
drivers/base/firmware_loader/firmware.h | 7 +-
drivers/base/firmware_loader/main.c | 143 ++++++++++---
drivers/firmware/efi/embedded-firmware.c | 21 +-
drivers/firmware/efi/embedded-firmware.h | 19 ++
fs/Makefile | 3 +-
fs/exec.c | 132 +-----------
fs/kernel_read_file.c | 189 ++++++++++++++++++
include/linux/efi_embedded_fw.h | 13 --
include/linux/firmware.h | 12 ++
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 | 19 +-
kernel/module.c | 24 ++-
lib/test_firmware.c | 159 +++++++++++++--
security/integrity/digsig.c | 8 +-
security/integrity/ima/ima_fs.c | 10 +-
security/integrity/ima/ima_main.c | 70 +++++--
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 17 +-
security/security.c | 26 ++-
security/selinux/hooks.c | 8 +-
.../selftests/firmware/fw_filesystem.sh | 91 +++++++++
tools/testing/selftests/firmware/settings | 8 +
tools/testing/selftests/kselftest/runner.sh | 6 +-
32 files changed, 860 insertions(+), 318 deletions(-)
create mode 100644 drivers/firmware/efi/embedded-firmware.h
create mode 100644 fs/kernel_read_file.c
create mode 100644 include/linux/kernel_read_file.h
create mode 100644 tools/testing/selftests/firmware/settings
--
2.25.1
^ permalink raw reply
* [PATCH v3 19/19] test_firmware: Test partial read support
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
From: Scott Branden <scott.branden@broadcom.com>
Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf():
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request
Also update firmware selftests to use the new partial read test API.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
lib/test_firmware.c | 154 ++++++++++++++++--
.../selftests/firmware/fw_filesystem.sh | 91 +++++++++++
2 files changed, 233 insertions(+), 12 deletions(-)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 62af792e151c..387acb94eeea 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -50,6 +50,9 @@ struct test_batched_req {
* @name: the name of the firmware file to look for
* @into_buf: when the into_buf is used if this is true
* request_firmware_into_buf() will be used instead.
+ * @buf_size: size of buf to allocate when into_buf is true
+ * @file_offset: file offset to request when calling request_firmware_into_buf
+ * @partial: partial read opt when calling request_firmware_into_buf
* @sync_direct: when the sync trigger is used if this is true
* request_firmware_direct() will be used instead.
* @send_uevent: whether or not to send a uevent for async requests
@@ -89,6 +92,9 @@ struct test_batched_req {
struct test_config {
char *name;
bool into_buf;
+ size_t buf_size;
+ size_t file_offset;
+ bool partial;
bool sync_direct;
bool send_uevent;
u8 num_requests;
@@ -183,6 +189,9 @@ static int __test_firmware_config_init(void)
test_fw_config->num_requests = TEST_FIRMWARE_NUM_REQS;
test_fw_config->send_uevent = true;
test_fw_config->into_buf = false;
+ test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE;
+ test_fw_config->file_offset = 0;
+ test_fw_config->partial = false;
test_fw_config->sync_direct = false;
test_fw_config->req_firmware = request_firmware;
test_fw_config->test_result = 0;
@@ -236,28 +245,35 @@ static ssize_t config_show(struct device *dev,
dev_name(dev));
if (test_fw_config->name)
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"name:\t%s\n",
test_fw_config->name);
else
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"name:\tEMTPY\n");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"num_requests:\t%u\n", test_fw_config->num_requests);
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"send_uevent:\t\t%s\n",
test_fw_config->send_uevent ?
"FW_ACTION_HOTPLUG" :
"FW_ACTION_NOHOTPLUG");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"into_buf:\t\t%s\n",
test_fw_config->into_buf ? "true" : "false");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "buf_size:\t%zu\n", test_fw_config->buf_size);
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "file_offset:\t%zu\n", test_fw_config->file_offset);
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "partial:\t\t%s\n",
+ test_fw_config->partial ? "true" : "false");
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"sync_direct:\t\t%s\n",
test_fw_config->sync_direct ? "true" : "false");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"read_fw_idx:\t%u\n", test_fw_config->read_fw_idx);
mutex_unlock(&test_fw_mutex);
@@ -315,6 +331,30 @@ static ssize_t test_dev_config_show_bool(char *buf, bool val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_size_t(const char *buf,
+ size_t size,
+ size_t *cfg)
+{
+ int ret;
+ long new;
+
+ ret = kstrtol(buf, 10, &new);
+ if (ret)
+ return ret;
+
+ mutex_lock(&test_fw_mutex);
+ *(size_t *)cfg = new;
+ mutex_unlock(&test_fw_mutex);
+
+ /* Always return full write size even if we didn't consume all */
+ return size;
+}
+
+static ssize_t test_dev_config_show_size_t(char *buf, size_t val)
+{
+ return snprintf(buf, PAGE_SIZE, "%zu\n", val);
+}
+
static ssize_t test_dev_config_show_int(char *buf, int val)
{
return snprintf(buf, PAGE_SIZE, "%d\n", val);
@@ -400,6 +440,83 @@ static ssize_t config_into_buf_show(struct device *dev,
}
static DEVICE_ATTR_RW(config_into_buf);
+static ssize_t config_buf_size_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int rc;
+
+ mutex_lock(&test_fw_mutex);
+ if (test_fw_config->reqs) {
+ pr_err("Must call release_all_firmware prior to changing config\n");
+ rc = -EINVAL;
+ mutex_unlock(&test_fw_mutex);
+ goto out;
+ }
+ mutex_unlock(&test_fw_mutex);
+
+ rc = test_dev_config_update_size_t(buf, count,
+ &test_fw_config->buf_size);
+
+out:
+ return rc;
+}
+
+static ssize_t config_buf_size_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return test_dev_config_show_size_t(buf, test_fw_config->buf_size);
+}
+static DEVICE_ATTR_RW(config_buf_size);
+
+static ssize_t config_file_offset_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int rc;
+
+ mutex_lock(&test_fw_mutex);
+ if (test_fw_config->reqs) {
+ pr_err("Must call release_all_firmware prior to changing config\n");
+ rc = -EINVAL;
+ mutex_unlock(&test_fw_mutex);
+ goto out;
+ }
+ mutex_unlock(&test_fw_mutex);
+
+ rc = test_dev_config_update_size_t(buf, count,
+ &test_fw_config->file_offset);
+
+out:
+ return rc;
+}
+
+static ssize_t config_file_offset_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return test_dev_config_show_size_t(buf, test_fw_config->file_offset);
+}
+static DEVICE_ATTR_RW(config_file_offset);
+
+static ssize_t config_partial_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ return test_dev_config_update_bool(buf,
+ count,
+ &test_fw_config->partial);
+}
+
+static ssize_t config_partial_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return test_dev_config_show_bool(buf, test_fw_config->partial);
+}
+static DEVICE_ATTR_RW(config_partial);
+
static ssize_t config_sync_direct_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -655,11 +772,21 @@ static int test_fw_run_batch_request(void *data)
if (!test_buf)
return -ENOSPC;
- req->rc = request_firmware_into_buf(&req->fw,
- req->name,
- req->dev,
- test_buf,
- TEST_FIRMWARE_BUF_SIZE);
+ if (test_fw_config->partial)
+ req->rc = request_partial_firmware_into_buf
+ (&req->fw,
+ req->name,
+ req->dev,
+ test_buf,
+ test_fw_config->buf_size,
+ test_fw_config->file_offset);
+ else
+ req->rc = request_firmware_into_buf
+ (&req->fw,
+ req->name,
+ req->dev,
+ test_buf,
+ test_fw_config->buf_size);
if (!req->fw)
kfree(test_buf);
} else {
@@ -932,6 +1059,9 @@ static struct attribute *test_dev_attrs[] = {
TEST_FW_DEV_ATTR(config_name),
TEST_FW_DEV_ATTR(config_num_requests),
TEST_FW_DEV_ATTR(config_into_buf),
+ TEST_FW_DEV_ATTR(config_buf_size),
+ TEST_FW_DEV_ATTR(config_file_offset),
+ TEST_FW_DEV_ATTR(config_partial),
TEST_FW_DEV_ATTR(config_sync_direct),
TEST_FW_DEV_ATTR(config_send_uevent),
TEST_FW_DEV_ATTR(config_read_fw_idx),
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh b/tools/testing/selftests/firmware/fw_filesystem.sh
index fcc281373b4d..c2a2a100114b 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -149,6 +149,26 @@ config_unset_into_buf()
echo 0 > $DIR/config_into_buf
}
+config_set_buf_size()
+{
+ echo $1 > $DIR/config_buf_size
+}
+
+config_set_file_offset()
+{
+ echo $1 > $DIR/config_file_offset
+}
+
+config_set_partial()
+{
+ echo 1 > $DIR/config_partial
+}
+
+config_unset_partial()
+{
+ echo 0 > $DIR/config_partial
+}
+
config_set_sync_direct()
{
echo 1 > $DIR/config_sync_direct
@@ -207,6 +227,35 @@ read_firmwares()
done
}
+read_partial_firmwares()
+{
+ if [ "$(cat $DIR/config_into_buf)" == "1" ]; then
+ fwfile="${FW_INTO_BUF}"
+ else
+ fwfile="${FW}"
+ fi
+
+ if [ "$1" = "xzonly" ]; then
+ fwfile="${fwfile}-orig"
+ fi
+
+ # Strip fwfile down to match partial offset and length
+ partial_data="$(cat $fwfile)"
+ partial_data="${partial_data:$2:$3}"
+
+ for i in $(seq 0 3); do
+ config_set_read_fw_idx $i
+
+ read_firmware="$(cat $DIR/read_firmware)"
+
+ # Verify the contents are what we expect.
+ if [ $read_firmware != $partial_data ]; then
+ echo "request #$i: partial firmware was not loaded" >&2
+ exit 1
+ fi
+ done
+}
+
read_firmwares_expect_nofile()
{
for i in $(seq 0 3); do
@@ -242,6 +291,21 @@ test_batched_request_firmware_into_buf_nofile()
echo "OK"
}
+test_request_partial_firmware_into_buf_nofile()
+{
+ echo -n "Test request_partial_firmware_into_buf() off=$1 size=$2 nofile: "
+ config_reset
+ config_set_name nope-test-firmware.bin
+ config_set_into_buf
+ config_set_partial
+ config_set_buf_size $2
+ config_set_file_offset $1
+ config_trigger_sync
+ read_firmwares_expect_nofile
+ release_all_firmware
+ echo "OK"
+}
+
test_batched_request_firmware_direct_nofile()
{
echo -n "Batched request_firmware_direct() nofile try #$1: "
@@ -356,6 +420,21 @@ test_request_firmware_nowait_custom()
echo "OK"
}
+test_request_partial_firmware_into_buf()
+{
+ echo -n "Test request_partial_firmware_into_buf() off=$1 size=$2: "
+ config_reset
+ config_set_name $TEST_FIRMWARE_INTO_BUF_FILENAME
+ config_set_into_buf
+ config_set_partial
+ config_set_buf_size $2
+ config_set_file_offset $1
+ config_trigger_sync
+ read_partial_firmwares normal $1 $2
+ release_all_firmware
+ echo "OK"
+}
+
# Only continue if batched request triggers are present on the
# test-firmware driver
test_config_present
@@ -383,6 +462,12 @@ for i in $(seq 1 5); do
test_request_firmware_nowait_custom $i normal
done
+# Partial loads cannot use fallback, so do not repeat tests.
+test_request_partial_firmware_into_buf 0 10
+test_request_partial_firmware_into_buf 0 5
+test_request_partial_firmware_into_buf 1 6
+test_request_partial_firmware_into_buf 2 10
+
# Test for file not found, errors are expected, the failure would be
# a hung task, which would require a hard reset.
echo
@@ -407,6 +492,12 @@ for i in $(seq 1 5); do
test_request_firmware_nowait_custom_nofile $i
done
+# Partial loads cannot use fallback, so do not repeat tests.
+test_request_partial_firmware_into_buf_nofile 0 10
+test_request_partial_firmware_into_buf_nofile 0 5
+test_request_partial_firmware_into_buf_nofile 1 6
+test_request_partial_firmware_into_buf_nofile 2 10
+
test "$HAS_FW_LOADER_COMPRESS" != "yes" && exit 0
# test with both files present
--
2.25.1
^ permalink raw reply related
* [PATCH v3 18/19] firmware: Add request_partial_firmware_into_buf()
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
From: Scott Branden <scott.branden@broadcom.com>
Add request_partial_firmware_into_buf() to allow for portions of a
firmware file to be read into a buffer. This is needed when large firmware
must be loaded in portions from a file on memory constrained systems.
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Co-developed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/firmware.h | 4 +
drivers/base/firmware_loader/main.c | 109 +++++++++++++++++++-----
include/linux/firmware.h | 12 +++
3 files changed, 105 insertions(+), 20 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 7ad5fe52bc72..3f6eda46b3a2 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -32,6 +32,8 @@
* @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
* the platform's main firmware. If both this fallback and the sysfs
* fallback are enabled, then this fallback will be tried first.
+ * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
+ * entire file.
*/
enum fw_opt {
FW_OPT_UEVENT = BIT(0),
@@ -41,6 +43,7 @@ enum fw_opt {
FW_OPT_NOCACHE = BIT(4),
FW_OPT_NOFALLBACK_SYSFS = BIT(5),
FW_OPT_FALLBACK_PLATFORM = BIT(6),
+ FW_OPT_PARTIAL = BIT(7),
};
enum fw_status {
@@ -68,6 +71,7 @@ struct fw_priv {
void *data;
size_t size;
size_t allocated_size;
+ size_t offset;
u32 opt_flags;
#ifdef CONFIG_FW_LOADER_PAGED_BUF
bool is_paged_buf;
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index 814a18cc51bd..7aa22bdc2f60 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -170,10 +170,19 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
struct firmware_cache *fwc,
void *dbuf,
size_t size,
+ size_t offset,
u32 opt_flags)
{
struct fw_priv *fw_priv;
+ /* For a partial read, the buffer must be preallocated. */
+ if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
+ return NULL;
+
+ /* Only partial reads are allowed to use an offset. */
+ if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
+ return NULL;
+
fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
if (!fw_priv)
return NULL;
@@ -188,6 +197,7 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
fw_priv->fwc = fwc;
fw_priv->data = dbuf;
fw_priv->allocated_size = size;
+ fw_priv->offset = offset;
fw_priv->opt_flags = opt_flags;
fw_state_init(fw_priv);
#ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -216,12 +226,17 @@ static int alloc_lookup_fw_priv(const char *fw_name,
struct fw_priv **fw_priv,
void *dbuf,
size_t size,
+ size_t offset,
u32 opt_flags)
{
struct fw_priv *tmp;
spin_lock(&fwc->lock);
- if (!(opt_flags & FW_OPT_NOCACHE)) {
+ /*
+ * Do not merge requests that are marked to be non-cached or
+ * are performing partial reads.
+ */
+ if (!(opt_flags & (FW_OPT_NOCACHE | FW_OPT_PARTIAL))) {
tmp = __lookup_fw_priv(fw_name);
if (tmp) {
kref_get(&tmp->ref);
@@ -232,7 +247,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,
}
}
- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, opt_flags);
+ tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags);
if (tmp) {
INIT_LIST_HEAD(&tmp->list);
if (!(opt_flags & FW_OPT_NOCACHE))
@@ -439,6 +454,12 @@ static int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
else
return fw_decompress_xz_pages(dev, fw_priv, in_size, in_buffer);
}
+#else
+static inline int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
+ size_t in_size, const void *in_buffer)
+{
+ return -ENOENT;
+}
#endif /* CONFIG_FW_LOADER_COMPRESS */
/* direct firmware loading support */
@@ -485,6 +506,9 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
return -ENOMEM;
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+ size_t file_size = 0;
+ size_t *file_size_ptr = NULL;
+
/* skip the unset customized path */
if (!fw_path[i][0])
continue;
@@ -498,9 +522,18 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
fw_priv->size = 0;
+ /*
+ * The total file size is only examined when doing a partial
+ * read; the "full read" case needs to fail if the whole
+ * firmware was not completely loaded.
+ */
+ if ((fw_priv->opt_flags & FW_OPT_PARTIAL) && buffer)
+ file_size_ptr = &file_size;
+
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, 0, &buffer, msize,
- NULL,
+ rc = kernel_read_file_from_path_initns(path, fw_priv->offset,
+ &buffer, msize,
+ file_size_ptr,
READING_FIRMWARE);
if (rc < 0) {
if (rc != -ENOENT)
@@ -691,7 +724,7 @@ int assign_fw(struct firmware *fw, struct device *device)
static int
_request_firmware_prepare(struct firmware **firmware_p, const char *name,
struct device *device, void *dbuf, size_t size,
- u32 opt_flags)
+ size_t offset, u32 opt_flags)
{
struct firmware *firmware;
struct fw_priv *fw_priv;
@@ -710,7 +743,7 @@ _request_firmware_prepare(struct firmware **firmware_p, const char *name,
}
ret = alloc_lookup_fw_priv(name, &fw_cache, &fw_priv, dbuf, size,
- opt_flags);
+ offset, opt_flags);
/*
* bind with 'priv' now to avoid warning in failure path
@@ -757,9 +790,10 @@ static void fw_abort_batch_reqs(struct firmware *fw)
static int
_request_firmware(const struct firmware **firmware_p, const char *name,
struct device *device, void *buf, size_t size,
- u32 opt_flags)
+ size_t offset, u32 opt_flags)
{
struct firmware *fw = NULL;
+ bool nondirect = false;
int ret;
if (!firmware_p)
@@ -771,18 +805,20 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
}
ret = _request_firmware_prepare(&fw, name, device, buf, size,
- opt_flags);
+ offset, opt_flags);
if (ret <= 0) /* error or already assigned */
goto out;
ret = fw_get_filesystem_firmware(device, fw->priv, "", NULL);
-#ifdef CONFIG_FW_LOADER_COMPRESS
- if (ret == -ENOENT)
+
+ /* Only full reads can support decompression, platform, and sysfs. */
+ if (!(opt_flags & FW_OPT_PARTIAL))
+ nondirect = true;
+
+ if (ret == -ENOENT && nondirect)
ret = fw_get_filesystem_firmware(device, fw->priv, ".xz",
fw_decompress_xz);
-#endif
-
- if (ret == -ENOENT)
+ if (ret == -ENOENT && nondirect)
ret = firmware_fallback_platform(fw->priv);
if (ret) {
@@ -790,7 +826,9 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
dev_warn(device,
"Direct firmware load for %s failed with error %d\n",
name, ret);
- ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
+ if (nondirect)
+ ret = firmware_fallback_sysfs(fw, name, device,
+ opt_flags, ret);
} else
ret = assign_fw(fw, device);
@@ -833,7 +871,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
/* Need to pin this module until return */
__module_get(THIS_MODULE);
- ret = _request_firmware(firmware_p, name, device, NULL, 0,
+ ret = _request_firmware(firmware_p, name, device, NULL, 0, 0,
FW_OPT_UEVENT);
module_put(THIS_MODULE);
return ret;
@@ -860,7 +898,7 @@ int firmware_request_nowarn(const struct firmware **firmware, const char *name,
/* Need to pin this module until return */
__module_get(THIS_MODULE);
- ret = _request_firmware(firmware, name, device, NULL, 0,
+ ret = _request_firmware(firmware, name, device, NULL, 0, 0,
FW_OPT_UEVENT | FW_OPT_NO_WARN);
module_put(THIS_MODULE);
return ret;
@@ -884,7 +922,7 @@ int request_firmware_direct(const struct firmware **firmware_p,
int ret;
__module_get(THIS_MODULE);
- ret = _request_firmware(firmware_p, name, device, NULL, 0,
+ ret = _request_firmware(firmware_p, name, device, NULL, 0, 0,
FW_OPT_UEVENT | FW_OPT_NO_WARN |
FW_OPT_NOFALLBACK_SYSFS);
module_put(THIS_MODULE);
@@ -909,7 +947,7 @@ int firmware_request_platform(const struct firmware **firmware,
/* Need to pin this module until return */
__module_get(THIS_MODULE);
- ret = _request_firmware(firmware, name, device, NULL, 0,
+ ret = _request_firmware(firmware, name, device, NULL, 0, 0,
FW_OPT_UEVENT | FW_OPT_FALLBACK_PLATFORM);
module_put(THIS_MODULE);
return ret;
@@ -965,13 +1003,44 @@ request_firmware_into_buf(const struct firmware **firmware_p, const char *name,
return -EOPNOTSUPP;
__module_get(THIS_MODULE);
- ret = _request_firmware(firmware_p, name, device, buf, size,
+ ret = _request_firmware(firmware_p, name, device, buf, size, 0,
FW_OPT_UEVENT | FW_OPT_NOCACHE);
module_put(THIS_MODULE);
return ret;
}
EXPORT_SYMBOL(request_firmware_into_buf);
+/**
+ * request_partial_firmware_into_buf() - load partial firmware into a previously allocated buffer
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded and DMA region allocated
+ * @buf: address of buffer to load firmware into
+ * @size: size of buffer
+ * @offset: offset into file to read
+ *
+ * This function works pretty much like request_firmware_into_buf except
+ * it allows a partial read of the file.
+ */
+int
+request_partial_firmware_into_buf(const struct firmware **firmware_p,
+ const char *name, struct device *device,
+ void *buf, size_t size, size_t offset)
+{
+ int ret;
+
+ if (fw_cache_is_setup(device, name))
+ return -EOPNOTSUPP;
+
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware_p, name, device, buf, size, offset,
+ FW_OPT_UEVENT | FW_OPT_NOCACHE |
+ FW_OPT_PARTIAL);
+ module_put(THIS_MODULE);
+ return ret;
+}
+EXPORT_SYMBOL(request_partial_firmware_into_buf);
+
/**
* release_firmware() - release the resource associated with a firmware image
* @fw: firmware resource to release
@@ -1004,7 +1073,7 @@ static void request_firmware_work_func(struct work_struct *work)
fw_work = container_of(work, struct firmware_work, work);
- _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0,
+ _request_firmware(&fw, fw_work->name, fw_work->device, NULL, 0, 0,
fw_work->opt_flags);
fw_work->cont(fw, fw_work->context);
put_device(fw_work->device); /* taken in request_firmware_nowait() */
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index cb3e2c06ed8a..c15acadc6cf4 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -53,6 +53,9 @@ int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size);
+int request_partial_firmware_into_buf(const struct firmware **firmware_p,
+ const char *name, struct device *device,
+ void *buf, size_t size, size_t offset);
void release_firmware(const struct firmware *fw);
#else
@@ -102,6 +105,15 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
return -EINVAL;
}
+static inline int request_partial_firmware_into_buf
+ (const struct firmware **firmware_p,
+ const char *name,
+ struct device *device,
+ void *buf, size_t size, size_t offset)
+{
+ return -EINVAL;
+}
+
#endif
int firmware_request_cache(struct device *device, const char *name);
--
2.25.1
^ permalink raw reply related
* [PATCH v3 16/19] fs/kernel_file_read: Add "offset" arg for partial reads
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
To perform partial reads, callers of kernel_read_file*() must have a
non-NULL file_size argument and a preallocated buffer. The new "offset"
argument can then be used to seek to specific locations in the file to
fill the buffer to, at most, "buf_size" per call.
Where possible, the LSM hooks can report whether a full file has been
read or not so that the contents can be reasoned about.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/main.c | 2 +-
fs/kernel_read_file.c | 78 ++++++++++++++++++++---------
include/linux/kernel_read_file.h | 8 +--
kernel/kexec_file.c | 4 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 3 +-
7 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index bd199404935f..d95249b5284e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -494,7 +494,7 @@ fw_get_filesystem_firmware(struct device *device, struct fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, &buffer, msize,
+ rc = kernel_read_file_from_path_initns(path, 0, &buffer, msize,
NULL,
READING_FIRMWARE);
if (rc < 0) {
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index d73bc3fa710a..90d255fbdd9b 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -9,6 +9,7 @@
* kernel_read_file() - read file contents into a kernel buffer
*
* @file file to read from
+ * @offset where to start reading from (see below).
* @buf pointer to a "void *" buffer for reading into (if
* *@buf is NULL, a buffer will be allocated, and
* @buf_size will be ignored)
@@ -19,19 +20,31 @@
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
+ * @offset must be 0 unless both @buf and @file_size are non-NULL
+ * (i.e. the caller must be expecting to read partial file contents
+ * via an already-allocated @buf, in at most @buf_size chunks, and
+ * will be able to determine when the entire file was read by
+ * checking @file_size). This isn't a recommended way to read a
+ * file, though, since it is possible that the contents might
+ * change between calls to kernel_read_file().
+ *
* 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,
+int kernel_read_file(struct file *file, loff_t offset, void **buf,
size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
loff_t i_size, pos;
- ssize_t bytes = 0;
+ size_t copied;
void *allocated = NULL;
+ bool whole_file;
int ret;
+ if (offset != 0 && (!*buf || !file_size))
+ return -EINVAL;
+
if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
@@ -39,19 +52,27 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id, true);
- if (ret)
- goto out;
-
i_size = i_size_read(file_inode(file));
if (i_size <= 0) {
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || i_size > buf_size) {
+ /* The file is too big for sane activities. */
+ if (i_size > INT_MAX) {
+ ret = -EFBIG;
+ goto out;
+ }
+ /* The entire file cannot be read in one buffer. */
+ if (!file_size && offset == 0 && i_size > buf_size) {
ret = -EFBIG;
goto out;
}
+
+ whole_file = (offset == 0 && i_size <= buf_size);
+ ret = security_kernel_read_file(file, id, whole_file);
+ if (ret)
+ goto out;
+
if (file_size)
*file_size = i_size;
@@ -62,9 +83,14 @@ int kernel_read_file(struct file *file, void **buf,
goto out;
}
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+ pos = offset;
+ copied = 0;
+ while (copied < buf_size) {
+ ssize_t bytes;
+ size_t wanted = min_t(size_t, buf_size - copied,
+ i_size - pos);
+
+ bytes = kernel_read(file, *buf + copied, wanted, &pos);
if (bytes < 0) {
ret = bytes;
goto out_free;
@@ -72,14 +98,17 @@ int kernel_read_file(struct file *file, void **buf,
if (bytes == 0)
break;
+ copied += bytes;
}
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
+ if (whole_file) {
+ if (pos != i_size) {
+ ret = -EIO;
+ goto out_free;
+ }
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ }
out_free:
if (ret < 0) {
@@ -91,11 +120,11 @@ int kernel_read_file(struct file *file, void **buf,
out:
allow_write_access(file);
- return ret == 0 ? pos : ret;
+ return ret == 0 ? copied : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf,
+int kernel_read_file_from_path(const char *path, loff_t offset, void **buf,
size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
@@ -109,14 +138,15 @@ 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, buf_size, file_size, id);
+ ret = kernel_read_file(file, offset, buf, buf_size, file_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,
- size_t buf_size, size_t *file_size,
+int kernel_read_file_from_path_initns(const char *path, loff_t offset,
+ void **buf, size_t buf_size,
+ size_t *file_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -135,14 +165,14 @@ 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, buf_size, file_size, id);
+ ret = kernel_read_file(file, offset, buf, buf_size, file_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, size_t buf_size,
- size_t *file_size,
+int kernel_read_file_from_fd(int fd, loff_t offset, void **buf,
+ size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -151,7 +181,7 @@ int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
+ ret = kernel_read_file(f.file, offset, buf, buf_size, file_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 023293eaf948..575ffa1031d3 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -35,19 +35,19 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
return kernel_read_file_str[id];
}
-int kernel_read_file(struct file *file,
+int kernel_read_file(struct file *file, loff_t offset,
void **buf, size_t buf_size,
size_t *file_size,
enum kernel_read_file_id id);
-int kernel_read_file_from_path(const char *path,
+int kernel_read_file_from_path(const char *path, loff_t offset,
void **buf, size_t buf_size,
size_t *file_size,
enum kernel_read_file_id id);
-int kernel_read_file_from_path_initns(const char *path,
+int kernel_read_file_from_path_initns(const char *path, loff_t offset,
void **buf, size_t buf_size,
size_t *file_size,
enum kernel_read_file_id id);
-int kernel_read_file_from_fd(int fd,
+int kernel_read_file_from_fd(int fd, loff_t offset,
void **buf, size_t buf_size,
size_t *file_size,
enum kernel_read_file_id id);
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 878ca684a3a1..45726bc8f6ce 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -221,7 +221,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
int ret;
void *ldata;
- ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
+ ret = kernel_read_file_from_fd(kernel_fd, 0, &image->kernel_buf,
INT_MAX, NULL, READING_KEXEC_IMAGE);
if (ret < 0)
return ret;
@@ -241,7 +241,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
#endif
/* It is possible that there no initramfs is being loaded */
if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
- ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
+ ret = kernel_read_file_from_fd(initrd_fd, 0, &image->initrd_buf,
INT_MAX, NULL,
READING_KEXEC_INITRAMFS);
if (ret < 0)
diff --git a/kernel/module.c b/kernel/module.c
index 90a4788dff9d..d353d1f67681 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -4007,7 +4007,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs, int, flags)
|MODULE_INIT_IGNORE_VERMAGIC))
return -EINVAL;
- err = kernel_read_file_from_fd(fd, &hdr, INT_MAX, NULL,
+ err = kernel_read_file_from_fd(fd, 0, &hdr, INT_MAX, NULL,
READING_MODULE);
if (err < 0)
return err;
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 8a523dfd7fd7..0f518dcfde05 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, INT_MAX, NULL,
+ rc = kernel_read_file_from_path(path, 0, &data, INT_MAX, NULL,
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 5fc56ccb6678..ea8ff8a07b36 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -284,7 +284,8 @@ static ssize_t ima_read_policy(char *path)
datap = path;
strsep(&datap, "\n");
- rc = kernel_read_file_from_path(path, &data, INT_MAX, NULL, READING_POLICY);
+ rc = kernel_read_file_from_path(path, 0, &data, INT_MAX, NULL,
+ READING_POLICY);
if (rc < 0) {
pr_err("Unable to open file: %s (%d)", path, rc);
return rc;
--
2.25.1
^ permalink raw reply related
* [PATCH v3 17/19] firmware: Store opt_flags in fw_priv
From: Kees Cook @ 2020-07-24 21:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Kees Cook, Scott Branden, Mimi Zohar, Luis Chamberlain,
Jessica Yu, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200724213640.389191-1-keescook@chromium.org>
Instead of passing opt_flags around so much, store it in the private
structure so it can be examined by internals without needing to add more
arguments to functions.
Co-developed-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Scott Branden <scott.branden@broadcom.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/base/firmware_loader/fallback.c | 11 +++-----
drivers/base/firmware_loader/fallback.h | 5 ++--
.../base/firmware_loader/fallback_platform.c | 4 +--
drivers/base/firmware_loader/firmware.h | 3 ++-
drivers/base/firmware_loader/main.c | 25 +++++++++++--------
5 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 7cfdfdcb819c..0a94c8739959 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -490,13 +490,11 @@ fw_create_instance(struct firmware *firmware, const char *fw_name,
/**
* fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
* @fw_sysfs: firmware sysfs information for the firmware to load
- * @opt_flags: flags of options, FW_OPT_*
* @timeout: timeout to wait for the load
*
* In charge of constructing a sysfs fallback interface for firmware loading.
**/
-static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- u32 opt_flags, long timeout)
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
{
int retval = 0;
struct device *f_dev = &fw_sysfs->dev;
@@ -518,7 +516,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
list_add(&fw_priv->pending_list, &pending_fw_head);
mutex_unlock(&fw_lock);
- if (opt_flags & FW_OPT_UEVENT) {
+ if (fw_priv->opt_flags & FW_OPT_UEVENT) {
fw_priv->need_uevent = true;
dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name);
@@ -580,10 +578,10 @@ static int fw_load_from_user_helper(struct firmware *firmware,
}
fw_sysfs->fw_priv = firmware->priv;
- ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
+ ret = fw_load_sysfs_fallback(fw_sysfs, timeout);
if (!ret)
- ret = assign_fw(firmware, device, opt_flags);
+ ret = assign_fw(firmware, device);
out_unlock:
usermodehelper_read_unlock();
@@ -625,7 +623,6 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
* @fw: pointer to firmware image
* @name: name of firmware file to look for
* @device: device for which firmware is being loaded
- * @opt_flags: options to control firmware loading behaviour
* @ret: return value from direct lookup which triggered the fallback mechanism
*
* This function is called if direct lookup for the firmware failed, it enables
diff --git a/drivers/base/firmware_loader/fallback.h b/drivers/base/firmware_loader/fallback.h
index 2afdb6adb23f..3af7205b302f 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -67,10 +67,9 @@ static inline void unregister_sysfs_loader(void)
#endif /* CONFIG_FW_LOADER_USER_HELPER */
#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
-int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags);
+int firmware_fallback_platform(struct fw_priv *fw_priv);
#else
-static inline int firmware_fallback_platform(struct fw_priv *fw_priv,
- u32 opt_flags)
+static inline int firmware_fallback_platform(struct fw_priv *fw_priv)
{
return -ENOENT;
}
diff --git a/drivers/base/firmware_loader/fallback_platform.c b/drivers/base/firmware_loader/fallback_platform.c
index 4d1157af0e86..38de68d7e973 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -8,13 +8,13 @@
#include "fallback.h"
#include "firmware.h"
-int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
+int firmware_fallback_platform(struct fw_priv *fw_priv)
{
const u8 *data;
size_t size;
int rc;
- if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
+ if (!(fw_priv->opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
rc = security_kernel_load_data(LOADING_FIRMWARE, true);
diff --git a/drivers/base/firmware_loader/firmware.h b/drivers/base/firmware_loader/firmware.h
index 933e2192fbe8..7ad5fe52bc72 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -68,6 +68,7 @@ struct fw_priv {
void *data;
size_t size;
size_t allocated_size;
+ u32 opt_flags;
#ifdef CONFIG_FW_LOADER_PAGED_BUF
bool is_paged_buf;
struct page **pages;
@@ -136,7 +137,7 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
__fw_state_set(fw_priv, FW_STATUS_DONE);
}
-int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags);
+int assign_fw(struct firmware *fw, struct device *device);
#ifdef CONFIG_FW_LOADER_PAGED_BUF
void fw_free_paged_buf(struct fw_priv *fw_priv);
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c
index d95249b5284e..814a18cc51bd 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -168,7 +168,9 @@ static int fw_cache_piggyback_on_request(const char *name);
static struct fw_priv *__allocate_fw_priv(const char *fw_name,
struct firmware_cache *fwc,
- void *dbuf, size_t size)
+ void *dbuf,
+ size_t size,
+ u32 opt_flags)
{
struct fw_priv *fw_priv;
@@ -186,6 +188,7 @@ static struct fw_priv *__allocate_fw_priv(const char *fw_name,
fw_priv->fwc = fwc;
fw_priv->data = dbuf;
fw_priv->allocated_size = size;
+ fw_priv->opt_flags = opt_flags;
fw_state_init(fw_priv);
#ifdef CONFIG_FW_LOADER_USER_HELPER
INIT_LIST_HEAD(&fw_priv->pending_list);
@@ -210,8 +213,10 @@ static struct fw_priv *__lookup_fw_priv(const char *fw_name)
/* Returns 1 for batching firmware requests with the same name */
static int alloc_lookup_fw_priv(const char *fw_name,
struct firmware_cache *fwc,
- struct fw_priv **fw_priv, void *dbuf,
- size_t size, u32 opt_flags)
+ struct fw_priv **fw_priv,
+ void *dbuf,
+ size_t size,
+ u32 opt_flags)
{
struct fw_priv *tmp;
@@ -227,7 +232,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,
}
}
- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size);
+ tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, opt_flags);
if (tmp) {
INIT_LIST_HEAD(&tmp->list);
if (!(opt_flags & FW_OPT_NOCACHE))
@@ -635,7 +640,7 @@ static int fw_add_devm_name(struct device *dev, const char *name)
}
#endif
-int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags)
+int assign_fw(struct firmware *fw, struct device *device)
{
struct fw_priv *fw_priv = fw->priv;
int ret;
@@ -654,8 +659,8 @@ int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags)
* should be fixed in devres or driver core.
*/
/* don't cache firmware handled without uevent */
- if (device && (opt_flags & FW_OPT_UEVENT) &&
- !(opt_flags & FW_OPT_NOCACHE)) {
+ if (device && (fw_priv->opt_flags & FW_OPT_UEVENT) &&
+ !(fw_priv->opt_flags & FW_OPT_NOCACHE)) {
ret = fw_add_devm_name(device, fw_priv->fw_name);
if (ret) {
mutex_unlock(&fw_lock);
@@ -667,7 +672,7 @@ int assign_fw(struct firmware *fw, struct device *device, u32 opt_flags)
* After caching firmware image is started, let it piggyback
* on request firmware.
*/
- if (!(opt_flags & FW_OPT_NOCACHE) &&
+ if (!(fw_priv->opt_flags & FW_OPT_NOCACHE) &&
fw_priv->fwc->state == FW_LOADER_START_CACHE) {
if (fw_cache_piggyback_on_request(fw_priv->fw_name))
kref_get(&fw_priv->ref);
@@ -778,7 +783,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
#endif
if (ret == -ENOENT)
- ret = firmware_fallback_platform(fw->priv, opt_flags);
+ ret = firmware_fallback_platform(fw->priv);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
@@ -787,7 +792,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
name, ret);
ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
- ret = assign_fw(fw, device, opt_flags);
+ ret = assign_fw(fw, device);
out:
if (ret < 0) {
--
2.25.1
^ permalink raw reply related
* [PATCH] watch_queue: Limit the number of watches a user can hold
From: David Howells @ 2020-07-24 22:17 UTC (permalink / raw)
To: torvalds
Cc: dhowells, jarkko.sakkinen, keyrings, linux-security-module,
linux-fsdevel, linux-kernel
Impose a limit on the number of watches that a user can hold so that they
can't use this mechanism to fill up all the available memory.
This is done by putting a counter in user_struct that's incremented when a
watch is allocated and decreased when it is released. If the number
exceeds the RLIMIT_NOFILE limit, the watch is rejected with EAGAIN.
This can be tested by the following means:
(1) Create a watch queue and attach it to fd 5 in the program given - in
this case, bash:
keyctl watch_session /tmp/nlog /tmp/gclog 5 bash
(2) In the shell, set the maximum number of files to, say, 99:
ulimit -n 99
(3) Add 200 keyrings:
for ((i=0; i<200; i++)); do keyctl newring a$i @s || break; done
(4) Try to watch all of the keyrings:
for ((i=0; i<200; i++)); do echo $i; keyctl watch_add 5 %:a$i || break; done
This should fail when the number of watches belonging to the user hits
99.
(5) Remove all the keyrings and all of those watches should go away:
for ((i=0; i<200; i++)); do keyctl unlink %:a$i; done
(6) Kill off the watch queue by exiting the shell spawned by
watch_session.
Fixes: c73be61cede5 ("pipe: Add general notification queue support")
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---
include/linux/sched/user.h | 3 +++
kernel/watch_queue.c | 8 ++++++++
2 files changed, 11 insertions(+)
diff --git a/include/linux/sched/user.h b/include/linux/sched/user.h
index 917d88edb7b9..a8ec3b6093fc 100644
--- a/include/linux/sched/user.h
+++ b/include/linux/sched/user.h
@@ -36,6 +36,9 @@ struct user_struct {
defined(CONFIG_NET) || defined(CONFIG_IO_URING)
atomic_long_t locked_vm;
#endif
+#ifdef CONFIG_WATCH_QUEUE
+ atomic_t nr_watches; /* The number of watches this user currently has */
+#endif
/* Miscellaneous per-user rate limit */
struct ratelimit_state ratelimit;
diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c
index f74020f6bd9d..0ef8f65bd2d7 100644
--- a/kernel/watch_queue.c
+++ b/kernel/watch_queue.c
@@ -393,6 +393,7 @@ static void free_watch(struct rcu_head *rcu)
struct watch *watch = container_of(rcu, struct watch, rcu);
put_watch_queue(rcu_access_pointer(watch->queue));
+ atomic_dec(&watch->cred->user->nr_watches);
put_cred(watch->cred);
}
@@ -452,6 +453,13 @@ int add_watch_to_object(struct watch *watch, struct watch_list *wlist)
watch->cred = get_current_cred();
rcu_assign_pointer(watch->watch_list, wlist);
+ if (atomic_inc_return(&watch->cred->user->nr_watches) >
+ task_rlimit(current, RLIMIT_NOFILE)) {
+ atomic_dec(&watch->cred->user->nr_watches);
+ put_cred(watch->cred);
+ return -EAGAIN;
+ }
+
spin_lock_bh(&wqueue->lock);
kref_get(&wqueue->usage);
kref_get(&watch->usage);
^ permalink raw reply related
* Re: [PATCH bpf-next v6 3/7] bpf: Generalize bpf_sk_storage
From: Martin KaFai Lau @ 2020-07-25 1:13 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200723115032.460770-4-kpsingh@chromium.org>
On Thu, Jul 23, 2020 at 01:50:28PM +0200, KP Singh wrote:
> From: KP Singh <kpsingh@google.com>
>
> Refactor the functionality in bpf_sk_storage.c so that concept of
> storage linked to kernel objects can be extended to other objects like
> inode, task_struct etc.
>
> Each new local storage will still be a separate map and provide its own
> set of helpers. This allows for future object specific extensions and
> still share a lot of the underlying implementation.
>
[ ... ]
> @@ -386,54 +407,28 @@ static int sk_storage_alloc(struct sock *sk,
> * Otherwise, it will become a leak (and other memory issues
> * during map destruction).
> */
> -static struct bpf_local_storage_data *
> -bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
> +struct bpf_local_storage_data *
> +bpf_local_storage_update(void *owner, struct bpf_map *map,
> + struct bpf_local_storage *local_storage, void *value,
> u64 map_flags)
> {
> struct bpf_local_storage_data *old_sdata = NULL;
> struct bpf_local_storage_elem *selem;
> - struct bpf_local_storage *local_storage;
> struct bpf_local_storage_map *smap;
> int err;
>
> - /* BPF_EXIST and BPF_NOEXIST cannot be both set */
> - if (unlikely((map_flags & ~BPF_F_LOCK) > BPF_EXIST) ||
> - /* BPF_F_LOCK can only be used in a value with spin_lock */
> - unlikely((map_flags & BPF_F_LOCK) && !map_value_has_spin_lock(map)))
> - return ERR_PTR(-EINVAL);
> -
> smap = (struct bpf_local_storage_map *)map;
> - local_storage = rcu_dereference(sk->sk_bpf_storage);
> - if (!local_storage || hlist_empty(&local_storage->list)) {
> - /* Very first elem for this object */
> - err = check_flags(NULL, map_flags);
This check_flags here is missing in the later sk_storage_update().
> - if (err)
> - return ERR_PTR(err);
> -
> - selem = bpf_selem_alloc(smap, sk, value, true);
> - if (!selem)
> - return ERR_PTR(-ENOMEM);
> -
> - err = sk_storage_alloc(sk, smap, selem);
> - if (err) {
> - kfree(selem);
> - atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
> - return ERR_PTR(err);
> - }
> -
> - return SDATA(selem);
> - }
>
> if ((map_flags & BPF_F_LOCK) && !(map_flags & BPF_NOEXIST)) {
> /* Hoping to find an old_sdata to do inline update
> * such that it can avoid taking the local_storage->lock
> * and changing the lists.
> */
> - old_sdata =
> - bpf_local_storage_lookup(local_storage, smap, false);
> + old_sdata = bpf_local_storage_lookup(local_storage, smap, false);
> err = check_flags(old_sdata, map_flags);
> if (err)
> return ERR_PTR(err);
> +
> if (old_sdata && selem_linked_to_storage(SELEM(old_sdata))) {
> copy_map_value_locked(map, old_sdata->data,
> value, false);
[ ... ]
> +static struct bpf_local_storage_data *
> +sk_storage_update(void *owner, struct bpf_map *map, void *value, u64 map_flags)
> +{
> + struct bpf_local_storage_data *old_sdata = NULL;
> + struct bpf_local_storage_elem *selem;
> + struct bpf_local_storage *local_storage;
> + struct bpf_local_storage_map *smap;
> + struct sock *sk;
> + int err;
> +
> + err = bpf_local_storage_check_update_flags(map, map_flags);
> + if (err)
> + return ERR_PTR(err);
> +
> + sk = owner;
> + local_storage = rcu_dereference(sk->sk_bpf_storage);
> + smap = (struct bpf_local_storage_map *)map;
> +
> + if (!local_storage || hlist_empty(&local_storage->list)) {
"check_flags(NULL, map_flags);" is gone in this refactoring.
This part of code is copied into the inode_storage_update()
in the latter patch which then has the same issue.
> + /* Very first elem */
> + selem = map->ops->map_selem_alloc(smap, owner, value, !old_sdata);
> + if (!selem)
> + return ERR_PTR(-ENOMEM);
> static int sk_storage_map_btf_id;
> const struct bpf_map_ops sk_storage_map_ops = {
> - .map_alloc_check = bpf_sk_storage_map_alloc_check,
> - .map_alloc = bpf_local_storage_map_alloc,
> - .map_free = bpf_local_storage_map_free,
> + .map_alloc_check = bpf_local_storage_map_alloc_check,
> + .map_alloc = sk_storage_map_alloc,
> + .map_free = sk_storage_map_free,
> .map_get_next_key = notsupp_get_next_key,
> .map_lookup_elem = bpf_fd_sk_storage_lookup_elem,
> .map_update_elem = bpf_fd_sk_storage_update_elem,
> .map_delete_elem = bpf_fd_sk_storage_delete_elem,
> - .map_check_btf = bpf_sk_storage_map_check_btf,
> + .map_check_btf = bpf_local_storage_map_check_btf,
> .map_btf_name = "bpf_local_storage_map",
> .map_btf_id = &sk_storage_map_btf_id,
> + .map_selem_alloc = sk_selem_alloc,
> + .map_local_storage_update = sk_storage_update,
> + .map_local_storage_unlink = unlink_sk_storage,
I think refactoring codes as map_selem_alloc, map_local_storage_update,
and map_local_storage_unlink is not the best option. The sk and inode
version of the above map_ops are mostly the same. Fixing the
issue like the one mentioned above need to fix both sk, inode, and
the future kernel-object code.
The only difference is sk charge omem and inode does not.
I have played around a little. I think adding the following three ops (pasted at
the end) is better and should be enough for both sk and inode. The inode
does not even have to implement the (un)charge ops at all.
That should remove the duplication for the followings:
- (sk|inode)_selem_alloc
- (sk|inode)_storage_update
- unlink_(sk|inode)_storage
- (sk|inode)_storage_alloc
Another bonus is the new bpf_local_storage_check_update_flags() and
bpf_local_storage_publish() will be no longer needed too.
I have hacked up this patch 3 change to compiler-test out this idea.
I will post in another email. Let me know wdyt.
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -33,6 +33,9 @@ struct btf;
struct btf_type;
struct exception_table_entry;
struct seq_operations;
+struct bpf_local_storage;
+struct bpf_local_storage_map;
+struct bpf_local_storage_elem;
extern struct idr btf_idr;
extern spinlock_t btf_idr_lock;
@@ -93,6 +96,13 @@ struct bpf_map_ops {
__poll_t (*map_poll)(struct bpf_map *map, struct file *filp,
struct poll_table_struct *pts);
+ /* Functions called by bpf_local_storage maps */
+ int (*map_local_storage_charge)(struct bpf_local_storage_map *smap,
+ void *owner, u32 size);
+ void (*map_local_storage_uncharge)(struct bpf_local_storage_map *smap,
+ void *owner, u32 size);
+ struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(struct bpf_local_storage_map *smap,
+ void *owner);
/* BTF name and id of struct allocated by map_alloc */
const char * const map_btf_name;
int *map_btf_id;
^ permalink raw reply
* [RFC PATCH bpf-next] bpf: POC on local_storage charge and uncharge map_ops
From: Martin KaFai Lau @ 2020-07-25 1:30 UTC (permalink / raw)
To: KP Singh, KP Singh
Cc: bpf, linux-kernel, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, Paul Turner, Jann Horn, Florent Revest
In-Reply-To: <20200723115032.460770-4-kpsingh@chromium.org>
It is a direct replacement of the patch 3 in discussion [1]
and to test out the idea on adding
map_local_storage_charge, map_local_storage_uncharge,
and map_owner_storage_ptr.
It is only compiler tested to demo the idea.
[1]: https://patchwork.ozlabs.org/project/netdev/patch/20200723115032.460770-4-kpsingh@chromium.org/
Signed-off-by: Martin KaFai Lau <kafai@fb.com>
---
include/linux/bpf.h | 10 ++
include/net/bpf_sk_storage.h | 51 +++++++
include/uapi/linux/bpf.h | 8 +-
net/core/bpf_sk_storage.c | 250 +++++++++++++++++++++------------
tools/include/uapi/linux/bpf.h | 8 +-
5 files changed, 236 insertions(+), 91 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 72221aea1c60..d4eab7ccbb51 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -33,6 +33,9 @@ struct btf;
struct btf_type;
struct exception_table_entry;
struct seq_operations;
+struct bpf_local_storage;
+struct bpf_local_storage_map;
+struct bpf_local_storage_elem;
extern struct idr btf_idr;
extern spinlock_t btf_idr_lock;
@@ -93,6 +96,13 @@ struct bpf_map_ops {
__poll_t (*map_poll)(struct bpf_map *map, struct file *filp,
struct poll_table_struct *pts);
+ /* Functions called by bpf_local_storage maps */
+ int (*map_local_storage_charge)(struct bpf_local_storage_map *smap,
+ void *owner, u32 size);
+ void (*map_local_storage_uncharge)(struct bpf_local_storage_map *smap,
+ void *owner, u32 size);
+ struct bpf_local_storage __rcu ** (*map_owner_storage_ptr)(struct bpf_local_storage_map *smap,
+ void *owner);
/* BTF name and id of struct allocated by map_alloc */
const char * const map_btf_name;
int *map_btf_id;
diff --git a/include/net/bpf_sk_storage.h b/include/net/bpf_sk_storage.h
index 950c5aaba15e..05b777950eb3 100644
--- a/include/net/bpf_sk_storage.h
+++ b/include/net/bpf_sk_storage.h
@@ -3,8 +3,15 @@
#ifndef _BPF_SK_STORAGE_H
#define _BPF_SK_STORAGE_H
+#include <linux/rculist.h>
+#include <linux/list.h>
+#include <linux/hash.h>
#include <linux/types.h>
#include <linux/spinlock.h>
+#include <linux/bpf.h>
+#include <net/sock.h>
+#include <uapi/linux/sock_diag.h>
+#include <uapi/linux/btf.h>
struct sock;
@@ -34,6 +41,50 @@ u16 bpf_local_storage_cache_idx_get(struct bpf_local_storage_cache *cache);
void bpf_local_storage_cache_idx_free(struct bpf_local_storage_cache *cache,
u16 idx);
+/* Helper functions for bpf_local_storage */
+int bpf_local_storage_map_alloc_check(union bpf_attr *attr);
+
+struct bpf_local_storage_map *bpf_local_storage_map_alloc(union bpf_attr *attr);
+
+struct bpf_local_storage_data *
+bpf_local_storage_lookup(struct bpf_local_storage *local_storage,
+ struct bpf_local_storage_map *smap,
+ bool cacheit_lockit);
+
+void bpf_local_storage_map_free(struct bpf_local_storage_map *smap);
+
+int bpf_local_storage_map_check_btf(const struct bpf_map *map,
+ const struct btf *btf,
+ const struct btf_type *key_type,
+ const struct btf_type *value_type);
+
+void bpf_selem_link_storage(struct bpf_local_storage *local_storage,
+ struct bpf_local_storage_elem *selem);
+
+bool bpf_selem_unlink_storage(struct bpf_local_storage *local_storage,
+ struct bpf_local_storage_elem *selem,
+ bool uncharge_omem);
+
+void bpf_selem_unlink(struct bpf_local_storage_elem *selem);
+
+void bpf_selem_link_map(struct bpf_local_storage_map *smap,
+ struct bpf_local_storage_elem *selem);
+
+void bpf_selem_unlink_map(struct bpf_local_storage_elem *selem);
+
+struct bpf_local_storage_elem *
+bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner, void *value,
+ bool charge_mem);
+
+int
+bpf_local_storage_alloc(void *owner,
+ struct bpf_local_storage_map *smap,
+ struct bpf_local_storage_elem *first_selem);
+
+struct bpf_local_storage_data *
+bpf_local_storage_update(void *owner, struct bpf_map *map, void *value,
+ u64 map_flags);
+
#ifdef CONFIG_BPF_SYSCALL
int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk);
struct bpf_sk_storage_diag *
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 54d0c886e3ba..b9d2e4792d08 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3630,9 +3630,13 @@ enum {
BPF_F_SYSCTL_BASE_NAME = (1ULL << 0),
};
-/* BPF_FUNC_sk_storage_get flags */
+/* BPF_FUNC_<local>_storage_get flags */
enum {
- BPF_SK_STORAGE_GET_F_CREATE = (1ULL << 0),
+ BPF_LOCAL_STORAGE_GET_F_CREATE = (1ULL << 0),
+ /* BPF_SK_STORAGE_GET_F_CREATE is only kept for backward compatibility
+ * and BPF_LOCAL_STORAGE_GET_F_CREATE must be used instead.
+ */
+ BPF_SK_STORAGE_GET_F_CREATE = BPF_LOCAL_STORAGE_GET_F_CREATE,
};
/* BPF_FUNC_read_branch_records flags. */
diff --git a/net/core/bpf_sk_storage.c b/net/core/bpf_sk_storage.c
index aa3e3a47acb5..ec54b9d424c8 100644
--- a/net/core/bpf_sk_storage.c
+++ b/net/core/bpf_sk_storage.c
@@ -83,7 +83,7 @@ struct bpf_local_storage_elem {
struct bpf_local_storage {
struct bpf_local_storage_data __rcu *cache[BPF_LOCAL_STORAGE_CACHE_SIZE];
struct hlist_head list; /* List of bpf_local_storage_elem */
- struct sock *owner; /* The object that owns the the above "list" of
+ void *owner; /* The object that owns the the above "list" of
* bpf_local_storage_elem.
*/
struct rcu_head rcu;
@@ -109,6 +109,33 @@ static int omem_charge(struct sock *sk, unsigned int size)
return -ENOMEM;
}
+static int mem_charge(struct bpf_local_storage_map *smap, void *owner, u32 size)
+{
+ struct bpf_map *map = &smap->map;
+
+ if (!map->ops->map_local_storage_charge)
+ return 0;
+
+ return map->ops->map_local_storage_charge(smap, owner, size);
+}
+
+static void mem_uncharge(struct bpf_local_storage_map *smap, void *owner,
+ u32 size)
+{
+ struct bpf_map *map = &smap->map;
+
+ if (map->ops->map_local_storage_uncharge)
+ map->ops->map_local_storage_uncharge(smap, owner, size);
+}
+
+static struct bpf_local_storage __rcu **
+owner_storage(struct bpf_local_storage_map *smap, void *owner)
+{
+ struct bpf_map *map = &smap->map;
+
+ return map->ops->map_owner_storage_ptr(smap, owner);
+}
+
static bool selem_linked_to_storage(const struct bpf_local_storage_elem *selem)
{
return !hlist_unhashed(&selem->snode);
@@ -119,13 +146,13 @@ static bool selem_linked_to_map(const struct bpf_local_storage_elem *selem)
return !hlist_unhashed(&selem->map_node);
}
-static struct bpf_local_storage_elem *
-bpf_selem_alloc(struct bpf_local_storage_map *smap, struct sock *sk,
- void *value, bool charge_omem)
+struct bpf_local_storage_elem *
+bpf_selem_alloc(struct bpf_local_storage_map *smap, void *owner,
+ void *value, bool charge_mem)
{
struct bpf_local_storage_elem *selem;
- if (charge_omem && omem_charge(sk, smap->elem_size))
+ if (charge_mem && mem_charge(smap, owner, smap->elem_size))
return NULL;
selem = kzalloc(smap->elem_size, GFP_ATOMIC | __GFP_NOWARN);
@@ -135,40 +162,42 @@ bpf_selem_alloc(struct bpf_local_storage_map *smap, struct sock *sk,
return selem;
}
- if (charge_omem)
- atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
+ if (charge_mem)
+ mem_uncharge(smap, owner, smap->elem_size);
return NULL;
}
-/* sk_storage->lock must be held and selem->sk_storage == sk_storage.
+/* local_storage->lock must be held and selem->sk_storage == sk_storage.
* The caller must ensure selem->smap is still valid to be
* dereferenced for its smap->elem_size and smap->cache_idx.
*/
-static bool bpf_selem_unlink_storage(struct bpf_local_storage *local_storage,
- struct bpf_local_storage_elem *selem,
- bool uncharge_omem)
+bool bpf_selem_unlink_storage(struct bpf_local_storage *local_storage,
+ struct bpf_local_storage_elem *selem,
+ bool uncharge_mem)
{
struct bpf_local_storage_map *smap;
bool free_local_storage;
- struct sock *sk;
+ void *owner;
smap = rcu_dereference(SDATA(selem)->smap);
- sk = local_storage->owner;
+ owner = local_storage->owner;
- /* All uncharging on sk->sk_omem_alloc must be done first.
- * sk may be freed once the last selem is unlinked from local_storage.
+ /* All uncharging on owner must be done first.
+ * The owner may be freed once the last selem is unlinked from
+ * local_storage.
*/
- if (uncharge_omem)
- atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
+ if (uncharge_mem)
+ mem_uncharge(smap, owner, smap->elem_size);
free_local_storage = hlist_is_singular_node(&selem->snode,
&local_storage->list);
if (free_local_storage) {
- atomic_sub(sizeof(struct bpf_local_storage), &sk->sk_omem_alloc);
+ mem_uncharge(smap, owner, sizeof(struct bpf_local_storage));
local_storage->owner = NULL;
- /* After this RCU_INIT, sk may be freed and cannot be used */
- RCU_INIT_POINTER(sk->sk_bpf_storage, NULL);
+
+ /* After this RCU_INIT, owner may be freed and cannot be used */
+ RCU_INIT_POINTER(*owner_storage(smap, owner), NULL);
/* local_storage is not freed now. local_storage->lock is
* still held and raw_spin_unlock_bh(&local_storage->lock)
@@ -214,14 +243,14 @@ static void __bpf_selem_unlink_storage(struct bpf_local_storage_elem *selem)
kfree_rcu(local_storage, rcu);
}
-static void bpf_selem_link_storage(struct bpf_local_storage *local_storage,
- struct bpf_local_storage_elem *selem)
+void bpf_selem_link_storage(struct bpf_local_storage *local_storage,
+ struct bpf_local_storage_elem *selem)
{
RCU_INIT_POINTER(selem->local_storage, local_storage);
hlist_add_head(&selem->snode, &local_storage->list);
}
-static void bpf_selem_unlink_map(struct bpf_local_storage_elem *selem)
+void bpf_selem_unlink_map(struct bpf_local_storage_elem *selem)
{
struct bpf_local_storage_map *smap;
struct bpf_local_storage_map_bucket *b;
@@ -238,8 +267,8 @@ static void bpf_selem_unlink_map(struct bpf_local_storage_elem *selem)
raw_spin_unlock_bh(&b->lock);
}
-static void bpf_selem_link_map(struct bpf_local_storage_map *smap,
- struct bpf_local_storage_elem *selem)
+void bpf_selem_link_map(struct bpf_local_storage_map *smap,
+ struct bpf_local_storage_elem *selem)
{
struct bpf_local_storage_map_bucket *b = select_bucket(smap, selem);
@@ -249,7 +278,7 @@ static void bpf_selem_link_map(struct bpf_local_storage_map *smap,
raw_spin_unlock_bh(&b->lock);
}
-static void bpf_selem_unlink(struct bpf_local_storage_elem *selem)
+void bpf_selem_unlink(struct bpf_local_storage_elem *selem)
{
/* Always unlink from map before unlinking from local_storage
* because selem will be freed after successfully unlinked from
@@ -259,7 +288,7 @@ static void bpf_selem_unlink(struct bpf_local_storage_elem *selem)
__bpf_selem_unlink_storage(selem);
}
-static struct bpf_local_storage_data *
+struct bpf_local_storage_data *
bpf_local_storage_lookup(struct bpf_local_storage *local_storage,
struct bpf_local_storage_map *smap,
bool cacheit_lockit)
@@ -325,40 +354,45 @@ static int check_flags(const struct bpf_local_storage_data *old_sdata,
return 0;
}
-static int sk_storage_alloc(struct sock *sk,
+int bpf_local_storage_alloc(void *owner,
struct bpf_local_storage_map *smap,
struct bpf_local_storage_elem *first_selem)
{
- struct bpf_local_storage *prev_sk_storage, *sk_storage;
+ struct bpf_local_storage *prev_storage, *storage;
+ struct bpf_local_storage **owner_storage_ptr;
int err;
- err = omem_charge(sk, sizeof(*sk_storage));
+ err = mem_charge(smap, owner, sizeof(*storage));
if (err)
return err;
- sk_storage = kzalloc(sizeof(*sk_storage), GFP_ATOMIC | __GFP_NOWARN);
- if (!sk_storage) {
+ storage = kzalloc(sizeof(*storage), GFP_ATOMIC | __GFP_NOWARN);
+ if (!storage) {
err = -ENOMEM;
goto uncharge;
}
- INIT_HLIST_HEAD(&sk_storage->list);
- raw_spin_lock_init(&sk_storage->lock);
- sk_storage->owner = sk;
- bpf_selem_link_storage(sk_storage, first_selem);
+ INIT_HLIST_HEAD(&storage->list);
+ raw_spin_lock_init(&storage->lock);
+ storage->owner = owner;
+
+ bpf_selem_link_storage(storage, first_selem);
bpf_selem_link_map(smap, first_selem);
- /* Publish sk_storage to sk. sk->sk_lock cannot be acquired.
- * Hence, atomic ops is used to set sk->sk_bpf_storage
- * from NULL to the newly allocated sk_storage ptr.
+
+ owner_storage_ptr =
+ (struct bpf_local_storage **)owner_storage(smap, owner);
+ /* Publish storage to the owner.
+ * Instead of using any lock of the kernel object (i.e. owner),
+ * cmpxchg will work with any kernel object regardless what
+ * the running context is, bh, irq...etc.
*
- * From now on, the sk->sk_bpf_storage pointer is protected
- * by the sk_storage->lock. Hence, when freeing
- * the sk->sk_bpf_storage, the sk_storage->lock must
- * be held before setting sk->sk_bpf_storage to NULL.
+ * From now on, the owner->storage pointer (e.g. sk->sk_bpf_storage)
+ * is protected by the storage->lock. Hence, when freeing
+ * the owner->storage, the storage->lock must be held before
+ * setting owner->storage ptr to NULL.
*/
- prev_sk_storage = cmpxchg((struct bpf_local_storage **)&sk->sk_bpf_storage,
- NULL, sk_storage);
- if (unlikely(prev_sk_storage)) {
+ prev_storage = cmpxchg(owner_storage_ptr, NULL, storage);
+ if (unlikely(prev_storage)) {
bpf_selem_unlink_map(first_selem);
err = -EAGAIN;
goto uncharge;
@@ -366,7 +400,7 @@ static int sk_storage_alloc(struct sock *sk,
/* Note that even first_selem was linked to smap's
* bucket->list, first_selem can be freed immediately
* (instead of kfree_rcu) because
- * bpf_sk_storage_map_free() does a
+ * bpf_local_storage_map_free() does a
* synchronize_rcu() before walking the bucket->list.
* Hence, no one is accessing selem from the
* bucket->list under rcu_read_lock().
@@ -376,8 +410,8 @@ static int sk_storage_alloc(struct sock *sk,
return 0;
uncharge:
- kfree(sk_storage);
- atomic_sub(sizeof(*sk_storage), &sk->sk_omem_alloc);
+ kfree(storage);
+ mem_uncharge(smap, owner, sizeof(*storage));
return err;
}
@@ -386,9 +420,9 @@ static int sk_storage_alloc(struct sock *sk,
* Otherwise, it will become a leak (and other memory issues
* during map destruction).
*/
-static struct bpf_local_storage_data *
-bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
- u64 map_flags)
+struct bpf_local_storage_data *
+bpf_local_storage_update(void *owner, struct bpf_map *map,
+ void *value, u64 map_flags)
{
struct bpf_local_storage_data *old_sdata = NULL;
struct bpf_local_storage_elem *selem;
@@ -403,21 +437,21 @@ bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
return ERR_PTR(-EINVAL);
smap = (struct bpf_local_storage_map *)map;
- local_storage = rcu_dereference(sk->sk_bpf_storage);
+ local_storage = rcu_dereference(*owner_storage(smap, owner));
if (!local_storage || hlist_empty(&local_storage->list)) {
- /* Very first elem for this object */
+ /* Very first elem for the owner */
err = check_flags(NULL, map_flags);
if (err)
return ERR_PTR(err);
- selem = bpf_selem_alloc(smap, sk, value, true);
+ selem = bpf_selem_alloc(smap, owner, value, true);
if (!selem)
return ERR_PTR(-ENOMEM);
- err = sk_storage_alloc(sk, smap, selem);
+ err = bpf_local_storage_alloc(owner, smap, selem);
if (err) {
kfree(selem);
- atomic_sub(smap->elem_size, &sk->sk_omem_alloc);
+ mem_uncharge(smap, owner, smap->elem_size);
return ERR_PTR(err);
}
@@ -429,8 +463,8 @@ bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
* such that it can avoid taking the local_storage->lock
* and changing the lists.
*/
- old_sdata =
- bpf_local_storage_lookup(local_storage, smap, false);
+ old_sdata = bpf_local_storage_lookup(local_storage, smap,
+ false);
err = check_flags(old_sdata, map_flags);
if (err)
return ERR_PTR(err);
@@ -474,7 +508,7 @@ bpf_local_storage_update(struct sock *sk, struct bpf_map *map, void *value,
* old_sdata will not be uncharged later during
* bpf_selem_unlink_storage().
*/
- selem = bpf_selem_alloc(smap, sk, value, !old_sdata);
+ selem = bpf_selem_alloc(smap, owner, value, !old_sdata);
if (!selem) {
err = -ENOMEM;
goto unlock_err;
@@ -566,7 +600,7 @@ void bpf_sk_storage_free(struct sock *sk)
* Thus, no elem can be added-to or deleted-from the
* sk_storage->list by the bpf_prog or by the bpf-map's syscall.
*
- * It is racing with bpf_sk_storage_map_free() alone
+ * It is racing with bpf_local_storage_map_free() alone
* when unlinking elem from the sk_storage->list and
* the map's bucket->list.
*/
@@ -586,17 +620,12 @@ void bpf_sk_storage_free(struct sock *sk)
kfree_rcu(sk_storage, rcu);
}
-static void bpf_local_storage_map_free(struct bpf_map *map)
+void bpf_local_storage_map_free(struct bpf_local_storage_map *smap)
{
struct bpf_local_storage_elem *selem;
- struct bpf_local_storage_map *smap;
struct bpf_local_storage_map_bucket *b;
unsigned int i;
- smap = (struct bpf_local_storage_map *)map;
-
- bpf_local_storage_cache_idx_free(&sk_cache, smap->cache_idx);
-
/* Note that this map might be concurrently cloned from
* bpf_sk_storage_clone. Wait for any existing bpf_sk_storage_clone
* RCU read section to finish before proceeding. New RCU
@@ -606,11 +635,12 @@ static void bpf_local_storage_map_free(struct bpf_map *map)
/* bpf prog and the userspace can no longer access this map
* now. No new selem (of this map) can be added
- * to the sk->sk_bpf_storage or to the map bucket's list.
+ * to the bpf_local_storage or to the map bucket's list.
*
* The elem of this map can be cleaned up here
* or
- * by bpf_sk_storage_free() during __sk_destruct().
+ * by bpf_local_storage_free() during the destruction of the
+ * owner object. eg. __sk_destruct.
*/
for (i = 0; i < (1U << smap->bucket_log); i++) {
b = &smap->buckets[i];
@@ -626,22 +656,31 @@ static void bpf_local_storage_map_free(struct bpf_map *map)
rcu_read_unlock();
}
- /* bpf_sk_storage_free() may still need to access the map.
- * e.g. bpf_sk_storage_free() has unlinked selem from the map
+ /* bpf_local_storage_free() may still need to access the map.
+ * e.g. bpf_local_storage_free() has unlinked selem from the map
* which then made the above while((selem = ...)) loop
* exited immediately.
*
- * However, the bpf_sk_storage_free() still needs to access
+ * However, the bpf_local_storage_free() still needs to access
* the smap->elem_size to do the uncharging in
* bpf_selem_unlink_storage().
*
* Hence, wait another rcu grace period for the
- * bpf_sk_storage_free() to finish.
+ * bpf_local_storage_free() to finish.
*/
synchronize_rcu();
kvfree(smap->buckets);
- kfree(map);
+ kfree(smap);
+}
+
+static void sk_storage_map_free(struct bpf_map *map)
+{
+ struct bpf_local_storage_map *smap;
+
+ smap = (struct bpf_local_storage_map *)map;
+ bpf_local_storage_cache_idx_free(&sk_cache, smap->cache_idx);
+ bpf_local_storage_map_free(smap);
}
/* U16_MAX is much more than enough for sk local storage
@@ -653,7 +692,7 @@ static void bpf_local_storage_map_free(struct bpf_map *map)
sizeof(struct bpf_local_storage_elem)), \
(U16_MAX - sizeof(struct bpf_local_storage_elem)))
-static int bpf_sk_storage_map_alloc_check(union bpf_attr *attr)
+int bpf_local_storage_map_alloc_check(union bpf_attr *attr)
{
if (attr->map_flags & ~BPF_LOCAL_STORAGE_CREATE_FLAG_MASK ||
!(attr->map_flags & BPF_F_NO_PREALLOC) ||
@@ -672,7 +711,7 @@ static int bpf_sk_storage_map_alloc_check(union bpf_attr *attr)
return 0;
}
-static struct bpf_map *bpf_local_storage_map_alloc(union bpf_attr *attr)
+struct bpf_local_storage_map *bpf_local_storage_map_alloc(union bpf_attr *attr)
{
struct bpf_local_storage_map *smap;
unsigned int i;
@@ -710,9 +749,21 @@ static struct bpf_map *bpf_local_storage_map_alloc(union bpf_attr *attr)
raw_spin_lock_init(&smap->buckets[i].lock);
}
- smap->elem_size = sizeof(struct bpf_local_storage_elem) + attr->value_size;
- smap->cache_idx = bpf_local_storage_cache_idx_get(&sk_cache);
+ smap->elem_size =
+ sizeof(struct bpf_local_storage_elem) + attr->value_size;
+
+ return smap;
+}
+
+static struct bpf_map *sk_storage_map_alloc(union bpf_attr *attr)
+{
+ struct bpf_local_storage_map *smap;
+ smap = bpf_local_storage_map_alloc(attr);
+ if (IS_ERR(smap))
+ return ERR_CAST(smap);
+
+ smap->cache_idx = bpf_local_storage_cache_idx_get(&sk_cache);
return &smap->map;
}
@@ -722,10 +773,10 @@ static int notsupp_get_next_key(struct bpf_map *map, void *key,
return -ENOTSUPP;
}
-static int bpf_sk_storage_map_check_btf(const struct bpf_map *map,
- const struct btf *btf,
- const struct btf_type *key_type,
- const struct btf_type *value_type)
+int bpf_local_storage_map_check_btf(const struct bpf_map *map,
+ const struct btf *btf,
+ const struct btf_type *key_type,
+ const struct btf_type *value_type)
{
u32 int_data;
@@ -856,7 +907,7 @@ int bpf_sk_storage_clone(const struct sock *sk, struct sock *newsk)
bpf_selem_link_map(smap, copy_selem);
bpf_selem_link_storage(new_sk_storage, copy_selem);
} else {
- ret = sk_storage_alloc(newsk, smap, copy_selem);
+ ret = bpf_local_storage_alloc(newsk, smap, copy_selem);
if (ret) {
kfree(copy_selem);
atomic_sub(smap->elem_size,
@@ -925,18 +976,43 @@ BPF_CALL_2(bpf_sk_storage_delete, struct bpf_map *, map, struct sock *, sk)
return -ENOENT;
}
+static int sk_storage_charge(struct bpf_local_storage_map *smap,
+ void *owner, u32 size)
+{
+ return omem_charge(owner, size);
+}
+
+static void sk_storage_uncharge(struct bpf_local_storage_map *smap,
+ void *owner, u32 size)
+{
+ struct sock *sk = owner;
+
+ atomic_sub(size, &sk->sk_omem_alloc);
+}
+
+static struct bpf_local_storage __rcu **
+sk_storage_ptr(struct bpf_local_storage_map *smap, void *owner)
+{
+ struct sock *sk = owner;
+
+ return &sk->sk_bpf_storage;
+}
+
static int sk_storage_map_btf_id;
const struct bpf_map_ops sk_storage_map_ops = {
- .map_alloc_check = bpf_sk_storage_map_alloc_check,
- .map_alloc = bpf_local_storage_map_alloc,
- .map_free = bpf_local_storage_map_free,
+ .map_alloc_check = bpf_local_storage_map_alloc_check,
+ .map_alloc = sk_storage_map_alloc,
+ .map_free = sk_storage_map_free,
.map_get_next_key = notsupp_get_next_key,
.map_lookup_elem = bpf_fd_sk_storage_lookup_elem,
.map_update_elem = bpf_fd_sk_storage_update_elem,
.map_delete_elem = bpf_fd_sk_storage_delete_elem,
- .map_check_btf = bpf_sk_storage_map_check_btf,
+ .map_check_btf = bpf_local_storage_map_check_btf,
.map_btf_name = "bpf_local_storage_map",
.map_btf_id = &sk_storage_map_btf_id,
+ .map_local_storage_charge = sk_storage_charge,
+ .map_local_storage_uncharge = sk_storage_uncharge,
+ .map_owner_storage_ptr = sk_storage_ptr,
};
const struct bpf_func_proto bpf_sk_storage_get_proto = {
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 54d0c886e3ba..b9d2e4792d08 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -3630,9 +3630,13 @@ enum {
BPF_F_SYSCTL_BASE_NAME = (1ULL << 0),
};
-/* BPF_FUNC_sk_storage_get flags */
+/* BPF_FUNC_<local>_storage_get flags */
enum {
- BPF_SK_STORAGE_GET_F_CREATE = (1ULL << 0),
+ BPF_LOCAL_STORAGE_GET_F_CREATE = (1ULL << 0),
+ /* BPF_SK_STORAGE_GET_F_CREATE is only kept for backward compatibility
+ * and BPF_LOCAL_STORAGE_GET_F_CREATE must be used instead.
+ */
+ BPF_SK_STORAGE_GET_F_CREATE = BPF_LOCAL_STORAGE_GET_F_CREATE,
};
/* BPF_FUNC_read_branch_records flags. */
--
2.24.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox