| Message ID | 20200724213640.389191-1-keescook@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Introduce partial kernel_read_file() support | expand |
On 2020-07-24 2:36 p.m., Kees Cook wrote: > v3: > - add reviews/acks > - add "IMA: Add support for file reads without contents" patch > - trim CC list, in case that's why vger ignored v2 > v2: [missing from lkml archives! (CC list too long?) repeating changes here] > - fix issues in firmware test suite > - add firmware partial read patches > - various bug fixes/cleanups > v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ > > Hi, > > Here's my tree for adding partial read support in kernel_read_file(), > which fixes a number of issues along the way. It's got Scott's firmware > and IMA patches ported and everything tests cleanly for me (even with > CONFIG_IMA_APPRAISE=y). > > I think the intention is for this to go via Greg's tree since Scott's > driver code will depend on it? v3 of this patch series looks good and passes all of my tests. Remaining patches Acked-by: Scott Branden <scott.branden@broadcom.com> I have added latest bcm-vk driver code to Kees' patch series and added it here: https://github.com/sbranden/linux/tree/kernel_read_file_for_kees_v3 If everyone finds Kees' patch series acceptable then the 3 patches adding the bcm-vk driver need to be added to the series. I can send the 3 patches out separately and then the two patch series can be combined in Greg or someone's tree if that works? Or if an in-kernel user beyond kernel selftest is needed for request_partial_firmware_into_buf in Kees' patch series then another PATCH v4 needs to be sent out including the bcm-vk driver. > > Thanks, > > -Kees > > > Kees Cook (15): Thanks for help Kees, it's works now. > test_firmware: Test platform fw loading on non-EFI systems > selftest/firmware: Add selftest timeout in settings > firmware_loader: EFI firmware loader must handle pre-allocated buffer > fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum > fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum > fs/kernel_read_file: Split into separate source file > fs/kernel_read_file: Remove redundant size argument > fs/kernel_read_file: Switch buffer size arg to size_t > fs/kernel_read_file: Add file_size output argument > LSM: Introduce kernel_post_load_data() hook > firmware_loader: Use security_post_load_data() > module: Call security_kernel_post_load_data() > LSM: Add "contents" flag to kernel_read_file hook > fs/kernel_file_read: Add "offset" arg for partial reads > firmware: Store opt_flags in fw_priv > > Scott Branden (4): > fs/kernel_read_file: Split into separate include file > IMA: Add support for file reads without contents > firmware: Add request_partial_firmware_into_buf() > test_firmware: Test partial read support > > drivers/base/firmware_loader/fallback.c | 19 +- > drivers/base/firmware_loader/fallback.h | 5 +- > .../base/firmware_loader/fallback_platform.c | 16 +- > drivers/base/firmware_loader/firmware.h | 7 +- > drivers/base/firmware_loader/main.c | 143 ++++++++++--- > drivers/firmware/efi/embedded-firmware.c | 21 +- > drivers/firmware/efi/embedded-firmware.h | 19 ++ > fs/Makefile | 3 +- > fs/exec.c | 132 +----------- > fs/kernel_read_file.c | 189 ++++++++++++++++++ > include/linux/efi_embedded_fw.h | 13 -- > include/linux/firmware.h | 12 ++ > include/linux/fs.h | 39 ---- > include/linux/ima.h | 19 +- > include/linux/kernel_read_file.h | 55 +++++ > include/linux/lsm_hook_defs.h | 6 +- > include/linux/lsm_hooks.h | 12 ++ > include/linux/security.h | 19 +- > kernel/kexec.c | 2 +- > kernel/kexec_file.c | 19 +- > kernel/module.c | 24 ++- > lib/test_firmware.c | 159 +++++++++++++-- > security/integrity/digsig.c | 8 +- > security/integrity/ima/ima_fs.c | 10 +- > security/integrity/ima/ima_main.c | 70 +++++-- > security/integrity/ima/ima_policy.c | 1 + > security/loadpin/loadpin.c | 17 +- > security/security.c | 26 ++- > security/selinux/hooks.c | 8 +- > .../selftests/firmware/fw_filesystem.sh | 91 +++++++++ > tools/testing/selftests/firmware/settings | 8 + > tools/testing/selftests/kselftest/runner.sh | 6 +- > 32 files changed, 860 insertions(+), 318 deletions(-) > create mode 100644 drivers/firmware/efi/embedded-firmware.h > create mode 100644 fs/kernel_read_file.c > create mode 100644 include/linux/kernel_read_file.h > create mode 100644 tools/testing/selftests/firmware/settings >
On Fri, Jul 24, 2020 at 02:36:21PM -0700, Kees Cook wrote: > v3: > - add reviews/acks > - add "IMA: Add support for file reads without contents" patch > - trim CC list, in case that's why vger ignored v2 > v2: [missing from lkml archives! (CC list too long?) repeating changes here] > - fix issues in firmware test suite > - add firmware partial read patches > - various bug fixes/cleanups > v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ > > Hi, > > Here's my tree for adding partial read support in kernel_read_file(), > which fixes a number of issues along the way. It's got Scott's firmware > and IMA patches ported and everything tests cleanly for me (even with > CONFIG_IMA_APPRAISE=y). > > I think the intention is for this to go via Greg's tree since Scott's > driver code will depend on it? I've applied the first 3 now, as I think I need some acks/reviewed-by from the subsystem owners of the other patches before I can take them. thanks, greg k-h
On Sat, Jul 25, 2020 at 12:05:55PM +0200, Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2020 at 02:36:21PM -0700, Kees Cook wrote: > > v3: > > - add reviews/acks > > - add "IMA: Add support for file reads without contents" patch > > - trim CC list, in case that's why vger ignored v2 > > v2: [missing from lkml archives! (CC list too long?) repeating changes here] > > - fix issues in firmware test suite > > - add firmware partial read patches > > - various bug fixes/cleanups > > v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ > > > > Hi, > > > > Here's my tree for adding partial read support in kernel_read_file(), > > which fixes a number of issues along the way. It's got Scott's firmware > > and IMA patches ported and everything tests cleanly for me (even with > > CONFIG_IMA_APPRAISE=y). > > > > I think the intention is for this to go via Greg's tree since Scott's > > driver code will depend on it? > > I've applied the first 3 now, as I think I need some acks/reviewed-by > from the subsystem owners of the other patches before I can take them. Sounds good; thanks! (I would argue 4 and 5 are also bug fixes, 6 is already Acked by hch and you, and 7 is a logical follow-up to 6, but I get your point.) James, Luis, Mimi, and Jessica, the bulk of these patches are LSM, firmware, IMA, and modules. How does this all look to you? And KP, you'd mentioned privately that you were interested in being able to use the new kernel_post_load_data LSM hook for better visibility into non-file-backed blob loading. Thanks! -Kees
On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote: > v3: > - add reviews/acks > - add "IMA: Add support for file reads without contents" patch > - trim CC list, in case that's why vger ignored v2 > v2: [missing from lkml archives! (CC list too long?) repeating changes here] > - fix issues in firmware test suite > - add firmware partial read patches > - various bug fixes/cleanups > v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ > > Hi, > > Here's my tree for adding partial read support in kernel_read_file(), > which fixes a number of issues along the way. It's got Scott's firmware > and IMA patches ported and everything tests cleanly for me (even with > CONFIG_IMA_APPRAISE=y). Thanks, Kees. Other than my comments on the new security_kernel_post_load_data() hook, the patch set is really nice. In addition to compiling with CONFIG_IMA_APPRAISE enabled, have you booted the kernel with the ima_policy=tcb? The tcb policy will add measurements to the IMA measurement list and extend the TPM with the file or buffer data digest. Are you seeing the firmware measurements, in particular the partial read measurement? thanks, Mimi
Hi Mimi/Kees, On 2020-07-27 4:16 a.m., Mimi Zohar wrote: > On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote: >> v3: >> - add reviews/acks >> - add "IMA: Add support for file reads without contents" patch >> - trim CC list, in case that's why vger ignored v2 >> v2: [missing from lkml archives! (CC list too long?) repeating changes here] >> - fix issues in firmware test suite >> - add firmware partial read patches >> - various bug fixes/cleanups >> v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ >> >> Hi, >> >> Here's my tree for adding partial read support in kernel_read_file(), >> which fixes a number of issues along the way. It's got Scott's firmware >> and IMA patches ported and everything tests cleanly for me (even with >> CONFIG_IMA_APPRAISE=y). > Thanks, Kees. Other than my comments on the new > security_kernel_post_load_data() hook, the patch set is really nice. > > In addition to compiling with CONFIG_IMA_APPRAISE enabled, have you > booted the kernel with the ima_policy=tcb? The tcb policy will add > measurements to the IMA measurement list and extend the TPM with the > file or buffer data digest. Are you seeing the firmware measurements, > in particular the partial read measurement? I booted the kernel with ima_policy=tcb. Unfortunately after enabling the following, fw_run_tests.sh does not run. mkdir /sys/kernel/security mount -t securityfs securityfs /sys/kernel/security echo "measure func=FIRMWARE_CHECK" > /sys/kernel/security/ima/policy echo "appraise func=FIRMWARE_CHECK appraise_type=imasig" > /sys/kernel/security/ima/policy ./fw_run_tests.sh [ 1296.258052] test_firmware: loading 'test-firmware.bin' [ 1296.263903] misc test_firmware: loading /lib/firmware/test-firmware.bin failed with error -13 [ 1296.263905] audit: type=1800 audit(1595905754.266:9): pid=5696 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=appraise_data cause=IMA-signature-required comm="fw_namespace" name="/lib/firmware/test-firmware.bin" dev="tmpfs" ino=4592 res=0 [ 1296.297085] misc test_firmware: Direct firmware load for test-firmware.bin failed with error -13 [ 1296.305947] test_firmware: load of 'test-firmware.bin' failed: -13 > > thanks, > > Mimi
On Mon, 2020-07-27 at 12:18 -0700, Scott Branden wrote: > Hi Mimi/Kees, > > On 2020-07-27 4:16 a.m., Mimi Zohar wrote: > > On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote: > >> v3: > >> - add reviews/acks > >> - add "IMA: Add support for file reads without contents" patch > >> - trim CC list, in case that's why vger ignored v2 > >> v2: [missing from lkml archives! (CC list too long?) repeating changes > here] > >> - fix issues in firmware test suite > >> - add firmware partial read patches > >> - various bug fixes/cleanups > >> v1: > https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ > >> > >> Hi, > >> > >> Here's my tree for adding partial read support in kernel_read_file(), > >> which fixes a number of issues along the way. It's got Scott's firmware > >> and IMA patches ported and everything tests cleanly for me (even with > >> CONFIG_IMA_APPRAISE=y). > > Thanks, Kees. Other than my comments on the new > > security_kernel_post_load_data() hook, the patch set is really nice. > > > > In addition to compiling with CONFIG_IMA_APPRAISE enabled, have you > > booted the kernel with the ima_policy=tcb? The tcb policy will add > > measurements to the IMA measurement list and extend the TPM with the > > file or buffer data digest. Are you seeing the firmware measurements, > > in particular the partial read measurement? > I booted the kernel with ima_policy=tcb. > > Unfortunately after enabling the following, fw_run_tests.sh does not run. > > mkdir /sys/kernel/security > mount -t securityfs securityfs /sys/kernel/security > echo "measure func=FIRMWARE_CHECK" > /sys/kernel/security/ima/policy > echo "appraise func=FIRMWARE_CHECK appraise_type=imasig" > > /sys/kernel/security/ima/policy > ./fw_run_tests.sh > > [ 1296.258052] test_firmware: loading 'test-firmware.bin' > [ 1296.263903] misc test_firmware: loading /lib/firmware/test-firmware.bin > failed with error -13 > [ 1296.263905] audit: type=1800 audit(1595905754.266:9): pid=5696 uid=0 > auid=4294967295 ses=4294967295 subj=kernel op=appraise_data cause=IMA- > signature-required comm="fw_namespace" name="/lib/firmware/test-firmware.bin" > dev="tmpfs" ino=4592 res=0 > [ 1296.297085] misc test_firmware: Direct firmware load for test-firmware.bin > failed with error -13 > [ 1296.305947] test_firmware: load of 'test-firmware.bin' failed: -13 The "appraise" rule verifies the IMA signature. Unless you signed the firmware (evmctl) and load the public key on the IMA keyring, that's to be expected. I assume you are seeing firmware measurements in the IMA measuremenet log. Mimi
Hi Mimi, On 2020-07-28 11:48 a.m., Mimi Zohar wrote: > On Mon, 2020-07-27 at 12:18 -0700, Scott Branden wrote: >> Hi Mimi/Kees, >> >> On 2020-07-27 4:16 a.m., Mimi Zohar wrote: >>> On Fri, 2020-07-24 at 14:36 -0700, Kees Cook wrote: >>>> v3: >>>> - add reviews/acks >>>> - add "IMA: Add support for file reads without contents" patch >>>> - trim CC list, in case that's why vger ignored v2 >>>> v2: [missing from lkml archives! (CC list too long?) repeating changes >> here] >>>> - fix issues in firmware test suite >>>> - add firmware partial read patches >>>> - various bug fixes/cleanups >>>> v1: >> https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ >>>> Hi, >>>> >>>> Here's my tree for adding partial read support in kernel_read_file(), >>>> which fixes a number of issues along the way. It's got Scott's firmware >>>> and IMA patches ported and everything tests cleanly for me (even with >>>> CONFIG_IMA_APPRAISE=y). >>> Thanks, Kees. Other than my comments on the new >>> security_kernel_post_load_data() hook, the patch set is really nice. >>> >>> In addition to compiling with CONFIG_IMA_APPRAISE enabled, have you >>> booted the kernel with the ima_policy=tcb? The tcb policy will add >>> measurements to the IMA measurement list and extend the TPM with the >>> file or buffer data digest. Are you seeing the firmware measurements, >>> in particular the partial read measurement? >> I booted the kernel with ima_policy=tcb. >> >> Unfortunately after enabling the following, fw_run_tests.sh does not run. >> >> mkdir /sys/kernel/security >> mount -t securityfs securityfs /sys/kernel/security >> echo "measure func=FIRMWARE_CHECK" > /sys/kernel/security/ima/policy >> echo "appraise func=FIRMWARE_CHECK appraise_type=imasig" > >> /sys/kernel/security/ima/policy >> ./fw_run_tests.sh >> >> [ 1296.258052] test_firmware: loading 'test-firmware.bin' >> [ 1296.263903] misc test_firmware: loading /lib/firmware/test-firmware.bin >> failed with error -13 >> [ 1296.263905] audit: type=1800 audit(1595905754.266:9): pid=5696 uid=0 >> auid=4294967295 ses=4294967295 subj=kernel op=appraise_data cause=IMA- >> signature-required comm="fw_namespace" name="/lib/firmware/test-firmware.bin" >> dev="tmpfs" ino=4592 res=0 >> [ 1296.297085] misc test_firmware: Direct firmware load for test-firmware.bin >> failed with error -13 >> [ 1296.305947] test_firmware: load of 'test-firmware.bin' failed: -13 > The "appraise" rule verifies the IMA signature. Unless you signed the firmware > (evmctl) and load the public key on the IMA keyring, that's to be expected. I > assume you are seeing firmware measurements in the IMA measuremenet log. Yes, I see the firmware measurements in the IMA measurement log. I have not signed the firmware nor loaded a public key on the IMA keyring. Therefore everything is working as expected. > > Mimi > Thanks, Scott
On Fri, Jul 24, 2020 at 02:36:21PM -0700, Kees Cook wrote: > v3: > - add reviews/acks > - add "IMA: Add support for file reads without contents" patch > - trim CC list, in case that's why vger ignored v2 > v2: [missing from lkml archives! (CC list too long?) repeating changes here] > - fix issues in firmware test suite > - add firmware partial read patches > - various bug fixes/cleanups > v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keescook@chromium.org/ For patches 1-10: Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> Luis