From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from e23smtp02.au.ibm.com ([202.81.31.144]:44148 "EHLO e23smtp02.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758970AbcAUMHA (ORCPT ); Thu, 21 Jan 2016 07:07:00 -0500 Received: from localhost by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 21 Jan 2016 22:06:55 +1000 Message-ID: <1453377959.9549.84.camel@linux.vnet.ibm.com> Subject: Re: [RFC PATCH v2 07/11] firmware: replace call to fw_read_file_contents() with kernel version From: Mimi Zohar To: "Luis R. Rodriguez" Cc: Johannes Berg , linux-security-module , kexec@lists.infradead.org, linux-modules@vger.kernel.org, fsdevel@vger.kernel.org, David Howells , David Woodhouse , Kees Cook , Dmitry Torokhov , Dmitry Kasatkin Date: Thu, 21 Jan 2016 07:05:59 -0500 In-Reply-To: References: <1453129886-20192-1-git-send-email-zohar@linux.vnet.ibm.com> <1453129886-20192-8-git-send-email-zohar@linux.vnet.ibm.com> <20160120233919.GM11277@wotan.suse.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Sender: owner-linux-modules@vger.kernel.org List-ID: 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 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