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