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(-)
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index de41621f4998..110a5ab2b46b 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -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 = {
--
2.34.1
On Fri, Dec 09, 2022 at 11:54:57AM -0800, Kees Cook wrote:
> 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?
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> security/loadpin/loadpin.c | 30 ++++++++++++++++++------------
> 1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index de41621f4998..110a5ab2b46b 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -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 = {
> --
> 2.34.1
On Wed, Dec 14, 2022 at 11:51:15AM +0800, Ping-Ke Shih wrote:
> From: Kees Cook <[email protected]>
>
> > 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]>
>
> Tested-by: Ping-Ke Shih <[email protected]>
Thanks for testing!
-Kees
>
> > ---
> > security/loadpin/loadpin.c | 30 ++++++++++++++++++------------
> > 1 file changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> > index de41621f4998..110a5ab2b46b 100644
> > --- a/security/loadpin/loadpin.c
> > +++ b/security/loadpin/loadpin.c
> > @@ -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 = {
> > --
> > 2.34.1
>
>
--
Kees Cook
On Mon, Dec 12, 2022 at 03:13:19PM -0600, Serge E. Hallyn wrote:
> On Fri, Dec 09, 2022 at 11:54:57AM -0800, Kees Cook wrote:
> > 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.
Thanks!
> 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
--
Kees Cook
On Tue, Dec 13, 2022 at 11:06 PM Kees Cook <[email protected]> wrote:
> On Mon, Dec 12, 2022 at 03:13:19PM -0600, Serge E. Hallyn wrote:
> > On Fri, Dec 09, 2022 at 11:54:57AM -0800, Kees Cook wrote:
> > > 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.
>
> Thanks!
>
> > 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.
--
paul-moore.com