Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

On Mon, 2015-12-21 at 22:44 +0100, Luis R. Rodriguez wrote:
> 
> Eventually, once we generalize a common read perhaps we should stuff this
> into VFS common code and provide arguments to enable callers to provide 
> restrictions or requirements. Let's work together on that after the holidays.
> 
> Let's consolidate notes here:
> 
> http://kernelnewbies.org/KernelProjects/common-kernel-loader

Sounds good!

Mimi


--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Luis R. Rodriguez]

On Sat, Dec 19, 2015 at 11:44:41PM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 22:06 +0100, Luis R. Rodriguez wrote:
> > On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 8524450..dcd902f 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -29,6 +29,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  
> > > @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> > > struct firmware_buf *fw_buf)
> > >   buf = vmalloc(size);
> > >   if (!buf)
> > >   return -ENOMEM;
> > > - rc = kernel_read(file, 0, buf, size);
> > > - if (rc != size) {
> > > - if (rc > 0)
> > > - rc = -EIO;
> > > +
> > > + rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> > > + if (rc == -EIO)
> > >   goto fail;
> > > + else if (rc != -EOPNOTSUPP) {
> > > + rc = kernel_read(file, 0, buf, size);
> > > + if (rc != size) {
> > > + if (rc > 0)
> > > + rc = -EIO;
> > > + goto fail;
> > > + }
> > >   }
> > >   rc = security_kernel_fw_from_file(file, buf, size);
> > >   if (rc)
> > 
> > This is one way, the other way is to generalize the kernel-read from path
> > routine. I have some changes which help generalize this routine a bit so
> > help on review there would be appreciated. 
> 
> Sure.  Where are the patches?

http://lkml.kernel.org/r/1431996325-8840-2-git-send-email-mcg...@do-not-panic.com

I'll post these in PATCH form now.

> > I'm personally indifferent
> > as to needing or not *now* a generic kernel read routine that is shared
> > for this purpose *but* since this patch set *also* seems to be adding
> > yet-another file reading I'm more inclined to wish for that to be addressed
> > now instead.
> > 
> > Please let me know if this logic is fair.
> 
> Commit e3c4abb - "integrity: define a new function
> integrity_read_file()" defined a method of reading a file from the
> kernel.  It's used to load an x509 key onto the IMA keyring for systems
> without an initramfs.   Dmitry's patch, included in this patch set,
> calls this function to load the IMA policy as well.  So this patch set
> isn't defining a new function for reading a file from the kernel.  It's
> using an existing one.

I see thanks, 

> FYI, sound/sound_firmware.c: do_mod_firmware_load() also reads a file.

Thanks, this should be generalized as well the only reason for a different
implementation I see here is the size constraint to 128k max. I think we can
move that crap check out to take advantage of a common read.

The integrity_read_file() seems rather generic as well and just skips
locking checks and security checks, a generic solution doesn't have to happen
now because as you note this has been in the kernel for a while.

Eventually, once we generalize a common read perhaps we should stuff this
into VFS common code and provide arguments to enable callers to provide 
restrictions or requirements. Let's work together on that after the holidays.

Let's consolidate notes here:

http://kernelnewbies.org/KernelProjects/common-kernel-loader

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

On Thu, 2015-12-17 at 22:06 +0100, Luis R. Rodriguez wrote:
> On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 8524450..dcd902f 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -29,6 +29,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  
> > @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> > struct firmware_buf *fw_buf)
> > buf = vmalloc(size);
> > if (!buf)
> > return -ENOMEM;
> > -   rc = kernel_read(file, 0, buf, size);
> > -   if (rc != size) {
> > -   if (rc > 0)
> > -   rc = -EIO;
> > +
> > +   rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> > +   if (rc == -EIO)
> > goto fail;
> > +   else if (rc != -EOPNOTSUPP) {
> > +   rc = kernel_read(file, 0, buf, size);
> > +   if (rc != size) {
> > +   if (rc > 0)
> > +   rc = -EIO;
> > +   goto fail;
> > +   }
> > }
> > rc = security_kernel_fw_from_file(file, buf, size);
> > if (rc)
> 
> This is one way, the other way is to generalize the kernel-read from path
> routine. I have some changes which help generalize this routine a bit so
> help on review there would be appreciated. 

Sure.  Where are the patches?

> I'm personally indifferent
> as to needing or not *now* a generic kernel read routine that is shared
> for this purpose *but* since this patch set *also* seems to be adding
> yet-another file reading I'm more inclined to wish for that to be addressed
> now instead.
> 
> Please let me know if this logic is fair.

Commit e3c4abb - "integrity: define a new function
integrity_read_file()" defined a method of reading a file from the
kernel.  It's used to load an x509 key onto the IMA keyring for systems
without an initramfs.   Dmitry's patch, included in this patch set,
calls this function to load the IMA policy as well.  So this patch set
isn't defining a new function for reading a file from the kernel.  It's
using an existing one.

FYI, sound/sound_firmware.c: do_mod_firmware_load() also reads a file.

Mimi

--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Luis R. Rodriguez]

On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..dcd902f 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -29,6 +29,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> struct firmware_buf *fw_buf)
>   buf = vmalloc(size);
>   if (!buf)
>   return -ENOMEM;
> - rc = kernel_read(file, 0, buf, size);
> - if (rc != size) {
> - if (rc > 0)
> - rc = -EIO;
> +
> + rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> + if (rc == -EIO)
>   goto fail;
> + else if (rc != -EOPNOTSUPP) {
> + rc = kernel_read(file, 0, buf, size);
> + if (rc != size) {
> + if (rc > 0)
> + rc = -EIO;
> + goto fail;
> + }
>   }
>   rc = security_kernel_fw_from_file(file, buf, size);
>   if (rc)

This is one way, the other way is to generalize the kernel-read from path
routine. I have some changes which help generalize this routine a bit so
help on review there would be appreciated. I'm personally indifferent
as to needing or not *now* a generic kernel read routine that is shared
for this purpose *but* since this patch set *also* seems to be adding
yet-another file reading I'm more inclined to wish for that to be addressed
now instead.

Please let me know if this logic is fair.

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 5/7] ima: measure and appraise firmware (improvement)

[Mimi Zohar]

Instead of reading the firmware twice, once for measuring/appraising
the firmware and again reading the file contents into memory, this
patch reads the firmware once.

Signed-off-by: Mimi Zohar 
---
 drivers/base/firmware_class.c | 15 +++
 include/linux/ima.h   |  2 +-
 security/integrity/ima/ima.h  |  2 +-
 security/integrity/ima/ima_appraise.c |  5 -
 security/integrity/ima/ima_main.c |  2 +-
 security/integrity/ima/ima_policy.c   | 23 +++
 security/integrity/integrity.h| 11 ---
 7 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8524450..dcd902f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
struct firmware_buf *fw_buf)
buf = vmalloc(size);
if (!buf)
return -ENOMEM;
-   rc = kernel_read(file, 0, buf, size);
-   if (rc != size) {
-   if (rc > 0)
-   rc = -EIO;
+
+   rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
+   if (rc == -EIO)
goto fail;
+   else if (rc != -EOPNOTSUPP) {
+   rc = kernel_read(file, 0, buf, size);
+   if (rc != size) {
+   if (rc > 0)
+   rc = -EIO;
+   goto fail;
+   }
}
rc = security_kernel_fw_from_file(file, buf, size);
if (rc)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 5d83ecf..7bd4e07 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -13,7 +13,7 @@
 #include 
 struct linux_binprm;
 
-enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, IMA_MAX_READ_CHECK};
+enum ima_read_hooks { KEXEC_CHECK = 1, INITRAMFS_CHECK, FIRMWARE_CHECK, 
IMA_MAX_READ_CHECK};
 
 #ifdef CONFIG_IMA
 extern int ima_bprm_check(struct linux_binprm *bprm);
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8dc9077..548b258 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -160,7 +160,7 @@ void ima_free_template_entry(struct ima_template_entry 
*entry);
 const char *ima_d_path(struct path *path, char **pathbuf);
 
 /* IMA policy related functions */
-enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, FIRMWARE_CHECK, POST_SETATTR};
+enum ima_hooks { FILE_CHECK = IMA_MAX_READ_CHECK, MMAP_CHECK, BPRM_CHECK, 
MODULE_CHECK, POST_SETATTR};
 
 int ima_match_policy(struct inode *inode, enum ima_hooks func, int mask,
 int flags);
diff --git a/security/integrity/ima/ima_appraise.c 
b/security/integrity/ima/ima_appraise.c
index e58df45..b83049b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -77,7 +77,6 @@ enum integrity_status ima_get_cache_status(struct 
integrity_iint_cache *iint,
case MODULE_CHECK:
return iint->ima_module_status;
case FIRMWARE_CHECK:
-   return iint->ima_firmware_status;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
return iint->ima_read_status;
@@ -101,8 +100,6 @@ static void ima_set_cache_status(struct 
integrity_iint_cache *iint,
iint->ima_module_status = status;
break;
case FIRMWARE_CHECK:
-   iint->ima_firmware_status = status;
-   break;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
iint->ima_read_status = status;
@@ -127,8 +124,6 @@ static void ima_cache_flags(struct integrity_iint_cache 
*iint, int func)
iint->flags |= (IMA_MODULE_APPRAISED | IMA_APPRAISED);
break;
case FIRMWARE_CHECK:
-   iint->flags |= (IMA_FIRMWARE_APPRAISED | IMA_APPRAISED);
-   break;
case KEXEC_CHECK:
case INITRAMFS_CHECK:
iint->flags |= (IMA_READ_APPRAISED | IMA_APPRAISED);
diff --git a/security/integrity/ima/ima_main.c 
b/security/integrity/ima/ima_main.c
index b8b0b8c..f9206cd 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -375,7 +375,7 @@ int ima_fw_from_file(struct file *file, char *buf, size_t 
size)
return -EACCES; /* INTEGRITY_UNKNOWN */
return 0;
}
-   return process_measurement(file, MAY_EXEC, FIRMWARE_CHECK, NULL, 0);
+   return 0;
 }
 
 static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c 
b/security/integrity/ima/ima_policy.c
index 6fe0ab9..a65cb2a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -102,7 +102,7 @@ static struct ima_rule_entry original_measurement_rules[] = 
{