Re: [RFC v6 13/62] powerpc: track allocation status of all pkeys
Hello Ram, I'm still going through the patches and haven't formed a full picture of the feature in my mind yet, so my comments today won't be particularly insightful... But hopefully the comments that I currently have will be helpful anyway. Ram Pai writes: > diff --git a/arch/powerpc/include/asm/pkeys.h > b/arch/powerpc/include/asm/pkeys.h > index 203d7de..09b268e 100644 > --- a/arch/powerpc/include/asm/pkeys.h > +++ b/arch/powerpc/include/asm/pkeys.h > @@ -2,21 +2,87 @@ > #define _ASM_PPC64_PKEYS_H > > extern bool pkey_inited; > -#define ARCH_VM_PKEY_FLAGS 0 > +#define arch_max_pkey() 32 > +#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > + VM_PKEY_BIT3 | VM_PKEY_BIT4) > +/* > + * Bits are in BE format. > + * NOTE: key 31, 1, 0 are not used. > + * key 0 is used by default. It give read/write/execute permission. > + * key 31 is reserved by the hypervisor. > + * key 1 is recommended to be not used. > + * PowerISA(3.0) page 1015, programming note. > + */ > +#define PKEY_INITIAL_ALLOCAION 0xc0000001 There's a typo in the macro name, should be "ALLOCATION". -- Thiago Jung Bauermann IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 17/62] powerpc: implementation for arch_set_user_pkey_access()
Ram Pai writes:
> @@ -113,10 +117,14 @@ static inline int arch_override_mprotect_pkey(struct
> vm_area_struct *vma,
> return 0;
> }
>
> +extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
> + unsigned long init_val);
> static inline int arch_set_user_pkey_access(struct task_struct *tsk, int
> pkey,
> unsigned long init_val)
> {
> - return 0;
> + if (!pkey_inited)
> + return -1;
> + return __arch_set_user_pkey_access(tsk, pkey, init_val);
> }
If non-zero, the return value of this function will be passed to
userspace by the pkey_alloc syscall. Shouldn't it be returning an errno
macro such as -EPERM?
Also, why are there both arch_set_user_pkey_access and
__arch_set_user_pkey_access? Is it a speed optimization so that the
early return is inlined into the caller? Ditto for execute_only_pkey
and __arch_override_mprotect_pkey.
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys
Ram Pai writes:
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -2,6 +2,18 @@
> #define _ASM_PPC64_PKEYS_H
>
> extern bool pkey_inited;
> +/* override any generic PKEY Permission defines */
> +#undef PKEY_DISABLE_ACCESS
> +#define PKEY_DISABLE_ACCESS0x1
> +#undef PKEY_DISABLE_WRITE
> +#define PKEY_DISABLE_WRITE 0x2
> +#undef PKEY_DISABLE_EXECUTE
> +#define PKEY_DISABLE_EXECUTE 0x4
> +#undef PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
> + PKEY_DISABLE_WRITE |\
> + PKEY_DISABLE_EXECUTE)
> +
Is it ok to #undef macros from another header? Especially since said
header is in uapi (include/uapi/asm-generic/mman-common.h).
Also, it's unnecessary to undef the _ACCESS and _WRITE macros since they
are identical to the original definition. And since these macros are
originally defined in an uapi header, the powerpc-specific ones should
be in an uapi header as well, if I understand it correctly.
An alternative solution is to define only PKEY_DISABLE_EXECUTE in
arch/powerpc/include/uapi/asm/mman.h and then test for its existence to
properly define PKEY_ACCESS_MASK in
include/uapi/asm-generic/mman-common.h. What do you think of the code
below?
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e31f5ee8e81f..67e6a3a343ae 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -4,17 +4,6 @@
#include
extern bool pkey_inited;
-/* override any generic PKEY Permission defines */
-#undef PKEY_DISABLE_ACCESS
-#define PKEY_DISABLE_ACCESS0x1
-#undef PKEY_DISABLE_WRITE
-#define PKEY_DISABLE_WRITE 0x2
-#undef PKEY_DISABLE_EXECUTE
-#define PKEY_DISABLE_EXECUTE 0x4
-#undef PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
- PKEY_DISABLE_WRITE |\
- PKEY_DISABLE_EXECUTE)
#define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
VM_PKEY_BIT3 | VM_PKEY_BIT4)
diff --git a/arch/powerpc/include/uapi/asm/mman.h
b/arch/powerpc/include/uapi/asm/mman.h
index ab45cc2f3101..dee43feb7c53 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -45,4 +45,6 @@
#define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT) /* 1GB HugeTLB Page */
#define MAP_HUGE_16GB (34 << MAP_HUGE_SHIFT) /* 16GB HugeTLB Page */
+#define PKEY_DISABLE_EXECUTE 0x4
+
#endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 72eb9a1bde79..777f8f8dff47 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -12,7 +12,7 @@
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
* more details.
*/
-#include
+#include
#include /* PKEY_* */
bool pkey_inited;
diff --git a/include/uapi/asm-generic/mman-common.h
b/include/uapi/asm-generic/mman-common.h
index 8c27db0c5c08..93e3841d9ada 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -74,7 +74,15 @@
#define PKEY_DISABLE_ACCESS0x1
#define PKEY_DISABLE_WRITE 0x2
+
+/* The arch-specific code may define PKEY_DISABLE_EXECUTE */
+#ifdef PKEY_DISABLE_EXECUTE
+#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS | \
+ PKEY_DISABLE_WRITE | \
+ PKEY_DISABLE_EXECUTE)
+#else
#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\
PKEY_DISABLE_WRITE)
+#endif
#endif /* __ASM_GENERIC_MMAN_COMMON_H */
> diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
> index 98d0391..b9ad98d 100644
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -73,6 +73,7 @@ int __arch_set_user_pkey_access(struct task_struct *tsk,
> int pkey,
> unsigned long init_val)
> {
> u64 new_amr_bits = 0x0ul;
> + u64 new_iamr_bits = 0x0ul;
>
> if (!is_pkey_enabled(pkey))
> return -1;
> @@ -85,5 +86,14 @@ int __arch_set_user_pkey_access(struct task_struct *tsk,
> int pkey,
>
> init_amr(pkey, new_amr_bits);
>
> + /*
> + * By default execute is disabled.
> + * To enable execute, PKEY_ENABLE_EXECUTE
> + * needs to be specified.
> + */
> + if ((init_val & PKEY_DISABLE_EXECUTE))
> + new_iamr_bits |= IAMR_EX_BIT;
> +
> + init_iamr(pkey, new_iamr_bits);
> return 0;
> }
The comment seems to be from an earlier version which has the logic
inverted, and there is no PKEY_ENABLE_EXECUTE. Should the comment be
updated to the following?
By default execute is enabled.
To disable execute, PKEY_DISABLE
Re: [RFC v6 19/62] powerpc: ability to create execute-disabled pkeys
Thiago Jung Bauermann writes: > diff --git a/arch/powerpc/include/asm/pkeys.h > b/arch/powerpc/include/asm/pkeys.h > index e31f5ee8e81f..67e6a3a343ae 100644 > --- a/arch/powerpc/include/asm/pkeys.h > +++ b/arch/powerpc/include/asm/pkeys.h > @@ -4,17 +4,6 @@ > #include > > extern bool pkey_inited; > -/* override any generic PKEY Permission defines */ > -#undef PKEY_DISABLE_ACCESS > -#define PKEY_DISABLE_ACCESS0x1 > -#undef PKEY_DISABLE_WRITE > -#define PKEY_DISABLE_WRITE 0x2 > -#undef PKEY_DISABLE_EXECUTE > -#define PKEY_DISABLE_EXECUTE 0x4 > -#undef PKEY_ACCESS_MASK > -#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ > - PKEY_DISABLE_WRITE |\ > - PKEY_DISABLE_EXECUTE) > > #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \ > VM_PKEY_BIT3 | VM_PKEY_BIT4) > diff --git a/arch/powerpc/include/uapi/asm/mman.h > b/arch/powerpc/include/uapi/asm/mman.h > index ab45cc2f3101..dee43feb7c53 100644 > --- a/arch/powerpc/include/uapi/asm/mman.h > +++ b/arch/powerpc/include/uapi/asm/mman.h > @@ -45,4 +45,6 @@ > #define MAP_HUGE_1GB (30 << MAP_HUGE_SHIFT) /* 1GB HugeTLB Page */ > #define MAP_HUGE_16GB(34 << MAP_HUGE_SHIFT) /* 16GB HugeTLB Page */ > > +#define PKEY_DISABLE_EXECUTE 0x4 > + > #endif /* _UAPI_ASM_POWERPC_MMAN_H */ > diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c > index 72eb9a1bde79..777f8f8dff47 100644 > --- a/arch/powerpc/mm/pkeys.c > +++ b/arch/powerpc/mm/pkeys.c > @@ -12,7 +12,7 @@ > * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for > * more details. > */ > -#include > +#include > #include /* PKEY_* */ > > bool pkey_inited; > diff --git a/include/uapi/asm-generic/mman-common.h > b/include/uapi/asm-generic/mman-common.h > index 8c27db0c5c08..93e3841d9ada 100644 > --- a/include/uapi/asm-generic/mman-common.h > +++ b/include/uapi/asm-generic/mman-common.h > @@ -74,7 +74,15 @@ > > #define PKEY_DISABLE_ACCESS 0x1 > #define PKEY_DISABLE_WRITE 0x2 > + > +/* The arch-specific code may define PKEY_DISABLE_EXECUTE */ > +#ifdef PKEY_DISABLE_EXECUTE > +#define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ > + PKEY_DISABLE_WRITE | \ > + PKEY_DISABLE_EXECUTE) > +#else > #define PKEY_ACCESS_MASK (PKEY_DISABLE_ACCESS |\ >PKEY_DISABLE_WRITE) > +#endif > > #endif /* __ASM_GENERIC_MMAN_COMMON_H */ Actually, I just noticed that arch/powerpc/include/uapi/asm/mman.h includes , so for the #ifdef above to work the former has to #define PKEY_DISABLE_EXECUTE before including the latter. -- Thiago Jung Bauermann IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 20/62] powerpc: store and restore the pkey state across context switches
Ram Pai writes: > Store and restore the AMR, IAMR and UMOR register state of the task > before scheduling out and after scheduling in, respectively. > > Signed-off-by: Ram Pai s/UMOR/UAMOR/ > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index 2ad725e..9429361 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -1096,6 +1096,11 @@ static inline void save_sprs(struct thread_struct *t) > t->tar = mfspr(SPRN_TAR); > } > #endif > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > + t->amr = mfspr(SPRN_AMR); > + t->iamr = mfspr(SPRN_IAMR); > + t->uamor = mfspr(SPRN_UAMOR); > +#endif > } > > static inline void restore_sprs(struct thread_struct *old_thread, > @@ -1131,6 +1136,14 @@ static inline void restore_sprs(struct thread_struct > *old_thread, > mtspr(SPRN_TAR, new_thread->tar); > } > #endif > +#ifdef CONFIG_PPC64_MEMORY_PROTECTION_KEYS > + if (old_thread->amr != new_thread->amr) > + mtspr(SPRN_AMR, new_thread->amr); > + if (old_thread->iamr != new_thread->iamr) > + mtspr(SPRN_IAMR, new_thread->iamr); > + if (old_thread->uamor != new_thread->uamor) > + mtspr(SPRN_UAMOR, new_thread->uamor); > +#endif > } Shouldn't the saving and restoring of the SPRs be guarded by a check for whether memory protection keys are enabled? What happens when trying to access these registers on a CPU which doesn't have them? -- Thiago Jung Bauermann IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 15/62] powerpc: helper functions to initialize AMR, IAMR and UMOR registers
Ram Pai writes:
> Introduce helper functions that can initialize the bits in the AMR,
> IAMR and UMOR register; the bits that correspond to the given pkey.
>
> Signed-off-by: Ram Pai
s/UMOR/UAMOR/ here and in the subject as well.
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -16,3 +16,47 @@
> #include /* PKEY_* */
>
> bool pkey_inited;
> +#define pkeyshift(pkey) ((arch_max_pkey()-pkey-1) * AMR_BITS_PER_PKEY)
> +
> +static inline void init_amr(int pkey, u8 init_bits)
> +{
> + u64 new_amr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> + u64 old_amr = read_amr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +
> + write_amr(old_amr | new_amr_bits);
> +}
> +
> +static inline void init_iamr(int pkey, u8 init_bits)
> +{
> + u64 new_iamr_bits = (((u64)init_bits & 0x3UL) << pkeyshift(pkey));
> + u64 old_iamr = read_iamr() & ~((u64)(0x3ul) << pkeyshift(pkey));
> +
> + write_amr(old_iamr | new_iamr_bits);
> +}
init_iamr should call write_iamr, not write_amr.
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 27/62] powerpc: helper to validate key-access permissions of a pte
Ram Pai writes:
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -201,3 +201,36 @@ int __arch_override_mprotect_pkey(struct vm_area_struct
> *vma, int prot,
>*/
> return vma_pkey(vma);
> }
> +
> +static bool pkey_access_permitted(int pkey, bool write, bool execute)
> +{
> + int pkey_shift;
> + u64 amr;
> +
> + if (!pkey)
> + return true;
> +
> + pkey_shift = pkeyshift(pkey);
> + if (!(read_uamor() & (0x3UL << pkey_shift)))
> + return true;
> +
> + if (execute && !(read_iamr() & (IAMR_EX_BIT << pkey_shift)))
> + return true;
> +
> + if (!write) {
> + amr = read_amr();
> + if (!(amr & (AMR_RD_BIT << pkey_shift)))
> + return true;
> + }
> +
> + amr = read_amr(); /* delay reading amr uptil absolutely needed */
Actually, this is causing amr to be read twice in case control enters
the "if (!write)" block above but doesn't enter the other if block nested
in it.
read_amr should be called only once, right before "if (!write)".
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 21/62] powerpc: introduce execute-only pkey
Ram Pai writes:
> --- a/arch/powerpc/mm/pkeys.c
> +++ b/arch/powerpc/mm/pkeys.c
> @@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct *tsk,
> int pkey,
> init_iamr(pkey, new_iamr_bits);
> return 0;
> }
> +
> +static inline bool pkey_allows_readwrite(int pkey)
> +{
> + int pkey_shift = pkeyshift(pkey);
> +
> + if (!(read_uamor() & (0x3UL << pkey_shift)))
> + return true;
> +
> + return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
> +}
> +
> +int __execute_only_pkey(struct mm_struct *mm)
> +{
> + bool need_to_set_mm_pkey = false;
> + int execute_only_pkey = mm->context.execute_only_pkey;
> + int ret;
> +
> + /* Do we need to assign a pkey for mm's execute-only maps? */
> + if (execute_only_pkey == -1) {
> + /* Go allocate one to use, which might fail */
> + execute_only_pkey = mm_pkey_alloc(mm);
> + if (execute_only_pkey < 0)
> + return -1;
> + need_to_set_mm_pkey = true;
> + }
> +
> + /*
> + * We do not want to go through the relatively costly
> + * dance to set AMR if we do not need to. Check it
> + * first and assume that if the execute-only pkey is
> + * readwrite-disabled than we do not have to set it
> + * ourselves.
> + */
> + if (!need_to_set_mm_pkey &&
> + !pkey_allows_readwrite(execute_only_pkey))
> + return execute_only_pkey;
> +
> + /*
> + * Set up AMR so that it denies access for everything
> + * other than execution.
> + */
> + ret = __arch_set_user_pkey_access(current, execute_only_pkey,
> + (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> + /*
> + * If the AMR-set operation failed somehow, just return
> + * 0 and effectively disable execute-only support.
> + */
> + if (ret) {
> + mm_set_pkey_free(mm, execute_only_pkey);
> + return -1;
> + }
> +
> + /* We got one, store it and use it from here on out */
> + if (need_to_set_mm_pkey)
> + mm->context.execute_only_pkey = execute_only_pkey;
> + return execute_only_pkey;
> +}
If you follow the code flow in __execute_only_pkey, the AMR and UAMOR
are read 3 times in total, and AMR is written twice. IAMR is read and
written twice. Since they are SPRs and access to them is slow (or isn't
it?), is it worth it to read them once in __execute_only_pkey and pass
down their values to the callees, and then write them once at the end of
the function?
This function is used both by the mmap syscall and the mprotect syscall
(but not by pkey_mprotect) if the requested protection is execute-only.
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 21/62] powerpc: introduce execute-only pkey
Ram Pai writes:
> On Fri, Jul 28, 2017 at 07:17:13PM -0300, Thiago Jung Bauermann wrote:
>>
>> Ram Pai writes:
>> > --- a/arch/powerpc/mm/pkeys.c
>> > +++ b/arch/powerpc/mm/pkeys.c
>> > @@ -97,3 +97,60 @@ int __arch_set_user_pkey_access(struct task_struct
>> > *tsk, int pkey,
>> >init_iamr(pkey, new_iamr_bits);
>> >return 0;
>> > }
>> > +
>> > +static inline bool pkey_allows_readwrite(int pkey)
>> > +{
>> > + int pkey_shift = pkeyshift(pkey);
>> > +
>> > + if (!(read_uamor() & (0x3UL << pkey_shift)))
>> > + return true;
>> > +
>> > + return !(read_amr() & ((AMR_RD_BIT|AMR_WR_BIT) << pkey_shift));
>> > +}
>> > +
>> > +int __execute_only_pkey(struct mm_struct *mm)
>> > +{
>> > + bool need_to_set_mm_pkey = false;
>> > + int execute_only_pkey = mm->context.execute_only_pkey;
>> > + int ret;
>> > +
>> > + /* Do we need to assign a pkey for mm's execute-only maps? */
>> > + if (execute_only_pkey == -1) {
>> > + /* Go allocate one to use, which might fail */
>> > + execute_only_pkey = mm_pkey_alloc(mm);
>> > + if (execute_only_pkey < 0)
>> > + return -1;
>> > + need_to_set_mm_pkey = true;
>> > + }
>> > +
>> > + /*
>> > + * We do not want to go through the relatively costly
>> > + * dance to set AMR if we do not need to. Check it
>> > + * first and assume that if the execute-only pkey is
>> > + * readwrite-disabled than we do not have to set it
>> > + * ourselves.
>> > + */
>> > + if (!need_to_set_mm_pkey &&
>> > + !pkey_allows_readwrite(execute_only_pkey))
> ^
> Here uamor and amr is read once each.
You are right. What confused me was that the call to mm_pkey_alloc above
also reads uamor and amr (and also iamr, and writes to all of those) but
if that function is called, then need_to_set_mm_pkey is true and
pkey_allows_readwrite won't be called.
>> > + return execute_only_pkey;
>> > +
>> > + /*
>> > + * Set up AMR so that it denies access for everything
>> > + * other than execution.
>> > + */
>> > + ret = __arch_set_user_pkey_access(current, execute_only_pkey,
>> > + (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
> ^^^
> here amr and iamr are written once each if the
> the function returns successfully.
__arch_set_user_pkey_access also reads uamor for the second time in its
call to is_pkey_enabled, and reads amr for the second time as well in
its calls to init_amr. The first reads are in either
pkey_allows_readwrite or pkey_status_change (called from
__arch_activate_pkey).
If need_to_set_mm_pkey is true, then the iamr read in init_iamr is the
2nd one during __execute_only_pkey's execution. In this case the writes
to amr and iamr will be the 2nd ones as well. The first reads and writes
are in pkey_status_change.
>> > + /*
>> > + * If the AMR-set operation failed somehow, just return
>> > + * 0 and effectively disable execute-only support.
>> > + */
>> > + if (ret) {
>> > + mm_set_pkey_free(mm, execute_only_pkey);
> ^^^
> here only if __arch_set_user_pkey_access() fails
> amr and iamr and uamor will be written once each.
I assume the error case isn't perfomance sensitive and didn't account
for mm_set_pkey_free in my analysis.
>> > + return -1;
>> > + }
>> > +
>> > + /* We got one, store it and use it from here on out */
>> > + if (need_to_set_mm_pkey)
>> > + mm->context.execute_only_pkey = execute_only_pkey;
>> > + return execute_only_pkey;
>> > +}
>>
>> If you follow the code flow in __execute_only_pkey, the AMR and UAMOR
>> are read 3 times in total, and AMR is written twice. IAMR is read and
>> written twice. Since they are SPRs and access to them is slow (or isn't
>> it?), is it worth it to read them once in __execute_only_pkey and pass
>> down their values to the callees, and then write them once at the end of
>> the function?
>
> If my calculations are right:
> uamor may be read once and may be written once.
> amr may be read once and is written once.
> iamr is written once.
> So not that bad, i think.
If I'm following the code correctly:
if need_to_set_mm_pkey = true:
uamor is read twice and written once.
amr is read twice and written twice.
iamr is read twice and written twice.
if need_to_set_mm_pkey = false:
uamor is read twice.
amr is read once or twice (depending on the value of uamor) and written
once.
iamr is read once and written once.
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 13/62] powerpc: track allocation status of all pkeys
Ram Pai writes:
> static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
> {
> - return -EINVAL;
> + if (!pkey_inited)
> + return -1;
Sorry, I missed this earlier but the pkey_free syscall will pass this
value to userspace so it needs to be an errno as well (-EINVAL?).
> +
> + if (!mm_pkey_is_allocated(mm, pkey))
> + return -EINVAL;
> +
> + mm_set_pkey_free(mm, pkey);
> +
> + return 0;
> }
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v6 21/62] powerpc: introduce execute-only pkey
Michael Ellerman writes:
> Thiago Jung Bauermann writes:
>> Ram Pai writes:
> ...
>>> +
>>> + /* We got one, store it and use it from here on out */
>>> + if (need_to_set_mm_pkey)
>>> + mm->context.execute_only_pkey = execute_only_pkey;
>>> + return execute_only_pkey;
>>> +}
>>
>> If you follow the code flow in __execute_only_pkey, the AMR and UAMOR
>> are read 3 times in total, and AMR is written twice. IAMR is read and
>> written twice. Since they are SPRs and access to them is slow (or isn't
>> it?),
>
> SPRs read/writes are slow, but they're not *that* slow in comparison to
> a system call (which I think is where this code is being called?).
Yes, this code runs on mprotect and mmap syscalls if the memory is
requested to have execute but not read nor write permissions.
> So we should try to avoid too many SPR read/writes, but at the same time
> we can accept more than the minimum if it makes the code much easier to
> follow.
Ok. Ram had asked me to suggest a way to optimize the SPR reads and
writes and I came up with the patch below. Do you think it's worth it?
The patch applies on top of this series, but if Ram includes it I think
he would break it up and merge it into the other patches.
--
Thiago Jung Bauermann
IBM Linux Technology Center
>From f6e73e67d325c4a1952c375072ca35156a9f2042 Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann
Date: Mon, 31 Jul 2017 20:22:59 -0300
Subject: [PATCH] powerpc: Cache protection key registers in
__execute_only_pkey
Pass around a struct with the contents of AMR, IAMR and AMOR, as well as
flags indicating whether those fields hold valid values and whether they
should be committed back to the registers.
Signed-off-by: Thiago Jung Bauermann
---
arch/powerpc/include/asm/pkeys.h | 18 --
arch/powerpc/mm/pkeys.c | 120 +--
2 files changed, 104 insertions(+), 34 deletions(-)
diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index e61ed6c332db..66f15dbc5855 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -129,12 +129,15 @@ static inline bool mm_pkey_is_allocated(struct mm_struct
*mm, int pkey)
mm_set_pkey_is_allocated(mm, pkey));
}
-extern void __arch_activate_pkey(int pkey);
+struct pkey_regs_cache;
+
+extern void __arch_activate_pkey(int pkey, struct pkey_regs_cache *regs);
extern void __arch_deactivate_pkey(int pkey);
/*
* Returns a positive, 5-bit key on success, or -1 on failure.
*/
-static inline int mm_pkey_alloc(struct mm_struct *mm)
+static inline int __mm_pkey_alloc(struct mm_struct *mm,
+ struct pkey_regs_cache *regs)
{
/*
* Note: this is the one and only place we make sure
@@ -162,10 +165,15 @@ static inline int mm_pkey_alloc(struct mm_struct *mm)
* enable the key in the hardware
*/
if (ret > 0)
- __arch_activate_pkey(ret);
+ __arch_activate_pkey(ret, regs);
return ret;
}
+static inline int mm_pkey_alloc(struct mm_struct *mm)
+{
+ return __mm_pkey_alloc(mm, NULL);
+}
+
static inline int mm_pkey_free(struct mm_struct *mm, int pkey)
{
if (!pkey_inited)
@@ -206,13 +214,13 @@ static inline int arch_override_mprotect_pkey(struct
vm_area_struct *vma,
}
extern int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
- unsigned long init_val);
+ unsigned long init_val, struct pkey_regs_cache *regs);
static inline int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
unsigned long init_val)
{
if (!pkey_inited)
return -EINVAL;
- return __arch_set_user_pkey_access(tsk, pkey, init_val);
+ return __arch_set_user_pkey_access(tsk, pkey, init_val, NULL);
}
static inline bool arch_pkeys_enabled(void)
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 1424c79f45f6..718ea23f8184 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -22,52 +22,92 @@ u32 initial_allocation_mask; /* bits set for
reserved keys */
#define PKEY_REG_BITS (sizeof(u64)*8)
#define pkeyshift(pkey) (PKEY_REG_BITS - ((pkey+1) * AMR_BITS_PER_PKEY))
-static bool is_pkey_enabled(int pkey)
+/*
+ * The registers controlling memory protection keys are expensive to access, so
+ * we want to cache their values in code paths that might need to use them more
+ * than once.
+ */
+struct pkey_regs_cache {
+ u64 amr;
+ u64 iamr;
+ u64 uamor;
+
+ bool amr_valid;
+ bool iamr_valid;
+ bool uamor_valid;
+
+ bool write_amr;
+ bool write_iamr;
+ bool write_uamor;
+};
+
+static bool is_pkey_enabled(int pkey, struct pkey_regs_cache *regs)
{
- return !!(read_uamor() &am
[PATCH v8 00/14] Appended signatures support for IMA appraisal
g_from_id" - Add stub in case CONFIG_INTEGRITY_SIGNATURE isn't set. - Patch "integrity: Introduce asymmetric_sig_has_known_key()" - New patch. - Patch "ima: Introduce is_ima_sig" - New patch, with code from "ima: Improvements in ima_appraise_measurement" - Patch "ima: Add modsig appraise_type option for module-style appended signatures" - Changed appraise_type to accept "imasig|modsig" instead of "modsig|imasig" to reflect the fact that now IMA only looks for the modsig after failing to find a suitable imasig stored in the xattr. - Added SPDX license identifier. - Patch "ima: Add functions to read and verify a modsig signature" - Changed ima_read_modsig() to abort loading the modsig if it uses a key which isn't known to IMA. - Changed ima_get_modsig_hash() to use pkcs7_get_digest(). - Patch "ima: Implement support for module-style appended signatures" - Added ima_xattr_sig_known_key() auxiliary function. - Call ima_read_modsig() directly from process_measurement() instead of from ima_appraise_measurement(), and only if there's no xattr signature or if the xattr signature uses a key which isn't known to IMA. - hash_algo in process_measurement() is always obtained from the xattr signature, never from the modsig. - Changes to ima_appraise_measurement() are a lot simpler now, and don't involve going back to the main switch statement a second time. - Pass xattr_value to evm_verifyxattr() unless xattr_value is a modsig. - Patch "ima: Write modsig to the measurement list" - Since now we determine whether we'll use an xattr sig or a modsig at the time they are read, there's no need to store a measurement again in the modsig case. Thus, this patch doesn't need to change ima_store_measurement() nor process_measurement() anymore. - Define new "d-sig" template field which holds the digest that is expected to match the one contained in the modsig. - Moved addition of ima_modsig_serialize_data() to patch "ima: Add functions to read and verify a modsig signature". - Increase MAX_TEMPLATE_NAME_LEN to 24. Changes since v4: - Patch "ima: Remove redundant conditional operator" - New patch. - Patch "ima: Remove some superfluous parentheses" - New patch. - Patch "evm, ima: Remove superfluous parentheses" - New patch. - Patch "evm, ima: Remove more superfluous parentheses" - New patch. - Patch "ima: Simplify ima_eventsig_init" - New patch. - Patch "ima: Improvements in ima_appraise_measurement" - New patch. - Patch "ima: Don't pass xattr value to EVM xattr verification." - New patch. - Patch "ima: Export func_tokens" - Split from patch "ima: Support module-style appended signatures for appraisal". - Patch "ima: Add modsig appraise_type option for module-style appended signatures" - Split from patch "ima: Support module-style appended signatures for appraisal". - Mention modsig option in Documentation/ABI/testing/ima_policy (suggested by Mimi Zohar). - Patch "ima: Add functions to read and verify a modsig signature" - Split from patch "ima: Support module-style appended signatures for appraisal". - Patch "ima: Implement support for module-style appended signatures" - Split from patch "ima: Support module-style appended signatures for appraisal". - In ima_appraise_measurement, change the logic of dealing with xattr errors in case the modsig verification fails. With this, process_xattr_error isn't needed anymore. - Patch "ima: Write modsig to the measurement list" - Split from patch "ima: Support module-style appended signatures for appraisal". - Added ima_current_template_has_sig function. - Removed hdr parameter from ima_modsig_serialize_data. - In ima_store_measurement, continue processing even if the given PCR is already measured if it's for a modsig. - In process_measurement, add exception to store measurement even if IMA_MEASURE is not set when appraising a modsig (suggested by Mimi Zohar). - Call is_ima_sig in ima_eventsig_init. Thiago Jung Bauermann (14): MODSIGN: Export module signature definitions PKCS#7: Refactor verify_pkcs7_signature() and add pkcs7_get_message_sig() PKCS#7: Introduce pkcs7_get_digest() integrity: Introduce struct evm_xattr integrity: Introduce integrity_keyring_from_id() integrity: Introduce asymmetric_sig_has_known_key() integrity: Select CONFIG_KEYS instead of depending on it ima: Introduce is_signed() ima: Export func_tokens ima: Add modsig appraise_type option for module-style appended signatures ima: Implement support for module-style appended
[PATCH v8 03/14] PKCS#7: Introduce pkcs7_get_digest()
IMA will need to access the digest of the PKCS7 message (as calculated by
the kernel) before the signature is verified, so introduce
pkcs7_get_digest() for that purpose.
Also, modify pkcs7_digest() to detect when the digest was already
calculated so that it doesn't have to do redundant work. Verifying that
sinfo->sig->digest isn't NULL is sufficient because both places which
allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info())
use kzalloc() so sig->digest is always initialized to zero.
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: David Howells
Cc: Herbert Xu
Cc: "David S. Miller"
---
crypto/asymmetric_keys/pkcs7_verify.c | 25 +
include/crypto/pkcs7.h| 3 +++
2 files changed, 28 insertions(+)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c
b/crypto/asymmetric_keys/pkcs7_verify.c
index 97c77f66b20d..e61f121bfc87 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -33,6 +33,10 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
+ /* The digest was calculated already. */
+ if (sig->digest)
+ return 0;
+
if (!sinfo->sig->hash_algo)
return -ENOPKG;
@@ -122,6 +126,27 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return ret;
}
+int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf, u8 *len)
+{
+ struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
+ int ret;
+
+ /*
+* This function doesn't support messages with more than one signature.
+*/
+ if (sinfo == NULL || sinfo->next != NULL)
+ return -EBADMSG;
+
+ ret = pkcs7_digest(pkcs7, sinfo);
+ if (ret)
+ return ret;
+
+ *buf = sinfo->sig->digest;
+ *len = sinfo->sig->digest_size;
+
+ return 0;
+}
+
/*
* Find the key (X.509 certificate) to use to verify a PKCS#7 message. PKCS#7
* uses the issuer's name and the issuing certificate serial number for
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 6f51d0cb6d12..cfaea9c37f4a 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -46,4 +46,7 @@ extern int pkcs7_verify(struct pkcs7_message *pkcs7,
extern int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
const void *data, size_t datalen);
+extern int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf,
+ u8 *len);
+
#endif /* _CRYPTO_PKCS7_H */
[PATCH v8 02/14] PKCS#7: Refactor verify_pkcs7_signature() and add pkcs7_get_message_sig()
IMA will need to verify a PKCS#7 which has already been parsed. For this
reason, factor out the code which does that from verify_pkcs7_signature()
into a new function which takes a struct pkcs7_message instead of a data
buffer.
In addition, IMA will need to know the key that signed a given PKCS#7
message, so add pkcs7_get_message_sig().
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: David Howells
Cc: David Woodhouse
Cc: Herbert Xu
Cc: "David S. Miller"
---
certs/system_keyring.c| 61 ---
crypto/asymmetric_keys/pkcs7_parser.c | 16 +++
include/crypto/pkcs7.h| 2 +
include/linux/verification.h | 10 +
4 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 81728717523d..dd8c5ef941ce 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -191,33 +191,27 @@ late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
/**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
* @data: The data to be verified (NULL if expecting internal data).
* @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
* @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
* (void *)1UL for all trusted keys).
* @usage: The use to which the key is being put.
* @view_content: Callback to gain access to content.
* @ctx: Context for callback.
*/
-int verify_pkcs7_signature(const void *data, size_t len,
- const void *raw_pkcs7, size_t pkcs7_len,
- struct key *trusted_keys,
- enum key_being_used_for usage,
- int (*view_content)(void *ctx,
- const void *data, size_t len,
- size_t asn1hdrlen),
- void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+struct pkcs7_message *pkcs7,
+struct key *trusted_keys,
+enum key_being_used_for usage,
+int (*view_content)(void *ctx,
+const void *data, size_t len,
+size_t asn1hdrlen),
+void *ctx)
{
- struct pkcs7_message *pkcs7;
int ret;
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -259,6 +253,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+ const void *raw_pkcs7, size_t pkcs7_len,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+ view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c
b/crypto/asymmetric_keys/pkcs7_parser.c
index f0d56e1a8b7e..8df9693f659f 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypt
[PATCH v8 01/14] MODSIGN: Export module signature definitions
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use validate_module_sig() without having to depend on
CONFIG_MODULE_SIG.
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: Jessica Yu
---
include/linux/module.h | 3 --
include/linux/module_signature.h | 47 ++
init/Kconfig | 6 ++-
kernel/Makefile | 2 +-
kernel/module.c | 1 +
kernel/module_signing.c | 82 ++--
6 files changed, 91 insertions(+), 50 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index fce6b4335e36..e49bbc5c66ef 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include
#include
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index ..862ddc25dbd1
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP,/* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type;/* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len;/* Length of signature data */
+};
+
+struct load_info;
+
+int validate_module_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+int mod_verify_sig(const void *mod, struct load_info *info);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index a4112e95724a..cd31593525ee 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1864,7 +1864,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
@@ -1879,6 +1879,10 @@ config MODULE_SIG
debuginfo strip done by some packagers (such as rpmbuild) and
inclusion into an initramfs that wants the module size reduced.
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
config MODULE_SIG_FORCE
bool "Require modules to be validly signed"
depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index 7343b3a9bff0..e56842571348 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -59,7 +59,7 @@ obj-y += up.o
endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..205c9eefd08d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f2075ce8e4b3..2912b7b1e814 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,44 @@
#include
#include
+#include
#include
#include
#include
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP,/* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
-
[PATCH v8 06/14] integrity: Introduce asymmetric_sig_has_known_key()
IMA will only look for a modsig if the xattr sig references a key which is
not in the expected kernel keyring. To that end, introduce
asymmetric_sig_has_known_key().
The logic of extracting the key used in the xattr sig is factored out from
asymmetric_verify() so that it can be used by the new function.
Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
---
security/integrity/digsig_asymmetric.c | 44 +++---
security/integrity/integrity.h | 8 +
2 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/security/integrity/digsig_asymmetric.c
b/security/integrity/digsig_asymmetric.c
index d775e03fbbcc..4c3c49f919f5 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -79,26 +79,48 @@ static struct key *request_asymmetric_key(struct key
*keyring, uint32_t keyid)
return key;
}
-int asymmetric_verify(struct key *keyring, const char *sig,
- int siglen, const char *data, int datalen)
+static struct key *asymmetric_key_from_sig(struct key *keyring, const char
*sig,
+ int siglen)
{
- struct public_key_signature pks;
- struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
- struct key *key;
- int ret = -ENOMEM;
+ const struct signature_v2_hdr *hdr = (struct signature_v2_hdr *) sig;
if (siglen <= sizeof(*hdr))
- return -EBADMSG;
+ return ERR_PTR(-EBADMSG);
siglen -= sizeof(*hdr);
if (siglen != be16_to_cpu(hdr->sig_size))
- return -EBADMSG;
+ return ERR_PTR(-EBADMSG);
if (hdr->hash_algo >= HASH_ALGO__LAST)
- return -ENOPKG;
+ return ERR_PTR(-ENOPKG);
+
+ return request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
+}
+
+bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
+ int siglen)
+{
+ struct key *key;
+
+ key = asymmetric_key_from_sig(keyring, sig, siglen);
+ if (IS_ERR_OR_NULL(key))
+ return false;
+
+ key_put(key);
+
+ return true;
+}
+
+int asymmetric_verify(struct key *keyring, const char *sig,
+ int siglen, const char *data, int datalen)
+{
+ struct public_key_signature pks;
+ struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
+ struct key *key;
+ int ret = -ENOMEM;
- key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
+ key = asymmetric_key_from_sig(keyring, sig, siglen);
if (IS_ERR(key))
return PTR_ERR(key);
@@ -110,7 +132,7 @@ int asymmetric_verify(struct key *keyring, const char *sig,
pks.digest = (u8 *)data;
pks.digest_size = datalen;
pks.s = hdr->sig;
- pks.s_size = siglen;
+ pks.s_size = siglen - sizeof(*hdr);
ret = verify_signature(key, &pks);
key_put(key);
pr_debug("%s() = %d\n", __func__, ret);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 91c41351e18a..ae3c79c63674 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -182,12 +182,20 @@ static inline int integrity_init_keyring(const unsigned
int id)
#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen);
+bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
+ int siglen);
#else
static inline int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen)
{
return -EOPNOTSUPP;
}
+
+static inline bool asymmetric_sig_has_known_key(struct key *keyring,
+ const char *sig, int siglen)
+{
+ return false;
+}
#endif
#ifdef CONFIG_IMA_LOAD_X509
[PATCH v8 05/14] integrity: Introduce integrity_keyring_from_id()
IMA will need to obtain the keyring used to verify file signatures so that
it can verify the module-style signature appended to files.
Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
---
security/integrity/digsig.c| 28 +---
security/integrity/integrity.h | 6 ++
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 5eacba858e4b..bfa27f166184 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -43,11 +43,10 @@ static const char * const
keyring_name[INTEGRITY_KEYRING_MAX] = {
#define restrict_link_to_ima restrict_link_by_builtin_trusted
#endif
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
- const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
{
- if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
- return -EINVAL;
+ if (id >= INTEGRITY_KEYRING_MAX)
+ return ERR_PTR(-EINVAL);
if (!keyring[id]) {
keyring[id] =
@@ -56,17 +55,32 @@ int integrity_digsig_verify(const unsigned int id, const
char *sig, int siglen,
int err = PTR_ERR(keyring[id]);
pr_err("no %s keyring: %d\n", keyring_name[id], err);
keyring[id] = NULL;
- return err;
+ return ERR_PTR(err);
}
}
+ return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+ const char *digest, int digestlen)
+{
+ struct key *keyring;
+
+ if (siglen < 2)
+ return -EINVAL;
+
+ keyring = integrity_keyring_from_id(id);
+ if (IS_ERR(keyring))
+ return PTR_ERR(keyring);
+
switch (sig[1]) {
case 1:
/* v1 API expect signature without xattr type */
- return digsig_verify(keyring[id], sig + 1, siglen - 1,
+ return digsig_verify(keyring, sig + 1, siglen - 1,
digest, digestlen);
case 2:
- return asymmetric_verify(keyring[id], sig, siglen,
+ return asymmetric_verify(keyring, sig, siglen,
digest, digestlen);
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 20ac02bf1b84..91c41351e18a 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -153,6 +153,7 @@ extern struct dentry *integrity_dir;
#ifdef CONFIG_INTEGRITY_SIGNATURE
+struct key *integrity_keyring_from_id(const unsigned int id);
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
const char *digest, int digestlen);
@@ -160,6 +161,11 @@ int __init integrity_init_keyring(const unsigned int id);
int __init integrity_load_x509(const unsigned int id, const char *path);
#else
+static inline struct key *integrity_keyring_from_id(const unsigned int id)
+{
+ return ERR_PTR(-EINVAL);
+}
+
static inline int integrity_digsig_verify(const unsigned int id,
const char *sig, int siglen,
const char *digest, int digestlen)
[PATCH v8 04/14] integrity: Introduce struct evm_xattr
Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.
The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.
So make this explicit in the code by removing the length specification from
the array in struct evm_ima_xattr_data. Also, change the name of the
element from digest to data since in most places the array doesn't hold a
digest.
A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/evm/evm_main.c | 8
security/integrity/ima/ima_appraise.c | 7 ---
security/integrity/integrity.h| 5 +
3 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/security/integrity/evm/evm_main.c
b/security/integrity/evm/evm_main.c
index 7f3f54d89a6e..a1b42d10efc7 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -169,7 +169,7 @@ static enum integrity_status evm_verify_hmac(struct dentry
*dentry,
/* check value type */
switch (xattr_data->type) {
case EVM_XATTR_HMAC:
- if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+ if (xattr_len != sizeof(struct evm_xattr)) {
evm_status = INTEGRITY_FAIL;
goto out;
}
@@ -179,7 +179,7 @@ static enum integrity_status evm_verify_hmac(struct dentry
*dentry,
xattr_value_len, &digest);
if (rc)
break;
- rc = crypto_memneq(xattr_data->digest, digest.digest,
+ rc = crypto_memneq(xattr_data->data, digest.digest,
SHA1_DIGEST_SIZE);
if (rc)
rc = -EINVAL;
@@ -523,7 +523,7 @@ int evm_inode_init_security(struct inode *inode,
const struct xattr *lsm_xattr,
struct xattr *evm_xattr)
{
- struct evm_ima_xattr_data *xattr_data;
+ struct evm_xattr *xattr_data;
int rc;
if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
@@ -533,7 +533,7 @@ int evm_inode_init_security(struct inode *inode,
if (!xattr_data)
return -ENOMEM;
- xattr_data->type = EVM_XATTR_HMAC;
+ xattr_data->data.type = EVM_XATTR_HMAC;
rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
if (rc < 0)
goto out;
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index deec1804a00a..8bcef90939f8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -167,7 +167,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data
*xattr_value,
return sig->hash_algo;
break;
case IMA_XATTR_DIGEST_NG:
- ret = xattr_value->digest[0];
+ /* first byte contains algorithm id */
+ ret = xattr_value->data[0];
if (ret < HASH_ALGO__LAST)
return ret;
break;
@@ -175,7 +176,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data
*xattr_value,
/* this is for backward compatibility */
if (xattr_len == 21) {
unsigned int zero = 0;
- if (!memcmp(&xattr_value->digest[16], &zero, 4))
+ if (!memcmp(&xattr_value->data[16], &zero, 4))
return HASH_ALGO_MD5;
else
return HASH_ALGO_SHA1;
@@ -274,7 +275,7 @@ int ima_appraise_measurement(enum ima_hooks func,
/* xattr length may be longer. md5 hash in previous
version occupied 20 bytes in xattr, instead of 16
*/
- rc = memcmp(&xattr_value->digest[hash_start],
+ rc = memcmp(&xattr_value->data[hash_start],
iint->ima_hash->digest,
iint->ima_hash->length);
else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index e60473b13a8d..20ac02bf1b84 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -79,6 +79,11 @@ enum evm_ima_xattr_type {
struct evm_ima_xattr_data {
u8 type;
+ u8 data[];
+} __packed;
+
+struct evm_xattr {
+ struct evm_ima_xattr_data data;
u8 digest[SHA1_DIGEST_SIZE];
} __packed;
[PATCH v8 08/14] ima: Introduce is_signed()
With the introduction of another IMA signature type (modsig), some places
will need to check for both of them. It is cleaner to do that if there's a
helper function to tell whether an xattr_value represents an IMA
signature.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 5 +
security/integrity/ima/ima_appraise.c | 7 +++
security/integrity/ima/ima_template_lib.c | 2 +-
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..e4f72b30cb28 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,11 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
+static inline bool is_signed(const struct evm_ima_xattr_data *xattr_value)
+{
+ return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+}
+
/*
* used to protect h_table and sha_table
*/
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index 8bcef90939f8..c6459408e6b2 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -325,15 +325,14 @@ int ima_appraise_measurement(enum ima_hooks func,
} else if (status != INTEGRITY_PASS) {
/* Fix mode, but don't replace file signatures. */
if ((ima_appraise & IMA_APPRAISE_FIX) &&
- (!xattr_value ||
-xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+ !is_signed(xattr_value)) {
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
/* Permit new files with file signatures, but without data. */
if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
- xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+ is_signed(xattr_value)) {
status = INTEGRITY_PASS;
}
@@ -448,7 +447,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char
*xattr_name,
if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
return -EINVAL;
ima_reset_appraise_flags(d_backing_inode(dentry),
- xvalue->type == EVM_IMA_XATTR_DIGSIG);
+is_signed(xvalue));
result = 0;
}
return result;
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 43752002c222..300912914b17 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -382,7 +382,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
- if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+ if (!is_signed(xattr_value))
return 0;
return ima_write_template_field_data(xattr_value, event_data->xattr_len,
[PATCH v8 09/14] ima: Export func_tokens
ima_read_modsig() will need it so that it can show an error message.
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h| 2 ++
security/integrity/ima/ima_policy.c | 12 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e4f72b30cb28..f0bc2a182cbf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -195,6 +195,8 @@ enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
};
+extern const char *const func_tokens[];
+
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr);
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index b20770704b6c..fd31e42c2bad 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1136,6 +1136,12 @@ void ima_delete_rules(void)
}
}
+#define __ima_hook_stringify(str) (#str),
+
+const char *const func_tokens[] = {
+ __ima_hooks(__ima_hook_stringify)
+};
+
#ifdef CONFIG_IMA_READ_POLICY
enum {
mask_exec = 0, mask_write, mask_read, mask_append
@@ -1148,12 +1154,6 @@ static const char *const mask_tokens[] = {
"MAY_APPEND"
};
-#define __ima_hook_stringify(str) (#str),
-
-static const char *const func_tokens[] = {
- __ima_hooks(__ima_hook_stringify)
-};
-
void *ima_policy_start(struct seq_file *m, loff_t *pos)
{
loff_t l = *pos;
[PATCH v8 12/14] ima: Add new "d-sig" template field
Define new "d-sig" template field which holds the digest that is expected
to match the one contained in the modsig.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
Documentation/security/IMA-templates.rst | 5
security/integrity/ima/ima.h | 9 +++
security/integrity/ima/ima_modsig.c | 23
security/integrity/ima/ima_template.c | 4 ++-
security/integrity/ima/ima_template_lib.c | 32 ++-
security/integrity/ima/ima_template_lib.h | 2 ++
6 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/Documentation/security/IMA-templates.rst
b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..f2a0f4225857 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -68,6 +68,11 @@ descriptors by adding their identifier to the format string
- 'd-ng': the digest of the event, calculated with an arbitrary hash
algorithm (field format: [:]digest, where the digest
prefix is shown only if the hash algorithm is not SHA1 or MD5);
+ - 'd-sig': the digest of the event for files that have an appended modsig.
This
+ field is calculated without including the modsig and thus will differ from
+ the total digest of the file, but it is what should match the digest
+ contained in the modsig (if it doesn't, the signature is invalid). It is
+ shown in the same format as 'd-ng';
- 'n-ng': the name of the event, without size limitations;
- 'sig': the file signature.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 312d60cee702..7f88e4b86156 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -313,6 +313,8 @@ bool ima_hook_supports_modsig(enum ima_hooks func);
int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
struct evm_ima_xattr_data **xattr_value,
int *xattr_len);
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+ const u8 **hash, u8 *len);
int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr);
void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
@@ -330,6 +332,13 @@ static inline int ima_read_modsig(enum ima_hooks func,
const void *buf,
return -EOPNOTSUPP;
}
+static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
+ enum hash_algo *algo, const u8 **hash,
+ u8 *len)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr)
{
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
index e095d35d804d..584d9d77b2c4 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -144,6 +144,29 @@ int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len,
return rc;
}
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+ const u8 **hash, u8 *len)
+{
+ struct modsig_hdr *modsig = (typeof(modsig)) hdr;
+ const struct public_key_signature *pks;
+ int i;
+
+ if (!hdr || hdr->type != IMA_MODSIG)
+ return -EINVAL;
+
+ pks = pkcs7_get_message_sig(modsig->pkcs7_msg);
+ if (!pks)
+ return -EBADMSG;
+
+ for (i = 0; i < HASH_ALGO__LAST; i++)
+ if (!strcmp(hash_algo_name[i], pks->hash_algo))
+ break;
+
+ *algo = i;
+
+ return pkcs7_get_digest(modsig->pkcs7_msg, hash, len);
+}
+
int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr)
{
diff --git a/security/integrity/ima/ima_template.c
b/security/integrity/ima/ima_template.c
index b631b8bc7624..045ad508cbb8 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -43,8 +43,10 @@ static const struct ima_template_field supported_fields[] = {
.field_show = ima_show_template_string},
{.field_id = "sig", .field_init = ima_eventsig_init,
.field_show = ima_show_template_sig},
+ {.field_id = "d-sig", .field_init = ima_eventdigest_sig_init,
+.field_show = ima_show_template_digest_ng},
};
-#define MAX_TEMPLATE_NAME_LEN 15
+#define MAX_TEMPLATE_NAME_LEN 24
static struct ima_template_desc *ima_template;
static struct ima_template_desc *lookup_template_desc(const char *name);
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 300912914b17..36d175816894 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@
[PATCH v8 13/14] ima: Write modsig to the measurement list
Add modsig support to the "sig" template field, allowing the the contents
of the modsig to be included in the measurement list.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 7 +++
security/integrity/ima/ima_modsig.c | 13 +
security/integrity/ima/ima_template_lib.c | 15 ++-
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 7f88e4b86156..8e1b1ddbe14f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -315,6 +315,7 @@ int ima_read_modsig(enum ima_hooks func, const void *buf,
loff_t buf_len,
int *xattr_len);
int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
const u8 **hash, u8 *len);
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len);
int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr);
void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
@@ -339,6 +340,12 @@ static inline int ima_get_modsig_hash(struct
evm_ima_xattr_data *hdr,
return -EOPNOTSUPP;
}
+static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data **data,
+ int *data_len)
+{
+ return -EOPNOTSUPP;
+}
+
static inline int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr)
{
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
index 584d9d77b2c4..e31ab7dc11db 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -167,6 +167,19 @@ int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
enum hash_algo *algo,
return pkcs7_get_digest(modsig->pkcs7_msg, hash, len);
}
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len)
+{
+ struct modsig_hdr *modsig = (struct modsig_hdr *) *data;
+
+ if (!*data || (*data)->type != IMA_MODSIG)
+ return -EINVAL;
+
+ *data = &modsig->raw_pkcs7;
+ *data_len = modsig->raw_pkcs7_len;
+
+ return 0;
+}
+
int ima_modsig_verify(const unsigned int keyring_id,
struct evm_ima_xattr_data *hdr)
{
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 36d175816894..417cd153ba60 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -411,10 +411,23 @@ int ima_eventsig_init(struct ima_event_data *event_data,
struct ima_field_data *field_data)
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
+ int xattr_len = event_data->xattr_len;
if (!is_signed(xattr_value))
return 0;
- return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+ /*
+* The xattr_value for IMA_MODSIG is a runtime structure containing
+* pointers. Get its raw data instead.
+*/
+ if (xattr_value->type == IMA_MODSIG) {
+ int rc;
+
+ rc = ima_modsig_serialize_data(&xattr_value, &xattr_len);
+ if (rc)
+ return rc;
+ }
+
+ return ima_write_template_field_data(xattr_value, xattr_len,
DATA_FMT_HEX, field_data);
}
[PATCH v8 11/14] ima: Implement support for module-style appended signatures
Implement the appraise_type=imasig|modsig option, allowing IMA to read and
verify modsig signatures.
In case a file has both an xattr signature and an appended modsig, IMA will
only use the appended signature if the key used by the xattr signature
isn't present in the IMA keyring.
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/Kconfig| 3 +
security/integrity/ima/ima.h | 36 ++-
security/integrity/ima/ima_appraise.c | 65 ++--
security/integrity/ima/ima_main.c | 17 ++-
security/integrity/ima/ima_modsig.c | 145 ++
security/integrity/integrity.h| 1 +
6 files changed, 256 insertions(+), 11 deletions(-)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index bba19f9ea184..0fb542455698 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -234,6 +234,9 @@ config IMA_APPRAISE_BOOTPARAM
config IMA_APPRAISE_MODSIG
bool "Support module-style signatures for appraisal"
depends on IMA_APPRAISE
+ depends on INTEGRITY_ASYMMETRIC_KEYS
+ select PKCS7_MESSAGE_PARSER
+ select MODULE_SIG_FORMAT
default n
help
Adds support for signatures appended to files. The format of the
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 69c06e2d7bd6..312d60cee702 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -156,7 +156,8 @@ void ima_init_template_list(void);
static inline bool is_signed(const struct evm_ima_xattr_data *xattr_value)
{
- return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+ return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+ xattr_value->type == IMA_MODSIG);
}
/*
@@ -253,6 +254,8 @@ enum integrity_status ima_get_cache_status(struct
integrity_iint_cache *iint,
enum ima_hooks func);
enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
int xattr_len);
+bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
+int xattr_len);
int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value);
@@ -291,6 +294,12 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
int xattr_len)
return ima_hash_algo;
}
+static inline bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data
+ *xattr_value, int xattr_len)
+{
+ return false;
+}
+
static inline int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value)
{
@@ -301,11 +310,36 @@ static inline int ima_read_xattr(struct dentry *dentry,
#ifdef CONFIG_IMA_APPRAISE_MODSIG
bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+ struct evm_ima_xattr_data **xattr_value,
+ int *xattr_len);
+int ima_modsig_verify(const unsigned int keyring_id,
+ struct evm_ima_xattr_data *hdr);
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
{
return false;
}
+
+static inline int ima_read_modsig(enum ima_hooks func, const void *buf,
+ loff_t buf_len,
+ struct evm_ima_xattr_data **xattr_value,
+ int *xattr_len)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline int ima_modsig_verify(const unsigned int keyring_id,
+ struct evm_ima_xattr_data *hdr)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
+{
+ kfree(hdr);
+}
#endif /* CONFIG_IMA_APPRAISE_MODSIG */
/* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index c6459408e6b2..27a1dbb52544 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -189,6 +189,22 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data
*xattr_value,
return ima_hash_algo;
}
+bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
+int xattr_len)
+{
+ struct key *keyring;
+
+ if (xattr_value->type != EVM_IMA_XATTR_DIGSIG)
+ return false;
+
+ keyring = integrity_keyring_from_id(INTEGRITY_KEYRING_IMA);
+ if (IS_ERR(keyring))
+ return false;
+
+ return asymmetric_sig_has_known_key(keyring, (const char *) xattr_value,
+ xattr_len);
+}
+
int ima_read_xattr(struct dentry *dentry,
[PATCH v8 14/14] ima: Store the measurement again when appraising a modsig
If the IMA template contains the 'sig' field, then the modsig should be
added to the measurement list when the file is appraised, and that is what
normally happens.
But If a measurement rule caused a file containing a modsig to be measured
before a different rule causes it to be appraised, the resulting
measurement entry will not contain the modsig because it is only fetched
during appraisal. When the appraisal rule triggers, it won't store a new
measurement containing the modsig because the file was already measured.
We need to detect that situation and store an additional measurement with
the modsig. This is done by defining the appraise subaction flag
IMA_READ_MEASURE and testing for it in process_measurement().
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 8 +++-
security/integrity/ima/ima_main.c | 16 +++-
security/integrity/ima/ima_policy.c | 59 ---
security/integrity/ima/ima_template.c | 27
security/integrity/integrity.h| 9 ++--
6 files changed, 110 insertions(+), 10 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 8e1b1ddbe14f..c39bed55f6d2 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -147,6 +147,7 @@ int ima_init_crypto(void);
void ima_putc(struct seq_file *m, void *data, int datalen);
void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
struct ima_template_desc *ima_template_desc_current(void);
+bool ima_current_template_has_sig(void);
int ima_restore_measurement_entry(struct ima_template_entry *entry);
int ima_restore_measurement_list(loff_t bufsize, void *buf);
int ima_measurements_show(struct seq_file *m, void *v);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 99dd1d53fc35..a7af114bc6b4 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -289,7 +289,13 @@ void ima_store_measurement(struct integrity_iint_cache
*iint,
xattr_len, NULL};
int violation = 0;
- if (iint->measured_pcrs & (0x1 << pcr))
+ /*
+* We still need to store the measurement in the case of MODSIG because
+* we only have its contents to put in the list at the time of
+* appraisal. See comment in store_measurement_again() for more details.
+*/
+ if (iint->measured_pcrs & (0x1 << pcr) &&
+ (!xattr_value || xattr_value->type != IMA_MODSIG))
return;
result = ima_alloc_init_template(&event_data, &entry);
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index d5abd22d502a..807b9b77b813 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -166,6 +166,20 @@ void ima_file_free(struct file *file)
ima_check_last_writer(iint, inode, file);
}
+/*
+ * A file measurement might already exist in the measurement list. Based on
+ * policy, include an additional file measurement containing the appended
+ * signature and file hash, without the appended signature (i.e., the 'd-sig'
+ * field).
+ */
+static bool store_measurement_again(struct integrity_iint_cache *iint,
+ struct evm_ima_xattr_data *xattr_value)
+{
+ return iint->flags & IMA_READ_MEASURE && xattr_value &&
+ xattr_value->type == IMA_MODSIG &&
+ ima_current_template_has_sig();
+}
+
static int process_measurement(struct file *file, const struct cred *cred,
u32 secid, char *buf, loff_t size, int mask,
enum ima_hooks func)
@@ -299,7 +313,7 @@ static int process_measurement(struct file *file, const
struct cred *cred,
if (!pathbuf) /* ima_rdwr_violation possibly pre-fetched */
pathname = ima_d_path(&file->f_path, &pathbuf, filename);
- if (action & IMA_MEASURE)
+ if (action & IMA_MEASURE || store_measurement_again(iint, xattr_value))
ima_store_measurement(iint, file, pathname,
xattr_value, xattr_len, pcr);
if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index 3e5a64053aa8..c0b39802e988 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -10,6 +10,9 @@
* - initialize default measure policy rules
*
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
@@ -367,7 +370,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
struct inode *inode,
* In addition to knowing that we need to
[PATCH v8 07/14] integrity: Select CONFIG_KEYS instead of depending on it
This avoids a dependency cycle in soon-to-be-introduced CONFIG_IMA_APPRAISE_MODSIG: it will select CONFIG_MODULE_SIG_FORMAT which in turn selects CONFIG_KEYS. Kconfig then complains that CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS. Signed-off-by: Thiago Jung Bauermann Signed-off-by: Mimi Zohar --- security/integrity/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index da9565891738..0d642e0317c7 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -17,8 +17,8 @@ if INTEGRITY config INTEGRITY_SIGNATURE bool "Digital signature verification using multiple keyrings" - depends on KEYS default n + select KEYS select SIGNATURE help This option enables digital signature verification support
[PATCH v8 10/14] ima: Add modsig appraise_type option for module-style appended signatures
Introduce the modsig keyword to the IMA policy syntax to specify that
a given hook should expect the file to have the IMA signature appended
to it. Here is how it can be used in a rule:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
With this rule, IMA will accept either a signature stored in the extended
attribute or an appended signature.
For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified. The actual modsig implementation
will be introduced separately.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
Documentation/ABI/testing/ima_policy | 6 +-
security/integrity/ima/Kconfig | 10 +
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9
security/integrity/ima/ima_modsig.c | 31
security/integrity/ima/ima_policy.c | 12 +--
security/integrity/integrity.h | 1 +
7 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..9d1dfd0a8891 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -37,7 +37,7 @@ Description:
euid:= decimal value
fowner:= decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] [imasig|modsig]
pcr:= decimal value
default policy:
@@ -103,3 +103,7 @@ Description:
measure func=KEXEC_KERNEL_CHECK pcr=4
measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+ Example of appraise rule allowing modsig appended signatures:
+
+ appraise func=KEXEC_KERNEL_CHECK
appraise_type=imasig|modsig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..bba19f9ea184 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -231,6 +231,16 @@ config IMA_APPRAISE_BOOTPARAM
This option enables the different "ima_appraise=" modes
(eg. fix, log) from the boot command line.
+config IMA_APPRAISE_MODSIG
+ bool "Support module-style signatures for appraisal"
+ depends on IMA_APPRAISE
+ default n
+ help
+ Adds support for signatures appended to files. The format of the
+ appended signature is the same used for signed kernel modules.
+ The modsig keyword can be used in the IMA policy to allow a hook
+ to accept such signatures.
+
config IMA_TRUSTED_KEYRING
bool "Require all keys on the .ima keyring be signed (deprecated)"
depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..31d57cdf2421 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_IMA) += ima.o
ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_template.o ima_template_lib.o
ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0bc2a182cbf..69c06e2d7bd6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -299,6 +299,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#else
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ return false;
+}
+#endif /* CONFIG_IMA_APPRAISE_MODSIG */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
new file mode 100644
index ..84d428cbbca8
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2018 IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann
+ */
+
+#include "ima.h"
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file, because only
they
+ * preload the contents of the file in a buffer. FILE_CHECK does that in some
+ * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but
+ * it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ switch (func) {
+
Re: [PATCH v8 00/14] Appended signatures support for IMA appraisal
Hello James, Thanks for you interest in these patches. James Morris writes: > On Fri, 16 Nov 2018, Thiago Jung Bauermann wrote: > >> On the OpenPOWER platform, secure boot and trusted boot are being >> implemented using IMA for taking measurements and verifying signatures. >> Since the kernel image on Power servers is an ELF binary, kernels are >> signed using the scripts/sign-file tool and thus use the same signature >> format as signed kernel modules. >> >> This patch series adds support in IMA for verifying those signatures. > > Are you saying you use IMA to verify kernels during boot? From a Linux > bootloader? Yes to both. OpenPOWER machines have embedded in their firmware a Linux kernel and initramfs to use as bootloader, using Petitboot. kexec is used to load the OS and boot it. >> It adds flexibility to OpenPOWER secure boot, because it allows it to boot >> kernels with the signature appended to them as well as kernels where the >> signature is stored in the IMA extended attribute. > > Just to clarify, with these patches, IMA will be able to verify the > native form of signed kernel modules? That wasn't my use case to develop the patches, but I just tested and it works. I just had to make a slight modification: there's a whitelist of IMA hooks that are allowed to use the module signature format (in the ima_hook_supports_modsig function), and I had to add MODULE_CHECK to it. The next version of the patches will have this change. The only difference is that IMA looks for a valid key in the IMA keyring, while the CONFIG_MODULE_SIG code looks for the module signing key in the builtin and secondary trusted keyrings. > i.e. without xattrs at all, and > this will work with existing signed modules? No xattrs at all, and yes. -- Thiago Jung Bauermann IBM Linux Technology Center
[PATCH v9 00/14] Appended signatures support for IMA appraisal
: Write modsig to the measurement list" (Suggested by Mimi Zohar). - Patch "ima: Write modsig to the measurement list" - Moved some functions from patch "ima: Add functions to read and verify a modsig signature" into this patch. - Moved code related to d-sig support to new patch. - Patch "ima: Store the measurement again when appraising a modsig" - New patch. Thiago Jung Bauermann (14): MODSIGN: Export module signature definitions PKCS#7: Refactor verify_pkcs7_signature() and add pkcs7_get_message_sig() PKCS#7: Introduce pkcs7_get_digest() integrity: Introduce struct evm_xattr integrity: Introduce integrity_keyring_from_id() integrity: Introduce asymmetric_sig_has_known_key() integrity: Select CONFIG_KEYS instead of depending on it ima: Introduce is_signed() ima: Export func_tokens ima: Add modsig appraise_type option for module-style appended signatures ima: Implement support for module-style appended signatures ima: Add new "d-sig" template field ima: Write modsig to the measurement list ima: Store the measurement again when appraising a modsig Documentation/ABI/testing/ima_policy | 6 +- Documentation/security/IMA-templates.rst | 5 + certs/system_keyring.c| 61 -- crypto/asymmetric_keys/pkcs7_parser.c | 16 ++ crypto/asymmetric_keys/pkcs7_verify.c | 27 +++ include/crypto/pkcs7.h| 5 + include/linux/module.h| 3 - include/linux/module_signature.h | 47 + include/linux/verification.h | 10 + init/Kconfig | 6 +- kernel/Makefile | 2 +- kernel/module.c | 1 + kernel/module_signing.c | 82 scripts/Makefile | 4 +- security/integrity/Kconfig| 2 +- security/integrity/digsig.c | 31 ++- security/integrity/digsig_asymmetric.c| 44 +++-- security/integrity/evm/evm_main.c | 8 +- security/integrity/ima/Kconfig| 13 ++ security/integrity/ima/Makefile | 1 + security/integrity/ima/ima.h | 62 ++ security/integrity/ima/ima_api.c | 9 +- security/integrity/ima/ima_appraise.c | 82 +++- security/integrity/ima/ima_main.c | 29 ++- security/integrity/ima/ima_modsig.c | 229 ++ security/integrity/ima/ima_policy.c | 81 ++-- security/integrity/ima/ima_template.c | 28 ++- security/integrity/ima/ima_template_lib.c | 49 - security/integrity/ima/ima_template_lib.h | 2 + security/integrity/integrity.h| 40 +++- 30 files changed, 861 insertions(+), 124 deletions(-) create mode 100644 include/linux/module_signature.h create mode 100644 security/integrity/ima/ima_modsig.c
[PATCH v9 01/14] MODSIGN: Export module signature definitions
IMA will use the module_signature format for append signatures, so export
the relevant definitions and factor out the code which verifies that the
appended signature trailer is valid.
Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it
and be able to use mod_check_sig() without having to depend on
CONFIG_MODULE_SIG.
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: Jessica Yu
---
include/linux/module.h | 3 --
include/linux/module_signature.h | 47 ++
init/Kconfig | 6 ++-
kernel/Makefile | 2 +-
kernel/module.c | 1 +
kernel/module_signing.c | 82 ++--
6 files changed, 91 insertions(+), 50 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index fce6b4335e36..e49bbc5c66ef 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -25,9 +25,6 @@
#include
#include
-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
-#define MODULE_SIG_STRING "~Module signature appended~\n"
-
/* Not Yet Implemented */
#define MODULE_SUPPORTED_DEVICE(name)
diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
new file mode 100644
index ..a3a629fc8c13
--- /dev/null
+++ b/include/linux/module_signature.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Module signature handling.
+ *
+ * Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells (dhowe...@redhat.com)
+ */
+
+#ifndef _LINUX_MODULE_SIGNATURE_H
+#define _LINUX_MODULE_SIGNATURE_H
+
+/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */
+#define MODULE_SIG_STRING "~Module signature appended~\n"
+
+enum pkey_id_type {
+ PKEY_ID_PGP,/* OpenPGP generated key ID */
+ PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
+ PKEY_ID_PKCS7, /* Signature in PKCS#7 message */
+};
+
+/*
+ * Module signature information block.
+ *
+ * The constituents of the signature section are, in order:
+ *
+ * - Signer's name
+ * - Key identifier
+ * - Signature data
+ * - Information block
+ */
+struct module_signature {
+ u8 algo; /* Public-key crypto algorithm [0] */
+ u8 hash; /* Digest algorithm [0] */
+ u8 id_type;/* Key identifier type [PKEY_ID_PKCS7] */
+ u8 signer_len; /* Length of signer's name [0] */
+ u8 key_id_len; /* Length of key identifier [0] */
+ u8 __pad[3];
+ __be32 sig_len;/* Length of signature data */
+};
+
+struct load_info;
+
+int mod_check_sig(const struct module_signature *ms, size_t file_len,
+ const char *name);
+int mod_verify_sig(const void *mod, struct load_info *info);
+
+#endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/init/Kconfig b/init/Kconfig
index a4112e95724a..cd31593525ee 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1864,7 +1864,7 @@ config MODULE_SRCVERSION_ALL
config MODULE_SIG
bool "Module signature verification"
depends on MODULES
- select SYSTEM_DATA_VERIFICATION
+ select MODULE_SIG_FORMAT
help
Check modules for valid signatures upon load: the signature
is simply appended to the module. For more information see
@@ -1879,6 +1879,10 @@ config MODULE_SIG
debuginfo strip done by some packagers (such as rpmbuild) and
inclusion into an initramfs that wants the module size reduced.
+config MODULE_SIG_FORMAT
+ def_bool n
+ select SYSTEM_DATA_VERIFICATION
+
config MODULE_SIG_FORCE
bool "Require modules to be validly signed"
depends on MODULE_SIG
diff --git a/kernel/Makefile b/kernel/Makefile
index 7343b3a9bff0..e56842571348 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -59,7 +59,7 @@ obj-y += up.o
endif
obj-$(CONFIG_UID16) += uid16.o
obj-$(CONFIG_MODULES) += module.o
-obj-$(CONFIG_MODULE_SIG) += module_signing.o
+obj-$(CONFIG_MODULE_SIG_FORMAT) += module_signing.o
obj-$(CONFIG_KALLSYMS) += kallsyms.o
obj-$(CONFIG_BSD_PROCESS_ACCT) += acct.o
obj-$(CONFIG_CRASH_CORE) += crash_core.o
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..205c9eefd08d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f2075ce8e4b3..5624e59981b4 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -11,36 +11,44 @@
#include
#include
+#include
#include
#include
#include
#include "module-internal.h"
-enum pkey_id_type {
- PKEY_ID_PGP,/* OpenPGP generated key ID */
- PKEY_ID_X509, /* X.509 arbitrary subjectKeyIdentifier */
- PKEY_ID_PKCS7,
[PATCH v9 03/14] PKCS#7: Introduce pkcs7_get_digest()
IMA will need to access the digest of the PKCS7 message (as calculated by
the kernel) before the signature is verified, so introduce
pkcs7_get_digest() for that purpose.
Also, modify pkcs7_digest() to detect when the digest was already
calculated so that it doesn't have to do redundant work. Verifying that
sinfo->sig->digest isn't NULL is sufficient because both places which
allocate sinfo->sig (pkcs7_parse_message() and pkcs7_note_signed_info())
use kzalloc() so sig->digest is always initialized to zero.
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: David Howells
Cc: Herbert Xu
Cc: "David S. Miller"
---
crypto/asymmetric_keys/pkcs7_verify.c | 27 +++
include/crypto/pkcs7.h| 3 +++
2 files changed, 30 insertions(+)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c
b/crypto/asymmetric_keys/pkcs7_verify.c
index 97c77f66b20d..ccf80a7b7d9b 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -33,6 +33,10 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
+ /* The digest was calculated already. */
+ if (sig->digest)
+ return 0;
+
if (!sinfo->sig->hash_algo)
return -ENOPKG;
@@ -122,6 +126,29 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return ret;
}
+int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf, u8 *len)
+{
+ struct pkcs7_signed_info *sinfo = pkcs7->signed_infos;
+ int ret;
+
+ /*
+* This function doesn't support messages with more than one signature.
+*/
+ if (sinfo == NULL || sinfo->next != NULL)
+ return -EBADMSG;
+
+ ret = pkcs7_digest(pkcs7, sinfo);
+ if (ret)
+ return ret;
+
+ if (buf)
+ *buf = sinfo->sig->digest;
+ if (len)
+ *len = sinfo->sig->digest_size;
+
+ return 0;
+}
+
/*
* Find the key (X.509 certificate) to use to verify a PKCS#7 message. PKCS#7
* uses the issuer's name and the issuing certificate serial number for
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 6f51d0cb6d12..cfaea9c37f4a 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -46,4 +46,7 @@ extern int pkcs7_verify(struct pkcs7_message *pkcs7,
extern int pkcs7_supply_detached_data(struct pkcs7_message *pkcs7,
const void *data, size_t datalen);
+extern int pkcs7_get_digest(struct pkcs7_message *pkcs7, const u8 **buf,
+ u8 *len);
+
#endif /* _CRYPTO_PKCS7_H */
[PATCH v9 04/14] integrity: Introduce struct evm_xattr
Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.
The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.
So make this explicit in the code by removing the length specification from
the array in struct evm_ima_xattr_data. Also, change the name of the
element from digest to data since in most places the array doesn't hold a
digest.
A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition, specifically the EVM HMAC code.
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
---
security/integrity/evm/evm_main.c | 8
security/integrity/ima/ima_appraise.c | 7 ---
security/integrity/integrity.h| 6 ++
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/security/integrity/evm/evm_main.c
b/security/integrity/evm/evm_main.c
index 7f3f54d89a6e..a1b42d10efc7 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -169,7 +169,7 @@ static enum integrity_status evm_verify_hmac(struct dentry
*dentry,
/* check value type */
switch (xattr_data->type) {
case EVM_XATTR_HMAC:
- if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+ if (xattr_len != sizeof(struct evm_xattr)) {
evm_status = INTEGRITY_FAIL;
goto out;
}
@@ -179,7 +179,7 @@ static enum integrity_status evm_verify_hmac(struct dentry
*dentry,
xattr_value_len, &digest);
if (rc)
break;
- rc = crypto_memneq(xattr_data->digest, digest.digest,
+ rc = crypto_memneq(xattr_data->data, digest.digest,
SHA1_DIGEST_SIZE);
if (rc)
rc = -EINVAL;
@@ -523,7 +523,7 @@ int evm_inode_init_security(struct inode *inode,
const struct xattr *lsm_xattr,
struct xattr *evm_xattr)
{
- struct evm_ima_xattr_data *xattr_data;
+ struct evm_xattr *xattr_data;
int rc;
if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
@@ -533,7 +533,7 @@ int evm_inode_init_security(struct inode *inode,
if (!xattr_data)
return -ENOMEM;
- xattr_data->type = EVM_XATTR_HMAC;
+ xattr_data->data.type = EVM_XATTR_HMAC;
rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
if (rc < 0)
goto out;
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index f6ac405daabb..dcb8226972cf 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -167,7 +167,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data
*xattr_value,
return sig->hash_algo;
break;
case IMA_XATTR_DIGEST_NG:
- ret = xattr_value->digest[0];
+ /* first byte contains algorithm id */
+ ret = xattr_value->data[0];
if (ret < HASH_ALGO__LAST)
return ret;
break;
@@ -175,7 +176,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data
*xattr_value,
/* this is for backward compatibility */
if (xattr_len == 21) {
unsigned int zero = 0;
- if (!memcmp(&xattr_value->digest[16], &zero, 4))
+ if (!memcmp(&xattr_value->data[16], &zero, 4))
return HASH_ALGO_MD5;
else
return HASH_ALGO_SHA1;
@@ -274,7 +275,7 @@ int ima_appraise_measurement(enum ima_hooks func,
/* xattr length may be longer. md5 hash in previous
version occupied 20 bytes in xattr, instead of 16
*/
- rc = memcmp(&xattr_value->digest[hash_start],
+ rc = memcmp(&xattr_value->data[hash_start],
iint->ima_hash->digest,
iint->ima_hash->length);
else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 3517d2852a07..2bf0fc51752b 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -79,6 +79,12 @@ enum evm_ima_xattr_type {
struct evm_ima_xattr_data {
u8 type;
+ u8 data[];
+} _
[PATCH v9 06/14] integrity: Introduce asymmetric_sig_has_known_key()
IMA will only look for a modsig if the xattr sig references a key which is
not in the expected kernel keyring. To that end, introduce
asymmetric_sig_has_known_key().
The logic of extracting the key used in the xattr sig is factored out from
asymmetric_verify() so that it can be used by the new function.
Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
---
security/integrity/digsig_asymmetric.c | 44 +++---
security/integrity/integrity.h | 8 +
2 files changed, 41 insertions(+), 11 deletions(-)
diff --git a/security/integrity/digsig_asymmetric.c
b/security/integrity/digsig_asymmetric.c
index d775e03fbbcc..4c3c49f919f5 100644
--- a/security/integrity/digsig_asymmetric.c
+++ b/security/integrity/digsig_asymmetric.c
@@ -79,26 +79,48 @@ static struct key *request_asymmetric_key(struct key
*keyring, uint32_t keyid)
return key;
}
-int asymmetric_verify(struct key *keyring, const char *sig,
- int siglen, const char *data, int datalen)
+static struct key *asymmetric_key_from_sig(struct key *keyring, const char
*sig,
+ int siglen)
{
- struct public_key_signature pks;
- struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
- struct key *key;
- int ret = -ENOMEM;
+ const struct signature_v2_hdr *hdr = (struct signature_v2_hdr *) sig;
if (siglen <= sizeof(*hdr))
- return -EBADMSG;
+ return ERR_PTR(-EBADMSG);
siglen -= sizeof(*hdr);
if (siglen != be16_to_cpu(hdr->sig_size))
- return -EBADMSG;
+ return ERR_PTR(-EBADMSG);
if (hdr->hash_algo >= HASH_ALGO__LAST)
- return -ENOPKG;
+ return ERR_PTR(-ENOPKG);
+
+ return request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
+}
+
+bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
+ int siglen)
+{
+ struct key *key;
+
+ key = asymmetric_key_from_sig(keyring, sig, siglen);
+ if (IS_ERR_OR_NULL(key))
+ return false;
+
+ key_put(key);
+
+ return true;
+}
+
+int asymmetric_verify(struct key *keyring, const char *sig,
+ int siglen, const char *data, int datalen)
+{
+ struct public_key_signature pks;
+ struct signature_v2_hdr *hdr = (struct signature_v2_hdr *)sig;
+ struct key *key;
+ int ret = -ENOMEM;
- key = request_asymmetric_key(keyring, be32_to_cpu(hdr->keyid));
+ key = asymmetric_key_from_sig(keyring, sig, siglen);
if (IS_ERR(key))
return PTR_ERR(key);
@@ -110,7 +132,7 @@ int asymmetric_verify(struct key *keyring, const char *sig,
pks.digest = (u8 *)data;
pks.digest_size = datalen;
pks.s = hdr->sig;
- pks.s_size = siglen;
+ pks.s_size = siglen - sizeof(*hdr);
ret = verify_signature(key, &pks);
key_put(key);
pr_debug("%s() = %d\n", __func__, ret);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 6f657260a964..dec5ab8cf9e9 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -194,12 +194,20 @@ static inline int __init integrity_load_cert(const
unsigned int id,
#ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen);
+bool asymmetric_sig_has_known_key(struct key *keyring, const char *sig,
+ int siglen);
#else
static inline int asymmetric_verify(struct key *keyring, const char *sig,
int siglen, const char *data, int datalen)
{
return -EOPNOTSUPP;
}
+
+static inline bool asymmetric_sig_has_known_key(struct key *keyring,
+ const char *sig, int siglen)
+{
+ return false;
+}
#endif
#ifdef CONFIG_IMA_LOAD_X509
[PATCH v9 05/14] integrity: Introduce integrity_keyring_from_id()
IMA will need to obtain the keyring used to verify file signatures so that
it can verify the module-style signature appended to files.
Signed-off-by: Thiago Jung Bauermann
Signed-off-by: Mimi Zohar
---
security/integrity/digsig.c| 28 +---
security/integrity/integrity.h | 6 ++
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 71c3200521d6..bbfa3085d1b5 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -44,11 +44,10 @@ static const char * const
keyring_name[INTEGRITY_KEYRING_MAX] = {
#define restrict_link_to_ima restrict_link_by_builtin_trusted
#endif
-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
- const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
{
- if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
- return -EINVAL;
+ if (id >= INTEGRITY_KEYRING_MAX)
+ return ERR_PTR(-EINVAL);
if (!keyring[id]) {
keyring[id] =
@@ -57,17 +56,32 @@ int integrity_digsig_verify(const unsigned int id, const
char *sig, int siglen,
int err = PTR_ERR(keyring[id]);
pr_err("no %s keyring: %d\n", keyring_name[id], err);
keyring[id] = NULL;
- return err;
+ return ERR_PTR(err);
}
}
+ return keyring[id];
+}
+
+int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
+ const char *digest, int digestlen)
+{
+ struct key *keyring;
+
+ if (siglen < 2)
+ return -EINVAL;
+
+ keyring = integrity_keyring_from_id(id);
+ if (IS_ERR(keyring))
+ return PTR_ERR(keyring);
+
switch (sig[1]) {
case 1:
/* v1 API expect signature without xattr type */
- return digsig_verify(keyring[id], sig + 1, siglen - 1,
+ return digsig_verify(keyring, sig + 1, siglen - 1,
digest, digestlen);
case 2:
- return asymmetric_verify(keyring[id], sig, siglen,
+ return asymmetric_verify(keyring, sig, siglen,
digest, digestlen);
}
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 2bf0fc51752b..6f657260a964 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -155,6 +155,7 @@ extern struct dentry *integrity_dir;
#ifdef CONFIG_INTEGRITY_SIGNATURE
+struct key *integrity_keyring_from_id(const unsigned int id);
int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
const char *digest, int digestlen);
@@ -164,6 +165,11 @@ int __init integrity_load_cert(const unsigned int id,
const char *source,
const void *data, size_t len, key_perm_t perm);
#else
+static inline struct key *integrity_keyring_from_id(const unsigned int id)
+{
+ return ERR_PTR(-EINVAL);
+}
+
static inline int integrity_digsig_verify(const unsigned int id,
const char *sig, int siglen,
const char *digest, int digestlen)
[PATCH v9 11/14] ima: Implement support for module-style appended signatures
Implement the appraise_type=imasig|modsig option, allowing IMA to read and
verify modsig signatures.
In case a file has both an xattr signature and an appended modsig, IMA will
only use the appended signature if the key used by the xattr signature
isn't present in the IMA keyring.
Also enable building the sign-file tool when CONFIG_IMA_APPRAISE_MODSIG is
enabled, so that the user can sign files using this format.
Signed-off-by: Thiago Jung Bauermann
---
scripts/Makefile | 4 +-
security/integrity/digsig.c | 3 +
security/integrity/ima/Kconfig| 3 +
security/integrity/ima/ima.h | 31 -
security/integrity/ima/ima_appraise.c | 68 ++-
security/integrity/ima/ima_main.c | 18 ++-
security/integrity/ima/ima_modsig.c | 162 ++
security/integrity/integrity.h| 10 ++
8 files changed, 291 insertions(+), 8 deletions(-)
diff --git a/scripts/Makefile b/scripts/Makefile
index ece52ff20171..a2cf10661925 100644
--- a/scripts/Makefile
+++ b/scripts/Makefile
@@ -17,7 +17,9 @@ hostprogs-$(CONFIG_VT) += conmakehash
hostprogs-$(BUILD_C_RECORDMCOUNT) += recordmcount
hostprogs-$(CONFIG_BUILDTIME_EXTABLE_SORT) += sortextable
hostprogs-$(CONFIG_ASN1)+= asn1_compiler
-hostprogs-$(CONFIG_MODULE_SIG) += sign-file
+ifneq ($(CONFIG_MODULE_SIG)$(CONFIG_IMA_APPRAISE_MODSIG),)
+hostprogs-y += sign-file
+endif
hostprogs-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += extract-cert
hostprogs-$(CONFIG_SYSTEM_EXTRA_CERTIFICATE) += insert-sys-cert
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index bbfa3085d1b5..c5585e75d5d9 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -75,6 +75,9 @@ int integrity_digsig_verify(const unsigned int id, const char
*sig, int siglen,
if (IS_ERR(keyring))
return PTR_ERR(keyring);
+ if (sig[0] == IMA_MODSIG)
+ return ima_modsig_verify(keyring, sig);
+
switch (sig[1]) {
case 1:
/* v1 API expect signature without xattr type */
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index bba19f9ea184..0fb542455698 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -234,6 +234,9 @@ config IMA_APPRAISE_BOOTPARAM
config IMA_APPRAISE_MODSIG
bool "Support module-style signatures for appraisal"
depends on IMA_APPRAISE
+ depends on INTEGRITY_ASYMMETRIC_KEYS
+ select PKCS7_MESSAGE_PARSER
+ select MODULE_SIG_FORMAT
default n
help
Adds support for signatures appended to files. The format of the
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 69c06e2d7bd6..753d59352718 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -156,7 +156,8 @@ void ima_init_template_list(void);
static inline bool is_signed(const struct evm_ima_xattr_data *xattr_value)
{
- return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+ return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
+ xattr_value->type == IMA_MODSIG);
}
/*
@@ -253,6 +254,9 @@ enum integrity_status ima_get_cache_status(struct
integrity_iint_cache *iint,
enum ima_hooks func);
enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
int xattr_len);
+bool ima_xattr_sig_known_key(enum ima_hooks func,
+const struct evm_ima_xattr_data *xattr_value,
+int xattr_len);
int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value);
@@ -291,6 +295,13 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
int xattr_len)
return ima_hash_algo;
}
+static inline bool ima_xattr_sig_known_key(enum ima_hooks func,
+ const struct evm_ima_xattr_data
+ *xattr_value, int xattr_len)
+{
+ return false;
+}
+
static inline int ima_read_xattr(struct dentry *dentry,
struct evm_ima_xattr_data **xattr_value)
{
@@ -301,11 +312,29 @@ static inline int ima_read_xattr(struct dentry *dentry,
#ifdef CONFIG_IMA_APPRAISE_MODSIG
bool ima_hook_supports_modsig(enum ima_hooks func);
+int ima_read_collect_modsig(enum ima_hooks func, const void *buf,
+ loff_t buf_len,
+ struct evm_ima_xattr_data **xattr_value,
+ int *xattr_len);
+void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
{
return false;
}
+
+static inline int ima_read_collect_mod
[PATCH v9 10/14] ima: Add modsig appraise_type option for module-style appended signatures
Introduce the modsig keyword to the IMA policy syntax to specify that
a given hook should expect the file to have the IMA signature appended
to it. Here is how it can be used in a rule:
appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig
With this rule, IMA will accept either a signature stored in the extended
attribute or an appended signature.
For now, the rule above will behave exactly the same as if
appraise_type=imasig was specified. The actual modsig implementation
will be introduced separately.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
Documentation/ABI/testing/ima_policy | 6 +-
security/integrity/ima/Kconfig | 10 +
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima.h | 9
security/integrity/ima/ima_modsig.c | 31
security/integrity/ima/ima_policy.c | 12 +--
security/integrity/integrity.h | 1 +
7 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/ima_policy
b/Documentation/ABI/testing/ima_policy
index 74c6702de74e..9d1dfd0a8891 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -37,7 +37,7 @@ Description:
euid:= decimal value
fowner:= decimal value
lsm:are LSM specific
- option: appraise_type:= [imasig]
+ option: appraise_type:= [imasig] [imasig|modsig]
pcr:= decimal value
default policy:
@@ -103,3 +103,7 @@ Description:
measure func=KEXEC_KERNEL_CHECK pcr=4
measure func=KEXEC_INITRAMFS_CHECK pcr=5
+
+ Example of appraise rule allowing modsig appended signatures:
+
+ appraise func=KEXEC_KERNEL_CHECK
appraise_type=imasig|modsig
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index a18f8c6d13b5..bba19f9ea184 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -231,6 +231,16 @@ config IMA_APPRAISE_BOOTPARAM
This option enables the different "ima_appraise=" modes
(eg. fix, log) from the boot command line.
+config IMA_APPRAISE_MODSIG
+ bool "Support module-style signatures for appraisal"
+ depends on IMA_APPRAISE
+ default n
+ help
+ Adds support for signatures appended to files. The format of the
+ appended signature is the same used for signed kernel modules.
+ The modsig keyword can be used in the IMA policy to allow a hook
+ to accept such signatures.
+
config IMA_TRUSTED_KEYRING
bool "Require all keys on the .ima keyring be signed (deprecated)"
depends on IMA_APPRAISE && SYSTEM_TRUSTED_KEYRING
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index d921dc4f9eb0..31d57cdf2421 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -9,5 +9,6 @@ obj-$(CONFIG_IMA) += ima.o
ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
ima_policy.o ima_template.o ima_template_lib.o
ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
+ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
obj-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0bc2a182cbf..69c06e2d7bd6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -299,6 +299,15 @@ static inline int ima_read_xattr(struct dentry *dentry,
#endif /* CONFIG_IMA_APPRAISE */
+#ifdef CONFIG_IMA_APPRAISE_MODSIG
+bool ima_hook_supports_modsig(enum ima_hooks func);
+#else
+static inline bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ return false;
+}
+#endif /* CONFIG_IMA_APPRAISE_MODSIG */
+
/* LSM based policy rules require audit */
#ifdef CONFIG_IMA_LSM_RULES
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
new file mode 100644
index ..08182bd7f445
--- /dev/null
+++ b/security/integrity/ima/ima_modsig.c
@@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * IMA support for appraising module-style appended signatures.
+ *
+ * Copyright (C) 2018 IBM Corporation
+ *
+ * Author:
+ * Thiago Jung Bauermann
+ */
+
+#include "ima.h"
+
+/**
+ * ima_hook_supports_modsig - can the policy allow modsig for this hook?
+ *
+ * modsig is only supported by hooks using ima_post_read_file, because only
they
+ * preload the contents of the file in a buffer. FILE_CHECK does that in some
+ * cases, but not when reached from vfs_open. POLICY_CHECK can support it, but
+ * it's not useful in practice because it's a text file so deny.
+ */
+bool ima_hook_supports_modsig(enum ima_hooks func)
+{
+ swi
[PATCH v9 08/14] ima: Introduce is_signed()
With the introduction of another IMA signature type (modsig), some places
will need to check for both of them. It is cleaner to do that if there's a
helper function to tell whether an xattr_value represents an IMA
signature.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 5 +
security/integrity/ima/ima_appraise.c | 7 +++
security/integrity/ima/ima_template_lib.c | 2 +-
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index cc12f3449a72..e4f72b30cb28 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,11 @@ unsigned long ima_get_binary_runtime_size(void);
int ima_init_template(void);
void ima_init_template_list(void);
+static inline bool is_signed(const struct evm_ima_xattr_data *xattr_value)
+{
+ return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
+}
+
/*
* used to protect h_table and sha_table
*/
diff --git a/security/integrity/ima/ima_appraise.c
b/security/integrity/ima/ima_appraise.c
index dcb8226972cf..085386c77b0b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -335,15 +335,14 @@ int ima_appraise_measurement(enum ima_hooks func,
} else if (status != INTEGRITY_PASS) {
/* Fix mode, but don't replace file signatures. */
if ((ima_appraise & IMA_APPRAISE_FIX) &&
- (!xattr_value ||
-xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
+ !is_signed(xattr_value)) {
if (!ima_fix_xattr(dentry, iint))
status = INTEGRITY_PASS;
}
/* Permit new files with file signatures, but without data. */
if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
- xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
+ is_signed(xattr_value)) {
status = INTEGRITY_PASS;
}
@@ -458,7 +457,7 @@ int ima_inode_setxattr(struct dentry *dentry, const char
*xattr_name,
if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
return -EINVAL;
ima_reset_appraise_flags(d_backing_inode(dentry),
- xvalue->type == EVM_IMA_XATTR_DIGSIG);
+is_signed(xvalue));
result = 0;
}
return result;
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 43752002c222..300912914b17 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -382,7 +382,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
- if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
+ if (!is_signed(xattr_value))
return 0;
return ima_write_template_field_data(xattr_value, event_data->xattr_len,
[PATCH v9 13/14] ima: Write modsig to the measurement list
Add modsig support to the "sig" template field, allowing the the contents
of the modsig to be included in the measurement list.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 7 +++
security/integrity/ima/ima_modsig.c | 13 +
security/integrity/ima/ima_template_lib.c | 15 ++-
3 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 40a6ddfdd9ea..55f8ef65cab4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -318,6 +318,7 @@ int ima_read_collect_modsig(enum ima_hooks func, const void
*buf,
int *xattr_len);
int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
const u8 **hash, u8 *len);
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len);
void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
@@ -340,6 +341,12 @@ static inline int ima_get_modsig_hash(struct
evm_ima_xattr_data *hdr,
return -EOPNOTSUPP;
}
+static inline int ima_modsig_serialize_data(struct evm_ima_xattr_data **data,
+ int *data_len)
+{
+ return -EOPNOTSUPP;
+}
+
static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
{
kfree(hdr);
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
index 587b79a9afef..0424f844c4c3 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -190,6 +190,19 @@ int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
enum hash_algo *algo,
return pkcs7_get_digest(modsig->pkcs7_msg, hash, len);
}
+int ima_modsig_serialize_data(struct evm_ima_xattr_data **data, int *data_len)
+{
+ struct modsig_hdr *modsig = (struct modsig_hdr *) *data;
+
+ if (!*data || (*data)->type != IMA_MODSIG)
+ return -EINVAL;
+
+ *data = &modsig->raw_pkcs7;
+ *data_len = modsig->raw_pkcs7_len;
+
+ return 0;
+}
+
int ima_modsig_verify(struct key *keyring, const void *hdr)
{
const struct modsig_hdr *modsig = (const struct modsig_hdr *) hdr;
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 36d175816894..417cd153ba60 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -411,10 +411,23 @@ int ima_eventsig_init(struct ima_event_data *event_data,
struct ima_field_data *field_data)
{
struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
+ int xattr_len = event_data->xattr_len;
if (!is_signed(xattr_value))
return 0;
- return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+ /*
+* The xattr_value for IMA_MODSIG is a runtime structure containing
+* pointers. Get its raw data instead.
+*/
+ if (xattr_value->type == IMA_MODSIG) {
+ int rc;
+
+ rc = ima_modsig_serialize_data(&xattr_value, &xattr_len);
+ if (rc)
+ return rc;
+ }
+
+ return ima_write_template_field_data(xattr_value, xattr_len,
DATA_FMT_HEX, field_data);
}
[PATCH v9 12/14] ima: Add new "d-sig" template field
Define new "d-sig" template field which holds the digest that is expected
to match the one contained in the modsig.
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
Documentation/security/IMA-templates.rst | 5
security/integrity/ima/ima.h | 9 +++
security/integrity/ima/ima_modsig.c | 23
security/integrity/ima/ima_template.c | 4 ++-
security/integrity/ima/ima_template_lib.c | 32 ++-
security/integrity/ima/ima_template_lib.h | 2 ++
6 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/Documentation/security/IMA-templates.rst
b/Documentation/security/IMA-templates.rst
index 2cd0e273cc9a..f2a0f4225857 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -68,6 +68,11 @@ descriptors by adding their identifier to the format string
- 'd-ng': the digest of the event, calculated with an arbitrary hash
algorithm (field format: [:]digest, where the digest
prefix is shown only if the hash algorithm is not SHA1 or MD5);
+ - 'd-sig': the digest of the event for files that have an appended modsig.
This
+ field is calculated without including the modsig and thus will differ from
+ the total digest of the file, but it is what should match the digest
+ contained in the modsig (if it doesn't, the signature is invalid). It is
+ shown in the same format as 'd-ng';
- 'n-ng': the name of the event, without size limitations;
- 'sig': the file signature.
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 753d59352718..40a6ddfdd9ea 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -316,6 +316,8 @@ int ima_read_collect_modsig(enum ima_hooks func, const void
*buf,
loff_t buf_len,
struct evm_ima_xattr_data **xattr_value,
int *xattr_len);
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+ const u8 **hash, u8 *len);
void ima_free_xattr_data(struct evm_ima_xattr_data *hdr);
#else
static inline bool ima_hook_supports_modsig(enum ima_hooks func)
@@ -331,6 +333,13 @@ static inline int ima_read_collect_modsig(enum ima_hooks
func, const void *buf,
return -EOPNOTSUPP;
}
+static inline int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr,
+ enum hash_algo *algo, const u8 **hash,
+ u8 *len)
+{
+ return -EOPNOTSUPP;
+}
+
static inline void ima_free_xattr_data(struct evm_ima_xattr_data *hdr)
{
kfree(hdr);
diff --git a/security/integrity/ima/ima_modsig.c
b/security/integrity/ima/ima_modsig.c
index f228f333509d..587b79a9afef 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -167,6 +167,29 @@ int ima_read_collect_modsig(enum ima_hooks func, const
void *buf,
return rc;
}
+int ima_get_modsig_hash(struct evm_ima_xattr_data *hdr, enum hash_algo *algo,
+ const u8 **hash, u8 *len)
+{
+ struct modsig_hdr *modsig = (typeof(modsig)) hdr;
+ const struct public_key_signature *pks;
+ int i;
+
+ if (!hdr || hdr->type != IMA_MODSIG)
+ return -EINVAL;
+
+ pks = pkcs7_get_message_sig(modsig->pkcs7_msg);
+ if (!pks)
+ return -EBADMSG;
+
+ for (i = 0; i < HASH_ALGO__LAST; i++)
+ if (!strcmp(hash_algo_name[i], pks->hash_algo))
+ break;
+
+ *algo = i;
+
+ return pkcs7_get_digest(modsig->pkcs7_msg, hash, len);
+}
+
int ima_modsig_verify(struct key *keyring, const void *hdr)
{
const struct modsig_hdr *modsig = (const struct modsig_hdr *) hdr;
diff --git a/security/integrity/ima/ima_template.c
b/security/integrity/ima/ima_template.c
index b631b8bc7624..045ad508cbb8 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -43,8 +43,10 @@ static const struct ima_template_field supported_fields[] = {
.field_show = ima_show_template_string},
{.field_id = "sig", .field_init = ima_eventsig_init,
.field_show = ima_show_template_sig},
+ {.field_id = "d-sig", .field_init = ima_eventdigest_sig_init,
+.field_show = ima_show_template_digest_ng},
};
-#define MAX_TEMPLATE_NAME_LEN 15
+#define MAX_TEMPLATE_NAME_LEN 24
static struct ima_template_desc *ima_template;
static struct ima_template_desc *lookup_template_desc(const char *name);
diff --git a/security/integrity/ima/ima_template_lib.c
b/security/integrity/ima/ima_template_lib.c
index 300912914b17..36d175816894 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -222,7 +222,8 @@ int ima_parse_buf(void *bufstart
[PATCH v9 09/14] ima: Export func_tokens
ima_read_modsig() will need it so that it can show an error message.
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h| 2 ++
security/integrity/ima/ima_policy.c | 12 ++--
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e4f72b30cb28..f0bc2a182cbf 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -195,6 +195,8 @@ enum ima_hooks {
__ima_hooks(__ima_hook_enumify)
};
+extern const char *const func_tokens[];
+
/* LIM API function definitions */
int ima_get_action(struct inode *inode, const struct cred *cred, u32 secid,
int mask, enum ima_hooks func, int *pcr);
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index d17a23b5c91d..b7ee342fbe4a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1138,6 +1138,12 @@ void ima_delete_rules(void)
}
}
+#define __ima_hook_stringify(str) (#str),
+
+const char *const func_tokens[] = {
+ __ima_hooks(__ima_hook_stringify)
+};
+
#ifdef CONFIG_IMA_READ_POLICY
enum {
mask_exec = 0, mask_write, mask_read, mask_append
@@ -1150,12 +1156,6 @@ static const char *const mask_tokens[] = {
"MAY_APPEND"
};
-#define __ima_hook_stringify(str) (#str),
-
-static const char *const func_tokens[] = {
- __ima_hooks(__ima_hook_stringify)
-};
-
void *ima_policy_start(struct seq_file *m, loff_t *pos)
{
loff_t l = *pos;
[PATCH v9 14/14] ima: Store the measurement again when appraising a modsig
If the IMA template contains the 'sig' field, then the modsig should be
added to the measurement list when the file is appraised, and that is what
normally happens.
But If a measurement rule caused a file containing a modsig to be measured
before a different rule causes it to be appraised, the resulting
measurement entry will not contain the modsig because it is only fetched
during appraisal. When the appraisal rule triggers, it won't store a new
measurement containing the modsig because the file was already measured.
We need to detect that situation and store an additional measurement with
the modsig. This is done by defining the appraise subaction flag
IMA_READ_MEASURE and testing for it in process_measurement().
Suggested-by: Mimi Zohar
Signed-off-by: Thiago Jung Bauermann
---
security/integrity/ima/ima.h | 1 +
security/integrity/ima/ima_api.c | 9 +++-
security/integrity/ima/ima_main.c | 17 ++--
security/integrity/ima/ima_policy.c | 59 ---
security/integrity/ima/ima_template.c | 24 +++
security/integrity/integrity.h| 9 ++--
6 files changed, 107 insertions(+), 12 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 55f8ef65cab4..c163d9bf248c 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -147,6 +147,7 @@ int ima_init_crypto(void);
void ima_putc(struct seq_file *m, void *data, int datalen);
void ima_print_digest(struct seq_file *m, u8 *digest, u32 size);
struct ima_template_desc *ima_template_desc_current(void);
+bool ima_template_has_sig(void);
int ima_restore_measurement_entry(struct ima_template_entry *entry);
int ima_restore_measurement_list(loff_t bufsize, void *buf);
int ima_measurements_show(struct seq_file *m, void *v);
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 99dd1d53fc35..cb72c9b7d84b 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -289,7 +289,14 @@ void ima_store_measurement(struct integrity_iint_cache
*iint,
xattr_len, NULL};
int violation = 0;
- if (iint->measured_pcrs & (0x1 << pcr))
+ /*
+* We still need to store the measurement in the case of MODSIG because
+* we only have its contents to put in the list at the time of
+* appraisal, but a file measurement from earlier might already exist in
+* the measurement list.
+*/
+ if (iint->measured_pcrs & (0x1 << pcr) &&
+ (!xattr_value || xattr_value->type != IMA_MODSIG))
return;
result = ima_alloc_init_template(&event_data, &entry);
diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 448be1e00bab..072cfb061a29 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -289,9 +289,20 @@ static int process_measurement(struct file *file, const
struct cred *cred,
*/
if (read_sig && iint->flags & IMA_MODSIG_ALLOWED &&
(xattr_len <= 0 || !ima_xattr_sig_known_key(func, xattr_value,
- xattr_len)))
- ima_read_collect_modsig(func, buf, size, &xattr_value,
- &xattr_len);
+ xattr_len))) {
+ rc = ima_read_collect_modsig(func, buf, size, &xattr_value,
+&xattr_len);
+
+ /*
+* A file measurement might already exist in the measurement
+* list. Based on policy, include an additional file measurement
+* containing the appended signature and file hash, without the
+* appended signature (i.e., the 'd-sig' field).
+*/
+ if (!rc && iint->flags & IMA_READ_MEASURE &&
+ ima_template_has_sig())
+ action |= IMA_MEASURE;
+ }
rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
if (rc != 0 && rc != -EBADF && rc != -EINVAL)
diff --git a/security/integrity/ima/ima_policy.c
b/security/integrity/ima/ima_policy.c
index c38a63f56b7b..1cce69197235 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -10,6 +10,9 @@
* - initialize default measure policy rules
*
*/
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
#include
#include
#include
@@ -369,7 +372,8 @@ static bool ima_match_rules(struct ima_rule_entry *rule,
struct inode *inode,
* In addition to knowing that we need to appraise the file in general,
* we need to differentiate between calling hooks, for hoo
[PATCH v9 07/14] integrity: Select CONFIG_KEYS instead of depending on it
This avoids a dependency cycle in soon-to-be-introduced CONFIG_IMA_APPRAISE_MODSIG: it will select CONFIG_MODULE_SIG_FORMAT which in turn selects CONFIG_KEYS. Kconfig then complains that CONFIG_INTEGRITY_SIGNATURE depends on CONFIG_KEYS. Signed-off-by: Thiago Jung Bauermann Signed-off-by: Mimi Zohar --- security/integrity/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig index 4b4d2aeef539..176905bef20a 100644 --- a/security/integrity/Kconfig +++ b/security/integrity/Kconfig @@ -17,8 +17,8 @@ if INTEGRITY config INTEGRITY_SIGNATURE bool "Digital signature verification using multiple keyrings" - depends on KEYS default n + select KEYS select SIGNATURE help This option enables digital signature verification support
[PATCH v9 02/14] PKCS#7: Refactor verify_pkcs7_signature() and add pkcs7_get_message_sig()
IMA will need to verify a PKCS#7 which has already been parsed. For this
reason, factor out the code which does that from verify_pkcs7_signature()
into a new function which takes a struct pkcs7_message instead of a data
buffer.
In addition, IMA will need to know the key that signed a given PKCS#7
message, so add pkcs7_get_message_sig().
Signed-off-by: Thiago Jung Bauermann
Reviewed-by: Mimi Zohar
Cc: David Howells
Cc: David Woodhouse
Cc: Herbert Xu
Cc: "David S. Miller"
---
certs/system_keyring.c| 61 ---
crypto/asymmetric_keys/pkcs7_parser.c | 16 +++
include/crypto/pkcs7.h| 2 +
include/linux/verification.h | 10 +
4 files changed, 73 insertions(+), 16 deletions(-)
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 81728717523d..dd8c5ef941ce 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -191,33 +191,27 @@ late_initcall(load_system_certificate_list);
#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
/**
- * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * verify_pkcs7_message_sig - Verify a PKCS#7-based signature on system data.
* @data: The data to be verified (NULL if expecting internal data).
* @len: Size of @data.
- * @raw_pkcs7: The PKCS#7 message that is the signature.
- * @pkcs7_len: The size of @raw_pkcs7.
+ * @pkcs7: The PKCS#7 message that is the signature.
* @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
* (void *)1UL for all trusted keys).
* @usage: The use to which the key is being put.
* @view_content: Callback to gain access to content.
* @ctx: Context for callback.
*/
-int verify_pkcs7_signature(const void *data, size_t len,
- const void *raw_pkcs7, size_t pkcs7_len,
- struct key *trusted_keys,
- enum key_being_used_for usage,
- int (*view_content)(void *ctx,
- const void *data, size_t len,
- size_t asn1hdrlen),
- void *ctx)
+int verify_pkcs7_message_sig(const void *data, size_t len,
+struct pkcs7_message *pkcs7,
+struct key *trusted_keys,
+enum key_being_used_for usage,
+int (*view_content)(void *ctx,
+const void *data, size_t len,
+size_t asn1hdrlen),
+void *ctx)
{
- struct pkcs7_message *pkcs7;
int ret;
- pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
- if (IS_ERR(pkcs7))
- return PTR_ERR(pkcs7);
-
/* The data should be detached - so we need to supply it. */
if (data && pkcs7_supply_detached_data(pkcs7, data, len) < 0) {
pr_err("PKCS#7 signature with non-detached data\n");
@@ -259,6 +253,41 @@ int verify_pkcs7_signature(const void *data, size_t len,
}
error:
+ pr_devel("<==%s() = %d\n", __func__, ret);
+ return ret;
+}
+
+/**
+ * verify_pkcs7_signature - Verify a PKCS#7-based signature on system data.
+ * @data: The data to be verified (NULL if expecting internal data).
+ * @len: Size of @data.
+ * @raw_pkcs7: The PKCS#7 message that is the signature.
+ * @pkcs7_len: The size of @raw_pkcs7.
+ * @trusted_keys: Trusted keys to use (NULL for builtin trusted keys only,
+ * (void *)1UL for all trusted keys).
+ * @usage: The use to which the key is being put.
+ * @view_content: Callback to gain access to content.
+ * @ctx: Context for callback.
+ */
+int verify_pkcs7_signature(const void *data, size_t len,
+ const void *raw_pkcs7, size_t pkcs7_len,
+ struct key *trusted_keys,
+ enum key_being_used_for usage,
+ int (*view_content)(void *ctx,
+ const void *data, size_t len,
+ size_t asn1hdrlen),
+ void *ctx)
+{
+ struct pkcs7_message *pkcs7;
+ int ret;
+
+ pkcs7 = pkcs7_parse_message(raw_pkcs7, pkcs7_len);
+ if (IS_ERR(pkcs7))
+ return PTR_ERR(pkcs7);
+
+ ret = verify_pkcs7_message_sig(data, len, pkcs7, trusted_keys, usage,
+ view_content, ctx);
+
pkcs7_free_message(pkcs7);
pr_devel("<==%s() = %d\n", __func__, ret);
return ret;
diff --git a/crypto/asymmetric_keys/pkcs7_parser.c
b/crypto/asymmetric_keys/pkcs7_parser.c
index f0d56e1a8b7e..8df9693f659f 100644
--- a/crypto/asymmetric_keys/pkcs7_parser.c
+++ b/crypt
Re: [PATCH v12 04/22] selftests/vm: typecast the pkey register
Dave Hansen writes:
> On 02/21/2018 05:55 PM, Ram Pai wrote:
>> -static inline unsigned int _rdpkey_reg(int line)
>> +static inline pkey_reg_t _rdpkey_reg(int line)
>> {
>> -unsigned int pkey_reg = __rdpkey_reg();
>> +pkey_reg_t pkey_reg = __rdpkey_reg();
>>
>> -dprintf4("rdpkey_reg(line=%d) pkey_reg: %x shadow: %x\n",
>> +dprintf4("rdpkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
>> line, pkey_reg, shadow_pkey_reg);
>> assert(pkey_reg == shadow_pkey_reg);
>
> Hmm. So we're using %lx for an int? Doesn't the compiler complain
> about this?
It doesn't because dprintf4() doesn't have the annotation that tells the
compiler that it takes printf-like arguments. Once I add it:
--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -54,6 +54,10 @@
#define DPRINT_IN_SIGNAL_BUF_SIZE 4096
extern int dprint_in_signal;
extern char dprint_in_signal_buffer[DPRINT_IN_SIGNAL_BUF_SIZE];
+
+#ifdef __GNUC__
+__attribute__((format(printf, 1, 2)))
+#endif
static inline void sigsafe_printf(const char *format, ...)
{
va_list ap;
Then it does complain about it. I'm working on a fix where each arch
will define a format string to use for its pkey_reg_t and use it like
this:
--- a/tools/testing/selftests/vm/pkey-helpers.h
+++ b/tools/testing/selftests/vm/pkey-helpers.h
@@ -19,6 +19,7 @@
#define u32 uint32_t
#define u64 uint64_t
#define pkey_reg_t u32
+#define PKEY_REG_FMT "%016x"
#ifdef __i386__
#ifndef SYS_mprotect_key
@@ -112,7 +113,8 @@ static inline pkey_reg_t _read_pkey_reg(int line)
{
pkey_reg_t pkey_reg = __read_pkey_reg();
- dprintf4("read_pkey_reg(line=%d) pkey_reg: %016lx shadow: %016lx\n",
+ dprintf4("read_pkey_reg(line=%d) pkey_reg: "PKEY_REG_FMT
+ " shadow: "PKEY_REG_FMT"\n",
line, pkey_reg, shadow_pkey_reg);
assert(pkey_reg == shadow_pkey_reg);
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
Dave Hansen writes:
> On 02/21/2018 05:55 PM, Ram Pai wrote:
>> --- a/tools/testing/selftests/vm/protection_keys.c
>> +++ b/tools/testing/selftests/vm/protection_keys.c
>> @@ -461,7 +461,7 @@ void pkey_disable_clear(int pkey, int flags)
>> pkey, pkey, pkey_rights);
>> pkey_assert(pkey_rights >= 0);
>>
>> -pkey_rights |= flags;
>> +pkey_rights &= ~flags;
>>
>> ret = pkey_set(pkey, pkey_rights, 0);
>> /* pkey_reg and flags have the same format */
>> @@ -475,7 +475,7 @@ void pkey_disable_clear(int pkey, int flags)
>> dprintf1("%s(%d) pkey_reg: 0x%016lx\n", __func__,
>> pkey, rdpkey_reg());
>> if (flags)
>> -assert(rdpkey_reg() > orig_pkey_reg);
>> +assert(rdpkey_reg() < orig_pkey_reg);
>> }
>>
>> void pkey_write_allow(int pkey)
>
> This seems so horribly wrong that I wonder how it worked in the first
> place. Any idea?
The code simply wasn't used. pkey_disable_clear() is called by
pkey_write_allow() and pkey_access_allow(), but before this patch series
nothing called either of these functions.
--
Thiago Jung Bauermann
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 07/22] selftests/vm: fixed bugs in pkey_disable_clear()
Dave Hansen writes: > On 03/28/2018 01:47 PM, Thiago Jung Bauermann wrote: >>>>if (flags) >>>> - assert(rdpkey_reg() > orig_pkey_reg); >>>> + assert(rdpkey_reg() < orig_pkey_reg); >>>> } >>>> >>>> void pkey_write_allow(int pkey) >>> This seems so horribly wrong that I wonder how it worked in the first >>> place. Any idea? >> The code simply wasn't used. pkey_disable_clear() is called by >> pkey_write_allow() and pkey_access_allow(), but before this patch series >> nothing called either of these functions. >> > > Ahh, that explains it. Can that get stuck in the changelog, please? Yes, will be in the next version. -- Thiago Jung Bauermann IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
