Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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@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