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 <linux/kexec.h>
> > > > #include <linux/mutex.h>
> > > > #include <linux/list.h>
> > > > +#include <linux/ima.h>
> > > > #include <crypto/hash.h>
> > > > #include <crypto/sha.h>
> > > > #include <linux/syscalls.h>
> > > > @@ -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 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
