Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Sun, May 12, 2024 at 09:32:40PM +0200, Joel Granados wrote: > On Sat, May 11, 2024 at 11:51:18AM +0200, Thomas Weißschuh wrote: > > Hi Kees, > > > > On 2024-05-08 10:11:35+, Kees Cook wrote: > > > On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > > > > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > > > > The series was split from my larger series sysctl-const series [0]. > > > > > It only focusses on the proc_handlers but is an important step to be > > > > > able to move all static definitions of ctl_table into .rodata. > > > > > > > > Split this per subsystem, please. > > > > > > I've done a few painful API transitions before, and I don't think the > > > complexity of these changes needs a per-subsystem constification pass. I > > > think this series is the right approach, but that patch 11 will need > > > coordination with Linus. We regularly do system-wide prototype changes > > > like this right at the end of the merge window before -rc1 comes out. > > > > That sounds good. > > > > > The requirements are pretty simple: it needs to be a obvious changes > > > (this certainly is) and as close to 100% mechanical as possible. I think > > > patch 11 easily qualifies. Linus should be able to run the same Coccinelle > > > script and get nearly the same results, etc. And all the other changes > > > need to have landed. This change also has no "silent failure" conditions: > > > anything mismatched will immediately stand out. > > > > Unfortunately coccinelle alone is not sufficient, as some helpers with > > different prototypes are called by handlers and themselves are calling > > handler and therefore need to change in the same commit. > > But if I add a diff for those on top of the coccinelle script to the > > changelog it should be obvious. > Judging by Kees' comment on "100% mechanical", it might be better just > having the diff and have Linus apply than rather than two step process? > Have not these types of PRs, so am interested in what folks think. I tried to soften it a little with my "*close* to 100%" modifier, and I think that patch basically matched that requirement, and where it had manual changes it was detailed in the commit log. I only split out the seccomp part because it could actually stand alone. So yeah, let's get the last of the subsystem specific stuff landed after -rc1, and it should be possible to finish it all up for 6.11. Yay! :) -Kees -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers
On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote: > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote: > > The series was split from my larger series sysctl-const series [0]. > > It only focusses on the proc_handlers but is an important step to be > > able to move all static definitions of ctl_table into .rodata. > > Split this per subsystem, please. I've done a few painful API transitions before, and I don't think the complexity of these changes needs a per-subsystem constification pass. I think this series is the right approach, but that patch 11 will need coordination with Linus. We regularly do system-wide prototype changes like this right at the end of the merge window before -rc1 comes out. The requirements are pretty simple: it needs to be a obvious changes (this certainly is) and as close to 100% mechanical as possible. I think patch 11 easily qualifies. Linus should be able to run the same Coccinelle script and get nearly the same results, etc. And all the other changes need to have landed. This change also has no "silent failure" conditions: anything mismatched will immediately stand out. So, have patches 1-10 go via their respective subsystems, and once all of those are in Linus's tree, send patch 11 as a stand-alone PR. (From patch 11, it looks like the seccomp read/write function changes could be split out? I'll do that now...) -Kees -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2] vmcore: replace strncpy with strscpy_pad
On Mon, Apr 01, 2024 at 06:39:55PM +, Justin Stitt wrote:
> strncpy() is in the process of being replaced as it is deprecated [1].
> We should move towards safer and less ambiguous string interfaces.
>
> Looking at vmcoredd_header's definition:
> | struct vmcoredd_header {
> | __u32 n_namesz; /* Name size */
> | __u32 n_descsz; /* Content size */
> | __u32 n_type; /* NT_VMCOREDD */
> | __u8 name[8]; /* LINUX\0\0\0 */
> | __u8 dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Device dump's name
> */
> | };
> ... we see that @name wants to be NUL-padded.
>
> We're copying data->dump_name which is defined as:
> | char dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Unique name of the dump */
> ... which shares the same size as vdd_hdr->dump_name. Let's make sure we
> NUL-pad this as well.
>
> Use strscpy_pad() which NUL-terminates and NUL-pads its destination
> buffers. Specifically, use the new 2-argument version of strscpy_pad
> introduced in Commit e6584c3964f2f ("string: Allow 2-argument
> strscpy()").
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Justin Stitt
Looks good; thanks!
Reviewed-by: Kees Cook
--
Kees Cook
Re: [PATCH] vmcore: replace strncpy with strtomem
On Wed, Mar 27, 2024 at 09:10:52PM +, Justin Stitt wrote:
> strncpy() is in the process of being replaced as it is deprecated in
> some situations [1]. While the specific use of strncpy that this patch
> targets is not exactly deprecated, the real mission is to rid the kernel
> of all its uses.
>
> Looking at vmcoredd_header's definition:
> | struct vmcoredd_header {
> | __u32 n_namesz; /* Name size */
> | __u32 n_descsz; /* Content size */
> | __u32 n_type; /* NT_VMCOREDD */
> | __u8 name[8]; /* LINUX\0\0\0 */
> | __u8 dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Device dump's name
> */
> | };
> ... we can see that both `name` and `dump_name` are u8s. It seems `name`
> wants to be NUL-padded (based on the comment above), but for the sake of
> symmetry lets NUL-pad both of these.
Do we have a way to know that dump_name is not parsed by userspace as a
NUL-terminated string?
>
> Mark these buffers as __nonstring and use strtomem_pad.
>
> Link:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings
> [1]
> Link: https://github.com/KSPP/linux/issues/90
> Cc: linux-harden...@vger.kernel.org
> Signed-off-by: Justin Stitt
> ---
> Note: build-tested only.
>
> Found with: $ rg "strncpy\("
> ---
> fs/proc/vmcore.c| 5 ++---
> include/uapi/linux/vmcore.h | 4 ++--
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/proc/vmcore.c b/fs/proc/vmcore.c
> index 1fb213f379a5..5d7ecf3b75e8 100644
> --- a/fs/proc/vmcore.c
> +++ b/fs/proc/vmcore.c
> @@ -1370,9 +1370,8 @@ static void vmcoredd_write_header(void *buf, struct
> vmcoredd_data *data,
> vdd_hdr->n_descsz = size + sizeof(vdd_hdr->dump_name);
> vdd_hdr->n_type = NT_VMCOREDD;
>
> - strncpy((char *)vdd_hdr->name, VMCOREDD_NOTE_NAME,
> - sizeof(vdd_hdr->name));
> - memcpy(vdd_hdr->dump_name, data->dump_name, sizeof(vdd_hdr->dump_name));
> + strtomem_pad(vdd_hdr->name, VMCOREDD_NOTE_NAME, 0);
> + strtomem_pad(vdd_hdr->dump_name, data->dump_name, 0);
> }
>
> /**
> diff --git a/include/uapi/linux/vmcore.h b/include/uapi/linux/vmcore.h
> index 3e9da91866ff..7053e2b62fa0 100644
> --- a/include/uapi/linux/vmcore.h
> +++ b/include/uapi/linux/vmcore.h
> @@ -11,8 +11,8 @@ struct vmcoredd_header {
> __u32 n_namesz; /* Name size */
> __u32 n_descsz; /* Content size */
> __u32 n_type; /* NT_VMCOREDD */
> - __u8 name[8]; /* LINUX\0\0\0 */
> - __u8 dump_name[VMCOREDD_MAX_NAME_BYTES]; /* Device dump's name */
> + __u8 name[8] __nonstring; /* LINUX\0\0\0 */
> + __u8 dump_name[VMCOREDD_MAX_NAME_BYTES] __nonstring; /* Device dump's
> name */
> };
Unfortunately since this is UAPI, we can't sanely use __nonstring here.
:(
--
Kees Cook
Re: [PATCH 05/10] seccomp: Remove the now superfluous sentinel elements from ctl_table array
On Tue, Nov 07, 2023 at 02:45:05PM +0100, Joel Granados via B4 Relay wrote: > From: Joel Granados > > This commit comes at the tail end of a greater effort to remove the > empty elements at the end of the ctl_table arrays (sentinels) which > will reduce the overall build time size of the kernel and run time > memory bloat by ~64 bytes per sentinel (further information Link : > https://lore.kernel.org/all/zo5yx5jfoggi%2f...@bombadil.infradead.org/) > > Remove sentinel element from seccomp_sysctl_table. > > Signed-off-by: Joel Granados Acked-by: Kees Cook -- Kees Cook
Re: [PATCH] kexec: Annotate struct crash_mem with __counted_by
On Fri, 22 Sep 2023 10:52:24 -0700, Kees Cook wrote: > Prepare for the coming implementation by GCC and Clang of the __counted_by > attribute. Flexible array members annotated with __counted_by can have > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family > functions). > > As found with Coccinelle[1], add __counted_by for struct crash_mem. > > [...] Applied to for-next/hardening, thanks! [1/1] kexec: Annotate struct crash_mem with __counted_by https://git.kernel.org/kees/c/15fcedd43a08 Take care, -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] kexec: Annotate struct crash_mem with __counted_by
On Sat, Sep 23, 2023 at 08:46:47AM +0800, Baoquan He wrote:
> On 09/22/23 at 10:52am, Kees Cook wrote:
> > Prepare for the coming implementation by GCC and Clang of the __counted_by
> > attribute. Flexible array members annotated with __counted_by can have
> > their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
> > (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
> > functions).
> >
> > As found with Coccinelle[1], add __counted_by for struct crash_mem.
> >
> > [1]
> > https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
> >
> > Cc: Eric Biederman
> > Cc: kexec@lists.infradead.org
> > Signed-off-by: Kees Cook
> > ---
> > include/linux/crash_core.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> > index 3426f6eef60b..5126a4fecb44 100644
> > --- a/include/linux/crash_core.h
> > +++ b/include/linux/crash_core.h
> > @@ -131,7 +131,7 @@ static inline void __init
> > reserve_crashkernel_generic(char *cmdline,
> > struct crash_mem {
> > unsigned int max_nr_ranges;
> > unsigned int nr_ranges;
> > - struct range ranges[];
> > + struct range ranges[] __counted_by(max_nr_ranges);
>
> This __counted_by() only makes sense when there's a obvious upper
> boundary, max_nr_ranges in this case.
Yes; it's designed to be the array element count used for the
allocation. For example with the above case:
nr_ranges += 2;
cmem = vzalloc(struct_size(cmem, ranges, nr_ranges));
if (!cmem)
return NULL;
cmem->max_nr_ranges = nr_ranges;
cmem->nr_ranges = 0;
nr_ranges is the max count of the elements.
_However_, if a structure (like this one) has _two_ counters, one for
"in use" and another for "max available", __counted_by could specify the
"in use" case, as long as array indexing only happens when that "in use"
has been updated. So, if it were:
struct crash_mem {
unsigned int max_nr_ranges;
unsigned int nr_ranges;
struct range ranges[] __counted_by(nr_ranges);
};
then this would trigger the bounds checking:
cmem->ranges[0] = some_range; /* "nr_ranges" is still 0 so index 0
isn't allowed */
cmem->nr_ranges ++;
but this would not:
cmem->nr_ranges ++; /* index 0 is now available for use. */
cmem->ranges[0] = some_range;
> This heavily depends and isn't much in kernel?
Which "this" do you mean? The tracking of max allocation is common.
Tracking max and "in use" happens in some places (like here), but is
less common.
> E.g struct swap_info_struct->avail_lists[].
This is even less common: tracking the count externally from the struct,
as done there with nr_node_ids. Shakeel asked a very similar question
and also pointed out nr_node_ids:
https://lore.kernel.org/all/202309221128.6AC35E3@keescook/
> Just curious, not related to this patch though.
I'm happy to answer questions! Yeah, as I said in the above thread,
I expect to expand what __counted_by can use, and I suspect (hope)
a global would be easier to add than an arbitrary expression. :)
-Kees
--
Kees Cook
[PATCH] kexec: Annotate struct crash_mem with __counted_by
Prepare for the coming implementation by GCC and Clang of the __counted_by
attribute. Flexible array members annotated with __counted_by can have
their accesses bounds-checked at run-time checking via CONFIG_UBSAN_BOUNDS
(for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family
functions).
As found with Coccinelle[1], add __counted_by for struct crash_mem.
[1]
https://github.com/kees/kernel-tools/blob/trunk/coccinelle/examples/counted_by.cocci
Cc: Eric Biederman
Cc: kexec@lists.infradead.org
Signed-off-by: Kees Cook
---
include/linux/crash_core.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 3426f6eef60b..5126a4fecb44 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -131,7 +131,7 @@ static inline void __init reserve_crashkernel_generic(char
*cmdline,
struct crash_mem {
unsigned int max_nr_ranges;
unsigned int nr_ranges;
- struct range ranges[];
+ struct range ranges[] __counted_by(max_nr_ranges);
};
extern int crash_exclude_mem_range(struct crash_mem *mem,
--
2.34.1
Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays
On September 11, 2023 6:55:32 PM PDT, Dave Airlie wrote: >On Tue, 12 Sept 2023 at 11:27, Kees Cook wrote: >> >> On September 8, 2023 12:59:39 PM PDT, Philipp Stanner >> wrote: >> >Hi! >> > >> >David Airlie suggested that we could implement new wrappers around >> >(v)memdup_user() for duplicating user arrays. >> > >> >This small patch series first implements the two new wrapper functions >> >memdup_array_user() and vmemdup_array_user(). They calculate the >> >array-sizes safely, i.e., they return an error in case of an overflow. >> > >> >It then implements the new wrappers in two components in kernel/ and two >> >in the drm-subsystem. >> > >> >In total, there are 18 files in the kernel that use (v)memdup_user() to >> >duplicate arrays. My plan is to provide patches for the other 14 >> >successively once this series has been merged. >> > >> > >> >Changes since v1: >> >- Insert new headers alphabetically ordered >> >- Remove empty lines in functions' docstrings >> >- Return -EOVERFLOW instead of -EINVAL from wrapper functions >> > >> > >> >@Andy: >> >I test-build it for UM on my x86_64. Builds successfully. >> >A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot >> >fine. >> > >> >If there is more I can do to verify the early boot stages are fine, >> >please let me know! >> > >> >P. >> > >> >Philipp Stanner (5): >> > string.h: add array-wrappers for (v)memdup_user() >> > kernel: kexec: copy user-array safely >> > kernel: watch_queue: copy user-array safely >> > drm_lease.c: copy user-array safely >> > drm: vmgfx_surface.c: copy user-array safely >> > >> > drivers/gpu/drm/drm_lease.c | 4 +-- >> > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +-- >> > include/linux/string.h | 40 + >> > kernel/kexec.c | 2 +- >> > kernel/watch_queue.c| 2 +- >> > 5 files changed, 46 insertions(+), 6 deletions(-) >> > >> >> Nice. For the series: >> >> Reviewed-by: Kees Cook > >Hey Kees, > >what tree do you think it would best to land this through? I'm happy >to send the initial set from a drm branch, but also happy to have it >land via someone with a better process. Feel free to take it via drm. Usually string.h doesn't get a lot of changes (and even then it's normally additive) so conflicts are rare/easy. :) -Kees -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 0/5] Introduce new wrappers to copy user-arrays
On September 8, 2023 12:59:39 PM PDT, Philipp Stanner wrote: >Hi! > >David Airlie suggested that we could implement new wrappers around >(v)memdup_user() for duplicating user arrays. > >This small patch series first implements the two new wrapper functions >memdup_array_user() and vmemdup_array_user(). They calculate the >array-sizes safely, i.e., they return an error in case of an overflow. > >It then implements the new wrappers in two components in kernel/ and two >in the drm-subsystem. > >In total, there are 18 files in the kernel that use (v)memdup_user() to >duplicate arrays. My plan is to provide patches for the other 14 >successively once this series has been merged. > > >Changes since v1: >- Insert new headers alphabetically ordered >- Remove empty lines in functions' docstrings >- Return -EOVERFLOW instead of -EINVAL from wrapper functions > > >@Andy: >I test-build it for UM on my x86_64. Builds successfully. >A kernel build (localmodconfig) for my Fedora38 @ x86_64 does also boot >fine. > >If there is more I can do to verify the early boot stages are fine, >please let me know! > >P. > >Philipp Stanner (5): > string.h: add array-wrappers for (v)memdup_user() > kernel: kexec: copy user-array safely > kernel: watch_queue: copy user-array safely > drm_lease.c: copy user-array safely > drm: vmgfx_surface.c: copy user-array safely > > drivers/gpu/drm/drm_lease.c | 4 +-- > drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 4 +-- > include/linux/string.h | 40 + > kernel/kexec.c | 2 +- > kernel/watch_queue.c | 2 +- > 5 files changed, 46 insertions(+), 6 deletions(-) > Nice. For the series: Reviewed-by: Kees Cook -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE
On Fri, 12 May 2023 02:25:28 +, Fangrui Song wrote: > Tools like readelf/llvm-readelf use p_align to parse a PT_NOTE program > header as an array of 4-byte entries or 8-byte entries. Currently, there > are workarounds[1] in place for Linux to treat p_align==0 as 4. However, > it would be more appropriate to set the correct alignment so that tools > do not have to rely on guesswork. FreeBSD coredumps set p_align to 4 as > well. > > [...] Applied to for-next/execve, thanks! [1/1] coredump, vmcore: Set p_align to 4 for PT_NOTE https://git.kernel.org/kees/c/60592fb6b67c -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] coredump, vmcore: Set p_align to 4 for PT_NOTE
On Fri, May 12, 2023 at 02:25:28AM +, Fangrui Song wrote:
> Tools like readelf/llvm-readelf use p_align to parse a PT_NOTE program
> header as an array of 4-byte entries or 8-byte entries. Currently, there
> are workarounds[1] in place for Linux to treat p_align==0 as 4. However,
> it would be more appropriate to set the correct alignment so that tools
> do not have to rely on guesswork. FreeBSD coredumps set p_align to 4 as
> well.
>
> [1]:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=82ed9683ec099d8205dc499ac84febc975235af6
The interesting bit from here is:
/* NB: Some note sections may have alignment value of 0 or 1. gABI
specifies that notes should be aligned to 4 bytes in 32-bit
objects and to 8 bytes in 64-bit objects. As a Linux extension,
we also support 4 byte alignment in 64-bit objects. If section
alignment is less than 4, we treate alignment as 4 bytes. */
if (align < 4)
align = 4;
else if (align != 4 && align != 8)
{
warn (_("Corrupt note: alignment %ld, expecting 4 or 8\n"),
(long) align);
return FALSE;
}
Should Linux use 8 for 64-bit processes to avoid the other special case?
(And do we need to make some changes to make sure we are actually
aligned?)
-Kees
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] kexec: move KEXEC_SIG_FORCE from arch/x86 to arch
On Thu, Feb 10, 2022 at 02:16:48PM +0100, Michal Suchanek wrote: > There is nothing x86-specific about KEXEC_SIG_FORCE > > Signed-off-by: Michal Suchanek > --- > arch/Kconfig | 7 +++ > arch/x86/Kconfig | 7 --- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 678a80713b21..dc2446f01ac1 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -24,6 +24,13 @@ config KEXEC_ELF > config HAVE_IMA_KEXEC > bool > > +config KEXEC_SIG_FORCE > + bool "Require a valid signature in kexec_file_load() syscall" > + depends on KEXEC_SIG > + help > + This option makes kernel signature verification mandatory for > + the kexec_file_load() syscall. > + > config SET_FS > bool > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 9f5bd41bf660..3ea648dad6b6 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2032,13 +2032,6 @@ config KEXEC_SIG > verification for the corresponding kernel image type being > loaded in order for this to work. > > -config KEXEC_SIG_FORCE > - bool "Require a valid signature in kexec_file_load() syscall" > - depends on KEXEC_SIG > - help > - This option makes kernel signature verification mandatory for > - the kexec_file_load() syscall. > - This means it is no longer folded under KEXEC_SIG in menuconfig, which makes it harder to find. I would prefer seeing KEXEC_SIG (and KEXEC_SIG_FORCE) moved out of the per-arch Kconfig files into a common location, and then arch Kconfig can add something like: select ARCH_SUPPORTS_KEXEC > config KEXEC_BZIMAGE_VERIFY_SIG > bool "Enable bzImage signature verification support" > depends on KEXEC_SIG > -- > 2.31.1 > -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1 1/1] kernel.h: Split out panic and oops helpers
On Tue, Apr 06, 2021 at 04:31:58PM +0300, Andy Shevchenko wrote: > kernel.h is being used as a dump for all kinds of stuff for a long time. > Here is the attempt to start cleaning it up by splitting out panic and > oops helpers. > > At the same time convert users in header and lib folder to use new header. > Though for time being include new header back to kernel.h to avoid twisted > indirected includes for existing users. > > Signed-off-by: Andy Shevchenko I like it! Do you have a multi-arch CI to do allmodconfig builds to double-check this? Acked-by: Kees Cook -Kees -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 01/18] test_firmware: Test platform fw loading on non-EFI systems
On Wed, Jul 29, 2020 at 12:48:06AM +, Luis Chamberlain wrote:
> On Wed, Jul 22, 2020 at 12:30:03PM -0700, Kees Cook wrote:
> > On non-EFI systems, it wasn't possible to test the platform firmware
> > loader because it will have never set "checked_fw" during __init.
> > Instead, allow the test code to override this check. Additionally split
> > the declarations into a private header file so it there is greater
> > enforcement of the symbol visibility.
> >
> > Fixes: 548193cba2a7 ("test_firmware: add support for
> > firmware_request_platform")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Kees Cook
>
> A *clearly* private symbol namespace would seem cleaner, example the existing:
>
> EXPORT_SYMBOL_NS_GPL(fw_fallback_config, FIRMWARE_LOADER_PRIVATE);
I'm respinning this now. It doesn't solve in-kernel visibility, but it
does solve module visibility, I guess. It's a simpler patch, and I think
gets the point across. Will send after build testing...
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH RFC 2/2] lkdtm: Add heap spraying test
On Mon, Aug 17, 2020 at 01:24:37PM -0500, Eric W. Biederman wrote: > Alexander Popov writes: > > > Add a simple test for CONFIG_SLAB_QUARANTINE. > > > > It performs heap spraying that aims to reallocate the recently freed heap > > object. This technique is used for exploiting use-after-free > > vulnerabilities in the kernel code. > > > > This test shows that CONFIG_SLAB_QUARANTINE breaks heap spraying > > exploitation technique. > > > > Signed-off-by: Alexander Popov > > Why put this test in the linux kernel dump test module? > > I have no problem with tests, and I may be wrong but this > does not look like you are testing to see if heap corruption > triggers a crash dump. Which is what the rest of the tests > in lkdtm are about. Seeing if the test triggers successfully > triggers a crash dump. The scope of LKDTM has shifted a bit, and I'm fine with tests that don't cause crashes as long as they're part of testing system-wide defenses, etc. It's easier to collect similar tests together (even if they don't break the system). -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
On Wed, Jul 22, 2020 at 03:29:26PM -0700, Scott Branden wrote:
> These changes don't pass the kernel-selftest for partial reads I added
> (which are at the end of this patch v2 series).
Oh, interesting. Is there any feedback in dmesg? I wonder if I have the
LSMs configured differently than you?
> See change below added for temp workaround for issue.
> > [...]
> > +
> > + whole_file = (offset == 0 && i_size <= buf_size);
> A hack to get this passing I added which probably breaks some security?
> if (whole_file) {
> > + ret = security_kernel_read_file(file, id, whole_file);
> > + if (ret)
> > + goto out;
> > +
> }
This would imply I did something wrong in the LSM hook refactoring (i.e.
some LSM is rejecting the !whole_file case, but if the entire call to
the hooks are skipped, it's okay).
What does this return on your test system:
echo $(cat /sys/kernel/security/lsm)
(I wonder if I have IMA configured differently...)
Mimi, have you had a chance to test these changes?
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
On Thu, Jul 23, 2020 at 10:41:07PM -0700, Scott Branden wrote:
>
>
> On 2020-07-23 12:15 p.m., Kees Cook wrote:
> > On Wed, Jul 22, 2020 at 03:29:26PM -0700, Scott Branden wrote:
> > > These changes don't pass the kernel-selftest for partial reads I added
> > > (which are at the end of this patch v2 series).
> > Oh, interesting. Is there any feedback in dmesg? I wonder if I have the
> > LSMs configured differently than you?
> I have no LSMs configured that I know of.
> Yes, there is failure in dmesg which is how I determined to add my
> workaround.
> Without workaround, dmesg log attached after booting and running
> fw_run_tests.h
> > > See change below added for temp workaround for issue.
> > > > [...]
> > > > +
> > > > + whole_file = (offset == 0 && i_size <= buf_size);
> > > A hack to get this passing I added which probably breaks some security?
> > > if (whole_file) {
> > > > + ret = security_kernel_read_file(file, id, whole_file);
> > > > + if (ret)
> > > > + goto out;
> > > > +
> > > }
> > This would imply I did something wrong in the LSM hook refactoring (i.e.
> > some LSM is rejecting the !whole_file case, but if the entire call to
> > the hooks are skipped, it's okay).
> >
> > What does this return on your test system:
> >
> > echo $(cat /sys/kernel/security/lsm)
> ima kernel configs are enabled but I don't enable security policies
> on the kernel command line.
>
> echo $(cat /sys/kernel/security/lsm)
> cat: /sys/kernel/security/lsm: No such file or directory
Oh, er... CONFIG_SECURITYFS is missing?
Can you send me your .config?
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
On Fri, Jul 24, 2020 at 11:23:37AM -0700, Kees Cook wrote:
> On Thu, Jul 23, 2020 at 10:41:07PM -0700, Scott Branden wrote:
> >
> >
> > On 2020-07-23 12:15 p.m., Kees Cook wrote:
> > > On Wed, Jul 22, 2020 at 03:29:26PM -0700, Scott Branden wrote:
> > > > These changes don't pass the kernel-selftest for partial reads I added
> > > > (which are at the end of this patch v2 series).
> > > Oh, interesting. Is there any feedback in dmesg? I wonder if I have the
> > > LSMs configured differently than you?
> > I have no LSMs configured that I know of.
> > Yes, there is failure in dmesg which is how I determined to add my
> > workaround.
> > Without workaround, dmesg log attached after booting and running
> > fw_run_tests.h
> > > > See change below added for temp workaround for issue.
> > > > > [...]
> > > > > +
> > > > > + whole_file = (offset == 0 && i_size <= buf_size);
> > > > A hack to get this passing I added which probably breaks some security?
> > > > if (whole_file) {
> > > > > + ret = security_kernel_read_file(file, id, whole_file);
> > > > > + if (ret)
> > > > > + goto out;
> > > > > +
> > > > }
> > > This would imply I did something wrong in the LSM hook refactoring (i.e.
> > > some LSM is rejecting the !whole_file case, but if the entire call to
> > > the hooks are skipped, it's okay).
> > >
> > > What does this return on your test system:
> > >
> > > echo $(cat /sys/kernel/security/lsm)
> > ima kernel configs are enabled but I don't enable security policies
> > on the kernel command line.
> >
> > echo $(cat /sys/kernel/security/lsm)
> > cat: /sys/kernel/security/lsm: No such file or directory
>
> Oh, er... CONFIG_SECURITYFS is missing?
>
> Can you send me your .config?
Ah, nevermind, I found my config mistake. I thought I had the right
setting, but I'd missed CONFIG_IMA_APPRAISE=y. With that enabled, the
firmware tests _correctly_ fail, since IMA can't appraise partial reads.
So, this doesn't look like a bug to me.
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
On Wed, Jul 22, 2020 at 11:23:43PM -0700, Scott Branden wrote: > I made an adjustment in the logic of the driver with use of > request_partial_firmware_into_buf and now everything is working. Excellent! Was there something wrong with how I ported the request_partial_firmware_into_buf() changes? (Should the behavior be changed in some way? I didn't change the self-tests, so I thought the behavior matched your original series.) > So only issue I find with this entire patch series is the problem > of security failing without the workaround below. Yup; I'm trying to reproduce this now too... -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
On Fri, Jul 24, 2020 at 12:03:36PM -0700, Scott Branden wrote: > Now I'm confused. The original patch series I made with IMA additions under > Mimi's direction > passed the kernel selftests with partial read. And > request_partial_firmware_into_buf therefore worked. > Your changes don't work with CONFIG_IMA_APPRAISE=y on? Is there a way to > make IMA ignore this file to make things work then? > Seems like another change is needed for IMA to ignore partial reads if it > can't appraise them? Ah yes, I missed this in porting your series[1] (i.e. calling process_measurement() with a valid "file" and NULL "buf" is handled correctly -- I misunderstood these changes). I will send a corrected patch. -Kees [1] https://lore.kernel.org/lkml/20200706232309.12010-10-scott.bran...@broadcom.com/ -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 17/18] firmware: Add request_partial_firmware_into_buf()
From: Scott Branden
Add request_partial_firmware_into_buf() to allow for portions of a
firmware file to be read into a buffer. This is needed when large firmware
must be loaded in portions from a file on memory constrained systems.
Signed-off-by: Scott Branden
Co-developed-by: Kees Cook
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/firmware.h | 4 +
drivers/base/firmware_loader/main.c | 109 +++-
include/linux/firmware.h| 12 +++
3 files changed, 105 insertions(+), 20 deletions(-)
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 7ad5fe52bc72..3f6eda46b3a2 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -32,6 +32,8 @@
* @FW_OPT_FALLBACK_PLATFORM: Enable fallback to device fw copy embedded in
* the platform's main firmware. If both this fallback and the sysfs
* fallback are enabled, then this fallback will be tried first.
+ * @FW_OPT_PARTIAL: Allow partial read of firmware instead of needing to read
+ * entire file.
*/
enum fw_opt {
FW_OPT_UEVENT = BIT(0),
@@ -41,6 +43,7 @@ enum fw_opt {
FW_OPT_NOCACHE = BIT(4),
FW_OPT_NOFALLBACK_SYSFS = BIT(5),
FW_OPT_FALLBACK_PLATFORM= BIT(6),
+ FW_OPT_PARTIAL = BIT(7),
};
enum fw_status {
@@ -68,6 +71,7 @@ struct fw_priv {
void *data;
size_t size;
size_t allocated_size;
+ size_t offset;
u32 opt_flags;
#ifdef CONFIG_FW_LOADER_PAGED_BUF
bool is_paged_buf;
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index 814a18cc51bd..7aa22bdc2f60 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -170,10 +170,19 @@ static struct fw_priv *__allocate_fw_priv(const char
*fw_name,
struct firmware_cache *fwc,
void *dbuf,
size_t size,
+ size_t offset,
u32 opt_flags)
{
struct fw_priv *fw_priv;
+ /* For a partial read, the buffer must be preallocated. */
+ if ((opt_flags & FW_OPT_PARTIAL) && !dbuf)
+ return NULL;
+
+ /* Only partial reads are allowed to use an offset. */
+ if (offset != 0 && !(opt_flags & FW_OPT_PARTIAL))
+ return NULL;
+
fw_priv = kzalloc(sizeof(*fw_priv), GFP_ATOMIC);
if (!fw_priv)
return NULL;
@@ -188,6 +197,7 @@ static struct fw_priv *__allocate_fw_priv(const char
*fw_name,
fw_priv->fwc = fwc;
fw_priv->data = dbuf;
fw_priv->allocated_size = size;
+ fw_priv->offset = offset;
fw_priv->opt_flags = opt_flags;
fw_state_init(fw_priv);
#ifdef CONFIG_FW_LOADER_USER_HELPER
@@ -216,12 +226,17 @@ static int alloc_lookup_fw_priv(const char *fw_name,
struct fw_priv **fw_priv,
void *dbuf,
size_t size,
+ size_t offset,
u32 opt_flags)
{
struct fw_priv *tmp;
spin_lock(>lock);
- if (!(opt_flags & FW_OPT_NOCACHE)) {
+ /*
+* Do not merge requests that are marked to be non-cached or
+* are performing partial reads.
+*/
+ if (!(opt_flags & (FW_OPT_NOCACHE | FW_OPT_PARTIAL))) {
tmp = __lookup_fw_priv(fw_name);
if (tmp) {
kref_get(>ref);
@@ -232,7 +247,7 @@ static int alloc_lookup_fw_priv(const char *fw_name,
}
}
- tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, opt_flags);
+ tmp = __allocate_fw_priv(fw_name, fwc, dbuf, size, offset, opt_flags);
if (tmp) {
INIT_LIST_HEAD(>list);
if (!(opt_flags & FW_OPT_NOCACHE))
@@ -439,6 +454,12 @@ static int fw_decompress_xz(struct device *dev, struct
fw_priv *fw_priv,
else
return fw_decompress_xz_pages(dev, fw_priv, in_size, in_buffer);
}
+#else
+static inline int fw_decompress_xz(struct device *dev, struct fw_priv *fw_priv,
+ size_t in_size, const void *in_buffer)
+{
+ return -ENOENT;
+}
#endif /* CONFIG_FW_LOADER_COMPRESS */
/* direct firmware loading support */
@@ -485,6 +506,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
return -ENOMEM;
for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+ size_t file_size = 0;
+ size_t *file_size_ptr = NULL;
+
/* skip the unset customized path */
if (!fw_
[PATCH v2 11/18] LSM: Introduce kernel_post_load_data() hook
There are a few places in the kernel where LSMs would like to have
visibility into the contents of a kernel buffer that has been loaded or
read. While security_kernel_post_read_file() (which includes the
buffer) exists as a pairing for security_kernel_read_file(), no such
hook exists to pair with security_kernel_load_data().
Earlier proposals for just using security_kernel_post_read_file() with a
NULL file argument were rejected (i.e. "file" should always be valid for
the security_..._file hooks, but it appears at least one case was
left in the kernel during earlier refactoring. (This will be fixed in
a subsequent patch.)
Since not all cases of security_kernel_load_data() can have a single
contiguous buffer made available to the LSM hook (e.g. kexec image
segments are separately loaded), there needs to be a way for the LSM to
reason about its expectations of the hook coverage. In order to handle
this, add a "contents" argument to the "kernel_load_data" hook that
indicates if the newly added "kernel_post_load_data" hook will be called
with the full contents once loaded. That way, LSMs requiring full contents
can choose to unilaterally reject "kernel_load_data" with contents=false
(which is effectively the existing hook coverage), but when contents=true
they can allow it and later evaluate the "kernel_post_load_data" hook
once the buffer is loaded.
With this change, LSMs can gain coverage over non-file-backed data loads
(e.g. init_module(2) and firmware userspace helper), which will happen
in subsequent patches.
Additionally prepare IMA to start processing these cases.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 2 +-
.../base/firmware_loader/fallback_platform.c | 2 +-
include/linux/ima.h | 12 +--
include/linux/lsm_hook_defs.h | 4 +++-
include/linux/lsm_hooks.h | 9
include/linux/security.h | 12 +--
kernel/kexec.c| 2 +-
kernel/module.c | 2 +-
security/integrity/ima/ima_main.c | 21 ++-
security/loadpin/loadpin.c| 2 +-
security/security.c | 18 +---
security/selinux/hooks.c | 2 +-
12 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 5327bfc6ba71..a196aacce22c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, false);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 6958ab1a8059..a12c79d47efc 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, false);
if (rc)
return rc;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 148636bfcc8f..502e36ad7804 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,9 @@ extern void ima_post_create_tmpfile(struct inode *inode);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
-extern int ima_load_data(enum kernel_load_data_id id);
+extern int ima_load_data(enum kernel_load_data_id id, bool contents);
+extern int ima_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
@@ -78,7 +80,13 @@ static inline int ima_file_mprotect(struct vm_area_struct
*vma,
return 0;
}
-static inline int ima_load_data(enum kernel_load_data_id id)
+static inline int ima_load_data(enum kernel_load_data_id id, bool contents)
+{
+ return 0;
+}
+
+static inline int ima_post_load_data(char *buf, loff_t size,
+enum kernel_load_data_id id)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/in
[PATCH v2 07/18] fs/kernel_read_file: Split into separate source file
These routines are used in places outside of exec(2), so in preparation
for refactoring them, move them into a separate source file,
fs/kernel_read_file.c.
Acked-by: Scott Branden
Signed-off-by: Kees Cook
---
fs/Makefile | 3 +-
fs/exec.c | 132
fs/kernel_read_file.c | 138 ++
3 files changed, 140 insertions(+), 133 deletions(-)
create mode 100644 fs/kernel_read_file.c
diff --git a/fs/Makefile b/fs/Makefile
index 2ce5112b02c8..a05fc247b2a7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
seq_file.o xattr.o libfs.o fs-writeback.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
- fs_types.o fs_context.o fs_parser.o fsopen.o
+ fs_types.o fs_context.o fs_parser.o fsopen.o \
+ kernel_read_file.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/exec.c b/fs/exec.c
index 07a7fe9ac5be..d619b79aab30 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
-loff_t max_size, enum kernel_read_file_id id)
-{
- loff_t i_size, pos;
- ssize_t bytes = 0;
- void *allocated = NULL;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (!*buf)
- *buf = allocated = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
- if (bytes < 0) {
- ret = bytes;
- goto out_free;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (allocated) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct file *file;
- struct path root;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- task_lock(_task);
- get_fs_root(init_task.fs, );
- task_unlock(_task);
-
- file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
- path_put();
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
-enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
defined(CONFIG_BINFMT_ELF_FDPIC)
ssize_t read_code(struct file *file, unsigned long addr, loff_
[PATCH v2 03/18] firmware_loader: EFI firmware loader must handle pre-allocated buffer
The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm
ware_request_platform()")
Cc: sta...@vger.kernel.org
Acked-by: Scott Branden
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (rc)
return rc; /* rc == -ENOENT when the fw was not found */
- fw_priv->data = vmalloc(size);
+ if (fw_priv->data && size > fw_priv->allocated_size)
+ return -ENOMEM;
+ if (!fw_priv->data)
+ fw_priv->data = vmalloc(size);
if (!fw_priv->data)
return -ENOMEM;
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 06/18] fs/kernel_read_file: Split into separate include file
From: Scott Branden
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig
Signed-off-by: Scott Branden
Reviewed-by: Christoph Hellwig
Acked-by: Greg Kroah-Hartman
Link:
https://lore.kernel.org/r/20200706232309.12010-2-scott.bran...@broadcom.com
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 38 -
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h| 51 +
include/linux/security.h| 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c| 1 +
15 files changed, 64 insertions(+), 38 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index c2f57cedcd6f..d4a413ea48ce 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
diff --git a/fs/exec.c b/fs/exec.c
index 2bf549757ce7..07a7fe9ac5be 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -23,6 +23,7 @@
* formats.
*/
+#include
#include
#include
#include
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f50a35d54a61..11dd6cc7de58 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,44 +2993,6 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how* nor *where*. */
-#define __kernel_read_file_id(id) \
- id(UNKNOWN, unknown)\
- id(FIRMWARE, firmware) \
- id(MODULE, kernel-module) \
- id(KEXEC_IMAGE, kexec-image)\
- id(KEXEC_INITRAMFS, kexec-initramfs)\
- id(POLICY, security-policy) \
- id(X509_CERTIFICATE, x509-certificate) \
- id(MAX_ID, )
-
-#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
-#define __fid_stringify(dummy, str) #str,
-
-enum kernel_read_file_id {
- __kernel_read_file_id(__fid_enumify)
-};
-
-static const char * const kernel_read_file_str[] = {
- __kernel_read_file_id(__fid_stringify)
-};
-
-static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
-{
- if ((unsigned)id >= READING_MAX_ID)
- return kernel_read_file_str[READING_UNKNOWN];
-
- return kernel_read_file_str[id];
-}
-
-extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *,
loff_t,
-enum kernel_read_file_id);
-extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..148636bfcc8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -7,6 +7,7 @@
#ifndef _LINUX_IMA_H
#define _LINUX_IMA_H
+#include
#include
#include
#include
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
new file mode 100644
index ..78cf3d7dc835
--- /dev/null
+++ b/include/linux/kernel_read_file.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_READ_FILE_H
+#define _LINUX_KERNEL_READ_FILE_H
+
+#include
+#include
+
+/* This is a list of *what* is being read, not *how* nor *where*. */
+#define __kernel_read_file_id(id) \
+ id(UNKNOWN, unknown)\
+ id(FIRMWARE, firmware) \
+ id(MODULE, kernel-module) \
+ id(KEXEC_IMAGE, kexec-image)\
+ id(KEXEC_INITRAMFS, kexec-initramfs)\
+ id(POLICY, security-policy) \
+ id(X509_CERTIFICATE, x509-certificate) \
+ id(MAX_ID, )
+
+#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
+#define __fid_stringify(dummy, str) #str,
+
+e
[PATCH v2 04/18] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures
(pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware
(pre-allocated buffer)")
Cc: sta...@vger.kernel.org
Acked-by: Scott Branden
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, ,
- , msize, id);
+ , msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user
*, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const
void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
[PATCH v2 14/18] LSM: Add "contents" flag to kernel_read_file hook
As with the kernel_load_data LSM hook, add a "contents" flag to the
kernel_read_file LSM hook that indicates whether the LSM can expect
a matching call to the kernel_post_read_file LSM hook with the full
contents of the file. With the coming addition of partial file read
support for kernel_read_file*() API, the LSM will no longer be able
to always see the entire contents of a file during the read calls.
For cases where the LSM must read examine the complete file contents,
it will need to do so on its own every time the kernel_read_file
hook is called with contents=false (or reject such cases). Adjust all
existing LSMs to retain existing behavior.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c | 2 +-
include/linux/ima.h | 6 --
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 3 +++
include/linux/security.h | 6 --
security/integrity/ima/ima_main.c | 10 +-
security/loadpin/loadpin.c| 14 --
security/security.c | 7 ---
security/selinux/hooks.c | 5 +++--
9 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 2e29c38eb4df..d73bc3fa710a 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -39,7 +39,7 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id);
+ ret = security_kernel_read_file(file, id, true);
if (ret)
goto out;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 502e36ad7804..259023039dc9 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,7 +23,8 @@ extern int ima_file_mprotect(struct vm_area_struct *vma,
unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id, bool contents);
extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
+bool contents);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
@@ -91,7 +92,8 @@ static inline int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index aaa2916bbae7..c2ded57c5d9b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -188,7 +188,7 @@ LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id
id, bool contents)
LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_read_file, struct file *file,
-enum kernel_read_file_id id)
+enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 812d626195fc..b66433b5aa15 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -650,6 +650,7 @@
* @file contains the file structure pointing to the file being read
* by the kernel.
* @id kernel read file identifier
+ * @contents if a subsequent @kernel_post_read_file will be called.
* Return 0 if permission is granted.
* @kernel_post_read_file:
* Read a file specified by userspace.
@@ -658,6 +659,8 @@
* @buf pointer to buffer containing the file contents.
* @size length of the file contents.
* @id kernel read file identifier
+ * This must be paired with a prior @kernel_read_file call that had
+ * @contents set to true.
* Return 0 if permission is granted.
* @task_fix_setuid:
* Update the module's state after setting one or more of the user
diff --git a/include/linux/security.h b/include/linux/security.h
index e748974c707b..a5d66b89cd6c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -390,7 +390,8 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
int security_kernel_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
+int security_kernel_read_file(struct file *file, enum kernel_read_
[PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
To perform partial reads, callers of kernel_read_file*() must have a
non-NULL file_size argument and a preallocated buffer. The new "offset"
argument can then be used to seek to specific locations in the file to
fill the buffer to, at most, "buf_size" per call.
Where possible, the LSM hooks can report whether a full file has been
read or not so that the contents can be reasoned about.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 2 +-
fs/kernel_read_file.c | 78 -
include/linux/kernel_read_file.h| 8 +--
kernel/kexec_file.c | 4 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 3 +-
7 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index bd199404935f..d95249b5284e 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -494,7 +494,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, , msize,
+ rc = kernel_read_file_from_path_initns(path, 0, , msize,
NULL,
READING_FIRMWARE);
if (rc < 0) {
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index d73bc3fa710a..90d255fbdd9b 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -9,6 +9,7 @@
* kernel_read_file() - read file contents into a kernel buffer
*
* @file file to read from
+ * @offset where to start reading from (see below).
* @bufpointer to a "void *" buffer for reading into (if
* *@buf is NULL, a buffer will be allocated, and
* @buf_size will be ignored)
@@ -19,19 +20,31 @@
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
+ * @offset must be 0 unless both @buf and @file_size are non-NULL
+ * (i.e. the caller must be expecting to read partial file contents
+ * via an already-allocated @buf, in at most @buf_size chunks, and
+ * will be able to determine when the entire file was read by
+ * checking @file_size). This isn't a recommended way to read a
+ * file, though, since it is possible that the contents might
+ * change between calls to kernel_read_file().
+ *
* Returns number of bytes read (no single read will be bigger
* than INT_MAX), or negative on error.
*
*/
-int kernel_read_file(struct file *file, void **buf,
+int kernel_read_file(struct file *file, loff_t offset, void **buf,
size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
loff_t i_size, pos;
- ssize_t bytes = 0;
+ size_t copied;
void *allocated = NULL;
+ bool whole_file;
int ret;
+ if (offset != 0 && (!*buf || !file_size))
+ return -EINVAL;
+
if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
@@ -39,19 +52,27 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id, true);
- if (ret)
- goto out;
-
i_size = i_size_read(file_inode(file));
if (i_size <= 0) {
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || i_size > buf_size) {
+ /* The file is too big for sane activities. */
+ if (i_size > INT_MAX) {
+ ret = -EFBIG;
+ goto out;
+ }
+ /* The entire file cannot be read in one buffer. */
+ if (!file_size && offset == 0 && i_size > buf_size) {
ret = -EFBIG;
goto out;
}
+
+ whole_file = (offset == 0 && i_size <= buf_size);
+ ret = security_kernel_read_file(file, id, whole_file);
+ if (ret)
+ goto out;
+
if (file_size)
*file_size = i_size;
@@ -62,9 +83,14 @@ int kernel_read_file(struct file *file, void **buf,
goto out;
}
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
+ pos = offset;
+ copied = 0;
+ while (copied < buf_size) {
+ ssize_t bytes;
+ size_t wanted = min_t(size_t, buf_size - copied,
+ i_size - pos);
+
+ bytes = kernel_read(file, *buf + copied, wanted, );
if (bytes < 0) {
r
[PATCH v2 10/18] fs/kernel_read_file: Add file_size output argument
In preparation for adding partial read support, add an optional output
argument to kernel_read_file*() that reports the file size so callers
can reason more easily about their reading progress.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/kernel_read_file.c | 19 +--
include/linux/kernel_read_file.h| 4
kernel/kexec_file.c | 4 ++--
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
7 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index f80c0d102be8..bd199404935f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -495,6 +495,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, , msize,
+ NULL,
READING_FIRMWARE);
if (rc < 0) {
if (rc != -ENOENT)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index e21a76001fff..2e29c38eb4df 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -14,6 +14,8 @@
* @buf_size will be ignored)
* @buf_size size of buf, if already allocated. If @buf not
* allocated, this is the largest size to allocate.
+ * @file_size if non-NULL, the full size of @file will be
+ * written here.
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
@@ -22,7 +24,8 @@
*
*/
int kernel_read_file(struct file *file, void **buf,
-size_t buf_size, enum kernel_read_file_id id)
+size_t buf_size, size_t *file_size,
+enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
@@ -49,6 +52,8 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EFBIG;
goto out;
}
+ if (file_size)
+ *file_size = i_size;
if (!*buf)
*buf = allocated = vmalloc(i_size);
@@ -91,7 +96,8 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- size_t buf_size, enum kernel_read_file_id id)
+ size_t buf_size, size_t *file_size,
+ enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -103,14 +109,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- size_t buf_size,
+ size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -129,13 +135,14 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -144,7 +151,7 @@ int kernel_read_file_from_fd(int fd, void **buf, size_t
buf_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, buf_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 910039e7593e..023293eaf948 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -37,15 +37,19 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
int kernel_read_file(struct file *file,
void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
void **buf, siz
[PATCH v2 09/18] fs/kernel_read_file: Switch buffer size arg to size_t
In preparation for further refactoring of kernel_read_file*(), rename
the "max_size" argument to the more accurate "buf_size", and correct
its type to size_t. Add kerndoc to explain the specifics of how the
arguments will be used. Note that with buf_size now size_t, it can no
longer be negative (and was never called with a negative value). Adjust
callers to use it as a "maximum size" when *buf is NULL.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c| 34 +++-
include/linux/kernel_read_file.h | 8
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index dc28a8def597..e21a76001fff 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,15 +5,31 @@
#include
#include
+/**
+ * kernel_read_file() - read file contents into a kernel buffer
+ *
+ * @file file to read from
+ * @bufpointer to a "void *" buffer for reading into (if
+ * *@buf is NULL, a buffer will be allocated, and
+ * @buf_size will be ignored)
+ * @buf_size size of buf, if already allocated. If @buf not
+ * allocated, this is the largest size to allocate.
+ * @id the kernel_read_file_id identifying the type of
+ * file contents being read (for LSMs to examine)
+ *
+ * Returns number of bytes read (no single read will be bigger
+ * than INT_MAX), or negative on error.
+ *
+ */
int kernel_read_file(struct file *file, void **buf,
-loff_t max_size, enum kernel_read_file_id id)
+size_t buf_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
void *allocated = NULL;
int ret;
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
ret = deny_write_access(file);
@@ -29,7 +45,7 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || i_size > buf_size) {
ret = -EFBIG;
goto out;
}
@@ -75,7 +91,7 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- loff_t max_size, enum kernel_read_file_id id)
+ size_t buf_size, enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -87,14 +103,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t max_size,
+ size_t buf_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -113,13 +129,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
+int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -128,7 +144,7 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t
max_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, max_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 0ca0bdbed1bd..910039e7593e 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -36,16 +36,16 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
}
int kernel_read_file(struct file *file,
-void **buf, loff_t max_size,
+void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
- void **buf, loff_t max_size,
+ void **buf, size_t buf
[PATCH v2 16/18] firmware: Store opt_flags in fw_priv
Instead of passing opt_flags around so much, store it in the private
structure so it can be examined by internals without needing to add more
arguments to functions.
Co-developed-by: Scott Branden
Signed-off-by: Scott Branden
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 11 +++-
drivers/base/firmware_loader/fallback.h | 5 ++--
.../base/firmware_loader/fallback_platform.c | 4 +--
drivers/base/firmware_loader/firmware.h | 3 ++-
drivers/base/firmware_loader/main.c | 25 +++
5 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 7cfdfdcb819c..0a94c8739959 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -490,13 +490,11 @@ fw_create_instance(struct firmware *firmware, const char
*fw_name,
/**
* fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
* @fw_sysfs: firmware sysfs information for the firmware to load
- * @opt_flags: flags of options, FW_OPT_*
* @timeout: timeout to wait for the load
*
* In charge of constructing a sysfs fallback interface for firmware loading.
**/
-static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
- u32 opt_flags, long timeout)
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
{
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -518,7 +516,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
list_add(_priv->pending_list, _fw_head);
mutex_unlock(_lock);
- if (opt_flags & FW_OPT_UEVENT) {
+ if (fw_priv->opt_flags & FW_OPT_UEVENT) {
fw_priv->need_uevent = true;
dev_set_uevent_suppress(f_dev, false);
dev_dbg(f_dev, "firmware: requesting %s\n", fw_priv->fw_name);
@@ -580,10 +578,10 @@ static int fw_load_from_user_helper(struct firmware
*firmware,
}
fw_sysfs->fw_priv = firmware->priv;
- ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
+ ret = fw_load_sysfs_fallback(fw_sysfs, timeout);
if (!ret)
- ret = assign_fw(firmware, device, opt_flags);
+ ret = assign_fw(firmware, device);
out_unlock:
usermodehelper_read_unlock();
@@ -625,7 +623,6 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
* @fw: pointer to firmware image
* @name: name of firmware file to look for
* @device: device for which firmware is being loaded
- * @opt_flags: options to control firmware loading behaviour
* @ret: return value from direct lookup which triggered the fallback mechanism
*
* This function is called if direct lookup for the firmware failed, it enables
diff --git a/drivers/base/firmware_loader/fallback.h
b/drivers/base/firmware_loader/fallback.h
index 2afdb6adb23f..3af7205b302f 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -67,10 +67,9 @@ static inline void unregister_sysfs_loader(void)
#endif /* CONFIG_FW_LOADER_USER_HELPER */
#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
-int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags);
+int firmware_fallback_platform(struct fw_priv *fw_priv);
#else
-static inline int firmware_fallback_platform(struct fw_priv *fw_priv,
-u32 opt_flags)
+static inline int firmware_fallback_platform(struct fw_priv *fw_priv)
{
return -ENOENT;
}
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 4d1157af0e86..38de68d7e973 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -8,13 +8,13 @@
#include "fallback.h"
#include "firmware.h"
-int firmware_fallback_platform(struct fw_priv *fw_priv, u32 opt_flags)
+int firmware_fallback_platform(struct fw_priv *fw_priv)
{
const u8 *data;
size_t size;
int rc;
- if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
+ if (!(fw_priv->opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
rc = security_kernel_load_data(LOADING_FIRMWARE, true);
diff --git a/drivers/base/firmware_loader/firmware.h
b/drivers/base/firmware_loader/firmware.h
index 933e2192fbe8..7ad5fe52bc72 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -68,6 +68,7 @@ struct fw_priv {
void *data;
size_t size;
size_t allocated_size;
+ u32 opt_flags;
#ifdef CONFIG_FW_LOADER_PAGED_BUF
bool is_paged_buf;
struct page **pages;
@@ -136,7 +137,7 @@ static inline void fw_state_done(struct fw_priv *fw_priv)
__fw_state_set(fw_priv, FW_STATUS_DONE);
}
-int assi
[PATCH v2 12/18] firmware_loader: Use security_post_load_data()
Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 8
.../base/firmware_loader/fallback_platform.c | 7 ++-
security/integrity/ima/ima_main.c | 20 +--
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index a196aacce22c..7cfdfdcb819c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -272,9 +272,9 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: map pages failed\n",
__func__);
else
- rc = security_kernel_post_read_file(NULL,
- fw_priv->data, fw_priv->size,
- READING_FIRMWARE);
+ rc =
security_kernel_post_load_data(fw_priv->data,
+ fw_priv->size,
+ LOADING_FIRMWARE);
/*
* Same logic as fw_load_abort, only the DONE bit
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE, false);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, true);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index a12c79d47efc..4d1157af0e86 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE, false);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, true);
if (rc)
return rc;
@@ -27,6 +27,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (fw_priv->data && size > fw_priv->allocated_size)
return -ENOMEM;
+
+ rc = security_kernel_post_load_data((u8 *)data, size, LOADING_FIRMWARE);
+ if (rc)
+ return rc;
+
if (!fw_priv->data)
fw_priv->data = vmalloc(size);
if (!fw_priv->data)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 85000dc8595c..1a7bc4c7437d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -648,15 +648,6 @@ int ima_post_read_file(struct file *file, void *buf,
loff_t size,
enum ima_hooks func;
u32 secid;
- if (!file && read_id == READING_FIRMWARE) {
- if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("Prevent firmware loading_store.\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0;
- }
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
}
break;
case LOADING_FIRMWARE:
- if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
+ if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) &&
!contents) {
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
*/
int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id
load_id)
{
+ if (load_id == LOADING_FIRMWARE) {
+ if ((ima_appraise & IMA_APPRA
[PATCH v2 18/18] test_firmware: Test partial read support
From: Scott Branden
Add additional hooks to test_firmware to pass in support
for partial file read using request_firmware_into_buf():
buf_size: size of buffer to request firmware into
partial: indicates that a partial file request is being made
file_offset: to indicate offset into file to request
Also update firmware selftests to use the new partial read test API.
Signed-off-by: Scott Branden
Co-developed-by: Kees Cook
Signed-off-by: Kees Cook
---
This merges Scott's two test patches into one and I refactored the
selftests to not be batched, test the no-file condition, and to not
pass needless arguments, etc.
---
lib/test_firmware.c | 154 --
.../selftests/firmware/fw_filesystem.sh | 91 +++
2 files changed, 233 insertions(+), 12 deletions(-)
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 62af792e151c..387acb94eeea 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -50,6 +50,9 @@ struct test_batched_req {
* @name: the name of the firmware file to look for
* @into_buf: when the into_buf is used if this is true
* request_firmware_into_buf() will be used instead.
+ * @buf_size: size of buf to allocate when into_buf is true
+ * @file_offset: file offset to request when calling request_firmware_into_buf
+ * @partial: partial read opt when calling request_firmware_into_buf
* @sync_direct: when the sync trigger is used if this is true
* request_firmware_direct() will be used instead.
* @send_uevent: whether or not to send a uevent for async requests
@@ -89,6 +92,9 @@ struct test_batched_req {
struct test_config {
char *name;
bool into_buf;
+ size_t buf_size;
+ size_t file_offset;
+ bool partial;
bool sync_direct;
bool send_uevent;
u8 num_requests;
@@ -183,6 +189,9 @@ static int __test_firmware_config_init(void)
test_fw_config->num_requests = TEST_FIRMWARE_NUM_REQS;
test_fw_config->send_uevent = true;
test_fw_config->into_buf = false;
+ test_fw_config->buf_size = TEST_FIRMWARE_BUF_SIZE;
+ test_fw_config->file_offset = 0;
+ test_fw_config->partial = false;
test_fw_config->sync_direct = false;
test_fw_config->req_firmware = request_firmware;
test_fw_config->test_result = 0;
@@ -236,28 +245,35 @@ static ssize_t config_show(struct device *dev,
dev_name(dev));
if (test_fw_config->name)
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"name:\t%s\n",
test_fw_config->name);
else
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"name:\tEMTPY\n");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"num_requests:\t%u\n", test_fw_config->num_requests);
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"send_uevent:\t\t%s\n",
test_fw_config->send_uevent ?
"FW_ACTION_HOTPLUG" :
"FW_ACTION_NOHOTPLUG");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"into_buf:\t\t%s\n",
test_fw_config->into_buf ? "true" : "false");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "buf_size:\t%zu\n", test_fw_config->buf_size);
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "file_offset:\t%zu\n", test_fw_config->file_offset);
+ len += scnprintf(buf + len, PAGE_SIZE - len,
+ "partial:\t\t%s\n",
+ test_fw_config->partial ? "true" : "false");
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"sync_direct:\t\t%s\n",
test_fw_config->sync_direct ? "true" : "false");
- len += scnprintf(buf+len, PAGE_SIZE - len,
+ len += scnprintf(buf + len, PAGE_SIZE - len,
"read_fw_idx:\t%u\n", test_fw_config->read_fw_idx);
mutex_unlock(_fw_mutex);
@@ -315,6 +331,30 @@ static ssize_t test_dev_config_show_bool(char *buf, bool
val)
return snprintf(buf, PAGE_SIZE, "%d\n", val);
}
+static int test_dev_config_update_size_t(const char *buf,
+size_t size,
+
[PATCH v2 13/18] module: Call security_kernel_post_load_data()
Now that there is an API for checking loaded contents for modules
loaded without a file, call into the LSM hooks.
Signed-off-by: Kees Cook
---
kernel/module.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index d56cb34d9a2f..90a4788dff9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2967,7 +2967,7 @@ static int copy_module_from_user(const void __user *umod,
unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_load_data(LOADING_MODULE, false);
+ err = security_kernel_load_data(LOADING_MODULE, true);
if (err)
return err;
@@ -2977,11 +2977,17 @@ static int copy_module_from_user(const void __user
*umod, unsigned long len,
return -ENOMEM;
if (copy_chunked_from_user(info->hdr, umod, info->len) != 0) {
- vfree(info->hdr);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
- return 0;
+ err = security_kernel_post_load_data((char *)info->hdr, info->len,
+LOADING_MODULE);
+out:
+ if (err)
+ vfree(info->hdr);
+
+ return err;
}
static void free_copy(struct load_info *info)
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 02/18] selftest/firmware: Add selftest timeout in settings
The firmware tests would always time out for me. Add a correct timeout, including details on how the value was reached. Additionally allow the test harness to skip comments in settings files and report how long a given timeout was. Signed-off-by: Kees Cook --- tools/testing/selftests/firmware/settings | 8 tools/testing/selftests/kselftest/runner.sh | 6 +- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/firmware/settings diff --git a/tools/testing/selftests/firmware/settings b/tools/testing/selftests/firmware/settings new file mode 100644 index ..085e664ee093 --- /dev/null +++ b/tools/testing/selftests/firmware/settings @@ -0,0 +1,8 @@ +# The async firmware timeout is set to 1 second (but ends up being effectively +# 2 seconds). There are 3 test configs, each done with and without firmware +# present, each with 2 "nowait" functions tested 5 times. Expected time for a +# normal execution should be 2 * 3 * 2 * 2 * 5 = 120 seconds for those alone. +# Additionally, fw_fallback may take 5 seconds for internal timeouts in each +# of the 3 configs, so at least another 15 seconds are needed. Add another +# 10 seconds for each testing config: 120 + 15 + 30 +timeout=165 diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh index 676b3a8b114d..cd5ddf979f15 100644 --- a/tools/testing/selftests/kselftest/runner.sh +++ b/tools/testing/selftests/kselftest/runner.sh @@ -53,6 +53,10 @@ run_one() settings="$BASE_DIR/$DIR/settings" if [ -r "$settings" ] ; then while read line ; do + # Skip comments. + if echo "$line" | grep -q '^#'; then + continue + fi field=$(echo "$line" | cut -d= -f1) value=$(echo "$line" | cut -d= -f2-) eval "kselftest_$field"="$value" @@ -80,7 +84,7 @@ run_one() echo "not ok $test_num $TEST_HDR_MSG # SKIP" elif [ $rc -eq $timeout_rc ]; then \ echo "#" - echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT" + echo "not ok $test_num $TEST_HDR_MSG # TIMEOUT $kselftest_timeout seconds" else echo "not ok $test_num $TEST_HDR_MSG # exit=$rc" fi) -- 2.25.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 00/18] Introduce partial kernel_read_file() support
v2: - fix issues in firmware test suite - add firmware partial read patches - various bug fixes/cleanups v1: https://lore.kernel.org/lkml/20200717174309.1164575-1-keesc...@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 now got Scott's firmware patches ported and everything tests clean for me. I think the intention is for this to go via Greg's tree since Scott's driver code will depend on it? Thanks, and let me know what you think, -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 (3): fs/kernel_read_file: Split into separate include file 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 | 58 -- 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, 849 insertions(+), 317 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 -- 2.25.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 01/18] test_firmware: Test platform fw loading on non-EFI systems
On non-EFI systems, it wasn't possible to test the platform firmware
loader because it will have never set "checked_fw" during __init.
Instead, allow the test code to override this check. Additionally split
the declarations into a private header file so it there is greater
enforcement of the symbol visibility.
Fixes: 548193cba2a7 ("test_firmware: add support for firmware_request_platform")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
drivers/firmware/efi/embedded-firmware.c | 21 -
drivers/firmware/efi/embedded-firmware.h | 19 +++
include/linux/efi_embedded_fw.h | 13 -
lib/test_firmware.c | 5 +
4 files changed, 40 insertions(+), 18 deletions(-)
create mode 100644 drivers/firmware/efi/embedded-firmware.h
diff --git a/drivers/firmware/efi/embedded-firmware.c
b/drivers/firmware/efi/embedded-firmware.c
index a1b199de9006..0fb03cd0a5a2 100644
--- a/drivers/firmware/efi/embedded-firmware.c
+++ b/drivers/firmware/efi/embedded-firmware.c
@@ -14,11 +14,22 @@
#include
#include
+#include "embedded-firmware.h"
+
+#ifdef CONFIG_TEST_FIRMWARE
+# define EFI_EMBEDDED_FW_VISIBILITY
+#else
+# define EFI_EMBEDDED_FW_VISIBILITY static
+#endif
+
+EFI_EMBEDDED_FW_VISIBILITY LIST_HEAD(efi_embedded_fw_list);
+EFI_EMBEDDED_FW_VISIBILITY bool efi_embedded_fw_checked;
+
/* Exported for use by lib/test_firmware.c only */
-LIST_HEAD(efi_embedded_fw_list);
+#ifdef CONFIG_TEST_FIRMWARE
EXPORT_SYMBOL_GPL(efi_embedded_fw_list);
-
-static bool checked_for_fw;
+EXPORT_SYMBOL_GPL(efi_embedded_fw_checked);
+#endif
static const struct dmi_system_id * const embedded_fw_table[] = {
#ifdef CONFIG_TOUCHSCREEN_DMI
@@ -119,14 +130,14 @@ void __init efi_check_for_embedded_firmwares(void)
}
}
- checked_for_fw = true;
+ efi_embedded_fw_checked = true;
}
int efi_get_embedded_fw(const char *name, const u8 **data, size_t *size)
{
struct efi_embedded_fw *iter, *fw = NULL;
- if (!checked_for_fw) {
+ if (!efi_embedded_fw_checked) {
pr_warn("Warning %s called while we did not check for embedded
fw\n",
__func__);
return -ENOENT;
diff --git a/drivers/firmware/efi/embedded-firmware.h
b/drivers/firmware/efi/embedded-firmware.h
new file mode 100644
index ..34113316d068
--- /dev/null
+++ b/drivers/firmware/efi/embedded-firmware.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _EFI_EMBEDDED_FW_INTERNAL_H_
+#define _EFI_EMBEDDED_FW_INTERNAL_H_
+
+/*
+ * This struct and efi_embedded_fw_list are private to the efi-embedded fw
+ * implementation they only in separate header for use by lib/test_firmware.c.
+ */
+struct efi_embedded_fw {
+ struct list_head list;
+ const char *name;
+ const u8 *data;
+ size_t length;
+};
+
+extern struct list_head efi_embedded_fw_list;
+extern bool efi_embedded_fw_checked;
+
+#endif /* _EFI_EMBEDDED_FW_INTERNAL_H_ */
diff --git a/include/linux/efi_embedded_fw.h b/include/linux/efi_embedded_fw.h
index 57eac5241303..4ad5db9f5312 100644
--- a/include/linux/efi_embedded_fw.h
+++ b/include/linux/efi_embedded_fw.h
@@ -7,19 +7,6 @@
#define EFI_EMBEDDED_FW_PREFIX_LEN 8
-/*
- * This struct and efi_embedded_fw_list are private to the efi-embedded fw
- * implementation they are in this header for use by lib/test_firmware.c only!
- */
-struct efi_embedded_fw {
- struct list_head list;
- const char *name;
- const u8 *data;
- size_t length;
-};
-
-extern struct list_head efi_embedded_fw_list;
-
/**
* struct efi_embedded_fw_desc - This struct is used by the EFI embedded-fw
* code to search for embedded firmwares.
diff --git a/lib/test_firmware.c b/lib/test_firmware.c
index 9fee2b93a8d1..62af792e151c 100644
--- a/lib/test_firmware.c
+++ b/lib/test_firmware.c
@@ -489,6 +489,7 @@ static ssize_t trigger_request_store(struct device *dev,
static DEVICE_ATTR_WO(trigger_request);
#ifdef CONFIG_EFI_EMBEDDED_FIRMWARE
+#include "../drivers/firmware/efi/embedded-firmware.h"
static ssize_t trigger_request_platform_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t count)
@@ -501,6 +502,7 @@ static ssize_t trigger_request_platform_store(struct device
*dev,
};
struct efi_embedded_fw efi_embedded_fw;
const struct firmware *firmware = NULL;
+ bool saved_efi_embedded_fw_checked;
char *name;
int rc;
@@ -513,6 +515,8 @@ static ssize_t trigger_request_platform_store(struct device
*dev,
efi_embedded_fw.data = (void *)test_data;
efi_embedded_fw.length = sizeof(test_data);
list_add(_embedded_fw.list, _embedded_fw_list);
+ saved_efi_embedded_fw_checked = efi_em
[PATCH v2 05/18] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and
firmware_request_platform()")
Cc: sta...@vger.kernel.org
Acked-by: Scott Branden
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2 08/18] fs/kernel_read_file: Remove redundant size argument
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 10 ++
fs/kernel_read_file.c | 20 +---
include/linux/kernel_read_file.h| 8
kernel/kexec_file.c | 14 +++---
kernel/module.c | 7 +++
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 6 --
7 files changed, 36 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..f80c0d102be8 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, ,
- , msize,
+ rc = kernel_read_file_from_path_initns(path, , msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
path, rc);
@@ -506,6 +505,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
+ rc = 0;
+
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include
#include
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path, void
**buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-i
Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
On Tue, Jul 21, 2020 at 02:43:07PM -0700, Scott Branden wrote:
> On 2020-07-17 10:43 a.m., Kees Cook wrote:
> > In preparation for refactoring kernel_read_file*(), remove the redundant
> > "size" argument which is not needed: it can be included in the return
> > code, with callers adjusted. (VFS reads already cannot be larger than
> > INT_MAX.)
> >
> > Signed-off-by: Kees Cook
> > ---
> > drivers/base/firmware_loader/main.c | 8
> > fs/kernel_read_file.c | 20 +---
> > include/linux/kernel_read_file.h| 8
> > kernel/kexec_file.c | 13 ++---
> > kernel/module.c | 7 +++
> > security/integrity/digsig.c | 5 +++--
> > security/integrity/ima/ima_fs.c | 5 +++--
> > 7 files changed, 32 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader/main.c
> > b/drivers/base/firmware_loader/main.c
> > index d4a413ea48ce..ea419c7d3d34 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device,
> > struct fw_priv *fw_priv,
> > size_t in_size,
> > const void *in_buffer))
> > {
> > - loff_t size;
> > + size_t size;
> > int i, len;
> > int rc = -ENOENT;
> > char *path;
> > @@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device,
> > struct fw_priv *fw_priv,
> > fw_priv->size = 0;
> > /* load firmware files from the mount namespace of init */
> > - rc = kernel_read_file_from_path_initns(path, ,
> > - , msize,
> > + rc = kernel_read_file_from_path_initns(path, , msize,
> >READING_FIRMWARE);
> > - if (rc) {
> > + if (rc < 0) {
> > if (rc != -ENOENT)
> > dev_warn(device, "loading %s failed with error
> > %d\n",
> > path, rc);
> > @@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device,
> > struct fw_priv *fw_priv,
> > path);
> > continue;
> > }
> > + size = rc;
> Change fails to return 0. Need rc = 0; here.
Oh nice; good catch! I'll fix this.
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 00/13] Introduce partial kernel_read_file() support
On Fri, Jul 17, 2020 at 12:17:02PM -0700, Scott Branden wrote: > Thanks for sending out. This looks different than your other patch series. Yes, it mutated in my head as I considered how all of this should hang together, which is why I wanted to get it sent before the weekend. I'm still trying to figure out why the fireware testsuite fails for me, etc. > We should get the first 5 patches accepted now though as they are > simple cleanups and fixes. That will reduce the number of outstanding > patches in the series. Agreed. I'd like to get some more eyes on it, but I can get it ready for -next. > At first glance the issue with the changes after that is the existing > API assumes it has read the whole file and failed if it did not. > Now, if the file is larger than the amount requested there is no indication? The intention is to have old API users unchanged and new users can use a pre-allocated buf (with buf_size) along with file_size to examine their partial read progress. If I broke the old API, that's a bug and I need to fix it, but that's why I wanted to start with the firmware test suite (basic things like module loading work fine after this series, but I wanted to really exercise the corners that the firmware suite pokes at). -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
On Fri, Jul 17, 2020 at 12:04:18PM -0700, Scott Branden wrote:
> On 2020-07-17 10:43 a.m., Kees Cook wrote:
> > In preparation for refactoring kernel_read_file*(), remove the redundant
> > "size" argument which is not needed: it can be included in the return
>
> I don't think the size argument is redundant though.
> The existing kernel_read_file functions always read the whole file.
> Now, what happens if the file is bigger than the buffer.
> How does kernel_read_file know it read the whole file by looking at the
> return value?
Yes; an entirely reasonable concern. This is why I add the file_size
output argument later in the series.
> > code, with callers adjusted. (VFS reads already cannot be larger than
> > INT_MAX.)
> > [...]
> > - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> > + if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
>
> Should this be SSIZE_MAX?
No, for two reasons: then we need to change the return value and likely
the callers need more careful checks, and more importantly, because the
VFS already limits single read actions to INT_MAX, so limits above this
make no sense. Win win! :)
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 03/13] fs/kernel_read_file: Remove FIRMWARE_EFI_EMBEDDED enum
The "FIRMWARE_EFI_EMBEDDED" enum is a "where", not a "what". It
should not be distinguished separately from just "FIRMWARE", as this
confuses the LSMs about what is being loaded. Additionally, there was
no actual validation of the firmware contents happening.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and
firmware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 2 +-
include/linux/fs.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 685edb7dd05a..6958ab1a8059 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE_EFI_EMBEDDED);
+ rc = security_kernel_load_data(LOADING_FIRMWARE);
if (rc)
return rc;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 95fc775ed937..f50a35d54a61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,11 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how*. */
+/* This is a list of *what* is being read, not *how* nor *where*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
id(KEXEC_INITRAMFS, kexec-initramfs)\
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 08/13] fs/kernel_read_file: Add file_size output argument
In preparation for adding partial read support, add an optional output
argument to kernel_read_file*() that reports the file size so callers
can reason more easily about their reading progress.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/kernel_read_file.c | 19 +--
include/linux/kernel_read_file.h| 4
kernel/kexec_file.c | 4 ++--
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
7 files changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ea419c7d3d34..3439a533927c 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -495,6 +495,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, , msize,
+ NULL,
READING_FIRMWARE);
if (rc < 0) {
if (rc != -ENOENT)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index e21a76001fff..2e29c38eb4df 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -14,6 +14,8 @@
* @buf_size will be ignored)
* @buf_size size of buf, if already allocated. If @buf not
* allocated, this is the largest size to allocate.
+ * @file_size if non-NULL, the full size of @file will be
+ * written here.
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
@@ -22,7 +24,8 @@
*
*/
int kernel_read_file(struct file *file, void **buf,
-size_t buf_size, enum kernel_read_file_id id)
+size_t buf_size, size_t *file_size,
+enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
@@ -49,6 +52,8 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EFBIG;
goto out;
}
+ if (file_size)
+ *file_size = i_size;
if (!*buf)
*buf = allocated = vmalloc(i_size);
@@ -91,7 +96,8 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- size_t buf_size, enum kernel_read_file_id id)
+ size_t buf_size, size_t *file_size,
+ enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -103,14 +109,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- size_t buf_size,
+ size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -129,13 +135,14 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, buf_size, id);
+ ret = kernel_read_file(file, buf, buf_size, file_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -144,7 +151,7 @@ int kernel_read_file_from_fd(int fd, void **buf, size_t
buf_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, buf_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 910039e7593e..023293eaf948 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -37,15 +37,19 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
int kernel_read_file(struct file *file,
void **buf, size_t buf_size,
+size_t *file_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
void **buf, siz
[PATCH 13/13] fs/kernel_file_read: Add "offset" arg for partial reads
To perform partial reads, callers of kernel_read_file*() must have a
non-NULL file_size argument and a preallocated buffer. The new "offset"
argument can then be used to seek to specific locations in the file to
fill the buffer to, at most, "buf_size" per call.
Where possible, the LSM hooks can report whether a full file has been
read or not so that the contents can be reasoned about.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 2 +-
fs/kernel_read_file.c | 78 -
include/linux/kernel_read_file.h| 8 +--
kernel/kexec_file.c | 4 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 3 +-
7 files changed, 65 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index 3439a533927c..fa540ca51961 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -494,7 +494,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, , msize,
+ rc = kernel_read_file_from_path_initns(path, 0, , msize,
NULL,
READING_FIRMWARE);
if (rc < 0) {
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index d73bc3fa710a..90d255fbdd9b 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -9,6 +9,7 @@
* kernel_read_file() - read file contents into a kernel buffer
*
* @file file to read from
+ * @offset where to start reading from (see below).
* @bufpointer to a "void *" buffer for reading into (if
* *@buf is NULL, a buffer will be allocated, and
* @buf_size will be ignored)
@@ -19,19 +20,31 @@
* @id the kernel_read_file_id identifying the type of
* file contents being read (for LSMs to examine)
*
+ * @offset must be 0 unless both @buf and @file_size are non-NULL
+ * (i.e. the caller must be expecting to read partial file contents
+ * via an already-allocated @buf, in at most @buf_size chunks, and
+ * will be able to determine when the entire file was read by
+ * checking @file_size). This isn't a recommended way to read a
+ * file, though, since it is possible that the contents might
+ * change between calls to kernel_read_file().
+ *
* Returns number of bytes read (no single read will be bigger
* than INT_MAX), or negative on error.
*
*/
-int kernel_read_file(struct file *file, void **buf,
+int kernel_read_file(struct file *file, loff_t offset, void **buf,
size_t buf_size, size_t *file_size,
enum kernel_read_file_id id)
{
loff_t i_size, pos;
- ssize_t bytes = 0;
+ size_t copied;
void *allocated = NULL;
+ bool whole_file;
int ret;
+ if (offset != 0 && (!*buf || !file_size))
+ return -EINVAL;
+
if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
@@ -39,19 +52,27 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id, true);
- if (ret)
- goto out;
-
i_size = i_size_read(file_inode(file));
if (i_size <= 0) {
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || i_size > buf_size) {
+ /* The file is too big for sane activities. */
+ if (i_size > INT_MAX) {
+ ret = -EFBIG;
+ goto out;
+ }
+ /* The entire file cannot be read in one buffer. */
+ if (!file_size && offset == 0 && i_size > buf_size) {
ret = -EFBIG;
goto out;
}
+
+ whole_file = (offset == 0 && i_size <= buf_size);
+ ret = security_kernel_read_file(file, id, whole_file);
+ if (ret)
+ goto out;
+
if (file_size)
*file_size = i_size;
@@ -62,9 +83,14 @@ int kernel_read_file(struct file *file, void **buf,
goto out;
}
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
+ pos = offset;
+ copied = 0;
+ while (copied < buf_size) {
+ ssize_t bytes;
+ size_t wanted = min_t(size_t, buf_size - copied,
+ i_size - pos);
+
+ bytes = kernel_read(file, *buf + copied, wanted, );
if (bytes < 0) {
r
[PATCH 09/13] LSM: Introduce kernel_post_load_data() hook
There are a few places in the kernel where LSMs would like to have
visibility into the contents of a kernel buffer that has been loaded or
read. While security_kernel_post_read_file() (which includes the
buffer) exists as a pairing for security_kernel_read_file(), no such
hook exists to pair with security_kernel_load_data().
Earlier proposals for just using security_kernel_post_read_file() with a
NULL file argument were rejected (i.e. "file" should always be valid for
the security_..._file hooks, but it appears at least one case was
left in the kernel during earlier refactoring. (This will be fixed in
a subsequent patch.)
Since not all cases of security_kernel_load_data() can have a single
contiguous buffer made available to the LSM hook (e.g. kexec image
segments are separately loaded), there needs to be a way for the LSM to
reason about its expectations of the hook coverage. In order to handle
this, add a "contents" argument to the "kernel_load_data" hook that
indicates if the newly added "kernel_post_load_data" hook will be called
with the full contents once loaded. That way, LSMs requiring full contents
can choose to unilaterally reject "kernel_load_data" with contents=false
(which is effectively the existing hook coverage), but when contents=true
they can allow it and later evaluate the "kernel_post_load_data" hook
once the buffer is loaded.
With this change, LSMs can gain coverage over non-file-backed data loads
(e.g. init_module(2) and firmware userspace helper), which will happen
in subsequent patches.
Additionally prepare IMA to start processing these cases.
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 2 +-
.../base/firmware_loader/fallback_platform.c | 2 +-
include/linux/ima.h | 12 +--
include/linux/lsm_hook_defs.h | 4 +++-
include/linux/lsm_hooks.h | 9
include/linux/security.h | 12 +--
kernel/kexec.c| 2 +-
kernel/module.c | 2 +-
security/integrity/ima/ima_main.c | 21 ++-
security/loadpin/loadpin.c| 2 +-
security/security.c | 18 +---
security/selinux/hooks.c | 2 +-
12 files changed, 73 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index 5327bfc6ba71..a196aacce22c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, false);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index 6958ab1a8059..a12c79d47efc 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, false);
if (rc)
return rc;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 148636bfcc8f..502e36ad7804 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -20,7 +20,9 @@ extern void ima_post_create_tmpfile(struct inode *inode);
extern void ima_file_free(struct file *file);
extern int ima_file_mmap(struct file *file, unsigned long prot);
extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
-extern int ima_load_data(enum kernel_load_data_id id);
+extern int ima_load_data(enum kernel_load_data_id id, bool contents);
+extern int ima_post_load_data(char *buf, loff_t size,
+ enum kernel_load_data_id id);
extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
@@ -78,7 +80,13 @@ static inline int ima_file_mprotect(struct vm_area_struct
*vma,
return 0;
}
-static inline int ima_load_data(enum kernel_load_data_id id)
+static inline int ima_load_data(enum kernel_load_data_id id, bool contents)
+{
+ return 0;
+}
+
+static inline int ima_post_load_data(char *buf, loff_t size,
+enum kernel_load_data_id id)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/in
[PATCH 12/13] LSM: Add "contents" flag to kernel_read_file hook
As with the kernel_load_data LSM hook, add a "contents" flag to the
kernel_read_file LSM hook that indicates whether the LSM can expect
a matching call to the kernel_post_read_file LSM hook with the full
contents of the file. With the coming addition of partial file read
support for kernel_read_file*() API, the LSM will no longer be able
to always see the entire contents of a file during the read calls.
For cases where the LSM must read examine the complete file contents,
it will need to do so on its own every time the kernel_read_file
hook is called with contents=false (or reject such cases). Adjust all
existing LSMs to retain existing behavior.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c | 2 +-
include/linux/ima.h | 6 --
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 3 +++
include/linux/security.h | 6 --
security/integrity/ima/ima_main.c | 10 +-
security/loadpin/loadpin.c| 14 --
security/security.c | 7 ---
security/selinux/hooks.c | 5 +++--
9 files changed, 41 insertions(+), 14 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 2e29c38eb4df..d73bc3fa710a 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -39,7 +39,7 @@ int kernel_read_file(struct file *file, void **buf,
if (ret)
return ret;
- ret = security_kernel_read_file(file, id);
+ ret = security_kernel_read_file(file, id, true);
if (ret)
goto out;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 502e36ad7804..259023039dc9 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -23,7 +23,8 @@ extern int ima_file_mprotect(struct vm_area_struct *vma,
unsigned long prot);
extern int ima_load_data(enum kernel_load_data_id id, bool contents);
extern int ima_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_read_file(struct file *file, enum kernel_read_file_id id,
+bool contents);
extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
@@ -91,7 +92,8 @@ static inline int ima_post_load_data(char *buf, loff_t size,
return 0;
}
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_read_file(struct file *file, enum kernel_read_file_id id,
+ bool contents)
{
return 0;
}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index aaa2916bbae7..c2ded57c5d9b 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -188,7 +188,7 @@ LSM_HOOK(int, 0, kernel_load_data, enum kernel_load_data_id
id, bool contents)
LSM_HOOK(int, 0, kernel_post_load_data, char *buf, loff_t size,
enum kernel_read_file_id id)
LSM_HOOK(int, 0, kernel_read_file, struct file *file,
-enum kernel_read_file_id id)
+enum kernel_read_file_id id, bool contents)
LSM_HOOK(int, 0, kernel_post_read_file, struct file *file, char *buf,
loff_t size, enum kernel_read_file_id id)
LSM_HOOK(int, 0, task_fix_setuid, struct cred *new, const struct cred *old,
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 812d626195fc..b66433b5aa15 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -650,6 +650,7 @@
* @file contains the file structure pointing to the file being read
* by the kernel.
* @id kernel read file identifier
+ * @contents if a subsequent @kernel_post_read_file will be called.
* Return 0 if permission is granted.
* @kernel_post_read_file:
* Read a file specified by userspace.
@@ -658,6 +659,8 @@
* @buf pointer to buffer containing the file contents.
* @size length of the file contents.
* @id kernel read file identifier
+ * This must be paired with a prior @kernel_read_file call that had
+ * @contents set to true.
* Return 0 if permission is granted.
* @task_fix_setuid:
* Update the module's state after setting one or more of the user
diff --git a/include/linux/security.h b/include/linux/security.h
index e748974c707b..a5d66b89cd6c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -390,7 +390,8 @@ int security_kernel_module_request(char *kmod_name);
int security_kernel_load_data(enum kernel_load_data_id id, bool contents);
int security_kernel_post_load_data(char *buf, loff_t size,
enum kernel_load_data_id id);
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
+int security_kernel_read_file(struct file *file, enum kernel_read_
[PATCH 04/13] fs/kernel_read_file: Split into separate include file
From: Scott Branden
Move kernel_read_file* out of linux/fs.h to its own linux/kernel_read_file.h
include file. That header gets pulled in just about everywhere
and doesn't really need functions not related to the general fs interface.
Suggested-by: Christoph Hellwig
Signed-off-by: Scott Branden
Reviewed-by: Christoph Hellwig
Acked-by: Greg Kroah-Hartman
Link:
https://lore.kernel.org/r/20200706232309.12010-2-scott.bran...@broadcom.com
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 1 +
fs/exec.c | 1 +
include/linux/fs.h | 38 -
include/linux/ima.h | 1 +
include/linux/kernel_read_file.h| 51 +
include/linux/security.h| 1 +
kernel/kexec_file.c | 1 +
kernel/module.c | 1 +
security/integrity/digsig.c | 1 +
security/integrity/ima/ima_fs.c | 1 +
security/integrity/ima/ima_main.c | 1 +
security/integrity/ima/ima_policy.c | 1 +
security/loadpin/loadpin.c | 1 +
security/security.c | 1 +
security/selinux/hooks.c| 1 +
15 files changed, 64 insertions(+), 38 deletions(-)
create mode 100644 include/linux/kernel_read_file.h
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index c2f57cedcd6f..d4a413ea48ce 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
diff --git a/fs/exec.c b/fs/exec.c
index 2bf549757ce7..07a7fe9ac5be 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -23,6 +23,7 @@
* formats.
*/
+#include
#include
#include
#include
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f50a35d54a61..11dd6cc7de58 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,44 +2993,6 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
-/* This is a list of *what* is being read, not *how* nor *where*. */
-#define __kernel_read_file_id(id) \
- id(UNKNOWN, unknown)\
- id(FIRMWARE, firmware) \
- id(MODULE, kernel-module) \
- id(KEXEC_IMAGE, kexec-image)\
- id(KEXEC_INITRAMFS, kexec-initramfs)\
- id(POLICY, security-policy) \
- id(X509_CERTIFICATE, x509-certificate) \
- id(MAX_ID, )
-
-#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
-#define __fid_stringify(dummy, str) #str,
-
-enum kernel_read_file_id {
- __kernel_read_file_id(__fid_enumify)
-};
-
-static const char * const kernel_read_file_str[] = {
- __kernel_read_file_id(__fid_stringify)
-};
-
-static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
-{
- if ((unsigned)id >= READING_MAX_ID)
- return kernel_read_file_str[READING_UNKNOWN];
-
- return kernel_read_file_str[id];
-}
-
-extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path(const char *, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
-extern int kernel_read_file_from_path_initns(const char *, void **, loff_t *,
loff_t,
-enum kernel_read_file_id);
-extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
- enum kernel_read_file_id);
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..148636bfcc8f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -7,6 +7,7 @@
#ifndef _LINUX_IMA_H
#define _LINUX_IMA_H
+#include
#include
#include
#include
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
new file mode 100644
index ..78cf3d7dc835
--- /dev/null
+++ b/include/linux/kernel_read_file.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _LINUX_KERNEL_READ_FILE_H
+#define _LINUX_KERNEL_READ_FILE_H
+
+#include
+#include
+
+/* This is a list of *what* is being read, not *how* nor *where*. */
+#define __kernel_read_file_id(id) \
+ id(UNKNOWN, unknown)\
+ id(FIRMWARE, firmware) \
+ id(MODULE, kernel-module) \
+ id(KEXEC_IMAGE, kexec-image)\
+ id(KEXEC_INITRAMFS, kexec-initramfs)\
+ id(POLICY, security-policy) \
+ id(X509_CERTIFICATE, x509-certificate) \
+ id(MAX_ID, )
+
+#define __fid_enumify(ENUM, dummy) READING_ ## ENUM,
+#define __fid_stringify(dummy, str) #str,
+
+e
[PATCH 07/13] fs/kernel_read_file: Switch buffer size arg to size_t
In preparation for further refactoring of kernel_read_file*(), rename
the "max_size" argument to the more accurate "buf_size", and correct
its type to size_t. Add kerndoc to explain the specifics of how the
arguments will be used. Note that with buf_size now size_t, it can no
longer be negative (and was never called with a negative value). Adjust
callers to use it as a "maximum size" when *buf is NULL.
Signed-off-by: Kees Cook
---
fs/kernel_read_file.c| 34 +++-
include/linux/kernel_read_file.h | 8
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
4 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index dc28a8def597..e21a76001fff 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,15 +5,31 @@
#include
#include
+/**
+ * kernel_read_file() - read file contents into a kernel buffer
+ *
+ * @file file to read from
+ * @bufpointer to a "void *" buffer for reading into (if
+ * *@buf is NULL, a buffer will be allocated, and
+ * @buf_size will be ignored)
+ * @buf_size size of buf, if already allocated. If @buf not
+ * allocated, this is the largest size to allocate.
+ * @id the kernel_read_file_id identifying the type of
+ * file contents being read (for LSMs to examine)
+ *
+ * Returns number of bytes read (no single read will be bigger
+ * than INT_MAX), or negative on error.
+ *
+ */
int kernel_read_file(struct file *file, void **buf,
-loff_t max_size, enum kernel_read_file_id id)
+size_t buf_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
ssize_t bytes = 0;
void *allocated = NULL;
int ret;
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ if (!S_ISREG(file_inode(file)->i_mode))
return -EINVAL;
ret = deny_write_access(file);
@@ -29,7 +45,7 @@ int kernel_read_file(struct file *file, void **buf,
ret = -EINVAL;
goto out;
}
- if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || i_size > buf_size) {
ret = -EFBIG;
goto out;
}
@@ -75,7 +91,7 @@ int kernel_read_file(struct file *file, void **buf,
EXPORT_SYMBOL_GPL(kernel_read_file);
int kernel_read_file_from_path(const char *path, void **buf,
- loff_t max_size, enum kernel_read_file_id id)
+ size_t buf_size, enum kernel_read_file_id id)
{
struct file *file;
int ret;
@@ -87,14 +103,14 @@ int kernel_read_file_from_path(const char *path, void
**buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t max_size,
+ size_t buf_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -113,13 +129,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, max_size, id);
+ ret = kernel_read_file(file, buf, buf_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
+int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
enum kernel_read_file_id id)
{
struct fd f = fdget(fd);
@@ -128,7 +144,7 @@ int kernel_read_file_from_fd(int fd, void **buf, loff_t
max_size,
if (!f.file)
goto out;
- ret = kernel_read_file(f.file, buf, max_size, id);
+ ret = kernel_read_file(f.file, buf, buf_size, id);
out:
fdput(f);
return ret;
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 0ca0bdbed1bd..910039e7593e 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -36,16 +36,16 @@ static inline const char *kernel_read_file_id_str(enum
kernel_read_file_id id)
}
int kernel_read_file(struct file *file,
-void **buf, loff_t max_size,
+void **buf, size_t buf_size,
enum kernel_read_file_id id);
int kernel_read_file_from_path(const char *path,
- void **buf, loff_t max_size,
+ void **buf, size_t buf
[PATCH 05/13] fs/kernel_read_file: Split into separate source file
These routines are used in places outside of exec(2), so in preparation
for refactoring them, move them into a separate source file,
fs/kernel_read_file.c.
Signed-off-by: Kees Cook
---
fs/Makefile | 3 +-
fs/exec.c | 132
fs/kernel_read_file.c | 138 ++
3 files changed, 140 insertions(+), 133 deletions(-)
create mode 100644 fs/kernel_read_file.c
diff --git a/fs/Makefile b/fs/Makefile
index 2ce5112b02c8..a05fc247b2a7 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -13,7 +13,8 @@ obj-y := open.o read_write.o file_table.o super.o \
seq_file.o xattr.o libfs.o fs-writeback.o \
pnode.o splice.o sync.o utimes.o d_path.o \
stack.o fs_struct.o statfs.o fs_pin.o nsfs.o \
- fs_types.o fs_context.o fs_parser.o fsopen.o
+ fs_types.o fs_context.o fs_parser.o fsopen.o \
+ kernel_read_file.o
ifeq ($(CONFIG_BLOCK),y)
obj-y += buffer.o block_dev.o direct-io.o mpage.o
diff --git a/fs/exec.c b/fs/exec.c
index 07a7fe9ac5be..d619b79aab30 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -923,138 +923,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
-loff_t max_size, enum kernel_read_file_id id)
-{
- loff_t i_size, pos;
- ssize_t bytes = 0;
- void *allocated = NULL;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (!*buf)
- *buf = allocated = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, );
- if (bytes < 0) {
- ret = bytes;
- goto out_free;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (allocated) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct file *file;
- struct path root;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- task_lock(_task);
- get_fs_root(init_task.fs, );
- task_unlock(_task);
-
- file = file_open_root(root.dentry, root.mnt, path, O_RDONLY, 0);
- path_put();
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
-enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
#if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
defined(CONFIG_BINFMT_ELF_FDPIC)
ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t
len)
diff
[PATCH 10/13] firmware_loader: Use security_post_load_data()
Now that security_post_load_data() is wired up, use it instead
of the NULL file argument style of security_post_read_file(),
and update the security_kernel_load_data() call to indicate that a
security_kernel_post_load_data() call is expected.
Wire up the IMA check to match earlier logic. Perhaps a generalized
change to ima_post_load_data() might look something like this:
return process_buffer_measurement(buf, size,
kernel_load_data_id_str(load_id),
read_idmap[load_id] ?: FILE_CHECK,
0, NULL);
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/fallback.c | 8
.../base/firmware_loader/fallback_platform.c | 7 ++-
security/integrity/ima/ima_main.c | 20 +--
3 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/drivers/base/firmware_loader/fallback.c
b/drivers/base/firmware_loader/fallback.c
index a196aacce22c..7cfdfdcb819c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -272,9 +272,9 @@ static ssize_t firmware_loading_store(struct device *dev,
dev_err(dev, "%s: map pages failed\n",
__func__);
else
- rc = security_kernel_post_read_file(NULL,
- fw_priv->data, fw_priv->size,
- READING_FIRMWARE);
+ rc =
security_kernel_post_load_data(fw_priv->data,
+ fw_priv->size,
+ LOADING_FIRMWARE);
/*
* Same logic as fw_load_abort, only the DONE bit
@@ -613,7 +613,7 @@ static bool fw_run_sysfs_fallback(u32 opt_flags)
return false;
/* Also permit LSMs and IMA to fail firmware sysfs fallback */
- ret = security_kernel_load_data(LOADING_FIRMWARE, false);
+ ret = security_kernel_load_data(LOADING_FIRMWARE, true);
if (ret < 0)
return false;
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index a12c79d47efc..4d1157af0e86 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -17,7 +17,7 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (!(opt_flags & FW_OPT_FALLBACK_PLATFORM))
return -ENOENT;
- rc = security_kernel_load_data(LOADING_FIRMWARE, false);
+ rc = security_kernel_load_data(LOADING_FIRMWARE, true);
if (rc)
return rc;
@@ -27,6 +27,11 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (fw_priv->data && size > fw_priv->allocated_size)
return -ENOMEM;
+
+ rc = security_kernel_post_load_data((u8 *)data, size, LOADING_FIRMWARE);
+ if (rc)
+ return rc;
+
if (!fw_priv->data)
fw_priv->data = vmalloc(size);
if (!fw_priv->data)
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 85000dc8595c..1a7bc4c7437d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -648,15 +648,6 @@ int ima_post_read_file(struct file *file, void *buf,
loff_t size,
enum ima_hooks func;
u32 secid;
- if (!file && read_id == READING_FIRMWARE) {
- if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
- (ima_appraise & IMA_APPRAISE_ENFORCE)) {
- pr_err("Prevent firmware loading_store.\n");
- return -EACCES; /* INTEGRITY_UNKNOWN */
- }
- return 0;
- }
-
/* permit signed certs */
if (!file && read_id == READING_X509_CERTIFICATE)
return 0;
@@ -706,7 +697,7 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
}
break;
case LOADING_FIRMWARE:
- if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE)) {
+ if (ima_enforce && (ima_appraise & IMA_APPRAISE_FIRMWARE) &&
!contents) {
pr_err("Prevent firmware sysfs fallback loading.\n");
return -EACCES; /* INTEGRITY_UNKNOWN */
}
@@ -739,6 +730,15 @@ int ima_load_data(enum kernel_load_data_id id, bool
contents)
*/
int ima_post_load_data(char *buf, loff_t size, enum kernel_load_data_id
load_id)
{
+ if (load_id == LOADING_FIRMWARE) {
+ if ((ima_appraise & IMA_APPRA
[PATCH 11/13] module: Call security_kernel_post_load_data()
Now that there is an API for checking loaded contents for modules
loaded without a file, call into the LSM hooks.
Cc: Jessica Yu
Signed-off-by: Kees Cook
---
kernel/module.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index d56cb34d9a2f..90a4788dff9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2967,7 +2967,7 @@ static int copy_module_from_user(const void __user *umod,
unsigned long len,
if (info->len < sizeof(*(info->hdr)))
return -ENOEXEC;
- err = security_kernel_load_data(LOADING_MODULE, false);
+ err = security_kernel_load_data(LOADING_MODULE, true);
if (err)
return err;
@@ -2977,11 +2977,17 @@ static int copy_module_from_user(const void __user
*umod, unsigned long len,
return -ENOMEM;
if (copy_chunked_from_user(info->hdr, umod, info->len) != 0) {
- vfree(info->hdr);
- return -EFAULT;
+ err = -EFAULT;
+ goto out;
}
- return 0;
+ err = security_kernel_post_load_data((char *)info->hdr, info->len,
+LOADING_MODULE);
+out:
+ if (err)
+ vfree(info->hdr);
+
+ return err;
}
static void free_copy(struct load_info *info)
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 02/13] fs/kernel_read_file: Remove FIRMWARE_PREALLOC_BUFFER enum
FIRMWARE_PREALLOC_BUFFER is a "how", not a "what", and confuses the LSMs
that are interested in filtering between types of things. The "how"
should be an internal detail made uninteresting to the LSMs.
Fixes: a098ecd2fa7d ("firmware: support loading into a pre-allocated buffer")
Fixes: fd90bc559bfb ("ima: based on policy verify firmware signatures
(pre-allocated buffer)")
Fixes: 4f0496d8ffa3 ("ima: based on policy warn about loading firmware
(pre-allocated buffer)")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/main.c | 5 ++---
fs/exec.c | 7 ---
include/linux/fs.h | 2 +-
kernel/module.c | 2 +-
security/integrity/digsig.c | 2 +-
security/integrity/ima/ima_fs.c | 2 +-
security/integrity/ima/ima_main.c | 6 ++
7 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index ca871b13524e..c2f57cedcd6f 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -465,14 +465,12 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
int i, len;
int rc = -ENOENT;
char *path;
- enum kernel_read_file_id id = READING_FIRMWARE;
size_t msize = INT_MAX;
void *buffer = NULL;
/* Already populated data member means we're loading into a buffer */
if (!decompress && fw_priv->data) {
buffer = fw_priv->data;
- id = READING_FIRMWARE_PREALLOC_BUFFER;
msize = fw_priv->allocated_size;
}
@@ -496,7 +494,8 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
/* load firmware files from the mount namespace of init */
rc = kernel_read_file_from_path_initns(path, ,
- , msize, id);
+ , msize,
+ READING_FIRMWARE);
if (rc) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
diff --git a/fs/exec.c b/fs/exec.c
index e6e8a9a70327..2bf549757ce7 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -927,6 +927,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
{
loff_t i_size, pos;
ssize_t bytes = 0;
+ void *allocated = NULL;
int ret;
if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
@@ -950,8 +951,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
goto out;
}
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
+ if (!*buf)
+ *buf = allocated = vmalloc(i_size);
if (!*buf) {
ret = -ENOMEM;
goto out;
@@ -980,7 +981,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out_free:
if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ if (allocated) {
vfree(*buf);
*buf = NULL;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3f881a892ea7..95fc775ed937 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2993,10 +2993,10 @@ static inline void i_readcount_inc(struct inode *inode)
#endif
extern int do_pipe_flags(int *, int);
+/* This is a list of *what* is being read, not *how*. */
#define __kernel_read_file_id(id) \
id(UNKNOWN, unknown)\
id(FIRMWARE, firmware) \
- id(FIRMWARE_PREALLOC_BUFFER, firmware) \
id(FIRMWARE_EFI_EMBEDDED, firmware) \
id(MODULE, kernel-module) \
id(KEXEC_IMAGE, kexec-image)\
diff --git a/kernel/module.c b/kernel/module.c
index 0c6573b98c36..26105148f4d2 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const char __user
*, uargs, int, flags)
{
struct load_info info = { };
loff_t size;
- void *hdr;
+ void *hdr = NULL;
int err;
err = may_init_module();
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index e9cbadade74b..ac02b7632353 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -169,7 +169,7 @@ int __init integrity_add_key(const unsigned int id, const
void *data,
int __init integrity_load_x509(const unsigned int id, const char *path)
{
- void *data;
+ void *data = NULL;
loff_t size;
in
[PATCH 01/13] firmware_loader: EFI firmware loader must handle pre-allocated buffer
The EFI platform firmware fallback would clobber any pre-allocated
buffers. Instead, correctly refuse to reallocate when too small (as
already done in the sysfs fallback), or perform allocation normally
when needed.
Fixes: e4c2c0ff00ec ("firmware: Add new platform fallback mechanism and firm
ware_request_platform()")
Cc: sta...@vger.kernel.org
Signed-off-by: Kees Cook
---
To aid in backporting, this change is made before moving
kernel_read_file() to separate header/source files.
---
drivers/base/firmware_loader/fallback_platform.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/base/firmware_loader/fallback_platform.c
b/drivers/base/firmware_loader/fallback_platform.c
index cdd2c9a9f38a..685edb7dd05a 100644
--- a/drivers/base/firmware_loader/fallback_platform.c
+++ b/drivers/base/firmware_loader/fallback_platform.c
@@ -25,7 +25,10 @@ int firmware_fallback_platform(struct fw_priv *fw_priv, u32
opt_flags)
if (rc)
return rc; /* rc == -ENOENT when the fw was not found */
- fw_priv->data = vmalloc(size);
+ if (fw_priv->data && size > fw_priv->allocated_size)
+ return -ENOMEM;
+ if (!fw_priv->data)
+ fw_priv->data = vmalloc(size);
if (!fw_priv->data)
return -ENOMEM;
--
2.25.1
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook
---
drivers/base/firmware_loader/main.c | 8
fs/kernel_read_file.c | 20 +---
include/linux/kernel_read_file.h| 8
kernel/kexec_file.c | 13 ++---
kernel/module.c | 7 +++
security/integrity/digsig.c | 5 +++--
security/integrity/ima/ima_fs.c | 5 +++--
7 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..ea419c7d3d34 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
size_t in_size,
const void *in_buffer))
{
- loff_t size;
+ size_t size;
int i, len;
int rc = -ENOENT;
char *path;
@@ -494,10 +494,9 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
fw_priv->size = 0;
/* load firmware files from the mount namespace of init */
- rc = kernel_read_file_from_path_initns(path, ,
- , msize,
+ rc = kernel_read_file_from_path_initns(path, , msize,
READING_FIRMWARE);
- if (rc) {
+ if (rc < 0) {
if (rc != -ENOENT)
dev_warn(device, "loading %s failed with error
%d\n",
path, rc);
@@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device, struct
fw_priv *fw_priv,
path);
continue;
}
+ size = rc;
dev_dbg(device, "Loading firmware from %s\n", path);
if (decompress) {
dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
#include
#include
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
ret = -EINVAL;
goto out;
}
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
ret = -EFBIG;
goto out;
}
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
}
ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
out_free:
if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void **buf, loff_t
*size,
out:
allow_write_access(file);
- return ret;
+ return ret == 0 ? pos : ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file);
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
loff_t max_size, enum kernel_read_file_id id)
{
struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const char *path, void
**buf, loff_t *size,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
int kernel_read_file_from_path_initns(const char *path, void **buf,
- loff_t *size, loff_t max_size,
+ loff_t max_size,
enum kernel_read_file_id id)
{
struct file *file;
@@ -115,13 +113,13 @@ int kernel_read_file_from_path_initns(const char *path,
void **buf,
if (IS_ERR(file))
return PTR_ERR(file);
- ret = kernel_read_file(file, buf, size, max_size, id);
+ ret = kernel_read_file(file, buf, max_size, id);
fput(file);
return ret;
}
EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
-int kernel_read_file_from_fd(int fd, vo
[PATCH 00/13] Introduce partial kernel_read_file() support
Hi, Here's my attempt at clearing the path to partial read support in kernel_read_file(), which fixes a number of issues along the way. I'm still fighting with the firmware test suite (it doesn't seem to pass for me even in stock v5.7... ?) But I don't want to block Scott's work[1] any this week, so here's the series as it is currently. The primary difference to Scott's approach is to avoid adding a new set of functions and just adapt the existing APIs to deal with "offset". Also, the fixes for the enum are first in the series so they can be backported without the header file relocation. I'll keep poking at the firmware tests... -Kees [1] https://lore.kernel.org/lkml/202007161415.10D015477@keescook/ Kees Cook (12): 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 Scott Branden (1): fs/kernel_read_file: Split into separate include file drivers/base/firmware_loader/fallback.c | 8 +- .../base/firmware_loader/fallback_platform.c | 12 +- drivers/base/firmware_loader/main.c | 13 +- fs/Makefile | 3 +- fs/exec.c | 132 +--- fs/kernel_read_file.c | 189 ++ 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 | 18 +- kernel/module.c | 24 ++- security/integrity/digsig.c | 8 +- security/integrity/ima/ima_fs.c | 9 +- security/integrity/ima/ima_main.c | 58 -- security/integrity/ima/ima_policy.c | 1 + security/loadpin/loadpin.c| 17 +- security/security.c | 26 ++- security/selinux/hooks.c | 8 +- 22 files changed, 432 insertions(+), 246 deletions(-) create mode 100644 fs/kernel_read_file.c create mode 100644 include/linux/kernel_read_file.h -- 2.25.1 ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH][next] kexec_file: Use array_size() helper in memcpy()
On Tue, Jun 16, 2020 at 01:20:41PM -0500, Gustavo A. R. Silva wrote: > Use array_size() instead of the open-coded version in memcpy(). These > sorts of multiplication factors need to be wrapped in array_size(). > > Also, while there, use the preferred form for passing a size of a struct. > The alternative form where struct name is spelled out hurts readability > and introduces an opportunity for a bug when the pointer variable type is > changed but the corresponding sizeof that is passed as argument is not. > > This issue was found with the help of Coccinelle and, audited and fixed > manually. > > Addresses-KSPP-ID: https://github.com/KSPP/linux/issues/83 > Signed-off-by: Gustavo A. R. Silva Reviewed-by: Kees Cook -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] kexec: dump kmessage before machine_kexec
On Fri, Jun 05, 2020 at 03:46:42PM -0400, Pavel Tatashin wrote: > kmsg_dump(KMSG_DUMP_SHUTDOWN) is called before > machine_restart(), machine_halt(), machine_power_off(), the only one that > is missing is machine_kexec(). > > The dmesg output that it contains can be used to study the shutdown > performance of both kernel and systemd during kexec reboot. > > Here is example of dmesg data collected after kexec: > > root@dplat-cp22:~# cat /sys/fs/pstore/dmesg-ramoops-0 | tail > ... > <6>[ 70.914592] psci: CPU3 killed (polled 0 ms) > <5>[ 70.915705] CPU4: shutdown > <6>[ 70.916643] psci: CPU4 killed (polled 4 ms) > <5>[ 70.917715] CPU5: shutdown > <6>[ 70.918725] psci: CPU5 killed (polled 0 ms) > <5>[ 70.919704] CPU6: shutdown > <6>[ 70.920726] psci: CPU6 killed (polled 4 ms) > <5>[ 70.921642] CPU7: shutdown > <6>[ 70.922650] psci: CPU7 killed (polled 0 ms) > > Signed-off-by: Pavel Tatashin Reviewed-by: Kees Cook -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/3] fs: reduce export usage of kerne_read*() calls
On Fri, May 22, 2020 at 03:24:32PM -0700, Scott Branden wrote:
> On 2020-05-18 5:37 a.m., Mimi Zohar wrote:
> > On Sun, 2020-05-17 at 23:22 -0700, Christoph Hellwig wrote:
> > > On Fri, May 15, 2020 at 09:29:33PM +, Luis Chamberlain wrote:
> > > > On Wed, May 13, 2020 at 11:17:36AM -0700, Christoph Hellwig wrote:
> > > > > Can you also move kernel_read_* out of fs.h? That header gets pulled
> > > > > in just about everywhere and doesn't really need function not related
> > > > > to the general fs interface.
> > > > Sure, where should I dump these?
> > > Maybe a new linux/kernel_read_file.h? Bonus points for a small top
> > > of the file comment explaining the point of the interface, which I
> > > still don't get :)
> > Instead of rolling your own method of having the kernel read a file,
> > which requires call specific security hooks, this interface provides a
> > single generic set of pre and post security hooks. The
> > kernel_read_file_id enumeration permits the security hook to
> > differentiate between callers.
> >
> > To comply with secure and trusted boot concepts, a file cannot be
> > accessible to the caller until after it has been measured and/or the
> > integrity (hash/signature) appraised.
> >
> > In some cases, the file was previously read twice, first to measure
> > and/or appraise the file and then read again into a buffer for
> > use. This interface reads the file into a buffer once, calls the
> > generic post security hook, before providing the buffer to the caller.
> > (Note using firmware pre-allocated memory might be an issue.)
> >
> > Partial reading firmware will result in needing to pre-read the entire
> > file, most likely on the security pre hook.
> The entire file may be very large and not fit into a buffer.
> Hence one of the reasons for a partial read of the file.
> For security purposes, you need to change your code to limit the amount
> of data it reads into a buffer at one time to not consume or run out of much
> memory.
Hm? That's not how whole-file hashing works. :)
These hooks need to finish their hashing and policy checking before they
can allow the rest of the code to move forward. (That's why it's a
security hook.) If kernel memory utilization is the primary concern,
then sure, things could be rearranged to do partial read and update the
hash incrementally, but the entire file still needs to be locked,
entirely hashed by hook, then read by the caller, then unlocked and
released.
So, if you want to have partial file reads work, you'll need to
rearchitect the way this works to avoid regressing the security coverage
of these operations.
So, probably, the code will look something like:
file = kernel_open_file_for_reading(...)
file = open...
disallow_writes(file);
while (processed < size-of-file) {
buf = read(file, size...)
security_file_read_partial(buf)
}
ret = security_file_read_finished(file);
if (ret < 0) {
allow_writes(file);
return PTR_ERR(ret);
}
return file;
while (processed < size-of-file) {
buf = read(file, size...)
firmware_send_partial(buf);
}
kernel_close_file_for_reading(file)
allow_writes(file);
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH 0/3] fs: reduce export usage of kerne_read*() calls
On Mon, May 18, 2020 at 08:37:42AM -0400, Mimi Zohar wrote: > Hi Christoph, > > On Sun, 2020-05-17 at 23:22 -0700, Christoph Hellwig wrote: > > On Fri, May 15, 2020 at 09:29:33PM +, Luis Chamberlain wrote: > > > On Wed, May 13, 2020 at 11:17:36AM -0700, Christoph Hellwig wrote: > > > > Can you also move kernel_read_* out of fs.h? That header gets pulled > > > > in just about everywhere and doesn't really need function not related > > > > to the general fs interface. > > > > > > Sure, where should I dump these? > > > > Maybe a new linux/kernel_read_file.h? Bonus points for a small top > > of the file comment explaining the point of the interface, which I > > still don't get :) > > Instead of rolling your own method of having the kernel read a file, > which requires call specific security hooks, this interface provides a > single generic set of pre and post security hooks. The > kernel_read_file_id enumeration permits the security hook to > differentiate between callers. > > To comply with secure and trusted boot concepts, a file cannot be > accessible to the caller until after it has been measured and/or the > integrity (hash/signature) appraised. > > In some cases, the file was previously read twice, first to measure > and/or appraise the file and then read again into a buffer for > use. This interface reads the file into a buffer once, calls the > generic post security hook, before providing the buffer to the caller. > (Note using firmware pre-allocated memory might be an issue.) > > Partial reading firmware will result in needing to pre-read the entire > file, most likely on the security pre hook. Well described! :) -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3] kernel: add panic_on_taint
On Sat, May 09, 2020 at 09:57:37AM -0400, Rafael Aquini wrote: > Analogously to the introduction of panic_on_warn, this patch > introduces a kernel option named panic_on_taint in order to > provide a simple and generic way to stop execution and catch > a coredump when the kernel gets tainted by any given taint flag. > > This is useful for debugging sessions as it avoids rebuilding > the kernel to explicitly add calls to panic() or BUG() into > code sites that introduce the taint flags of interest. > Another, perhaps less frequent, use for this option would be > as a mean for assuring a security policy (in paranoid mode) > case where no single taint is allowed for the running system. > > Suggested-by: Qian Cai > Signed-off-by: Rafael Aquini Reviewed-by: Kees Cook -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC 21/43] x86/KASLR: PKRAM: support physical kaslr
On Wed, May 06, 2020 at 05:41:47PM -0700, Anthony Yznaga wrote: > Avoid regions of memory that contain preserved pages when computing > slots used to select where to put the decompressed kernel. This is changing the slot-walking code instead of updating mem_avoid_overlap() -- that's where the check for a "reserved" memory area should live. For example, this is how both mem_avoid_memmap() and the setup_data memory areas are handled. Is there a reason mem_avoid_overlap() can't be used here? -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH 09/11] kallsyms: hide layout and expose seed
On Thu, Feb 27, 2020 at 10:42:53AM +0800, Baoquan He wrote: > On 02/06/20 at 09:51am, Kristen Carlson Accardi wrote: > > On Thu, 2020-02-06 at 04:32 -0800, Kees Cook wrote: > > > > In the past, making kallsyms entirely unreadable seemed to break > > > weird > > > stuff in userspace. How about having an alternative view that just > > > contains a alphanumeric sort of the symbol names (and they will > > > continue > > > to have zeroed addresses for unprivileged users)? > > > > > > Or perhaps we wait to hear about this causing a problem, and deal > > > with > > > it then? :) > > > > > > > Yeah - I don't know what people want here. Clearly, we can't leave > > kallsyms the way it is. Removing it entirely is a pretty fast way to > > figure out how people use it though :). > > Kexec-tools and makedumpfile are the users of /proc/kallsyms currently. > We use kallsyms to get page_offset_base and _stext. AIUI, those run as root so they'd be able to consume the uncensored output. -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V34 09/29] kexec_file: Restrict at runtime if the kernel is locked down
On Fri, Jun 21, 2019 at 05:03:38PM -0700, Matthew Garrett wrote: > From: Jiri Bohac > > When KEXEC_SIG is not enabled, kernel should not load images through > kexec_file systemcall if the kernel is locked down. > > [Modified by David Howells to fit with modifications to the previous patch > and to return -EPERM if the kernel is locked down for consistency with > other lockdowns. Modified by Matthew Garrett to remove the IMA > integration, which will be replaced by integrating with the IMA > architecture policy patches.] > > Signed-off-by: Jiri Bohac Reviewed-by: Kees Cook -Kees > Signed-off-by: David Howells > Signed-off-by: Matthew Garrett > Reviewed-by: Jiri Bohac > cc: kexec@lists.infradead.org > --- > kernel/kexec_file.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c > index eec7e5bb2a08..27adb4312b03 100644 > --- a/kernel/kexec_file.c > +++ b/kernel/kexec_file.c > @@ -237,7 +237,10 @@ kimage_file_prepare_segments(struct kimage *image, int > kernel_fd, int initrd_fd, > goto out; > } > > - ret = 0; > + ret = security_locked_down(LOCKDOWN_KEXEC); > + if (ret) > + goto out; > + > break; > > /* All other errors are fatal, including nomem, unparseable > -- > 2.22.0.410.gd8fdbe21b5-goog > -- Kees Cook
Re: [PATCH V34 07/29] Copy secure_boot flag in boot params across kexec reboot
On Fri, Jun 21, 2019 at 05:03:36PM -0700, Matthew Garrett wrote: > From: Dave Young > > Kexec reboot in case secure boot being enabled does not keep the secure > boot mode in new kernel, so later one can load unsigned kernel via legacy > kexec_load. In this state, the system is missing the protections provided > by secure boot. > > Adding a patch to fix this by retain the secure_boot flag in original > kernel. > > secure_boot flag in boot_params is set in EFI stub, but kexec bypasses the > stub. Fixing this issue by copying secure_boot flag across kexec reboot. > > Signed-off-by: Dave Young Reviewed-by: Kees Cook -Kees > Signed-off-by: David Howells > Signed-off-by: Matthew Garrett > cc: kexec@lists.infradead.org > --- > arch/x86/kernel/kexec-bzimage64.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/x86/kernel/kexec-bzimage64.c > b/arch/x86/kernel/kexec-bzimage64.c > index 22f60dd26460..4243359ac509 100644 > --- a/arch/x86/kernel/kexec-bzimage64.c > +++ b/arch/x86/kernel/kexec-bzimage64.c > @@ -182,6 +182,7 @@ setup_efi_state(struct boot_params *params, unsigned long > params_load_addr, > if (efi_enabled(EFI_OLD_MEMMAP)) > return 0; > > + params->secure_boot = boot_params.secure_boot; > ei->efi_loader_signature = current_ei->efi_loader_signature; > ei->efi_systab = current_ei->efi_systab; > ei->efi_systab_hi = current_ei->efi_systab_hi; > -- > 2.22.0.410.gd8fdbe21b5-goog > -- Kees Cook ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH V34 06/29] kexec_load: Disable at runtime if the kernel is locked down
On Fri, Jun 21, 2019 at 05:03:35PM -0700, Matthew Garrett wrote:
> From: Matthew Garrett
>
> The kexec_load() syscall permits the loading and execution of arbitrary
> code in ring 0, which is something that lock-down is meant to prevent. It
> makes sense to disable kexec_load() in this situation.
>
> This does not affect kexec_file_load() syscall which can check for a
> signature on the image to be booted.
>
> Signed-off-by: David Howells
Reviewed-by: Kees Cook
-Kees
> Signed-off-by: Matthew Garrett
> Acked-by: Dave Young
> cc: kexec@lists.infradead.org
> ---
> include/linux/security.h | 1 +
> kernel/kexec.c | 8
> security/lockdown/lockdown.c | 1 +
> 3 files changed, 10 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 200175c8605a..00a31ab2e5ba 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -84,6 +84,7 @@ enum lockdown_reason {
> LOCKDOWN_NONE,
> LOCKDOWN_MODULE_SIGNATURE,
> LOCKDOWN_DEV_MEM,
> + LOCKDOWN_KEXEC,
> LOCKDOWN_INTEGRITY_MAX,
> LOCKDOWN_CONFIDENTIALITY_MAX,
> };
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index 68559808fdfa..ec3f07a4b1c0 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -207,6 +207,14 @@ static inline int kexec_load_check(unsigned long
> nr_segments,
> if (result < 0)
> return result;
>
> + /*
> + * kexec can be used to circumvent module loading restrictions, so
> + * prevent loading in that case
> + */
> + result = security_locked_down(LOCKDOWN_KEXEC);
> + if (result)
> + return result;
> +
> /*
>* Verify we have a legal set of flags
>* This leaves us room for future extensions.
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 565c87451f0f..08fcd8116db3 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -20,6 +20,7 @@ static char
> *lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
> [LOCKDOWN_NONE] = "none",
> [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
> [LOCKDOWN_DEV_MEM] = "/dev/mem,kmem,port",
> + [LOCKDOWN_KEXEC] = "kexec of unsigned images",
> [LOCKDOWN_INTEGRITY_MAX] = "integrity",
> [LOCKDOWN_CONFIDENTIALITY_MAX] = "confidentiality",
> };
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
--
Kees Cook
Re: [PATCH v2] x86_64, vmcoreinfo: Append 'page_offset_base' to vmcoreinfo
On Tue, Nov 27, 2018 at 3:29 PM, Baoquan He wrote:
> On 11/27/18 at 02:16pm, Kees Cook wrote:
>> Why is KERNELOFFSET= not sufficient?
>>
>> See commit b6085a865762 ("x86, kaslr: export offset in VMCOREINFO ELF notes")
>>
>> + vmcoreinfo_append_str("KERNELOFFSET=%lx\n",
>> + (unsigned long)&_text - __START_KERNEL);
>
> KERNELOFFSET is virtual address delta after kernel text KASLR, namely
> the offset from the original default kernel text virtual address,
> 0x8800.
>
> While after memory region KASLR in kernel_randomize_memory(), the
> starting address of the direct mapping of physical memory, PAGE_OFFSET,
> is changed too. We need get it to analyze memory in makedumpfile/crash.
> Currently we deduce it from elf program segment of kcore:
> Program Headers:
> Type Offset VirtAddr PhysAddr
> FileSizMemSiz Flags Align
> ..
>
> LOAD 0x0a62c0004000 0x8a62c0001000 0x1000
> 0x0009c000 0x0009c000 RWE1000
>
> page_offset = 0x8a62c0001000 - 0x1000;
> Since we put the direct mapping segments at the bottom part of kcore, we
> can always get page_offset right.
>
> Thanks
> Baoquan
>
>>
>> -Kees
>>
>> >> diff --git a/arch/x86/kernel/machine_kexec_64.c
>> >> b/arch/x86/kernel/machine_kexec_64.c
>> >> index 4c8acdfdc5a7..6161d77c5bfb 100644
>> >> --- a/arch/x86/kernel/machine_kexec_64.c
>> >> +++ b/arch/x86/kernel/machine_kexec_64.c
>> >> @@ -356,6 +356,9 @@ void arch_crash_save_vmcoreinfo(void)
>> >> VMCOREINFO_SYMBOL(init_top_pgt);
>> >> vmcoreinfo_append_str("NUMBER(pgtable_l5_enabled)=%d\n",
>> >> pgtable_l5_enabled());
>> >> +#ifdef CONFIG_RANDOMIZE_BASE
Okay, gotcha. In that case, shouldn't this be CONFIG_RANDOMIZE_MEMORY?
-Kees
>> >> + VMCOREINFO_NUMBER(page_offset_base);
>> >> +#endif
>> >>
>> >> #ifdef CONFIG_NUMA
>> >> VMCOREINFO_SYMBOL(node_data);
>>
>> --
>> Kees Cook
>>
>> ___
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v2] x86_64, vmcoreinfo: Append 'page_offset_base' to vmcoreinfo
On Wed, Nov 21, 2018 at 3:39 AM, Borislav Petkov wrote:
> + Kees.
>
> On Fri, Nov 16, 2018 at 03:17:49AM +0530, Bhupesh Sharma wrote:
>> x86_64 kernel uses 'page_offset_base' variable to point to the
>> start of direct mapping of all physical memory. This variable
>> is also updated for KASLR boot cases, so this can be exported
>> via vmcoreinfo as a standard ABI between kernel and user-space,
>> to allow user-space utilities to use the same for calculating
>> the start of direct mapping of all physical memory.
Why is KERNELOFFSET= not sufficient?
See commit b6085a865762 ("x86, kaslr: export offset in VMCOREINFO ELF notes")
+ vmcoreinfo_append_str("KERNELOFFSET=%lx\n",
+ (unsigned long)&_text - __START_KERNEL);
-Kees
>> diff --git a/arch/x86/kernel/machine_kexec_64.c
>> b/arch/x86/kernel/machine_kexec_64.c
>> index 4c8acdfdc5a7..6161d77c5bfb 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -356,6 +356,9 @@ void arch_crash_save_vmcoreinfo(void)
>> VMCOREINFO_SYMBOL(init_top_pgt);
>> vmcoreinfo_append_str("NUMBER(pgtable_l5_enabled)=%d\n",
>> pgtable_l5_enabled());
>> +#ifdef CONFIG_RANDOMIZE_BASE
>> + VMCOREINFO_NUMBER(page_offset_base);
>> +#endif
>>
>> #ifdef CONFIG_NUMA
>> VMCOREINFO_SYMBOL(node_data);
--
Kees Cook
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 8/8] ima: based on policy warn about loading firmware (pre-allocated buffer)
On Fri, Jul 13, 2018 at 11:06 AM, Mimi Zohar wrote:
> Some systems are memory constrained but they need to load very large
> firmwares. The firmware subsystem allows drivers to request this
> firmware be loaded from the filesystem, but this requires that the
> entire firmware be loaded into kernel memory first before it's provided
> to the driver. This can lead to a situation where we map the firmware
> twice, once to load the firmware into kernel memory and once to copy the
> firmware into the final resting place.
>
> To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> into a pre-allocated buffer") introduced request_firmware_into_buf() API
> that allows drivers to request firmware be loaded directly into a
> pre-allocated buffer.
>
> Do devices using pre-allocated memory run the risk of the firmware being
> accessible to the device prior to the completion of IMA's signature
> verification any more than when using two buffers? (Refer to mailing list
> discussion[1]).
>
> Only on systems with an IOMMU can the access be prevented. As long as
> the signature verification completes prior to the DMA map is performed,
> the device can not access the buffer. This implies that the same buffer
> can not be re-used. Can we ensure the buffer has not been DMA mapped
> before using the pre-allocated buffer?
>
> [1] https://lkml.org/lkml/2018/7/10/56
>
> Signed-off-by: Mimi Zohar
> Cc: Luis R. Rodriguez
> Cc: Stephen Boyd
> Cc: Bjorn Andersson
> Cc: Ard Biesheuvel
I can't decide if it's worth adding the link (maybe using the
lkml.kernel.org url[1]) directly in the code or not.
Either way:
Reviewed-by: Kees Cook
-Kees
[1]
https://lkml.kernel.org/r/CAKv+Gu-knHeBRGqo+2pb3X9cCjwovEykoXUf=dzyp7ajpos...@mail.gmail.com
--
Kees Cook
Pixel Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 7/8] module: replace the existing LSM hook in init_module
On Fri, Jul 13, 2018 at 11:06 AM, Mimi Zohar wrote: > Both the init_module and finit_module syscalls call either directly > or indirectly the security_kernel_read_file LSM hook. This patch > replaces the direct call in init_module with a call to the new > security_kernel_load_data hook and makes the corresponding changes > in SELinux, LoadPin, and IMA. > > Signed-off-by: Mimi Zohar > Cc: Jeff Vander Stoep > Cc: Casey Schaufler > Cc: Kees Cook > Acked-by: Jessica Yu > Acked-by: Paul Moore Acked-by: Kees Cook Thanks! -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 6/8] ima: add build time policy
On Fri, Jul 13, 2018 at 11:06 AM, Mimi Zohar wrote: > IMA by default does not measure, appraise or audit files, but can be > enabled at runtime by specifying a builtin policy on the boot command line > or by loading a custom policy. > > This patch defines a build time policy, which verifies kernel modules, > firmware, kexec image, and/or the IMA policy signatures. This build time > policy is automatically enabled at runtime and persists after loading a > custom policy. > > Signed-off-by: Mimi Zohar Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 5/8] ima: based on policy require signed firmware (sysfs fallback)
On Fri, Jul 13, 2018 at 11:06 AM, Mimi Zohar wrote: > With an IMA policy requiring signed firmware, this patch prevents > the sysfs fallback method of loading firmware. > > Signed-off-by: Mimi Zohar > Cc: Luis R. Rodriguez > Cc: Matthew Garrett Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 4/8] firmware: add call to LSM hook before firmware sysfs fallback
On Fri, Jul 13, 2018 at 11:05 AM, Mimi Zohar wrote: > Add an LSM hook prior to allowing firmware sysfs fallback loading. > > Signed-off-by: Mimi Zohar > Acked-by: Luis R. Rodriguez Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 3/8] ima: based on policy require signed kexec kernel images
On Fri, Jul 13, 2018 at 11:05 AM, Mimi Zohar wrote: > The original kexec_load syscall can not verify file signatures, nor can > the kexec image be measured. Based on policy, deny the kexec_load > syscall. > > Signed-off-by: Mimi Zohar > Cc: Eric Biederman > Cc: Kees Cook Reviewed-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 2/8] kexec: add call to LSM hook in original kexec_load syscall
On Fri, Jul 13, 2018 at 11:05 AM, Mimi Zohar wrote: > In order for LSMs and IMA-appraisal to differentiate between kexec_load > and kexec_file_load syscalls, both the original and new syscalls must > call an LSM hook. This patch adds a call to security_kernel_load_data() > in the original kexec_load syscall. > > Signed-off-by: Mimi Zohar > Cc: Eric Biederman > Cc: Kees Cook > Acked-by: Serge Hallyn Acked-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v6 1/8] security: define new LSM hook named security_kernel_load_data
On Fri, Jul 13, 2018 at 11:05 AM, Mimi Zohar wrote: > Differentiate between the kernel reading a file specified by userspace > from the kernel loading a buffer containing data provided by userspace. > This patch defines a new LSM hook named security_kernel_load_data(). > > Signed-off-by: Mimi Zohar > Cc: Eric Biederman > Cc: Luis R. Rodriguez > Cc: Kees Cook > Cc: Casey Schaufler > Acked-by: Serge Hallyn Acked-by: Kees Cook -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
On Fri, Jun 1, 2018 at 12:25 PM, Luis R. Rodriguez wrote:
> On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
>> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
>> > Some systems are memory constrained but they need to load very large
>> > firmwares. The firmware subsystem allows drivers to request this
>> > firmware be loaded from the filesystem, but this requires that the
>> > entire firmware be loaded into kernel memory first before it's provided
>> > to the driver. This can lead to a situation where we map the firmware
>> > twice, once to load the firmware into kernel memory and once to copy the
>> > firmware into the final resting place.
>> >
>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
>> > that allows drivers to request firmware be loaded directly into a
>> > pre-allocated buffer. The QCOM_MDT_LOADER calls dma_alloc_coherent() to
>> > allocate this buffer. According to Documentation/DMA-API.txt,
>> >
>> > Consistent memory is memory for which a write by either the
>> > device or the processor can immediately be read by the processor
>> > or device without having to worry about caching effects. (You
>> > may however need to make sure to flush the processor's write
>> > buffers before telling devices to read that memory.)
>> >
>> > Devices using pre-allocated DMA memory run the risk of the firmware
>> > being accessible by the device prior to the kernel's firmware signature
>> > verification has completed.
>>
>> Indeed. And since its DMA memory we have *no idea* what can happen in
>> terms of consumption of this firmware from hardware, when it would start
>> consuming it in particular.
>>
>> If the device has its own hardware firmware verification mechanism this is
>> completely obscure to us, but it may however suffice certain security
>> policies.
>>
>> The problem here lies in the conflicting security policies of the kernel
>> wanting
>> to not give away firmware until its complete and the current inability to
>> enable
>> us to have platforms suggest they trust hardware won't do something stupid.
>> This becomes an issue since the semantics of the firmware API preallocated
>> buffer do not require currently allow the kernel to inform LSMs of the fact
>> that a buffer is DMA memory or not, and a way for certain platforms then
>> to say that such use is fine for specific devices.
>>
>> Given a pointer can we determine if a piece of memory is DMA or not?
>
> FWIW
>
> Vlastimil suggests page_zone() or virt_to_page() may be able to.
I don't see a PAGEFLAG for DMA, but I do see ZONE_DMA for
page_zone()... So maybe something like
struct page *page;
page = virt_to_page(address);
if (!page)
fail closed...
if (page_zone(page) == ZONE_DMA)
handle dma case...
else
non-dma
But I've CCed Laura and Rik, who I always lean on when I have these
kinds of page questions...
-Kees
--
Kees Cook
Pixel Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
On Tue, Jun 5, 2018 at 2:35 PM, Mimi Zohar wrote:
> On Tue, 2018-06-05 at 12:45 -0700, Kees Cook wrote:
>
>> And if you must have a separate enum, please change this to fail
>> closed instead of open (and mark the fall-through):
>>
>> int rc = -EPERM;
>>
>> switch (id) {
>> case LOADING_MODULE:
>> rc = loadpin_read_file(NULL, READING_MODULE);
>> /* Fall-through */
>> default:
>> break;
>> }
>
> This will fail the sysfs firmware fallback loading and the kexec_load
> syscall without any message, as you have for init_module. Is that
> what you want?
I'd prefer there be a full mapping of the enums so that everything
gets passed into loadpin_read_file() :)
Can the enum be shared or is that nonsensical?
-Kees
--
Kees Cook
Pixel Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
On Thu, May 31, 2018 at 8:23 AM, Mimi Zohar wrote:
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 5fa191252c8f..a9c07bfbc338 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -173,9 +173,24 @@ static int loadpin_read_file(struct file *file, enum
> kernel_read_file_id id)
> return 0;
> }
>
> +static int loadpin_load_data(enum kernel_load_data_id id)
> +{
> + int rc = 0;
> +
> + switch (id) {
> + case LOADING_MODULE:
> + rc = loadpin_read_file(NULL, READING_MODULE);
> + default:
> + break;
> + }
> +
> + return rc;
> +}
Is it worth keeping the same enum between the two hooks? That would
simplify this a bit since it could just pass the id without remapping.
And if you must have a separate enum, please change this to fail
closed instead of open (and mark the fall-through):
int rc = -EPERM;
switch (id) {
case LOADING_MODULE:
rc = loadpin_read_file(NULL, READING_MODULE);
/* Fall-through */
default:
break;
}
Thanks!
-Kees
> +
> static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
> LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> + LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
> };
>
> void __init loadpin_add_hooks(void)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 02ebd1585eaf..475aed9ee2c7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4059,6 +4059,20 @@ static int selinux_kernel_read_file(struct file *file,
> return rc;
> }
>
> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> +{
> + int rc = 0;
> +
> + switch (id) {
> + case LOADING_MODULE:
> + rc = selinux_kernel_module_from_file(NULL);
> + default:
> + break;
> + }
> +
> + return rc;
> +}
> +
> static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
> {
> return avc_has_perm(_state,
> @@ -6950,6 +6964,7 @@ static struct security_hook_list selinux_hooks[]
> __lsm_ro_after_init = {
> LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
> LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> + LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
> LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
> LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> --
> 2.7.5
>
--
Kees Cook
Pixel Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
On Tue, Jun 5, 2018 at 6:25 AM, Serge E. Hallyn wrote: > Quoting Kees Cook (keesc...@chromium.org): >> On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn wrote: >> > Personally I agree with Eric and prefer a new hook. I don't feel strongly >> > enough about it to keep bikeshedding, but since this set already exists, >> > it seems like the way to go. >> >> And the new hook is "load stuff without a file descriptor"? > > Yes. Load stuff based on my own credentials not those attached > to a file. Okay, I can live with that. :) -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn wrote: > Personally I agree with Eric and prefer a new hook. I don't feel strongly > enough about it to keep bikeshedding, but since this set already exists, > it seems like the way to go. And the new hook is "load stuff without a file descriptor"? -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
On Mon, Jun 4, 2018 at 7:03 AM, Mimi Zohar wrote: > On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote: >> Instead of adding the security_kernel_read_file LSM hook - or defining a >> wrapper for security_kernel_read_file LSM hook and adding it, or >> renaming the existing hook to security_kernel_read_data() and adding it >> - in places where the kernel isn't reading a file, this version of the >> patch set defines a new LSM hook named security_kernel_load_data(). >> >> The new LSM hook does not replace the existing security_kernel_read_file >> LSM hook, which is still needed, but defines a new LSM hook allowing >> LSMs and IMA-appraisal the opportunity to fail loading userspace >> provided file/data. >> >> The only difference between the two LSM hooks is the LSM hook name and a >> file descriptor. Whether this is cause enough for requiring a new LSM >> hook, is left to the security community. > > Paul does not have a preference as to adding a new LSM hook or calling > the existing hook. Either way is fine, as long as both the new and > existing hooks call the existing function. > > Casey didn't like the idea of a wrapper. > James suggested renaming the LSM hook. > > The maintainers for the callers of the LSM hook prefer a meaningful > LSM hook name. The "null" argument is not as much of a concern. Only > Eric seems to be asking for a separate, new LSM hook, without the > "null" argument. > > Unless someone really objects, to accommodate Eric we'll define a new > LSM hook named security_kernel_load_data. Eric, are you planning on > Ack'ing patches 1 & 2? I'm sorry I'm late to review this series. Reading through what you have, it seems like the existing hook is fine. If the name has slipped, we can rename it, but I think adding another hook for the same logical action (loading something into the kernel) is confusing. It seems that only patches needed are 2 & 4 (new hook callsites), 5, 6 & 7 (IMA coverage and policy). 1 and 8 seem needless to me. If the objection is that isn't use on non-file objects, sure, rename it. But I don't see a _logical_ difference between the proposed and existing callsites. enum kernel_read_file_id covers the "type" already -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v2] kexec_file: Adjust declaration of kexec_purgatory
Defining kexec_purgatory as a zero-length char array upsets compile
time size checking. Since this is built on a per-arch basis, define
it as an unsized char array (like is done for other similar things,
e.g. linker sections). This silences the warning generated by the future
CONFIG_FORTIFY_SOURCE, which did not like the memcmp() of a "0 byte"
array. This drops the __weak and uses an extern instead, since both
users define kexec_purgatory.
Cc: Daniel Micay <danielmi...@gmail.com>
Signed-off-by: Kees Cook <keesc...@chromium.org>
---
v2: use extern instead of __weak void *; ebiederm
---
kernel/kexec_file.c | 7 ---
kernel/kexec_internal.h | 2 ++
2 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index b118735fea9d..7a147a7add2e 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -26,13 +26,6 @@
#include
#include "kexec_internal.h"
-/*
- * Declare these symbols weak so that if architecture provides a purgatory,
- * these will be overridden.
- */
-char __weak kexec_purgatory[0];
-size_t __weak kexec_purgatory_size = 0;
-
static int kexec_calculate_store_digests(struct kimage *image);
/* Architectures can provide this probe function */
diff --git a/kernel/kexec_internal.h b/kernel/kexec_internal.h
index 799a8a452187..50dfcb039a41 100644
--- a/kernel/kexec_internal.h
+++ b/kernel/kexec_internal.h
@@ -17,6 +17,8 @@ extern struct mutex kexec_mutex;
#ifdef CONFIG_KEXEC_FILE
#include
void kimage_file_post_load_cleanup(struct kimage *image);
+extern char kexec_purgatory[];
+extern size_t kexec_purgatory_size;
#else /* CONFIG_KEXEC_FILE */
static inline void kimage_file_post_load_cleanup(struct kimage *image) { }
#endif /* CONFIG_KEXEC_FILE */
--
2.7.4
--
Kees Cook
Pixel Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] kexec_file: Adjust type of kexec_purgatory
On Tue, May 9, 2017 at 5:15 PM, Eric W. Biederman <ebied...@xmission.com> wrote: > Kees Cook <keesc...@chromium.org> writes: >> kernel/kexec_file.c:33:13: warning: array ‘kexec_purgatory’ assumed to >> have one element >> char __weak kexec_purgatory[]; >> ^~~ > > Nor does "void *kexec_purgatory" as that says at the address known as > kexec_purgatory is a void pointer not a blob a bytes that can be used > for something interesting. > > Better to get rid of the __weak and deal with that fallout. Agreed. This is actually pretty easy, since only x86 and PPC use kexec_file, and both define purgatories. I'll send an updated patch. Thanks! -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] kexec_file: Adjust type of kexec_purgatory
On Tue, May 9, 2017 at 4:13 PM, Daniel Micay <danielmi...@gmail.com> wrote: > On Tue, 2017-05-09 at 16:06 -0700, Kees Cook wrote: >> Defining kexec_purgatory as a zero-length char array upsets compile >> time size checking. Since this is entirely runtime sized, switch >> this to void *. This silences the warning generated by the future >> CONFIG_FORTIFY_SOURCE, which did not like the memcmp() of a "0 byte" >> array. >> >> Cc: Daniel Micay <danielmi...@gmail.com> >> Signed-off-by: Kees Cook <keesc...@chromium.org> >> --- >> kernel/kexec_file.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c >> index b118735fea9d..bc86f85f1329 100644 >> --- a/kernel/kexec_file.c >> +++ b/kernel/kexec_file.c >> @@ -30,7 +30,7 @@ >> * Declare these symbols weak so that if architecture provides a >> purgatory, >> * these will be overridden. >> */ >> -char __weak kexec_purgatory[0]; >> +void * __weak kexec_purgatory; >> size_t __weak kexec_purgatory_size = 0; >> >> static int kexec_calculate_store_digests(struct kimage *image); >> -- >> 2.7.4 > > It seems more correct to use char `char __weak kexec_purgatory[]`, > otherwise isn't __builtin_object_size ending up as 8, which is still > wrong? I tried [], that was my instinct, too, but since this is a __weak and not an extern, that doesn't work: kernel/kexec_file.c:33:13: warning: array ‘kexec_purgatory’ assumed to have one element char __weak kexec_purgatory[]; ^~~ -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
[PATCH] kexec_file: Adjust type of kexec_purgatory
Defining kexec_purgatory as a zero-length char array upsets compile time size checking. Since this is entirely runtime sized, switch this to void *. This silences the warning generated by the future CONFIG_FORTIFY_SOURCE, which did not like the memcmp() of a "0 byte" array. Cc: Daniel Micay <danielmi...@gmail.com> Signed-off-by: Kees Cook <keesc...@chromium.org> --- kernel/kexec_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c index b118735fea9d..bc86f85f1329 100644 --- a/kernel/kexec_file.c +++ b/kernel/kexec_file.c @@ -30,7 +30,7 @@ * Declare these symbols weak so that if architecture provides a purgatory, * these will be overridden. */ -char __weak kexec_purgatory[0]; +void * __weak kexec_purgatory; size_t __weak kexec_purgatory_size = 0; static int kexec_calculate_store_digests(struct kimage *image); -- 2.7.4 -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH] /proc/kcore: Update physical address for kcore ram and text
On Mon, Jan 30, 2017 at 11:00 AM, Pratyush Anand <pan...@redhat.com> wrote: > CCing Andrew and Kees for their review comments. > > > On Wednesday 25 January 2017 10:14 AM, Pratyush Anand wrote: >> Currently all the p_paddr of PT_LOAD headers are assigned to 0, which is >> not true and could be misleading, since 0 is a valid physical address. >> >> User space tools like makedumpfile needs to know physical address for >> PT_LOAD segments of direct mapped regions. Therefore this patch updates >> paddr for such regions. It also sets an invalid paddr (-1) for other >> regions, so that user space tool can know whether a physical address >> provided in PT_LOAD is correct or not. >> >> Signed-off-by: Pratyush Anand <pan...@redhat.com> >> --- >> fs/proc/kcore.c | 5 - >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/fs/proc/kcore.c b/fs/proc/kcore.c >> index 0b80ad87b4d6..ea9f3d1ae830 100644 >> --- a/fs/proc/kcore.c >> +++ b/fs/proc/kcore.c >> @@ -373,7 +373,10 @@ static void elf_kcore_store_hdr(char *bufp, int >> nphdr, int dataoff) >> phdr->p_flags = PF_R|PF_W|PF_X; >> phdr->p_offset = kc_vaddr_to_offset(m->addr) + dataoff; >> phdr->p_vaddr = (size_t)m->addr; >> - phdr->p_paddr = 0; >> + if (m->type == KCORE_RAM || m->type == KCORE_TEXT) >> + phdr->p_paddr = __pa(m->addr); >> + else >> + phdr->p_paddr = (elf_addr_t)-1; >> phdr->p_filesz = phdr->p_memsz = m->size; >> phdr->p_align = PAGE_SIZE; >> } >> Well, CONFIG_PROC_KCORE is a generalized root KASLR exposure (though there are lots of such exposures). Why is the actual physical address needed? Can this just report the virtual address instead? Then the tool can build a map, but it looks like an identity map, rather than creating a new physical/virtual memory ASLR offset exposure? -Kees -- Kees Cook Pixel Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v1] kdump, vmcoreinfo: report memory sections virtual addresses
On Thu, Aug 18, 2016 at 7:41 PM, Baoquan He <b...@redhat.com> wrote:
>
> This makes sense. Makedumpfile need this to parse memory sections.
Yup, good addition.
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
>
> Ack.
>
> Acked-by: Baoquan He <b...@redhat.com>
>
> On 08/18/16 at 07:47am, Thomas Garnier wrote:
>> KASLR memory randomization can randomize the base of the physical memory
>> mapping (PAGE_OFFSET), vmalloc (VMALLOC_START) and vmemmap
>> (VMEMMAP_START). Adding these variables on VMCOREINFO so tools can
>> easily identify the base of each memory section.
>>
>> Signed-off-by: Thomas Garnier <thgar...@google.com>
>> ---
>> Based on next-20160817
>> ---
>> arch/x86/kernel/machine_kexec_64.c | 3 +++
>> include/linux/kexec.h | 6 ++
>> 2 files changed, 9 insertions(+)
>>
>> diff --git a/arch/x86/kernel/machine_kexec_64.c
>> b/arch/x86/kernel/machine_kexec_64.c
>> index fc3389f..b1f15a2 100644
>> --- a/arch/x86/kernel/machine_kexec_64.c
>> +++ b/arch/x86/kernel/machine_kexec_64.c
>> @@ -338,6 +338,9 @@ void arch_crash_save_vmcoreinfo(void)
>> vmcoreinfo_append_str("KERNELOFFSET=%lx\n",
>> kaslr_offset());
>> VMCOREINFO_PHYS_BASE(phys_base);
>> + VMCOREINFO_PAGE_OFFSET(PAGE_OFFSET);
>> + VMCOREINFO_VMALLOC_START(VMALLOC_START);
>> + VMCOREINFO_VMEMMAP_START(VMEMMAP_START);
>> }
>>
>> /* arch-dependent functionality related to kexec file-based syscall */
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index d3ae429..cd3874c 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -261,6 +261,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
>> vmcoreinfo_append_str("CONFIG_%s=y\n", #name)
>> #define VMCOREINFO_PHYS_BASE(value) \
>> vmcoreinfo_append_str("PHYS_BASE=%lx\n", (unsigned long)value)
>
> Could it be better to define only one MACRO like VMCOREINFO_
>> +#define VMCOREINFO_PAGE_OFFSET(value) \
>> + vmcoreinfo_append_str("PAGE_OFFSET=%lx\n", (unsigned long)value)
>> +#define VMCOREINFO_VMALLOC_START(value) \
>> + vmcoreinfo_append_str("VMALLOC_START=%lx\n", (unsigned long)value)
>> +#define VMCOREINFO_VMEMMAP_START(value) \
>> + vmcoreinfo_append_str("VMEMMAP_START=%lx\n", (unsigned long)value)
>>
>> extern struct kimage *kexec_image;
>> extern struct kimage *kexec_crash_image;
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>>
>> ___
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec
--
Kees Cook
Nexus Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec failures with DEBUG_RODATA
On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux <li...@armlinux.org.uk> wrote: > In fact, the apparent confusion over this reinforces my belief that we > should _not_ give the size of the uncompressed image at all. > > The boot environment must be setup such that there is room for the > uncompressed image (aligned currently to 256 bytes) followed by the > size of the compressed image, with any appended DTBs included. > Anything which is located below that is likely to get trampled by > the decompressor. Okay, sounds reasonable to me. :) -Kees -- Kees Cook Chrome OS & Brillo Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: kexec failures with DEBUG_RODATA
On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux
<li...@armlinux.org.uk> wrote:
> On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
>> I'm much less familiar with the ARM decompression stub, but is there a
>> boot image header (like x86 has)? If not, perhaps we can invent one,
>> and it can carry all the details needed for a bootloader to do the
>> right things.
>
> With a bit of tinkering around, I now have this:
>
> <.data>:
>0: e1a0nop ; (mov r0, r0)
>4: e1a0nop ; (mov r0, r0)
>8: e1a0nop ; (mov r0, r0)
>c: e1a0nop ; (mov r0, r0)
> 10: e1a0nop ; (mov r0, r0)
> 14: e1a0nop ; (mov r0, r0)
> 18: e1a0nop ; (mov r0, r0)
> 1c: e1a0nop ; (mov r0, r0)
> 20: ea0fb 0x64
>
> Then follows the existing "header" which we've had there for years:
>
> 24: 016f2818; LE magic number
> 28: ; LE zImage start address (always zero now)
> 2c: 00431fe0; LE zImage _edata
> 30: 04030201; endian flag
>
> And now comes the new header:
>
> 34: 016f2818; LE magic number
Should this be a different magic from the existing header's magic?
> 38: 0001; LE version number (v1)
Should a "size" follow the version number instead of the explicit 64
bytes of zeros?
> 3c: 01287000; LE total space required for decompressor
> 40: 00e54000; LE uncompressed image size
>
> Up to 64 bytes available here for future expansion, currently filled
> with zeros.
> ...
>
> Remainder of the zImage code:
> 64: e10f9000mrs r9, CPSR
>
> I'm rather on the fence whether we need to give the uncompressed image
> size - the important thing is the size of memory that's required for
> the decompressor to run, which is sizeof(uncompressed kernel) rounded
> up to 256 bytes, and the relocated decompressor image size.
I think it's important information since it allows a boot loader to
figure out if there's room in a given range for the result. While
there's no relocation support yet, if we gain it on ARM and we want to
support KASLR, it will be very handy to have the uncompressed size
available.
>
> The "total space required for decompressor" is slightly cheating at the
> figure - I'm including the uncompressed image rounded up and the entire
> compressed image in that size, so it's a safe over-estimate.
>
> I'm not sure there's a need to provide the uncompressed image size, the
> boot environment shouldn't have a reason to know that, so I'm tempted to
> omit it.
>
> We could dispense with the endian conversions, and push the responsibility
> for interpreting that onto the reader of this data: we have the endian
> flag in the existing header block, so the boot environment can work out
> the endianness of the image and apply fixups as appropriate.
>
> Why generate this in the linker script? We need the size of the zImage
> here, which is only known to the linker.
>
> diff --git a/arch/arm/boot/compressed/Makefile
> b/arch/arm/boot/compressed/Makefile
> index d50430c40045..1d5467e05250 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -119,6 +119,10 @@ asflags-y := -DZIMAGE
> KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \
> awk 'END{print $$3}')
> LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
> +
> +KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image)
> +LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE)
> +
> # Supply ZRELADDR to the decompressor via a linker symbol.
> ifneq ($(CONFIG_AUTO_ZRELADDR),y)
> LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index e2e0dcb42ca2..395c60dcc4f7 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -131,11 +131,7 @@ start:
> THUMB(badrr12, 1f )
> THUMB(bx r12 )
>
> - .word _magic_sig @ Magic numbers to help the loader
> - .word _magic_start@ absolute load/run zImage address
> - .word _magic_end @ zImage end address
> - .word
Re: kexec failures with DEBUG_RODATA
On Tue, Jun 14, 2016 at 10:59 AM, Russell King - ARM Linux <li...@armlinux.org.uk> wrote: > Guys, > > Having added Keystone2 support to kexec, and asking TI to validate > linux-next with mainline kexec-tools, I received two reports from > them. > > The first was a report of success, but was kexecing a 4.4 kernel > from linux-next. > > The second was a failure report, kexecing current linux-next from > linux-next on this platform. However, my local tests (using my > 4.7-rc3 derived kernel) showed there to be no problem. > > Building my 4.7-rc3 derived kernel with TI's configuration they > were using with linux-next similarly failed. So, it came down to > a configuration difference. > > After trialling several configurations, it turns out that the > failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled > on TI's kernel but not mine. Why should this make any difference? > > Well, CONFIG_DEBUG_RODATA has the side effect that the kernel > contains a lot of additional padding - we pad out to section size > (1MB) the ELF sections with differing attributes. This should not > normally be a problem, except kexec contains this assumption: > > /* Otherwise, assume the maximum kernel compression ratio > * is 4, and just to be safe, place ramdisk after that */ > initrd_base = base + _ALIGN(len * 4, getpagesize()); > > Now, first things first. Don't get misled by the comment - it's > totally false. That may be what's desired, but that is far from > what actually happens in reality. > > "base" is _not_ the address of the start of the kernel image, but > is the base address of the start of the region that the kernel is > to be loaded into - remember that the kernel is normally loaded > 32k higher than the start of memory. This 32k offset is _not_ > included in either "base" nor "len". So, even if we did want to > assume that there was a maximum compression ratio of 4, the above > always calculates 32k short of that value. > > The other invalid thing here is this whole "maximum kernel compression > ratio" assumption. Consider this non-DEBUG_RODATA kernel image: > >textdata bss dec hex filename > 6583513 2273816 215344 9072673 8a7021 ../build/ks2/vmlinux > > This results in an image and zimage of: > -rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image > -rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage > > which is a ratio of about a 49%. On entry to the decompressor, the > compressed image will be relocated above the expected resulting > kernel size. So, let's say that it's relocated to 9MB. This means > the zImage will occupy around 9MB-14MB above the start of memory. > Going by the 4x ratio, we place the other images at 16.7MB. This > leaves around 2.7MB free. So that's probably fine... but think > about this. We assumed a ratio of 4x, but really we're in a rather > tight squeeze - we actually have only about 50% of the compressed > image size spare. > > Now let's look at the DEBUG_RODATA case: > >textdata bss dec hex filename > 6585305 2273952 215344 9074601 8a77a9 ../build/ks2/vmlinux > > And the resulting sizes: > -rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image > -rwxrwxr-x 1 rmk rmk 4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage > > That's a compression ratio of about 29%. Still within the 4x limit, > but going through the same calculation above shows that we end up > totally overflowing the available space this time. > > That's exactly the same kernel configuration except for > CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the > decompressed image size without affecting the compressed size. > > We've known for some time that this ratio of 4x doesn't work - we > used to use the same assumption in the decompressor when self- > relocating, and we found that there are images which achieve a > better compression ratio and make this invalid. Yet, the 4x thing > has persisted in kexec code... and buggily too. > > Since the kernel now has CONFIG_DEBUG_RODATA by default, this means > that these kinds of ratio-based assumptions are even more invalid > than they have been. > > Right now, a zImage doesn't advertise the size of its uncompressed > image, but I think with things like CONFIG_DEBUG_RODATA, we can no > longer make assumptions like we have done in the past, and we need > the zImage to provide this information so that the boot environment > can be setup sanely by boot loaders/kexec rather than relying on > broken heuristics like this. > > Thoughts? I'm muc
Re: Removal of the kernel code/data/bss resources does break kexec/kdump
On Thu, Apr 14, 2016 at 6:02 PM, Linus Torvalds <torva...@linux-foundation.org> wrote: > On Thu, Apr 14, 2016 at 1:27 PM, Emrah Demir <e...@abdsec.com> wrote: >> On 2016-04-14 13:40, Linus Torvalds wrote: >>> >>> >>> Actually, %pK is horrible in /proc and /sys files, and does the wrong >>> thing. >> >> I agree with that, but for now there is no way to make things right in /proc >> or /sys. > > Well, there is now. > > I've pushed out my attempt at fixing things properly. Please check > that kexec works - and if kexec ends up reading that file as non-root, > I don't know what to say/do. > > Here's the three relevant cases: > >cat /proc/iomem >sudo cat /proc/iomem >sudo cat < /proc/iomem > > and two of them will now show the resource ranges as just plain > zeroes. But yes, it needed extra infrastructure to be able to get this > right. > > Note that while %pK is always wrong in /proc and /sys files, in this > case it would have been particularly wrong, since the values can be > 64-bit even on a 32-bit architecture, so trying to show them as > pointers would have gotten not just the capability handling wrong, it > would have truncated a 64-bit value to 32 bits in that case. Yup, that's why I was saying I was going to try to cook something up for -next. It isn't a trivial change. :) Thanks for fixing it up! -Kees -- Kees Cook Chrome OS & Brillo Security ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 04/22] firmware: simplify dev_*() print messages for generic helpers
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> From: "Luis R. Rodriguez" <mcg...@kernel.org>
>
> Simplify a few of the *generic* shared dev_warn() and dev_dbg()
> print messages for three reasons:
>
> 0) Historically firmware_class code was added to help
>get device driver firmware binaries but these days
>request_firmware*() helpers are being repurposed for
>general *system data* needed by the kernel.
>
> 1) This will also help generalize shared code as much as possible
>later in the future in consideration for a new extensible firmware
>API which will enable to separate usermode helper code out as much
>as possible.
>
> 2) Kees Cook pointed out the the prints already have the device
>associated as dev_*() helpers are used, that should help identify
>the user and case in which the helpers are used. That should provide
>enough context and simplifies the messages further.
>
> v4: generalize debug/warn messages even further as suggested by
> Kees Cook.
>
> Cc: Rusty Russell <ru...@rustcorp.com.au>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Cc: David Howells <dhowe...@redhat.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Casey Schaufler <ca...@schaufler-ca.com>
> Cc: Ming Lei <ming@canonical.com>
> Cc: Takashi Iwai <ti...@suse.de>
> Cc: Vojtěch Pavlík <vojt...@suse.cz>
> Cc: Kyle McMartin <k...@kernel.org>
> Cc: Matthew Garrett <mj...@srcf.ucam.org>
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> drivers/base/firmware_class.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8524450..3358f5d 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -353,15 +353,15 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> rc = fw_read_file_contents(file, buf);
> fput(file);
> if (rc)
> - dev_warn(device, "firmware, attempted to load %s, but
> failed with error %d\n",
> - path, rc);
> + dev_warn(device, "loading %s failed with error %d\n",
> +path, rc);
> else
> break;
> }
> __putname(path);
>
> if (!rc) {
> - dev_dbg(device, "firmware: direct-loading firmware %s\n",
> + dev_dbg(device, "direct-loading %s\n",
> buf->fw_id);
> mutex_lock(_lock);
> set_bit(FW_STATUS_DONE, >status);
> @@ -1051,7 +1051,7 @@ _request_firmware_prepare(struct firmware **firmware_p,
> const char *name,
> }
>
> if (fw_get_builtin_firmware(firmware, name)) {
> - dev_dbg(device, "firmware: using built-in firmware %s\n",
> name);
> + dev_dbg(device, "using built-in %s\n", name);
> return 0; /* assigned */
> }
>
> --
> 2.1.0
>
--
Kees Cook
Chrome OS & Brillo Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
[PATCH v3.1] firmware: clean up filesystem load exit path
This makes the error and success paths more readable while trying to
load firmware from the filesystem.
Signed-off-by: Kees Cook <keesc...@chromium.org>
Cc: Josh Boyer <jwbo...@fedoraproject.org>
Cc: David Howells <dhowe...@redhat.com>
Cc: Luis R. Rodriguez <mcg...@kernel.org>
Cc: Mimi Zohar <zo...@linux.vnet.ibm.com>
---
Suggested as an alternative to "[PATCH v3 06/22] firmware: fold successful fw
read early"
---
drivers/base/firmware_class.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 62e052b50aa9..484afbd783f8 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -352,19 +352,17 @@ static int fw_get_filesystem_firmware(struct device
*device,
continue;
rc = fw_read_file_contents(file, buf);
fput(file);
- if (rc)
+ if (rc) {
dev_warn(device, "loading %s failed with error %d\n",
path, rc);
- else
- break;
- }
- __putname(path);
-
- if (!rc) {
+ continue;
+ }
dev_dbg(device, "direct-loading %s\n",
buf->fw_id);
fw_finish_direct_load(device, buf);
+ break;
}
+ __putname(path);
return rc;
}
--
2.6.3
--
Kees Cook
Chrome OS & Brillo Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 12/22] vfs: define kernel_read_file_from_path
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> This patch defines kernel_read_file_from_path(), a wrapper for the VFS
> common kernel_read_file().
>
> Changelog:
> - Separated from the IMA patch
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> fs/exec.c | 22 ++
> include/linux/fs.h | 2 ++
> 2 files changed, 24 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index cd2b5b2..5629958 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -884,6 +884,28 @@ out:
> }
> EXPORT_SYMBOL_GPL(kernel_read_file);
>
> +int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> + loff_t max_size, enum kernel_read_file_id id)
> +{
> + struct file *file;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + file = filp_open(path, O_RDONLY, 0);
> + if (IS_ERR(file)) {
> + ret = PTR_ERR(file);
> + pr_err("Unable to open file: %s (%d)", path, ret);
> + return ret;
> + }
> +
> + ret = kernel_read_file(file, buf, size, max_size, id);
> + fput(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> +
> ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t
> len)
> {
> ssize_t res = vfs_read(file, (void __user *)addr, len, );
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1458ca5..962c491 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2533,6 +2533,8 @@ enum kernel_read_file_id {
> extern int kernel_read(struct file *, loff_t, char *, unsigned long);
> extern int kernel_read_file(struct file *, void **, loff_t *, loff_t,
> enum kernel_read_file_id);
> +extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> + enum kernel_read_file_id);
> extern ssize_t kernel_write(struct file *, const char *, size_t, loff_t);
> extern ssize_t __kernel_write(struct file *, const char *, size_t, loff_t *);
> extern struct file * open_exec(const char *);
> --
> 2.1.0
>
--
Kees Cook
Chrome OS & Brillo Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 13/22] firmware: replace call to fw_read_file_contents() with kernel version
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> Replace the fw_read_file_contents with kernel_file_read_from_path().
>
> Although none of the upstreamed LSMs define a kernel_fw_from_file hook,
> IMA is called by the security function to prevent unsigned firmware from
> being loaded and to measure/appraise signed firmware, based on policy.
>
> Instead of reading the firmware twice, once for measuring/appraising the
> firmware and again for reading the firmware contents into memory, the
> kernel_post_read_file() security hook calculates the file hash based on
> the in memory file buffer. The firmware is read once.
>
> This patch removes the LSM kernel_fw_from_file() hook and security call.
>
> Changelog v3:
> - remove kernel_fw_from_file hook
> - use kernel_file_read_from_path() - requested by Luis
> v2:
> - reordered and squashed firmware patches
> - fix MAX firmware size (Kees Cook)
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> drivers/base/firmware_class.c | 48
> +++
> include/linux/fs.h| 1 +
> include/linux/ima.h | 6 -
> include/linux/lsm_hooks.h | 11 -
> include/linux/security.h | 7 --
> security/integrity/ima/ima_main.c | 21 -
> security/security.c | 13 ---
> 7 files changed, 19 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index c658cec..e06a3d8 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -291,37 +291,6 @@ 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 void fw_finish_direct_load(struct device *device,
> struct firmware_buf *buf)
> {
> @@ -334,6 +303,7 @@ static void fw_finish_direct_load(struct device *device,
> static int fw_get_filesystem_firmware(struct device *device,
>struct firmware_buf *buf)
> {
> + loff_t size;
> int i, len;
> int rc = -ENOENT;
> char *path;
> @@ -343,8 +313,6 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> return -ENOMEM;
>
> for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> - struct file *file;
> -
> /* skip the unset customized path */
> if (!fw_path[i][0])
> continue;
> @@ -356,12 +324,11 @@ static int fw_get_filesystem_firmware(struct device
> *device,
> break;
> }
>
> - file = filp_open(path, O_RDONLY, 0);
> - if (IS_ERR(file))
> - continue;
> - rc = fw_read_file_contents(file, buf);
> - fput(file);
> + buf->size = 0;
> + rc = kernel_read_file_from_path(path, >data, ,
> + INT_MAX, READING_FIRMWARE);
> if (rc == 0) {
> + buf->size = (size_t) size;
> dev_dbg(device, "direct-loading %s\n",
> buf->fw_id);
> fw_finish_direct_load(device, buf);
> @@ -689,8 +656,9 @@ static ssize_t firmware_loading_store(struct device *dev,
> dev_err(dev, "%s: map pages failed\n",
>
Re: [PATCH v3 14/22] security: define kernel_read_file hook
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> The kernel_read_file security hook is called prior to reading the file
> into memory.
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> fs/exec.c | 4
> include/linux/ima.h | 6 ++
> include/linux/lsm_hooks.h | 8
> include/linux/security.h | 7 +++
> security/integrity/ima/ima_main.c | 16
> security/security.c | 12
> 6 files changed, 53 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 5629958..1d39c4e 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -842,6 +842,10 @@ int kernel_read_file(struct file *file, void **buf,
> loff_t *size,
> if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> return -EINVAL;
>
> + ret = security_kernel_read_file(file, id);
> + if (ret)
> + return ret;
> +
> i_size = i_size_read(file_inode(file));
> if (max_size > 0 && i_size > max_size)
> return -EFBIG;
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 7aea486..6adcaea 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -19,6 +19,7 @@ extern int ima_file_check(struct file *file, int mask, int
> opened);
> extern void ima_file_free(struct file *file);
> extern int ima_file_mmap(struct file *file, unsigned long prot);
> extern int ima_module_check(struct file *file);
> +extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
> extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
> enum kernel_read_file_id id);
>
> @@ -48,6 +49,11 @@ static inline int ima_module_check(struct file *file)
> return 0;
> }
>
> +static inline int ima_read_file(struct file *file, enum kernel_read_file_id
> id)
> +{
> + return 0;
> +}
> +
> static inline int ima_post_read_file(struct file *file, void *buf, loff_t
> size,
> enum kernel_read_file_id id)
> {
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7d04a12..d32b7bd 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -552,6 +552,12 @@
> * the kernel module to load. If the module is being loaded from a blob,
> * this argument will be NULL.
> * Return 0 if permission is granted.
> + * @kernel_read_file:
> + * Read a file specified by userspace.
> + * @file contains the file structure pointing to the file being read
> + * by the kernel.
> + * @id kernel read file identifier
> + * Return 0 if permission is granted.
> * @kernel_post_read_file:
> * Read a file specified by userspace.
> * @file contains the file structure pointing to the file being read
> @@ -1455,6 +1461,7 @@ union security_list_options {
> int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
> int (*kernel_module_request)(char *kmod_name);
> int (*kernel_module_from_file)(struct file *file);
> + int (*kernel_read_file)(struct file *file, enum kernel_read_file_id
> id);
> int (*kernel_post_read_file)(struct file *file, char *buf, loff_t
> size,
> enum kernel_read_file_id id);
> int (*task_fix_setuid)(struct cred *new, const struct cred *old,
> @@ -1715,6 +1722,7 @@ struct security_hook_heads {
> struct list_head cred_transfer;
> struct list_head kernel_act_as;
> struct list_head kernel_create_files_as;
> + struct list_head kernel_read_file;
> struct list_head kernel_post_read_file;
> struct list_head kernel_module_request;
> struct list_head kernel_module_from_file;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index cee1349..071fb74 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -302,6 +302,7 @@ int security_kernel_act_as(struct cred *new, u32 secid);
> int security_kernel_create_files_as(struct cred *new, struct inode *inode);
> int security_kernel_module_request(char *kmod_name);
> int security_kernel_module_from_file(struct file *file);
> +int security_kernel_read_file(struct file *file, enum kernel_read_file_id
> id);
> int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>enum kernel_read_file_id id);
> int security_task_fix_setuid(struct cred *new, const struct cred *old,
> @@ -86
Re: [PATCH v3 15/22] vfs: define kernel_copy_file_from_fd()
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> This patch defines kernel_read_file_from_fd(), a wrapper for the VFS
> common kernel_read_file().
>
> Changelog:
> - Separated from the kernel modules patch
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> fs/exec.c | 16
> include/linux/fs.h | 2 ++
> 2 files changed, 18 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 1d39c4e..f3a0ce2 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -910,6 +910,22 @@ int kernel_read_file_from_path(char *path, void **buf,
> loff_t *size,
> }
> EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
>
> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t
> max_size,
> +enum kernel_read_file_id id)
> +{
> + struct fd f = fdget(fd);
> + int ret = -EBADF;
> +
> + if (!f.file)
> + goto out;
> +
> + ret = kernel_read_file(f.file, buf, size, max_size, id);
> +out:
> + fdput(f);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
> +
> ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t
> len)
> {
> ssize_t res = vfs_read(file, (void __user *)addr, len, );
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 2a9670a..5ba806b 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2536,6 +2536,8 @@ extern int kernel_read_file(struct file *, void **,
> loff_t *, loff_t,
> enum kernel_read_file_id);
> extern int kernel_read_file_from_path(char *, void **, loff_t *, loff_t,
> enum kernel_read_file_id);
> +extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
> + enum kernel_read_file_id);
> extern ssize_t kernel_write(struct file *, const char *, size_t, loff_t);
> extern ssize_t __kernel_write(struct file *, const char *, size_t, loff_t *);
> extern struct file * open_exec(const char *);
> --
> 2.1.0
>
--
Kees Cook
Chrome OS & Brillo Security
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v3 07/22] vfs: define a generic function to read a file from the kernel
On Wed, Feb 3, 2016 at 11:06 AM, Mimi Zohar <zo...@linux.vnet.ibm.com> wrote:
> For a while it was looked down upon to directly read files from Linux.
> These days there exists a few mechanisms in the kernel that do just
> this though to load a file into a local buffer. There are minor but
> important checks differences on each. This patch set is the first
> attempt at resolving some of these differences.
>
> This patch introduces a common function for reading files from the kernel
> with the corresponding security post-read hook and function.
>
> Changelog v3:
> - additional bounds checking - Luis
> v2:
> - To simplify patch review, re-ordered patches
>
> Signed-off-by: Mimi Zohar <zo...@linux.vnet.ibm.com>
> Reviewed-by: Luis R. Rodriguez <mcg...@suse.com>
Acked-by: Kees Cook <keesc...@chromium.org>
-Kees
> ---
> fs/exec.c | 53
> +++
> include/linux/fs.h| 1 +
> include/linux/lsm_hooks.h | 9
> include/linux/security.h | 7 +++
> security/security.c | 7 +++
> 5 files changed, 77 insertions(+)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index b06623a..742de7a 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -56,6 +56,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -831,6 +832,58 @@ int kernel_read(struct file *file, loff_t offset,
>
> EXPORT_SYMBOL(kernel_read);
>
> +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> +loff_t max_size)
> +{
> + loff_t i_size, pos;
> + ssize_t bytes = 0;
> + int ret;
> +
> + if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> + return -EINVAL;
> +
> + i_size = i_size_read(file_inode(file));
> + if (max_size > 0 && i_size > max_size)
> + return -EFBIG;
> + if (i_size <= 0)
> + return -EINVAL;
> +
> + *buf = vmalloc(i_size);
> + if (!*buf)
> + return -ENOMEM;
> +
> + pos = 0;
> + while (pos < i_size) {
> + bytes = kernel_read(file, pos, (char *)(*buf) + pos,
> + i_size - pos);
> + if (bytes < 0) {
> + ret = bytes;
> + goto out;
> + }
> +
> + if (bytes == 0)
> + break;
> + pos += bytes;
> + }
> +
> + if (pos != i_size) {
> + ret = -EIO;
> + goto out;
> + }
> +
> + ret = security_kernel_post_read_file(file, *buf, i_size);
> + if (!ret)
> + *size = pos;
> +
> +out:
> + if (ret < 0) {
> + vfree(*buf);
> + *buf = NULL;
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file);
> +
> ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t
> len)
> {
> ssize_t res = vfs_read(file, (void __user *)addr, len, );
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3aa5142..93ca379 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2527,6 +2527,7 @@ static inline void i_readcount_inc(struct inode *inode)
> extern int do_pipe_flags(int *, int);
>
> extern int kernel_read(struct file *, loff_t, char *, unsigned long);
> +extern int kernel_read_file(struct file *, void **, loff_t *, loff_t);
> extern ssize_t kernel_write(struct file *, const char *, size_t, loff_t);
> extern ssize_t __kernel_write(struct file *, const char *, size_t, loff_t *);
> extern struct file * open_exec(const char *);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 71969de..f82631c 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -561,6 +561,13 @@
> * the kernel module to load. If the module is being loaded from a blob,
> * this argument will be NULL.
> * Return 0 if permission is granted.
> + * @kernel_post_read_file:
> + * Read a file specified by userspace.
> + * @file contains the file structure pointing to the file being read
> + * by the kernel.
> + * @buf pointer to buffer containing the file contents.
> + * @size length of the file contents.
> + * Return 0 if permission is granted.
> * @task_fix_setuid:
> * Update the module's state after setting one or more of the user
> * identity attributes of the current process. The @flags parameter
> @@ -1457,6 +1464,7 @@ union security_list_options {
> i
