On Thu, Jan 21, 2016 at 4:05 AM, Mimi Zohar <zohar at linux.vnet.ibm.com> wrote:
> 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?
Yes, the firmware might have different license. Its also easier for
users to use standard mechanisms to verify.
>> 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.
OK!
>> >> 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
> };
Looks good to me. Please add a kdoc section to force us to have to
document each one.
> Agreed, the last field of kernel_read_file() and the wrappers should be
> the enumeration.
Great.
>> 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.
Great.
Luis
