| Message ID | 20221209195453.never.494-kees@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LoadPin: Ignore the "contents" argument of the LSM hooks | expand |
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 <paul@paul-moore.com> > Cc: James Morris <jmorris@namei.org> > Cc: "Serge E. Hallyn" <serge@hallyn.com> Acked-by: Serge Hallyn <serge@hallyn.com> 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: linux-security-module@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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 <keescook@chromium.org> > > > 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 <paul@paul-moore.com> > > Cc: James Morris <jmorris@namei.org> > > Cc: "Serge E. Hallyn" <serge@hallyn.com> > > Cc: linux-security-module@vger.kernel.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Tested-by: Ping-Ke Shih <pkshih@realtek.com> 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 > >
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 <paul@paul-moore.com> > > Cc: James Morris <jmorris@namei.org> > > Cc: "Serge E. Hallyn" <serge@hallyn.com> > > Acked-by: Serge Hallyn <serge@hallyn.com> > > 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
On Tue, Dec 13, 2022 at 11:06 PM Kees Cook <keescook@chromium.org> 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 <paul@paul-moore.com> > > > Cc: James Morris <jmorris@namei.org> > > > Cc: "Serge E. Hallyn" <serge@hallyn.com> > > > > Acked-by: Serge Hallyn <serge@hallyn.com> > > > > 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.
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 = {
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 <paul@paul-moore.com> Cc: James Morris <jmorris@namei.org> Cc: "Serge E. Hallyn" <serge@hallyn.com> Cc: linux-security-module@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- security/loadpin/loadpin.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-)