Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On Thu, Aug 16, 2018 at 09:11:06AM +0800, Dave Young wrote:
> On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> > Signed-off-by: Yannik Sembritzki
> > ---
> > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > certs/system_keyring.c | 3 ++-
> > crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
> > include/linux/verification.h | 3 +++
> > 4 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 74628275..97d199a3 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> > {
> > return verify_pefile_signature(kernel, kernel_len,
> > - ((struct key *)1UL),
> > + TRUST_SECONDARY_KEYRING,
>
> Instead of fix your 1st patch in 2nd patch, I would suggest to
> switch the patch order. In 1st patch change the common code to use
> the new macro and in 2nd patch you can directly fix the kexec code
> with TRUST_SECONDARY_KEYRING.
I agree. It looks cleaner that first patch change the common code and
introduce the macro to replace 1UL. And second patch makes use of that
macro in kexec bzImage64 verification.
Thanks
Vivek
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On Thu, Aug 16, 2018 at 09:11:06AM +0800, Dave Young wrote:
> On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> > Signed-off-by: Yannik Sembritzki
> > ---
> > arch/x86/kernel/kexec-bzimage64.c | 2 +-
> > certs/system_keyring.c | 3 ++-
> > crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
> > include/linux/verification.h | 3 +++
> > 4 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kexec-bzimage64.c
> > b/arch/x86/kernel/kexec-bzimage64.c
> > index 74628275..97d199a3 100644
> > --- a/arch/x86/kernel/kexec-bzimage64.c
> > +++ b/arch/x86/kernel/kexec-bzimage64.c
> > @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> > static int bzImage64_verify_sig(const char *kernel, unsigned long
> > kernel_len)
> > {
> > return verify_pefile_signature(kernel, kernel_len,
> > - ((struct key *)1UL),
> > + TRUST_SECONDARY_KEYRING,
>
> Instead of fix your 1st patch in 2nd patch, I would suggest to
> switch the patch order. In 1st patch change the common code to use
> the new macro and in 2nd patch you can directly fix the kexec code
> with TRUST_SECONDARY_KEYRING.
I agree. It looks cleaner that first patch change the common code and
introduce the macro to replace 1UL. And second patch makes use of that
macro in kexec bzImage64 verification.
Thanks
Vivek
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On Thu, Aug 16, 2018 at 04:02:59PM +0800, Dave Young wrote: > On 08/16/18 at 09:43am, Yannik Sembritzki wrote: > > On 16.08.2018 03:11, Dave Young wrote: > > > Instead of fix your 1st patch in 2nd patch, I would suggest to > > > switch the patch order. In 1st patch change the common code to use > > > the new macro and in 2nd patch you can directly fix the kexec code > > > with TRUST_SECONDARY_KEYRING. > > My reasoning for doing it in this order was that the first patch which > > fixes the bug itself should be merged into stable, while the refactoring > > doesn't necessarily have to. I'm not familiar with the linux development > > process, so please correct me if this should be done in another fashion. > > Frankly I'm not sure about the stable process. But personally I do not > like the order. > > Cced Greg for opinions about stable concern. It's up to what the maintainer of the subsystem wants to do here. I will take what they recommend. thanks, greg k-h
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On Thu, Aug 16, 2018 at 04:02:59PM +0800, Dave Young wrote: > On 08/16/18 at 09:43am, Yannik Sembritzki wrote: > > On 16.08.2018 03:11, Dave Young wrote: > > > Instead of fix your 1st patch in 2nd patch, I would suggest to > > > switch the patch order. In 1st patch change the common code to use > > > the new macro and in 2nd patch you can directly fix the kexec code > > > with TRUST_SECONDARY_KEYRING. > > My reasoning for doing it in this order was that the first patch which > > fixes the bug itself should be merged into stable, while the refactoring > > doesn't necessarily have to. I'm not familiar with the linux development > > process, so please correct me if this should be done in another fashion. > > Frankly I'm not sure about the stable process. But personally I do not > like the order. > > Cced Greg for opinions about stable concern. It's up to what the maintainer of the subsystem wants to do here. I will take what they recommend. thanks, greg k-h
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 08/16/18 at 09:43am, Yannik Sembritzki wrote: > On 16.08.2018 03:11, Dave Young wrote: > > Instead of fix your 1st patch in 2nd patch, I would suggest to > > switch the patch order. In 1st patch change the common code to use > > the new macro and in 2nd patch you can directly fix the kexec code > > with TRUST_SECONDARY_KEYRING. > My reasoning for doing it in this order was that the first patch which > fixes the bug itself should be merged into stable, while the refactoring > doesn't necessarily have to. I'm not familiar with the linux development > process, so please correct me if this should be done in another fashion. Frankly I'm not sure about the stable process. But personally I do not like the order. Cced Greg for opinions about stable concern. > > Yannik
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 08/16/18 at 09:43am, Yannik Sembritzki wrote: > On 16.08.2018 03:11, Dave Young wrote: > > Instead of fix your 1st patch in 2nd patch, I would suggest to > > switch the patch order. In 1st patch change the common code to use > > the new macro and in 2nd patch you can directly fix the kexec code > > with TRUST_SECONDARY_KEYRING. > My reasoning for doing it in this order was that the first patch which > fixes the bug itself should be merged into stable, while the refactoring > doesn't necessarily have to. I'm not familiar with the linux development > process, so please correct me if this should be done in another fashion. Frankly I'm not sure about the stable process. But personally I do not like the order. Cced Greg for opinions about stable concern. > > Yannik
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 16.08.2018 03:11, Dave Young wrote: > Instead of fix your 1st patch in 2nd patch, I would suggest to > switch the patch order. In 1st patch change the common code to use > the new macro and in 2nd patch you can directly fix the kexec code > with TRUST_SECONDARY_KEYRING. My reasoning for doing it in this order was that the first patch which fixes the bug itself should be merged into stable, while the refactoring doesn't necessarily have to. I'm not familiar with the linux development process, so please correct me if this should be done in another fashion. Yannik
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 16.08.2018 03:11, Dave Young wrote: > Instead of fix your 1st patch in 2nd patch, I would suggest to > switch the patch order. In 1st patch change the common code to use > the new macro and in 2nd patch you can directly fix the kexec code > with TRUST_SECONDARY_KEYRING. My reasoning for doing it in this order was that the first patch which fixes the bug itself should be merged into stable, while the refactoring doesn't necessarily have to. I'm not familiar with the linux development process, so please correct me if this should be done in another fashion. Yannik
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> Signed-off-by: Yannik Sembritzki
> ---
> arch/x86/kernel/kexec-bzimage64.c | 2 +-
> certs/system_keyring.c | 3 ++-
> crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
> include/linux/verification.h | 3 +++
> 4 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 74628275..97d199a3 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
> {
> return verify_pefile_signature(kernel, kernel_len,
> - ((struct key *)1UL),
> + TRUST_SECONDARY_KEYRING,
Instead of fix your 1st patch in 2nd patch, I would suggest to
switch the patch order. In 1st patch change the common code to use
the new macro and in 2nd patch you can directly fix the kexec code
with TRUST_SECONDARY_KEYRING.
> VERIFYING_KEXEC_PE_SIGNATURE);
> }
> #endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b2..777ac7d2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
>
> if (!trusted_keys) {
> trusted_keys = builtin_trusted_keys;
> - } else if (trusted_keys == (void *)1UL) {
> + } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> trusted_keys = secondary_trusted_keys;
> #else
> diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
> b/crypto/asymmetric_keys/pkcs7_key_type.c
> index e284d9cb..0783e555 100644
> --- a/crypto/asymmetric_keys/pkcs7_key_type.c
> +++ b/crypto/asymmetric_keys/pkcs7_key_type.c
> @@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
> *prep)
>
> return verify_pkcs7_signature(NULL, 0,
> prep->data, prep->datalen,
> - (void *)1UL, usage,
> + TRUST_SECONDARY_KEYRING, usage,
> pkcs7_view_content, prep);
> }
>
> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index a10549a6..c00c1143 100644
> --- a/include/linux/verification.h
> +++ b/include/linux/verification.h
> @@ -12,6 +12,9 @@
> #ifndef _LINUX_VERIFICATION_H
> #define _LINUX_VERIFICATION_H
>
> +// Allow both builtin trusted keys and secondary trusted keys
It would be better to use commenting style /*
> +#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
> +
> /*
> * The use to which an asymmetric key is being put.
> */
> --
> 2.17.1
>
>
Thanks
Dave
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
On 08/16/18 at 12:07am, Yannik Sembritzki wrote:
> Signed-off-by: Yannik Sembritzki
> ---
> arch/x86/kernel/kexec-bzimage64.c | 2 +-
> certs/system_keyring.c | 3 ++-
> crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
> include/linux/verification.h | 3 +++
> 4 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/kexec-bzimage64.c
> b/arch/x86/kernel/kexec-bzimage64.c
> index 74628275..97d199a3 100644
> --- a/arch/x86/kernel/kexec-bzimage64.c
> +++ b/arch/x86/kernel/kexec-bzimage64.c
> @@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
> static int bzImage64_verify_sig(const char *kernel, unsigned long
> kernel_len)
> {
> return verify_pefile_signature(kernel, kernel_len,
> - ((struct key *)1UL),
> + TRUST_SECONDARY_KEYRING,
Instead of fix your 1st patch in 2nd patch, I would suggest to
switch the patch order. In 1st patch change the common code to use
the new macro and in 2nd patch you can directly fix the kexec code
with TRUST_SECONDARY_KEYRING.
> VERIFYING_KEXEC_PE_SIGNATURE);
> }
> #endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 6251d1b2..777ac7d2 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -15,6 +15,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
>
> if (!trusted_keys) {
> trusted_keys = builtin_trusted_keys;
> - } else if (trusted_keys == (void *)1UL) {
> + } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> trusted_keys = secondary_trusted_keys;
> #else
> diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
> b/crypto/asymmetric_keys/pkcs7_key_type.c
> index e284d9cb..0783e555 100644
> --- a/crypto/asymmetric_keys/pkcs7_key_type.c
> +++ b/crypto/asymmetric_keys/pkcs7_key_type.c
> @@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
> *prep)
>
> return verify_pkcs7_signature(NULL, 0,
> prep->data, prep->datalen,
> - (void *)1UL, usage,
> + TRUST_SECONDARY_KEYRING, usage,
> pkcs7_view_content, prep);
> }
>
> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index a10549a6..c00c1143 100644
> --- a/include/linux/verification.h
> +++ b/include/linux/verification.h
> @@ -12,6 +12,9 @@
> #ifndef _LINUX_VERIFICATION_H
> #define _LINUX_VERIFICATION_H
>
> +// Allow both builtin trusted keys and secondary trusted keys
It would be better to use commenting style /*
> +#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
> +
> /*
> * The use to which an asymmetric key is being put.
> */
> --
> 2.17.1
>
>
Thanks
Dave
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
Signed-off-by: Yannik Sembritzki
---
arch/x86/kernel/kexec-bzimage64.c | 2 +-
certs/system_keyring.c | 3 ++-
crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
include/linux/verification.h | 3 +++
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 74628275..97d199a3 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
{
return verify_pefile_signature(kernel, kernel_len,
- ((struct key *)1UL),
+ TRUST_SECONDARY_KEYRING,
VERIFYING_KEXEC_PE_SIGNATURE);
}
#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b2..777ac7d2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
if (!trusted_keys) {
trusted_keys = builtin_trusted_keys;
- } else if (trusted_keys == (void *)1UL) {
+ } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
trusted_keys = secondary_trusted_keys;
#else
diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
b/crypto/asymmetric_keys/pkcs7_key_type.c
index e284d9cb..0783e555 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
*prep)
return verify_pkcs7_signature(NULL, 0,
prep->data, prep->datalen,
- (void *)1UL, usage,
+ TRUST_SECONDARY_KEYRING, usage,
pkcs7_view_content, prep);
}
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6..c00c1143 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -12,6 +12,9 @@
#ifndef _LINUX_VERIFICATION_H
#define _LINUX_VERIFICATION_H
+// Allow both builtin trusted keys and secondary trusted keys
+#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
+
/*
* The use to which an asymmetric key is being put.
*/
--
2.17.1
Re: [PATCH 2/2] [FIXED v2] Replace magic for trusting the secondary keyring with #define
Signed-off-by: Yannik Sembritzki
---
arch/x86/kernel/kexec-bzimage64.c | 2 +-
certs/system_keyring.c | 3 ++-
crypto/asymmetric_keys/pkcs7_key_type.c | 2 +-
include/linux/verification.h | 3 +++
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/kexec-bzimage64.c
b/arch/x86/kernel/kexec-bzimage64.c
index 74628275..97d199a3 100644
--- a/arch/x86/kernel/kexec-bzimage64.c
+++ b/arch/x86/kernel/kexec-bzimage64.c
@@ -532,7 +532,7 @@ static int bzImage64_cleanup(void *loader_data)
static int bzImage64_verify_sig(const char *kernel, unsigned long
kernel_len)
{
return verify_pefile_signature(kernel, kernel_len,
- ((struct key *)1UL),
+ TRUST_SECONDARY_KEYRING,
VERIFYING_KEXEC_PE_SIGNATURE);
}
#endif
diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 6251d1b2..777ac7d2 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -230,7 +231,7 @@ int verify_pkcs7_signature(const void *data, size_t len,
if (!trusted_keys) {
trusted_keys = builtin_trusted_keys;
- } else if (trusted_keys == (void *)1UL) {
+ } else if (trusted_keys == TRUST_SECONDARY_KEYRING) {
#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
trusted_keys = secondary_trusted_keys;
#else
diff --git a/crypto/asymmetric_keys/pkcs7_key_type.c
b/crypto/asymmetric_keys/pkcs7_key_type.c
index e284d9cb..0783e555 100644
--- a/crypto/asymmetric_keys/pkcs7_key_type.c
+++ b/crypto/asymmetric_keys/pkcs7_key_type.c
@@ -63,7 +63,7 @@ static int pkcs7_preparse(struct key_preparsed_payload
*prep)
return verify_pkcs7_signature(NULL, 0,
prep->data, prep->datalen,
- (void *)1UL, usage,
+ TRUST_SECONDARY_KEYRING, usage,
pkcs7_view_content, prep);
}
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a10549a6..c00c1143 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -12,6 +12,9 @@
#ifndef _LINUX_VERIFICATION_H
#define _LINUX_VERIFICATION_H
+// Allow both builtin trusted keys and secondary trusted keys
+#define TRUST_SECONDARY_KEYRING ((struct key *)1UL)
+
/*
* The use to which an asymmetric key is being put.
*/
--
2.17.1
