Signature verification is an important security feature, to protect
system from being attacked with a kernel of unknown origin. Kexec
rebooting is a way to replace the running kernel, hence need be
secured carefully.
In the current code of handling signature verification of kexec kernel,
the logic is very twisted. It mixes signature verification, IMA signature
appraising and kexec lockdown.
If there is no KEXEC_SIG_FORCE, kexec kernel image doesn't have one of
signature, the supported crypto, and key, we don't think this is wrong,
Unless kexec lockdown is executed. IMA is considered as another kind of
signature appraising method.
If kexec kernel image has signature/crypto/key, it has to go through the
signature verification and pass. Otherwise it's seen as verification
failure, and won't be loaded.
Seems kexec kernel image with an unqualified signature is even worse than
those w/o signature at all, this sounds very unreasonable. E.g. If people
get a unsigned kernel to load, or a kernel signed with expired key, which
one is more dangerous?
So, here, let's simplify the logic to improve code readability. If the
KEXEC_SIG_FORCE enabled or kexec lockdown enabled, signature verification
is mandated. Otherwise, we lift the bar for any kernel image.
Signed-off-by: Lianbo Jiang <[email protected]>
---
kernel/kexec_file.c | 37 ++++++-------------------------------
1 file changed, 6 insertions(+), 31 deletions(-)
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index faa74d5f6941..e4bdf0c42f35 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -181,52 +181,27 @@ void kimage_file_post_load_cleanup(struct kimage *image)
static int
kimage_validate_signature(struct kimage *image)
{
- const char *reason;
int ret;
ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
image->kernel_buf_len);
- switch (ret) {
- case 0:
- break;
+ if (ret) {
+ pr_debug("kernel signature verification failed (%d).\n", ret);
- /* Certain verification errors are non-fatal if we're not
- * checking errors, provided we aren't mandating that there
- * must be a valid signature.
- */
- case -ENODATA:
- reason = "kexec of unsigned image";
- goto decide;
- case -ENOPKG:
- reason = "kexec of image with unsupported crypto";
- goto decide;
- case -ENOKEY:
- reason = "kexec of image with unavailable key";
- decide:
- if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
- pr_notice("%s rejected\n", reason);
+ if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE))
return ret;
- }
- /* If IMA is guaranteed to appraise a signature on the kexec
+ /*
+ * If IMA is guaranteed to appraise a signature on the kexec
* image, permit it even if the kernel is otherwise locked
* down.
*/
if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
security_locked_down(LOCKDOWN_KEXEC))
return -EPERM;
-
- return 0;
-
- /* All other errors are fatal, including nomem, unparseable
- * signatures and signature check failures - even if signatures
- * aren't required.
- */
- default:
- pr_notice("kernel signature verification failed (%d).\n", ret);
}
- return ret;
+ return 0;
}
#endif
--
2.17.1
On 05/25/20 at 01:23pm, Lianbo Jiang wrote:
> Signature verification is an important security feature, to protect
> system from being attacked with a kernel of unknown origin. Kexec
> rebooting is a way to replace the running kernel, hence need be
> secured carefully.
>
> In the current code of handling signature verification of kexec kernel,
> the logic is very twisted. It mixes signature verification, IMA signature
> appraising and kexec lockdown.
>
> If there is no KEXEC_SIG_FORCE, kexec kernel image doesn't have one of
> signature, the supported crypto, and key, we don't think this is wrong,
> Unless kexec lockdown is executed. IMA is considered as another kind of
> signature appraising method.
>
> If kexec kernel image has signature/crypto/key, it has to go through the
> signature verification and pass. Otherwise it's seen as verification
> failure, and won't be loaded.
>
> Seems kexec kernel image with an unqualified signature is even worse than
> those w/o signature at all, this sounds very unreasonable. E.g. If people
> get a unsigned kernel to load, or a kernel signed with expired key, which
> one is more dangerous?
>
> So, here, let's simplify the logic to improve code readability. If the
> KEXEC_SIG_FORCE enabled or kexec lockdown enabled, signature verification
> is mandated. Otherwise, we lift the bar for any kernel image.
>
> Signed-off-by: Lianbo Jiang <[email protected]>
> ---
> kernel/kexec_file.c | 37 ++++++-------------------------------
> 1 file changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index faa74d5f6941..e4bdf0c42f35 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -181,52 +181,27 @@ void kimage_file_post_load_cleanup(struct kimage *image)
> static int
> kimage_validate_signature(struct kimage *image)
> {
> - const char *reason;
> int ret;
>
> ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
> image->kernel_buf_len);
> - switch (ret) {
> - case 0:
> - break;
> + if (ret) {
> + pr_debug("kernel signature verification failed (%d).\n", ret);
>
> - /* Certain verification errors are non-fatal if we're not
> - * checking errors, provided we aren't mandating that there
> - * must be a valid signature.
> - */
> - case -ENODATA:
> - reason = "kexec of unsigned image";
> - goto decide;
> - case -ENOPKG:
> - reason = "kexec of image with unsupported crypto";
> - goto decide;
> - case -ENOKEY:
> - reason = "kexec of image with unavailable key";
> - decide:
> - if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
> - pr_notice("%s rejected\n", reason);
> + if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE))
> return ret;
> - }
>
> - /* If IMA is guaranteed to appraise a signature on the kexec
> + /*
> + * If IMA is guaranteed to appraise a signature on the kexec
> * image, permit it even if the kernel is otherwise locked
> * down.
> */
> if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
> security_locked_down(LOCKDOWN_KEXEC))
> return -EPERM;
> -
> - return 0;
> -
> - /* All other errors are fatal, including nomem, unparseable
> - * signatures and signature check failures - even if signatures
> - * aren't required.
> - */
> - default:
> - pr_notice("kernel signature verification failed (%d).\n", ret);
> }
>
> - return ret;
> + return 0;
> }
> #endif
>
> --
> 2.17.1
>
Acked-by: Dave Young <[email protected]>
Thanks
Dave
On Mon, May 25, 2020 at 01:23:51PM +0800, Lianbo Jiang wrote:
> So, here, let's simplify the logic to improve code readability. If the
> KEXEC_SIG_FORCE enabled or kexec lockdown enabled, signature verification
> is mandated. Otherwise, we lift the bar for any kernel image.
I agree completely; in fact that was my intention when
introducing the code, but I got overruled about the return codes:
https://lore.kernel.org/lkml/[email protected]/
I like this simplification very much, except this part:
> + if (ret) {
> + pr_debug("kernel signature verification failed (%d).\n", ret);
...
> - pr_notice("kernel signature verification failed (%d).\n", ret);
I think the log level should stay at most PR_NOTICE when the
verification failure results in rejecting the kernel. Perhaps
even lower.
In case verification is not enforced and the failure is
ignored, KERN_DEBUG seems reasonable.
Regards,
--
Jiri Bohac <[email protected]>
SUSE Labs, Prague, Czechia
在 2020年05月26日 21:59, Jiri Bohac 写道:
> On Mon, May 25, 2020 at 01:23:51PM +0800, Lianbo Jiang wrote:
>> So, here, let's simplify the logic to improve code readability. If the
>> KEXEC_SIG_FORCE enabled or kexec lockdown enabled, signature verification
>> is mandated. Otherwise, we lift the bar for any kernel image.
>
> I agree completely; in fact that was my intention when
> introducing the code, but I got overruled about the return codes:
> https://lore.kernel.org/lkml/[email protected]/
>
> I like this simplification very much, except this part:
>
>> + if (ret) {
>> + pr_debug("kernel signature verification failed (%d).\n", ret);
>
> ...
>
>> - pr_notice("kernel signature verification failed (%d).\n", ret);
>
> I think the log level should stay at most PR_NOTICE when the
> verification failure results in rejecting the kernel. Perhaps
> even lower.
>
Thank you for the comment, Jiri Bohac.
I like the idea of staying at most PR_NOTICE, but the pr_notice() will output
some messages that kernel could want to ignore, such as the case you mentioned
below.
> In case verification is not enforced and the failure is
> ignored, KERN_DEBUG seems reasonable.
>
Yes, good understanding. It seems that the pr_debug() is still a good option here?
Any other thoughts?
Thanks.
Lianbo
> Regards,
>
在 2020年05月27日 11:15, lijiang 写道:
> 在 2020年05月26日 21:59, Jiri Bohac 写道:
>> On Mon, May 25, 2020 at 01:23:51PM +0800, Lianbo Jiang wrote:
>>> So, here, let's simplify the logic to improve code readability. If the
>>> KEXEC_SIG_FORCE enabled or kexec lockdown enabled, signature verification
>>> is mandated. Otherwise, we lift the bar for any kernel image.
>>
>> I agree completely; in fact that was my intention when
>> introducing the code, but I got overruled about the return codes:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>> I like this simplification very much, except this part:
>>
>>> + if (ret) {
>>> + pr_debug("kernel signature verification failed (%d).\n", ret);
>>
>> ...
>>
>>> - pr_notice("kernel signature verification failed (%d).\n", ret);
>>
>> I think the log level should stay at most PR_NOTICE when the
>> verification failure results in rejecting the kernel. Perhaps
>> even lower.
>>
>
> Thank you for the comment, Jiri Bohac.
>
> I like the idea of staying at most PR_NOTICE, but the pr_notice() will output
> some messages that kernel could want to ignore, such as the case you mentioned
> below.
>
>> In case verification is not enforced and the failure is
>> ignored, KERN_DEBUG seems reasonable.
>>
>
> Yes, good understanding. It seems that the pr_debug() is still a good option here?
> Any other thoughts?
>
Or the following change looks better? What's your opinion?
static int
kimage_validate_signature(struct kimage *image)
{
int ret;
ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
image->kernel_buf_len);
if (ret) {
if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
pr_notice("Enforced kernel signature verification failed (%d).\n", ret);
return ret;
}
/*
* If IMA is guaranteed to appraise a signature on the kexec
* image, permit it even if the kernel is otherwise locked
* down.
*/
if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
security_locked_down(LOCKDOWN_KEXEC))
return -EPERM;
pr_debug("kernel signature verification failed (%d).\n", ret);
}
return 0;
}
Thanks.
Lianbo
> Thanks.
> Lianbo
>
>
>> Regards,
>>
On Wed, May 27, 2020 at 12:08:12PM +0800, lijiang wrote:
> Or the following change looks better? What's your opinion?
>
> static int
> kimage_validate_signature(struct kimage *image)
> {
> int ret;
>
> ret = arch_kexec_kernel_verify_sig(image, image->kernel_buf,
> image->kernel_buf_len);
> if (ret) {
>
> if (IS_ENABLED(CONFIG_KEXEC_SIG_FORCE)) {
> pr_notice("Enforced kernel signature verification failed (%d).\n", ret);
> return ret;
> }
>
> /*
> * If IMA is guaranteed to appraise a signature on the kexec
> * image, permit it even if the kernel is otherwise locked
> * down.
> */
> if (!ima_appraise_signature(READING_KEXEC_IMAGE) &&
> security_locked_down(LOCKDOWN_KEXEC))
> return -EPERM;
>
> pr_debug("kernel signature verification failed (%d).\n", ret);
> }
>
> return 0;
> }
Looks good to me, thanks!
--
Jiri Bohac <[email protected]>
SUSE Labs, Prague, Czechia