On Wed, 2016-01-20 at 15:56 -0800, Luis R. Rodriguez wrote:
> On Wed, Jan 20, 2016 at 3:39 PM, Luis R. Rodriguez <mcgrof at suse.com> wrote:
> >> @@ -350,13 +321,18 @@ static int fw_get_filesystem_firmware(struct device *device,
> >> file = filp_open(path, O_RDONLY, 0);
> >> if (IS_ERR(file))
> >> continue;
> >> - rc = fw_read_file_contents(file, buf);
> >> +
> >> + buf->size = 0;
> >> + rc = kernel_read_file(file, &buf->data, &size, INT_MAX,
> >> + FIRMWARE_CHECK);
> >
> > The way kernel firmware signing was implemented was that we'd first read the
> > foo.sig (or whatever extension we use).
Was there a reason for using a detached signature and not using the same
method as kernel modules?
> The same kernel_read_file() would be
> > used if this gets applied so this would still works well with that. Of course
> > folks using IMA and their own security policy would just disable the kernel
> > fw signing facility.
Right, support for not measuring/appraising the firmware and sig would
be supported in the policy.
> >> diff --git a/include/linux/ima.h b/include/linux/ima.h
> >> index ae91938..0a7f039 100644
> >> --- a/include/linux/ima.h
> >> +++ b/include/linux/ima.h
> >> @@ -16,6 +16,7 @@ struct linux_binprm;
> >> enum ima_policy_id {
> >> KEXEC_CHECK = 1,
> >> INITRAMFS_CHECK,
> >> + FIRMWARE_CHECK,
> >> IMA_MAX_READ_CHECK
> >> };
> >
> > The only thing that is worth questioning here in light of kernel fw signing is
> > what int policy_id do we use? Would we be OK to add FIRMWARE_SIG_CHECK later
> > While at it, perhaps kernel_read_file() last argument should be enum
> > ima_policy_id then. If the policy_id is going to be used elsewhere outside of
> > IMA then perhaps ima.h is not the best place for it ? Would fs.h for type of
> > file be OK ? Then we'd have a list of known file types the kernel reads.
I would definitely prefer the enumeration be defined at the VFS layer.
For example,
enum kernel_read_file_id {
READING_KEXEC_IMAGE,
READING_KEXEC_INITRAMFS,
READING_FIRMWARE,
READING_FIRMWARE_SIG,
READING_MAX_ID
};
Agreed, the last field of kernel_read_file() and the wrappers should be
the enumeration.
> Actually your patch #9 "ima: load policy using path" defines
> kernel_read_file_from_path and since the firmware code uses a path
> this code would be much cleaner if instead you used that. It'd mean
> more code sharing and would make firmware code cleaner. Could you
> re-order that to go first and then later this as its first user?
> Perhaps add the helper for the firmware patch.
Thanks, I missed that. I'll include this change in the next version.
Mimi
