[v8,4/4] kexec, KEYS, s390: Make use of built-in and secondary keyring for signature verification

Commit Message

Coiby Xu May 12, 2022, 7:01 a.m. UTC

From: Michal Suchanek <[e-mail]>

commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
adds support for KEXEC_SIG verification with keys from platform keyring
but the built-in keys and secondary keyring are not used.

Add support for the built-in keys and secondary keyring as x86 does.

Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
Cc: [e-mail]
Cc: Philipp Rudo <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Signed-off-by: Michal Suchanek <[e-mail]>
Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
Acked-by: Baoquan He <[e-mail]>
Signed-off-by: Coiby Xu <[e-mail]>
---
 arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Heiko Carstens May 18, 2022, 11:29 a.m. UTC | #1

On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> From: Michal Suchanek <[e-mail]>
> 
> commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> adds support for KEXEC_SIG verification with keys from platform keyring
> but the built-in keys and secondary keyring are not used.
> 
> Add support for the built-in keys and secondary keyring as x86 does.
> 
> Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> Cc: [e-mail]
> Cc: Philipp Rudo <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Signed-off-by: Michal Suchanek <[e-mail]>
> Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
> Acked-by: Baoquan He <[e-mail]>
> Signed-off-by: Coiby Xu <[e-mail]>
> ---
>  arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)

As far as I can tell this doesn't have any dependency to the other
patches in this series, so should I pick this up for the s390 tree, or
how will this go upstream?

In any case:
Acked-by: Heiko Carstens <[e-mail]>

Baoquan He May 19, 2022, 12:39 a.m. UTC | #2

On 05/18/22 at 01:29pm, Heiko Carstens wrote:
> On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> > From: Michal Suchanek <[e-mail]>
> > 
> > commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > adds support for KEXEC_SIG verification with keys from platform keyring
> > but the built-in keys and secondary keyring are not used.
> > 
> > Add support for the built-in keys and secondary keyring as x86 does.
> > 
> > Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > Cc: [e-mail]
> > Cc: Philipp Rudo <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Signed-off-by: Michal Suchanek <[e-mail]>
> > Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
> > Acked-by: Baoquan He <[e-mail]>
> > Signed-off-by: Coiby Xu <[e-mail]>
> > ---
> >  arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> As far as I can tell this doesn't have any dependency to the other
> patches in this series, so should I pick this up for the s390 tree, or
> how will this go upstream?

Thanks, Heiko.

I want to ask Mimi if this can be taken into KEYS-ENCRYPTED tree.
Otherwise I will ask Andrew to help pick this whole series.

Surely, this patch 4 can be taken into s390 seperately since it's
independent, both looks good.

Thanks
Baoquan

Mimi Zohar May 19, 2022, 11:56 a.m. UTC | #3

[Cc'ing Jarkko, linux-integrity]

On Thu, 2022-05-19 at 08:39 +0800, Baoquan He wrote:
> On 05/18/22 at 01:29pm, Heiko Carstens wrote:
> > On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> > > From: Michal Suchanek <[e-mail]>
> > > 
> > > commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > adds support for KEXEC_SIG verification with keys from platform keyring
> > > but the built-in keys and secondary keyring are not used.
> > > 
> > > Add support for the built-in keys and secondary keyring as x86 does.
> > > 
> > > Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > Cc: [e-mail]
> > > Cc: Philipp Rudo <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Signed-off-by: Michal Suchanek <[e-mail]>
> > > Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
> > > Acked-by: Baoquan He <[e-mail]>
> > > Signed-off-by: Coiby Xu <[e-mail]>
> > > ---
> > >  arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > As far as I can tell this doesn't have any dependency to the other
> > patches in this series, so should I pick this up for the s390 tree, or
> > how will this go upstream?
> 
> Thanks, Heiko.
> 
> I want to ask Mimi if this can be taken into KEYS-ENCRYPTED tree.
> Otherwise I will ask Andrew to help pick this whole series.
> 
> Surely, this patch 4 can be taken into s390 seperately since it's
> independent, both looks good.

KEYS-ENCRYTPED is a type of key, unrelated to using the .platform,
.builtin, .machine, or .secondary keyrings.  One of the main reasons
for this patch set is to use the new ".machine" keyring, which, if
enabled, is linked to the "secondary" keyring.  However, the only
reference to the ".machine" keyring is in the cover letter, not any of
the patch descriptions.  Since this is the basis for the system's
integrity, this seems like a pretty big omission.

From patch 2/4:
"The code in bzImage64_verify_sig makes use of system keyrings
including
.buitin_trusted_keys, .secondary_trusted_keys and .platform keyring to
verify signed kernel image as PE file..."

From patch 3/4:
"This patch allows to verify arm64 kernel image signature using not
only
.builtin_trusted_keys but also .platform and .secondary_trusted_keys
keyring."

From patch 4/4:
"... with keys from platform keyring but the built-in keys and
secondary keyring are not used."

This patch set could probably go through KEYS/KEYRINGS_INTEGRITY, but
it's kind of late to be asking.  Has it been in linux-next?  Should I
assume this patch set has been fully tested or can we get some "tags"?

thanks,

Mimi

Baoquan He May 19, 2022, 2:22 p.m. UTC | #4

On 05/19/22 at 07:56am, Mimi Zohar wrote:
> [Cc'ing Jarkko, linux-integrity]
> 
> On Thu, 2022-05-19 at 08:39 +0800, Baoquan He wrote:
> > On 05/18/22 at 01:29pm, Heiko Carstens wrote:
> > > On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> > > > From: Michal Suchanek <[e-mail]>
> > > > 
> > > > commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > > adds support for KEXEC_SIG verification with keys from platform keyring
> > > > but the built-in keys and secondary keyring are not used.
> > > > 
> > > > Add support for the built-in keys and secondary keyring as x86 does.
> > > > 
> > > > Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > > Cc: [e-mail]
> > > > Cc: Philipp Rudo <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Signed-off-by: Michal Suchanek <[e-mail]>
> > > > Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
> > > > Acked-by: Baoquan He <[e-mail]>
> > > > Signed-off-by: Coiby Xu <[e-mail]>
> > > > ---
> > > >  arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > As far as I can tell this doesn't have any dependency to the other
> > > patches in this series, so should I pick this up for the s390 tree, or
> > > how will this go upstream?
> > 
> > Thanks, Heiko.
> > 
> > I want to ask Mimi if this can be taken into KEYS-ENCRYPTED tree.
> > Otherwise I will ask Andrew to help pick this whole series.
> > 
> > Surely, this patch 4 can be taken into s390 seperately since it's
> > independent, both looks good.
> 
> KEYS-ENCRYTPED is a type of key, unrelated to using the .platform,
> .builtin, .machine, or .secondary keyrings.  One of the main reasons
> for this patch set is to use the new ".machine" keyring, which, if
> enabled, is linked to the "secondary" keyring.  However, the only
> reference to the ".machine" keyring is in the cover letter, not any of
> the patch descriptions.  Since this is the basis for the system's
> integrity, this seems like a pretty big omission.
> 
> From patch 2/4:
> "The code in bzImage64_verify_sig makes use of system keyrings
> including
> .buitin_trusted_keys, .secondary_trusted_keys and .platform keyring to
> verify signed kernel image as PE file..."
> 
> From patch 3/4:
> "This patch allows to verify arm64 kernel image signature using not
> only
> .builtin_trusted_keys but also .platform and .secondary_trusted_keys
> keyring."
> 
> From patch 4/4:
> "... with keys from platform keyring but the built-in keys and
> secondary keyring are not used."
> 
> This patch set could probably go through KEYS/KEYRINGS_INTEGRITY, but
> it's kind of late to be asking.  Has it been in linux-next?  Should I
> assume this patch set has been fully tested or can we get some "tags"?

Right, it should be KEYS/KEYRINGS_INTEGRITY related, I made mistaken.
Now it got two ACKs from Michal and me. Michal met the same issue on
arm64 and posted another series of patches, finally Coiby integrated
Michal's patch and his to make this patchset. That would be great if
this can get reviewing from experts on key/keyring. Surely, Coiby need
update the patch log to add the '.machine' keyring into patch logs as
you pointed out.

IIRC, Coiby has tested it on x86_64/arm64, not sure if he took test on
s390. No, this hasn't been in linux-next.

Michal Suchanek May 19, 2022, 5:11 p.m. UTC | #5

On Thu, May 19, 2022 at 10:22:15PM +0800, Baoquan He wrote:
> On 05/19/22 at 07:56am, Mimi Zohar wrote:
> > [Cc'ing Jarkko, linux-integrity]
> > 
> > On Thu, 2022-05-19 at 08:39 +0800, Baoquan He wrote:
> > > On 05/18/22 at 01:29pm, Heiko Carstens wrote:
> > > > On Thu, May 12, 2022 at 03:01:23PM +0800, Coiby Xu wrote:
> > > > > From: Michal Suchanek <[e-mail]>
> > > > > 
> > > > > commit e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > > > adds support for KEXEC_SIG verification with keys from platform keyring
> > > > > but the built-in keys and secondary keyring are not used.
> > > > > 
> > > > > Add support for the built-in keys and secondary keyring as x86 does.
> > > > > 
> > > > > Fixes: e23a8020ce4e ("s390/kexec_file: Signature verification prototype")
> > > > > Cc: [e-mail]
> > > > > Cc: Philipp Rudo <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Signed-off-by: Michal Suchanek <[e-mail]>
> > > > > Reviewed-by: "Lee, Chun-Yi" <[e-mail]>
> > > > > Acked-by: Baoquan He <[e-mail]>
> > > > > Signed-off-by: Coiby Xu <[e-mail]>
> > > > > ---
> > > > >  arch/s390/kernel/machine_kexec_file.c | 18 +++++++++++++-----
> > > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > 
> > > > As far as I can tell this doesn't have any dependency to the other
> > > > patches in this series, so should I pick this up for the s390 tree, or
> > > > how will this go upstream?
> > > 
> > > Thanks, Heiko.
> > > 
> > > I want to ask Mimi if this can be taken into KEYS-ENCRYPTED tree.
> > > Otherwise I will ask Andrew to help pick this whole series.
> > > 
> > > Surely, this patch 4 can be taken into s390 seperately since it's
> > > independent, both looks good.
> > 
> > KEYS-ENCRYTPED is a type of key, unrelated to using the .platform,
> > .builtin, .machine, or .secondary keyrings.  One of the main reasons
> > for this patch set is to use the new ".machine" keyring, which, if
> > enabled, is linked to the "secondary" keyring.  However, the only
> > reference to the ".machine" keyring is in the cover letter, not any of
> > the patch descriptions.  Since this is the basis for the system's
> > integrity, this seems like a pretty big omission.
> > 
> > From patch 2/4:
> > "The code in bzImage64_verify_sig makes use of system keyrings
> > including
> > .buitin_trusted_keys, .secondary_trusted_keys and .platform keyring to
> > verify signed kernel image as PE file..."
> > 
> > From patch 3/4:
> > "This patch allows to verify arm64 kernel image signature using not
> > only
> > .builtin_trusted_keys but also .platform and .secondary_trusted_keys
> > keyring."
> > 
> > From patch 4/4:
> > "... with keys from platform keyring but the built-in keys and
> > secondary keyring are not used."
> > 
> > This patch set could probably go through KEYS/KEYRINGS_INTEGRITY, but
> > it's kind of late to be asking.  Has it been in linux-next?  Should I
> > assume this patch set has been fully tested or can we get some "tags"?
> 
> Right, it should be KEYS/KEYRINGS_INTEGRITY related, I made mistaken.
> Now it got two ACKs from Michal and me. Michal met the same issue on
> arm64 and posted another series of patches, finally Coiby integrated
> Michal's patch and his to make this patchset. That would be great if
> this can get reviewing from experts on key/keyring. Surely, Coiby need
> update the patch log to add the '.machine' keyring into patch logs as
> you pointed out.
> 
> IIRC, Coiby has tested it on x86_64/arm64, not sure if he took test on
> s390. No, this hasn't been in linux-next.

I used the s390 code on powerpc and there it did not work because the
built-in key was needed to verify the kernel.

I did not really run this on s390, only ported the fix I needed on
powerpc back to s390.

Thanks

Michal

Coiby Xu June 16, 2022, 1:46 a.m. UTC | #6

Hi Mimi,

>> >
>> > This patch set could probably go through KEYS/KEYRINGS_INTEGRITY, but
>> > it's kind of late to be asking.  Has it been in linux-next?  Should I
>> > assume this patch set has been fully tested or can we get some "tags"?
>>
[...]
>>
>> IIRC, Coiby has tested it on x86_64/arm64, not sure if he took test on
>> s390. No, this hasn't been in linux-next.

For arm64, recently I did a new round of test and the patches works as
expected,
   1. Build 5.19.0-rc2
   2. generate keys and add them to .secondary_trusted_keys, MOK, UEFI
      db; 
   3. sign different kernel images with different keys including keys
      from .builtin_trusted_key, .secondary_trusted_keys keyring, UEFI db
      key and MOK key 
   4. Without lockdown, all kernel images can be kexec'ed; with lockdown
      enabled, only the kernel image signed by the key from
      .builtin_trusted_key can be kexec'ed

Then I build a new kernel with the patches applied and confirm all
kernel images can be kexec'ed.

>
>I used the s390 code on powerpc and there it did not work because the
>built-in key was needed to verify the kernel.
>
>I did not really run this on s390, only ported the fix I needed on
>powerpc back to s390.

For 390, I commented out the code that skips signature verification
when secure boot is not enabled since I couldn't find a machine that
supports secure boot and confirm before applying the patch, kernel
images signed by keys from .builtin_trusted_key, .secondary_trusted_keys
couldn't be kexec'ed when lockdown is enabled; after applying the
patch, those kernel images could be kexec'ed. 

>
>Thanks
>
>Michal
>

Patch

diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index 8f43575a4dd3..fc6d5f58debe 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -31,6 +31,7 @@  int s390_verify_sig(const char *kernel, unsigned long kernel_len)
 	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
 	struct module_signature *ms;
 	unsigned long sig_len;
+	int ret;
 
 	/* Skip signature verification when not secure IPLed. */
 	if (!ipl_secure_flag)
@@ -65,11 +66,18 @@  int s390_verify_sig(const char *kernel, unsigned long kernel_len)
 		return -EBADMSG;
 	}
 
-	return verify_pkcs7_signature(kernel, kernel_len,
-				      kernel + kernel_len, sig_len,
-				      VERIFY_USE_PLATFORM_KEYRING,
-				      VERIFYING_MODULE_SIGNATURE,
-				      NULL, NULL);
+	ret = verify_pkcs7_signature(kernel, kernel_len,
+				     kernel + kernel_len, sig_len,
+				     VERIFY_USE_SECONDARY_KEYRING,
+				     VERIFYING_MODULE_SIGNATURE,
+				     NULL, NULL);
+	if (ret == -ENOKEY && IS_ENABLED(CONFIG_INTEGRITY_PLATFORM_KEYRING))
+		ret = verify_pkcs7_signature(kernel, kernel_len,
+					     kernel + kernel_len, sig_len,
+					     VERIFY_USE_PLATFORM_KEYRING,
+					     VERIFYING_MODULE_SIGNATURE,
+					     NULL, NULL);
+	return ret;
 }
 #endif /* CONFIG_KEXEC_SIG */