Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs
On Thu, Dec 17, 2015 at 07:32:10AM -0500, Mimi Zohar wrote: > On Thu, 2015-12-17 at 14:45 +0800, Dave Young wrote: > > On 12/08/15 at 02:15pm, Mimi Zohar wrote: > > > There's a lot of code duplication for reading a file by the kernel (eg. > > > kexec, firmware, kernel modules, ...). Each place does it just a bit > > > differently than the other. There should be a single function for > > > reading and calculating the file hash at the same time. > > > > If you want to address the duplication for reading file, IMHO you can > > introduce a general interface to read file in kernel space. But I do not > > think the reading + calculating only interface is a good way. > > Ok. As described above, the call would read the buffer into memory and > then call IMA to calculate the buffer hash. > > (If someone else is interested in getting involved in kernel > development, cleaning up this code duplication is a good, relatively > small, self contained project.) I'm with Dave though, I realize that's work but I can help do it with you. 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 4/7] ima: measure and appraise kexec image and initramfs
On Thu, 2015-12-17 at 14:45 +0800, Dave Young wrote:
> On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> > On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> > > On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
> > >
> > > [..]
> > > > #ifdef CONFIG_IMA_APPRAISE
> > > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > > index b70ada0..18c4a84 100644
> > > > --- a/kernel/kexec_file.c
> > > > +++ b/kernel/kexec_file.c
> > > > @@ -18,6 +18,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> > > >
> > > > static int kexec_calculate_store_digests(struct kimage *image);
> > > >
> > > > -static int copy_file_from_fd(int fd, void **buf, unsigned long
> > > > *buf_len)
> > > > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > > > +void **buf, unsigned long *buf_len)
> > > > {
> > > > struct fd f = fdget(fd);
> > > > int ret;
> > > > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf,
> > > > unsigned long *buf_len)
> > > > goto out;
> > > > }
> > > >
> > > > + ret = ima_read_and_process_file(f.file, read_func, *buf,
> > > > stat.size);
> > > > + if (ret != -EOPNOTSUPP)
> > > > + goto out_free;
> > > > +
> > >
> > > Hi Mimi,
> > >
> > > I am wondering why are we designing this function to also read the file.
> >
> > > Looks like we just want an hook which calls into ima so that ima can
> > > apply its *additional* policies on kernel and initramfs. If caller is
> > > allocating the buffer, then caller can continue to read the file as well.
> > > In fact that simplifies the code. Now caller which need to check
> > > -EOPNOTSUPP and continue to read the file anyway.
> >
> > IMA is calculating the file hash as it reads the file. The file hash is
> > then used for normal IMA processing - adding the measurement to the
> > measurement list and verifying the file's integrity.
> >
> > > IOW, if caller still has to maintain the code to read the file, then it
> > > is better that ima exports a hook which callers calls after reading the
> > > file and ima can do its own verification.
> >
> > There's no sense in reading the file twice. The file hash should be
> > calculated as the file is being read. Either the existing function to
> > read the file needs to support calculating the file hash or it should
> > call IMA.
>
> Can IMA provide a function to calculate the hash from a buffer?
Yes, I think Dmitry might already have a buffer hash function. If we
use the buffer hash, we could then move the ima_read_and_process_file()
from before to after the existing file read function. The
ima_read_and_process_file() would need to be renamed, but the parameters
would remain the same. I think that would work.
> >
> > There's a lot of code duplication for reading a file by the kernel (eg.
> > kexec, firmware, kernel modules, ...). Each place does it just a bit
> > differently than the other. There should be a single function for
> > reading and calculating the file hash at the same time.
>
> If you want to address the duplication for reading file, IMHO you can
> introduce a general interface to read file in kernel space. But I do not
> think the reading + calculating only interface is a good way.
Ok. As described above, the call would read the buffer into memory and
then call IMA to calculate the buffer hash.
(If someone else is interested in getting involved in kernel
development, cleaning up this code duplication is a good, relatively
small, self contained project.)
> >
> > > Also why do we call second parameter as "read_func". I am really not
> > > passing a function read function. May be something lile file_type might
> > > be better.
> >
> > The read_func identifies the caller of ima_read_and_process_file().
> > The IMA policy would then look like:
> >
> > measure func=KEXEC_CHECK
> > appraise func=KEXEC_CHECK appraise_type=imasig
> > #
> > measure func=INITRAMFS_CHECK
> > appraise func=INITRAMFS_CHECK appraise_type=imasig
> > #
> > measure func=FIRMWARE_CHECK
> > appraise func=FIRMWARE_CHECK appraise_type=imasig
> > #
> > measure func=POLICY_CHECK
> > appraise func=POLICY_CHECK appraise_type=imasig
> >
> > > So how does this additional IMA specific policies help (on top of existing
> > > signature verification mechanism we have for kernel).
> >
> > The existing kexec signature verification is limited to verifying the
> > kexec image on x86_64. It does not verify the kexec image on other
> > architectures, nor does it verify the initramfs. The original method
> > for verifying a files' integrity was IMA. We have need for verifying
> > both the kexec image and initramfs.
> >
> > Mimi
> >
> Thanks
> Dave
Thank you for your suggestion.
Mimi
--
To unsubscribe from this list: send
Re: [PATCH v1 4/7] ima: measure and appraise kexec image and initramfs
On 12/08/15 at 02:15pm, Mimi Zohar wrote:
> On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> > On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
> >
> > [..]
> > > #ifdef CONFIG_IMA_APPRAISE
> > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > > index b70ada0..18c4a84 100644
> > > --- a/kernel/kexec_file.c
> > > +++ b/kernel/kexec_file.c
> > > @@ -18,6 +18,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> > >
> > > static int kexec_calculate_store_digests(struct kimage *image);
> > >
> > > -static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> > > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > > + void **buf, unsigned long *buf_len)
> > > {
> > > struct fd f = fdget(fd);
> > > int ret;
> > > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf,
> > > unsigned long *buf_len)
> > > goto out;
> > > }
> > >
> > > + ret = ima_read_and_process_file(f.file, read_func, *buf, stat.size);
> > > + if (ret != -EOPNOTSUPP)
> > > + goto out_free;
> > > +
> >
> > Hi Mimi,
> >
> > I am wondering why are we designing this function to also read the file.
>
> > Looks like we just want an hook which calls into ima so that ima can
> > apply its *additional* policies on kernel and initramfs. If caller is
> > allocating the buffer, then caller can continue to read the file as well.
> > In fact that simplifies the code. Now caller which need to check
> > -EOPNOTSUPP and continue to read the file anyway.
>
> IMA is calculating the file hash as it reads the file. The file hash is
> then used for normal IMA processing - adding the measurement to the
> measurement list and verifying the file's integrity.
>
> > IOW, if caller still has to maintain the code to read the file, then it
> > is better that ima exports a hook which callers calls after reading the
> > file and ima can do its own verification.
>
> There's no sense in reading the file twice. The file hash should be
> calculated as the file is being read. Either the existing function to
> read the file needs to support calculating the file hash or it should
> call IMA.
Can IMA provide a function to calculate the hash from a buffer?
>
> There's a lot of code duplication for reading a file by the kernel (eg.
> kexec, firmware, kernel modules, ...). Each place does it just a bit
> differently than the other. There should be a single function for
> reading and calculating the file hash at the same time.
If you want to address the duplication for reading file, IMHO you can
introduce a general interface to read file in kernel space. But I do not
think the reading + calculating only interface is a good way.
>
> > Also why do we call second parameter as "read_func". I am really not
> > passing a function read function. May be something lile file_type might
> > be better.
>
> The read_func identifies the caller of ima_read_and_process_file().
> The IMA policy would then look like:
>
> measure func=KEXEC_CHECK
> appraise func=KEXEC_CHECK appraise_type=imasig
> #
> measure func=INITRAMFS_CHECK
> appraise func=INITRAMFS_CHECK appraise_type=imasig
> #
> measure func=FIRMWARE_CHECK
> appraise func=FIRMWARE_CHECK appraise_type=imasig
> #
> measure func=POLICY_CHECK
> appraise func=POLICY_CHECK appraise_type=imasig
>
> > So how does this additional IMA specific policies help (on top of existing
> > signature verification mechanism we have for kernel).
>
> The existing kexec signature verification is limited to verifying the
> kexec image on x86_64. It does not verify the kexec image on other
> architectures, nor does it verify the initramfs. The original method
> for verifying a files' integrity was IMA. We have need for verifying
> both the kexec image and initramfs.
>
> Mimi
>
Thanks
Dave
--
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 4/7] ima: measure and appraise kexec image and initramfs
On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
[..]
> #ifdef CONFIG_IMA_APPRAISE
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index b70ada0..18c4a84 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -18,6 +18,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
>
> static int kexec_calculate_store_digests(struct kimage *image);
>
> -static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> + void **buf, unsigned long *buf_len)
> {
> struct fd f = fdget(fd);
> int ret;
> @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf, unsigned
> long *buf_len)
> goto out;
> }
>
> + ret = ima_read_and_process_file(f.file, read_func, *buf, stat.size);
> + if (ret != -EOPNOTSUPP)
> + goto out_free;
> +
Hi Mimi,
I am wondering why are we designing this function to also read the file.
Looks like we just want an hook which calls into ima so that ima can
apply its *additional* policies on kernel and initramfs. If caller is
allocating the buffer, then caller can continue to read the file as well.
In fact that simplifies the code. Now caller which need to check
-EOPNOTSUPP and continue to read the file anyway.
IOW, if caller still has to maintain the code to read the file, then it
is better that ima exports a hook which callers calls after reading the
file and ima can do its own verification.
Also why do we call second parameter as "read_func". I am really not
passing a function read function. May be something lile file_type might
be better.
So how does this additional IMA specific policies help (on top of existing
signature verification mechanism we have for kernel).
Thanks
Vivek
--
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 4/7] ima: measure and appraise kexec image and initramfs
On Tue, 2015-12-08 at 13:32 -0500, Vivek Goyal wrote:
> On Tue, Dec 08, 2015 at 01:01:21PM -0500, Mimi Zohar wrote:
>
> [..]
> > #ifdef CONFIG_IMA_APPRAISE
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index b70ada0..18c4a84 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -18,6 +18,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -33,7 +34,8 @@ size_t __weak kexec_purgatory_size = 0;
> >
> > static int kexec_calculate_store_digests(struct kimage *image);
> >
> > -static int copy_file_from_fd(int fd, void **buf, unsigned long *buf_len)
> > +static int copy_file_from_fd(int fd, enum ima_read_hooks read_func,
> > +void **buf, unsigned long *buf_len)
> > {
> > struct fd f = fdget(fd);
> > int ret;
> > @@ -65,14 +67,17 @@ static int copy_file_from_fd(int fd, void **buf,
> > unsigned long *buf_len)
> > goto out;
> > }
> >
> > + ret = ima_read_and_process_file(f.file, read_func, *buf, stat.size);
> > + if (ret != -EOPNOTSUPP)
> > + goto out_free;
> > +
>
> Hi Mimi,
>
> I am wondering why are we designing this function to also read the file.
> Looks like we just want an hook which calls into ima so that ima can
> apply its *additional* policies on kernel and initramfs. If caller is
> allocating the buffer, then caller can continue to read the file as well.
> In fact that simplifies the code. Now caller which need to check
> -EOPNOTSUPP and continue to read the file anyway.
IMA is calculating the file hash as it reads the file. The file hash is
then used for normal IMA processing - adding the measurement to the
measurement list and verifying the file's integrity.
> IOW, if caller still has to maintain the code to read the file, then it
> is better that ima exports a hook which callers calls after reading the
> file and ima can do its own verification.
There's no sense in reading the file twice. The file hash should be
calculated as the file is being read. Either the existing function to
read the file needs to support calculating the file hash or it should
call IMA.
There's a lot of code duplication for reading a file by the kernel (eg.
kexec, firmware, kernel modules, ...). Each place does it just a bit
differently than the other. There should be a single function for
reading and calculating the file hash at the same time.
> Also why do we call second parameter as "read_func". I am really not
> passing a function read function. May be something lile file_type might
> be better.
The read_func identifies the caller of ima_read_and_process_file().
The IMA policy would then look like:
measure func=KEXEC_CHECK
appraise func=KEXEC_CHECK appraise_type=imasig
#
measure func=INITRAMFS_CHECK
appraise func=INITRAMFS_CHECK appraise_type=imasig
#
measure func=FIRMWARE_CHECK
appraise func=FIRMWARE_CHECK appraise_type=imasig
#
measure func=POLICY_CHECK
appraise func=POLICY_CHECK appraise_type=imasig
> So how does this additional IMA specific policies help (on top of existing
> signature verification mechanism we have for kernel).
The existing kexec signature verification is limited to verifying the
kexec image on x86_64. It does not verify the kexec image on other
architectures, nor does it verify the initramfs. The original method
for verifying a files' integrity was IMA. We have need for verifying
both the kexec image and initramfs.
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
