mbox series

[v3,00/19] Introduce partial kernel_read_file() support

Message ID 20200724213640.389191-1-keescook@chromium.org
Headers show
Series Introduce partial kernel_read_file() support | expand

Message

Kees Cook July 24, 2020, 9:36 p.m. UTC
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?

Thanks,

-Kees


Kees Cook (15):
  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

Comments

Scott Branden July 25, 2020, 5:14 a.m. UTC | #1
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
>
Greg KH July 25, 2020, 10:05 a.m. UTC | #2
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
Kees Cook July 25, 2020, 3:48 p.m. UTC | #3
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
Mimi Zohar July 27, 2020, 11:16 a.m. UTC | #4
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
Scott Branden July 27, 2020, 7:18 p.m. UTC | #5
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
Mimi Zohar July 28, 2020, 6:48 p.m. UTC | #6
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
Scott Branden July 28, 2020, 7:56 p.m. UTC | #7
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
Luis Chamberlain July 29, 2020, 1:19 a.m. UTC | #8
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