mbox series

[0/4] Fix misused kernel_read_file() enums

Message ID 20200707081926.3688096-1-keescook@chromium.org (mailing list archive)
Headers show
Series Fix misused kernel_read_file() enums | expand

Message

Kees Cook July 7, 2020, 8:19 a.m. UTC
Hi,

In looking for closely at the additions that got made to the
kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
*kinds* of files for the LSM to reason about. They are a "how" and
"where", respectively. Remove these improper aliases and refactor the
code to adapt to the changes.

Additionally adds in missing calls to security_kernel_post_read_file()
in the platform firmware fallback path (to match the sysfs firmware
fallback path) and in module loading. I considered entirely removing
security_kernel_post_read_file() hook since it is technically unused,
but IMA probably wants to be able to measure EFI-stored firmware images,
so I wired it up and matched it for modules, in case anyone wants to
move the module signature checks out of the module core and into an LSM
to avoid the current layering violations.

This touches several trees, and I suspect it would be best to go through
James's LSM tree.

Thanks!

-Kees

Kees Cook (4):
  firmware_loader: EFI firmware loader must handle pre-allocated buffer
  fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
  fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
  module: Add hook for security_kernel_post_read_file()

 drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
 drivers/base/firmware_loader/main.c              |  5 ++---
 fs/exec.c                                        |  7 ++++---
 include/linux/fs.h                               |  3 +--
 include/linux/lsm_hooks.h                        |  6 +++++-
 kernel/module.c                                  |  7 ++++++-
 security/integrity/ima/ima_main.c                |  6 ++----
 7 files changed, 30 insertions(+), 16 deletions(-)

Comments

Greg KH July 7, 2020, 9:31 a.m. UTC | #1
On Tue, Jul 07, 2020 at 01:19:22AM -0700, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.
> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.
> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.
> 
> Thanks!
> 
> -Kees
> 
> Kees Cook (4):
>   firmware_loader: EFI firmware loader must handle pre-allocated buffer
>   fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>   fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>   module: Add hook for security_kernel_post_read_file()

Looks good to me, thanks for fixing this up:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Mimi Zohar July 7, 2020, 3:36 p.m. UTC | #2
Hi Kees,

On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.

Thank you for adding the missing calls and the firmware pre allocated
buffer comment update.

> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.

IMa has always verified kernel module signatures.  Recently appended
kernel module signature support was added to IMA.  The same appended
signature format is also being used to sign and verify the kexec
kernel image.

With IMA's new kernel module appended signature support and patch 4/4
in this series, IMA won't be limit to the finit_module syscall, but
could support the init_module syscall as well.

> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.

Sure.

thanks!

Mimi
Kees Cook July 7, 2020, 9:45 p.m. UTC | #3
On Tue, Jul 07, 2020 at 11:36:04AM -0400, Mimi Zohar wrote:
> Hi Kees,
> 
> On Tue, 2020-07-07 at 01:19 -0700, Kees Cook wrote:
> > Hi,
> > 
> > In looking for closely at the additions that got made to the
> > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > *kinds* of files for the LSM to reason about. They are a "how" and
> > "where", respectively. Remove these improper aliases and refactor the
> > code to adapt to the changes.
> 
> Thank you for adding the missing calls and the firmware pre allocated
> buffer comment update.
> 
> > 
> > Additionally adds in missing calls to security_kernel_post_read_file()
> > in the platform firmware fallback path (to match the sysfs firmware
> > fallback path) and in module loading. I considered entirely removing
> > security_kernel_post_read_file() hook since it is technically unused,
> > but IMA probably wants to be able to measure EFI-stored firmware images,
> > so I wired it up and matched it for modules, in case anyone wants to
> > move the module signature checks out of the module core and into an LSM
> > to avoid the current layering violations.
> 
> IMa has always verified kernel module signatures.  Recently appended

Right, but not through the kernel_post_read_file() hook, nor via
out-of-band hooks in kernel/module.c. I was just meaning that future
work could be done here to regularize module_sig_check() into an actual
LSM (which could, in theory, be extended to kexec() to avoid code
duplication there, as kimage_validate_signature() has some overlap with
mod_verify_sig()). into a bit more normal of an LSM.

As far as IMA and regularizing things, though, what about fixing IMA to
not manually stack:

$ grep -B3 ima_ security/security.c
        ret = call_int_hook(bprm_check_security, 0, bprm);
        if (ret)
                return ret;
        return ima_bprm_check(bprm);
--
        ret = call_int_hook(file_mprotect, 0, vma, reqprot, prot);
        if (ret)
                return ret;
        return ima_file_mprotect(vma, prot);
...

Can IMA implement a hook-last method to join the regular stacking routines?

> kernel module signature support was added to IMA.  The same appended
> signature format is also being used to sign and verify the kexec
> kernel image.
> 
> With IMA's new kernel module appended signature support and patch 4/4
> in this series, IMA won't be limit to the finit_module syscall, but
> could support the init_module syscall as well.

Exactly.

> > This touches several trees, and I suspect it would be best to go through
> > James's LSM tree.
> 
> Sure.

Is this an "Acked-by"? :)
Hans de Goede July 8, 2020, 11:01 a.m. UTC | #4
Hi,

On 7/7/20 10:19 AM, Kees Cook wrote:
> Hi,
> 
> In looking for closely at the additions that got made to the
> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> *kinds* of files for the LSM to reason about. They are a "how" and
> "where", respectively. Remove these improper aliases and refactor the
> code to adapt to the changes.
> 
> Additionally adds in missing calls to security_kernel_post_read_file()
> in the platform firmware fallback path (to match the sysfs firmware
> fallback path) and in module loading. I considered entirely removing
> security_kernel_post_read_file() hook since it is technically unused,
> but IMA probably wants to be able to measure EFI-stored firmware images,
> so I wired it up and matched it for modules, in case anyone wants to
> move the module signature checks out of the module core and into an LSM
> to avoid the current layering violations.
> 
> This touches several trees, and I suspect it would be best to go through
> James's LSM tree.
> 
> Thanks!


I've done some quick tests on this series to make sure that
the efi embedded-firmware support did not regress.
That still works fine, so this series is;

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> 
> -Kees
> 
> Kees Cook (4):
>    firmware_loader: EFI firmware loader must handle pre-allocated buffer
>    fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>    fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>    module: Add hook for security_kernel_post_read_file()
> 
>   drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
>   drivers/base/firmware_loader/main.c              |  5 ++---
>   fs/exec.c                                        |  7 ++++---
>   include/linux/fs.h                               |  3 +--
>   include/linux/lsm_hooks.h                        |  6 +++++-
>   kernel/module.c                                  |  7 ++++++-
>   security/integrity/ima/ima_main.c                |  6 ++----
>   7 files changed, 30 insertions(+), 16 deletions(-)
>
Hans de Goede July 8, 2020, 11:37 a.m. UTC | #5
Hi,

On 7/8/20 1:01 PM, Hans de Goede wrote:
> Hi,
> 
> On 7/7/20 10:19 AM, Kees Cook wrote:
>> Hi,
>>
>> In looking for closely at the additions that got made to the
>> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
>> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
>> *kinds* of files for the LSM to reason about. They are a "how" and
>> "where", respectively. Remove these improper aliases and refactor the
>> code to adapt to the changes.
>>
>> Additionally adds in missing calls to security_kernel_post_read_file()
>> in the platform firmware fallback path (to match the sysfs firmware
>> fallback path) and in module loading. I considered entirely removing
>> security_kernel_post_read_file() hook since it is technically unused,
>> but IMA probably wants to be able to measure EFI-stored firmware images,
>> so I wired it up and matched it for modules, in case anyone wants to
>> move the module signature checks out of the module core and into an LSM
>> to avoid the current layering violations.
>>
>> This touches several trees, and I suspect it would be best to go through
>> James's LSM tree.
>>
>> Thanks!
> 
> 
> I've done some quick tests on this series to make sure that
> the efi embedded-firmware support did not regress.
> That still works fine, so this series is;
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

I made a mistake during testing I was not actually running the
kernel with the patches added.

After fixing that I did find a problem, patch 4/4:
"module: Add hook for security_kernel_post_read_file()"

Breaks module-loading for me. This is with the 4 patches
on top of 5.8.0-rc4, so this might just be because I'm
not using the right base.

With patch 4/4 reverted things work fine for me.

So, please only add my Tested-by to patches 1-3.

Regards,

Hans



>> Kees Cook (4):
>>    firmware_loader: EFI firmware loader must handle pre-allocated buffer
>>    fs: Remove FIRMWARE_PREALLOC_BUFFER from kernel_read_file() enums
>>    fs: Remove FIRMWARE_EFI_EMBEDDED from kernel_read_file() enums
>>    module: Add hook for security_kernel_post_read_file()
>>
>>   drivers/base/firmware_loader/fallback_platform.c | 12 ++++++++++--
>>   drivers/base/firmware_loader/main.c              |  5 ++---
>>   fs/exec.c                                        |  7 ++++---
>>   include/linux/fs.h                               |  3 +--
>>   include/linux/lsm_hooks.h                        |  6 +++++-
>>   kernel/module.c                                  |  7 ++++++-
>>   security/integrity/ima/ima_main.c                |  6 ++----
>>   7 files changed, 30 insertions(+), 16 deletions(-)
>>
Luis Chamberlain July 8, 2020, 11:55 a.m. UTC | #6
On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/8/20 1:01 PM, Hans de Goede wrote:
> > Hi,
> > 
> > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > Hi,
> > > 
> > > In looking for closely at the additions that got made to the
> > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > "where", respectively. Remove these improper aliases and refactor the
> > > code to adapt to the changes.
> > > 
> > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > in the platform firmware fallback path (to match the sysfs firmware
> > > fallback path) and in module loading. I considered entirely removing
> > > security_kernel_post_read_file() hook since it is technically unused,
> > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > so I wired it up and matched it for modules, in case anyone wants to
> > > move the module signature checks out of the module core and into an LSM
> > > to avoid the current layering violations.
> > > 
> > > This touches several trees, and I suspect it would be best to go through
> > > James's LSM tree.
> > > 
> > > Thanks!
> > 
> > 
> > I've done some quick tests on this series to make sure that
> > the efi embedded-firmware support did not regress.
> > That still works fine, so this series is;
> > 
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> I made a mistake during testing I was not actually running the
> kernel with the patches added.
> 
> After fixing that I did find a problem, patch 4/4:
> "module: Add hook for security_kernel_post_read_file()"
> 
> Breaks module-loading for me. This is with the 4 patches
> on top of 5.8.0-rc4, so this might just be because I'm
> not using the right base.
> 
> With patch 4/4 reverted things work fine for me.
> 
> So, please only add my Tested-by to patches 1-3.

BTW is there any testing covered by the selftests for the firmware
laoder which would have caputured this? If not can you extend
it with something to capture this case you ran into?

  Luis
Hans de Goede July 8, 2020, 11:58 a.m. UTC | #7
Hi,

On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/8/20 1:01 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 7/7/20 10:19 AM, Kees Cook wrote:
>>>> Hi,
>>>>
>>>> In looking for closely at the additions that got made to the
>>>> kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
>>>> and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
>>>> *kinds* of files for the LSM to reason about. They are a "how" and
>>>> "where", respectively. Remove these improper aliases and refactor the
>>>> code to adapt to the changes.
>>>>
>>>> Additionally adds in missing calls to security_kernel_post_read_file()
>>>> in the platform firmware fallback path (to match the sysfs firmware
>>>> fallback path) and in module loading. I considered entirely removing
>>>> security_kernel_post_read_file() hook since it is technically unused,
>>>> but IMA probably wants to be able to measure EFI-stored firmware images,
>>>> so I wired it up and matched it for modules, in case anyone wants to
>>>> move the module signature checks out of the module core and into an LSM
>>>> to avoid the current layering violations.
>>>>
>>>> This touches several trees, and I suspect it would be best to go through
>>>> James's LSM tree.
>>>>
>>>> Thanks!
>>>
>>>
>>> I've done some quick tests on this series to make sure that
>>> the efi embedded-firmware support did not regress.
>>> That still works fine, so this series is;
>>>
>>> Tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> I made a mistake during testing I was not actually running the
>> kernel with the patches added.
>>
>> After fixing that I did find a problem, patch 4/4:
>> "module: Add hook for security_kernel_post_read_file()"
>>
>> Breaks module-loading for me. This is with the 4 patches
>> on top of 5.8.0-rc4, so this might just be because I'm
>> not using the right base.
>>
>> With patch 4/4 reverted things work fine for me.
>>
>> So, please only add my Tested-by to patches 1-3.
> 
> BTW is there any testing covered by the selftests for the firmware
> laoder which would have caputured this? If not can you extend
> it with something to capture this case you ran into?

This was not a firmware-loading issue. For me in my tests,
which were limited to 1 device, patch 4/4, which only touches
the module-loading code, stopped module loading from working.

Since my test device has / on an eMMC and the kernel config
I'm using has mmc-block as a module, things just hung in the
initrd since no modules could be loaded, so I did not debug
this any further. Dropping  patch 4/4 from my local tree
solved this.

Regards,

Hans
Luis Chamberlain July 8, 2020, 1:30 p.m. UTC | #8
On Wed, Jul 08, 2020 at 01:58:47PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> > On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 7/8/20 1:01 PM, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > > > Hi,
> > > > > 
> > > > > In looking for closely at the additions that got made to the
> > > > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > > > "where", respectively. Remove these improper aliases and refactor the
> > > > > code to adapt to the changes.
> > > > > 
> > > > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > > > in the platform firmware fallback path (to match the sysfs firmware
> > > > > fallback path) and in module loading. I considered entirely removing
> > > > > security_kernel_post_read_file() hook since it is technically unused,
> > > > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > > > so I wired it up and matched it for modules, in case anyone wants to
> > > > > move the module signature checks out of the module core and into an LSM
> > > > > to avoid the current layering violations.
> > > > > 
> > > > > This touches several trees, and I suspect it would be best to go through
> > > > > James's LSM tree.
> > > > > 
> > > > > Thanks!
> > > > 
> > > > 
> > > > I've done some quick tests on this series to make sure that
> > > > the efi embedded-firmware support did not regress.
> > > > That still works fine, so this series is;
> > > > 
> > > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > I made a mistake during testing I was not actually running the
> > > kernel with the patches added.
> > > 
> > > After fixing that I did find a problem, patch 4/4:
> > > "module: Add hook for security_kernel_post_read_file()"
> > > 
> > > Breaks module-loading for me. This is with the 4 patches
> > > on top of 5.8.0-rc4, so this might just be because I'm
> > > not using the right base.
> > > 
> > > With patch 4/4 reverted things work fine for me.
> > > 
> > > So, please only add my Tested-by to patches 1-3.
> > 
> > BTW is there any testing covered by the selftests for the firmware
> > laoder which would have caputured this? If not can you extend
> > it with something to capture this case you ran into?
> 
> This was not a firmware-loading issue. For me in my tests,
> which were limited to 1 device, patch 4/4, which only touches
> the module-loading code, stopped module loading from working.
> 
> Since my test device has / on an eMMC and the kernel config
> I'm using has mmc-block as a module, things just hung in the
> initrd since no modules could be loaded, so I did not debug
> this any further. Dropping  patch 4/4 from my local tree
> solved this.

Thanks Hans!

Kees, would test_kmod.c and the respective selftest would have picked
this issue up?

  Luis
Kees Cook July 9, 2020, 2 a.m. UTC | #9
On Wed, Jul 08, 2020 at 01:30:04PM +0000, Luis Chamberlain wrote:
> On Wed, Jul 08, 2020 at 01:58:47PM +0200, Hans de Goede wrote:
> > Hi,
> > 
> > On 7/8/20 1:55 PM, Luis Chamberlain wrote:
> > > On Wed, Jul 08, 2020 at 01:37:41PM +0200, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 7/8/20 1:01 PM, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 7/7/20 10:19 AM, Kees Cook wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > In looking for closely at the additions that got made to the
> > > > > > kernel_read_file() enums, I noticed that FIRMWARE_PREALLOC_BUFFER
> > > > > > and FIRMWARE_EFI_EMBEDDED were added, but they are not appropriate
> > > > > > *kinds* of files for the LSM to reason about. They are a "how" and
> > > > > > "where", respectively. Remove these improper aliases and refactor the
> > > > > > code to adapt to the changes.
> > > > > > 
> > > > > > Additionally adds in missing calls to security_kernel_post_read_file()
> > > > > > in the platform firmware fallback path (to match the sysfs firmware
> > > > > > fallback path) and in module loading. I considered entirely removing
> > > > > > security_kernel_post_read_file() hook since it is technically unused,
> > > > > > but IMA probably wants to be able to measure EFI-stored firmware images,
> > > > > > so I wired it up and matched it for modules, in case anyone wants to
> > > > > > move the module signature checks out of the module core and into an LSM
> > > > > > to avoid the current layering violations.
> > > > > > 
> > > > > > This touches several trees, and I suspect it would be best to go through
> > > > > > James's LSM tree.
> > > > > > 
> > > > > > Thanks!
> > > > > 
> > > > > 
> > > > > I've done some quick tests on this series to make sure that
> > > > > the efi embedded-firmware support did not regress.
> > > > > That still works fine, so this series is;
> > > > > 
> > > > > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > > > 
> > > > I made a mistake during testing I was not actually running the
> > > > kernel with the patches added.
> > > > 
> > > > After fixing that I did find a problem, patch 4/4:
> > > > "module: Add hook for security_kernel_post_read_file()"
> > > > 
> > > > Breaks module-loading for me. This is with the 4 patches
> > > > on top of 5.8.0-rc4, so this might just be because I'm
> > > > not using the right base.
> > > > 
> > > > With patch 4/4 reverted things work fine for me.
> > > > 
> > > > So, please only add my Tested-by to patches 1-3.
> > > 
> > > BTW is there any testing covered by the selftests for the firmware
> > > laoder which would have caputured this? If not can you extend
> > > it with something to capture this case you ran into?
> > 
> > This was not a firmware-loading issue. For me in my tests,
> > which were limited to 1 device, patch 4/4, which only touches
> > the module-loading code, stopped module loading from working.
> > 
> > Since my test device has / on an eMMC and the kernel config
> > I'm using has mmc-block as a module, things just hung in the
> > initrd since no modules could be loaded, so I did not debug
> > this any further. Dropping  patch 4/4 from my local tree
> > solved this.
> 
> Thanks Hans!
> 
> Kees, would test_kmod.c and the respective selftest would have picked
> this issue up?

I need to check -- I got a (possibly related) 0day report on it too.

Since I have to clean it up further based on Mimi's comments, and adapt
it a bit for Scott's series, I'll need to get a v2 spun for sure. :)