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

["Luis R. Rodriguez" <mcgrof@suse.com>]

On Wed, Jan 20, 2016 at 3:39 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> On Mon, Jan 18, 2016 at 10:11:22AM -0500, Mimi Zohar wrote:
>> Replace fw_read_file_contents() for reading a file with the common VFS
>> kernel_read_file() function.  A benefit of calling kernel_read_file()
>> to read the firmware is the firmware is read only once, instead of once
>> for measuring/appraising the firmware and again for reading the file
>> contents into memory.
>>
>> This patch retains the kernel_fw_from_file() hook, which is called from
>> security_kernel_post_read_file(), but removes the
>> sercurity_kernel_fw_from_file() function.
>>
>> Changelog:
>> - reordered and squashed firmware patches
>> - fix MAX firmware size (Kees Cook)
>>
>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> ---
>>  drivers/base/firmware_class.c         | 48 ++++++++++-------------------------
>>  include/linux/ima.h                   |  7 +----
>>  include/linux/security.h              |  8 +-----
>>  security/integrity/ima/ima.h          |  1 -
>>  security/integrity/ima/ima_appraise.c |  8 ------
>>  security/integrity/ima/ima_main.c     | 18 +++++--------
>>  security/integrity/ima/ima_policy.c   | 26 +++++++++----------
>>  security/integrity/integrity.h        | 11 +++-----
>>  security/security.c                   | 28 ++++++++++----------
>>  9 files changed, 54 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> index 8524450..cc175f1 100644
>> --- a/drivers/base/firmware_class.c
>> +++ b/drivers/base/firmware_class.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/syscore_ops.h>
>>  #include <linux/reboot.h>
>>  #include <linux/security.h>
>> +#include <linux/ima.h>
>>
>>  #include <generated/utsrelease.h>
>>
>> @@ -291,40 +292,10 @@ static const char * const fw_path[] = {
>>  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
>>  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
>>
>> -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
>> -{
>> -     int size;
>> -     char *buf;
>> -     int rc;
>> -
>> -     if (!S_ISREG(file_inode(file)->i_mode))
>> -             return -EINVAL;
>> -     size = i_size_read(file_inode(file));
>> -     if (size <= 0)
>> -             return -EINVAL;
>> -     buf = vmalloc(size);
>> -     if (!buf)
>> -             return -ENOMEM;
>> -     rc = kernel_read(file, 0, buf, size);
>> -     if (rc != size) {
>> -             if (rc > 0)
>> -                     rc = -EIO;
>> -             goto fail;
>> -     }
>> -     rc = security_kernel_fw_from_file(file, buf, size);
>> -     if (rc)
>> -             goto fail;
>> -     fw_buf->data = buf;
>> -     fw_buf->size = size;
>> -     return 0;
>> -fail:
>> -     vfree(buf);
>> -     return rc;
>> -}
>> -
>>  static int fw_get_filesystem_firmware(struct device *device,
>>                                      struct firmware_buf *buf)
>>  {
>> +     loff_t size;
>>       int i, len;
>>       int rc = -ENOENT;
>>       char *path;
>> @@ -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). 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.
>
>> 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.

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.

 Luis