LoadPin: Ignore the "contents" argument of the LSM hooks

PATCH In review Kees Cook 3 participants 5 messages Dec 9, 2022 – Dec 15, 2022

Subsystems (2)

Security (LSM) Loadpin security module

lore

Share & tools

lore

Kees Cook

author

2022-12-09 19:55 UTC #0 lore raw

PATCH 1/1 LoadPin: Ignore the "contents" argument of the LSM hooks

LoadPin only enforces the read-only origin of kernel file reads. Whether or not it was a partial read isn't important. Remove the overly conservative checks so that things like partial firmware reads will succeed (i.e. reading a firmware header).

Fixes 2039bda1fa8d ("LSM: Add "contents" flag to kernel_read_file hook")

Cc Paul Moore <[email protected]>

Cc James Morris <[email protected]>

Cc "Serge E. Hallyn" <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

security/loadpin/loadpin.c +18 ~~-12~~

index de41621f4998..110a5ab2b46b 100644
@@ -122,21 +122,11 @@ 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,
-			     bool contents)
+static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 {
 	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]) {

@@ -192,9 +182,25 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
 	return 0;
 }
 
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
+			     bool contents)
+{
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so we can ignore the "are full contents available"
+	 * argument here.
+	 */
+	return loadpin_check(file, 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, contents);
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so a NULL file is passed, and we can ignore the
+	 * state of "contents".
+	 */
+	return loadpin_check(NULL, (enum kernel_read_file_id) id);
 }
 
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {

Signature · 1 line

2.34.1

Serge E. Hallyn

maintainer Acked-by on v1 1/1

2022-12-12 21:15 UTC #1 lore raw

Quoted 9 lines · Kees Cook

LoadPin only enforces the read-only origin of kernel file reads. Whether or not it was a partial read isn't important. Remove the overly conservative checks so that things like partial firmware reads will succeed (i.e. reading a firmware header).
Fixes 2039bda1fa8d ("LSM: Add "contents" flag to kernel_read_file hook")
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>

Acked-by Serge Hallyn <[email protected]>

Seems reasonable.

So the patch which introduced this was
2039bda1f: LSM: Add "contents" flag to kernel_read_file hook It sounds like the usage of @contents which it added to ima still makes sense. But what about the selinux_kernel_read_file() one?

Quoted 62 lines

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 security/loadpin/loadpin.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

security/loadpin/loadpin.c +18 ~~-12~~

index de41621f4998..110a5ab2b46b 100644
@@ -122,21 +122,11 @@ 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,
-			     bool contents)
+static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 {
 	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]) {

@@ -192,9 +182,25 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
 	return 0;
 }
 
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
+			     bool contents)
+{
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so we can ignore the "are full contents available"
+	 * argument here.
+	 */
+	return loadpin_check(file, 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, contents);
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so a NULL file is passed, and we can ignore the
+	 * state of "contents".
+	 */
+	return loadpin_check(NULL, (enum kernel_read_file_id) id);
 }
 
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {

Signature · 1 line

2.34.1

Kees Cook

author

2022-12-14 04:07 UTC #2 lore raw

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Kees Cook
LoadPin only enforces the read-only origin of kernel file reads. Whether or not it was a partial read isn't important. Remove the overly conservative checks so that things like partial firmware reads will succeed (i.e. reading a firmware header).

Quoted 8 lines

Quoted 4 lines
Fixes 2039bda1fa8d ("LSM: Add "contents" flag to kernel_read_file hook")
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Acked-by Serge Hallyn <[email protected]>
Seems reasonable.

Thanks!

Quoted 4 lines

So the patch which introduced this was
2039bda1f: LSM: Add "contents" flag to kernel_read_file hook It sounds like the usage of @contents which it added to ima still makes sense. But what about the selinux_kernel_read_file() one?

I think those continue to make sense since those LSM may be sensitive to the _content_ (rather than the _origin_) of the file.

-Kees

Signature · 1 line

Kees Cook

Paul Moore

maintainer

2022-12-15 20:16 UTC #3 lore raw

Quoted 18 lines · Kees Cook

Quoted 6 lines · Serge E. Hallyn
Quoted 5 lines · Kees Cook
LoadPin only enforces the read-only origin of kernel file reads. Whether or not it was a partial read isn't important. Remove the overly conservative checks so that things like partial firmware reads will succeed (i.e. reading a firmware header).
Quoted 8 lines
Quoted 4 lines
Fixes 2039bda1fa8d ("LSM: Add "contents" flag to kernel_read_file hook")
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Acked-by Serge Hallyn <[email protected]>
Seems reasonable.
Thanks!

Quoted 7 lines

Quoted 4 lines
So the patch which introduced this was
2039bda1f: LSM: Add "contents" flag to kernel_read_file hook It sounds like the usage of @contents which it added to ima still makes sense. But what about the selinux_kernel_read_file() one?
I think those continue to make sense since those LSM may be sensitive to the _content_ (rather than the _origin_) of the file.

Agreed. When @contents is false SELinux does a permission check between the calling process and itself, but when @contents is true it performs a check between the calling process and the file being read.

Signature · 1 line

paul-moore.com

Kees Cook

author

2022-12-14 04:06 UTC #4 lore raw

In reply to Kees Cook

Quoted 7 lines · Ping-Ke Shih

From: Kees Cook <[email protected]>
Quoted 5 lines
LoadPin only enforces the read-only origin of kernel file reads. Whether or not it was a partial read isn't important. Remove the overly conservative checks so that things like partial firmware reads will succeed (i.e. reading a firmware header).

Quoted 8 lines

Quoted 6 lines
Fixes 2039bda1fa8d ("LSM: Add "contents" flag to kernel_read_file hook")
Cc Paul Moore <[email protected]>
Cc James Morris <[email protected]>
Cc "Serge E. Hallyn" <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Tested-by Ping-Ke Shih <[email protected]>

Thanks for testing!

-Kees

Quoted 63 lines

Quoted 61 lines

---
 security/loadpin/loadpin.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

security/loadpin/loadpin.c +18 ~~-12~~

index de41621f4998..110a5ab2b46b 100644
@@ -122,21 +122,11 @@ 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,
-			     bool contents)
+static int loadpin_check(struct file *file, enum kernel_read_file_id id)
 {
 	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]) {

@@ -192,9 +182,25 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
 	return 0;
 }
 
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
+			     bool contents)
+{
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so we can ignore the "are full contents available"
+	 * argument here.
+	 */
+	return loadpin_check(file, 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, contents);
+	/*
+	 * LoadPin only cares about the _origin_ of a file, not its
+	 * contents, so a NULL file is passed, and we can ignore the
+	 * state of "contents".
+	 */
+	return loadpin_check(NULL, (enum kernel_read_file_id) id);
 }
 
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {

Signature · 1 line

2.34.1

Signature · 1 line

Kees Cook