mbox series

[v2,0/6] KEXEC_SIG with appended signature

Message ID cover.1637862358.git.msuchanek@suse.de (mailing list archive)
Headers show
Series KEXEC_SIG with appended signature | expand

Message

Michal Suchanek Nov. 25, 2021, 6:02 p.m. UTC
Hello,

This is resend of the KEXEC_SIG patchset.

The first patch is new because it'a a cleanup that does not require any
change to the module verification code.

The second patch is the only one that is intended to change any
functionality.

The rest only deduplicates code but I did not receive any review on that
part so I don't know if it's desirable as implemented.

The first two patches can be applied separately without the rest.

Thanks

Michal

Michal Suchanek (6):
  s390/kexec_file: Don't opencode appended signature check.
  powerpc/kexec_file: Add KEXEC_SIG support.
  kexec_file: Don't opencode appended signature verification.
  module: strip the signature marker in the verification function.
  module: Use key_being_used_for for log messages in
    verify_appended_signature
  module: Move duplicate mod_check_sig users code to mod_parse_sig

 arch/powerpc/Kconfig                     | 11 +++++
 arch/powerpc/kexec/elf_64.c              | 14 ++++++
 arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 include/linux/module_signature.h         |  1 +
 include/linux/verification.h             |  4 ++
 kernel/module-internal.h                 |  2 -
 kernel/module.c                          | 12 +++--
 kernel/module_signature.c                | 56 +++++++++++++++++++++++-
 kernel/module_signing.c                  | 33 +++++++-------
 security/integrity/ima/ima_modsig.c      | 22 ++--------
 11 files changed, 113 insertions(+), 85 deletions(-)

Comments

Heiko Carstens Nov. 30, 2021, 3:28 p.m. UTC | #1
On Thu, Nov 25, 2021 at 07:02:38PM +0100, Michal Suchanek wrote:
> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)

For all patches which touch s390:
Acked-by: Heiko Carstens <hca@linux.ibm.com>
Baoquan He Dec. 1, 2021, 2:37 a.m. UTC | #2
Hi,

On 11/25/21 at 07:02pm, Michal Suchanek wrote:
> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.

Do you have the link of your 1st version?

And after going through the whole series, it doesn't tell what this
patch series intends to do in cover-letter or patch log.

Thanks
Baoquan

> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)
> 
> -- 
> 2.31.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
Michal Suchanek Dec. 1, 2021, 11:48 a.m. UTC | #3
Hello,

On Wed, Dec 01, 2021 at 10:37:47AM +0800, Baoquan He wrote:
> Hi,
> 
> On 11/25/21 at 07:02pm, Michal Suchanek wrote:
> > Hello,
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> 
> Do you have the link of your 1st version?

This is the previous version:
https://lore.kernel.org/lkml/cover.1635948742.git.msuchanek@suse.de/

Thanks

Michal

> And after going through the whole series, it doesn't tell what this
> patch series intends to do in cover-letter or patch log.
> 
> Thanks
> Baoquan
> 
> > 
> > The first two patches can be applied separately without the rest.
> > 
> > Thanks
> > 
> > Michal
> > 
> > Michal Suchanek (6):
> >   s390/kexec_file: Don't opencode appended signature check.
> >   powerpc/kexec_file: Add KEXEC_SIG support.
> >   kexec_file: Don't opencode appended signature verification.
> >   module: strip the signature marker in the verification function.
> >   module: Use key_being_used_for for log messages in
> >     verify_appended_signature
> >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > 
> >  arch/powerpc/Kconfig                     | 11 +++++
> >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> >  include/linux/module_signature.h         |  1 +
> >  include/linux/verification.h             |  4 ++
> >  kernel/module-internal.h                 |  2 -
> >  kernel/module.c                          | 12 +++--
> >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> >  kernel/module_signing.c                  | 33 +++++++-------
> >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> >  11 files changed, 113 insertions(+), 85 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> > 
>
Philipp Rudo Dec. 7, 2021, 4:10 p.m. UTC | #4
Hi Michal,

i finally had the time to take a closer look at the series. Except for
the nit in patch 4 and my personal preference in patch 6 the code looks
good to me.

What I don't like are the commit messages on the first commits. In my
opinion they are so short that they are almost useless. For example in
patch 2 there is absolutely no explanation why you can simply copy the
s390 over to ppc. Or in patch 3 you are silently changing the error
code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if
you could improve them a little.

Thanks
Philipp

On Thu, 25 Nov 2021 19:02:38 +0100
Michal Suchanek <msuchanek@suse.de> wrote:

> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)
>
Michal Suchanek Dec. 7, 2021, 5:32 p.m. UTC | #5
On Tue, Dec 07, 2021 at 05:10:14PM +0100, Philipp Rudo wrote:
> Hi Michal,
> 
> i finally had the time to take a closer look at the series. Except for
> the nit in patch 4 and my personal preference in patch 6 the code looks
> good to me.
> 
> What I don't like are the commit messages on the first commits. In my
> opinion they are so short that they are almost useless. For example in
> patch 2 there is absolutely no explanation why you can simply copy the
> s390 over to ppc.

They use the same signature format. I suppose I can add a note saying
that.

> Or in patch 3 you are silently changing the error
> code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if

Not sure what I should do about this. The different implementations use
different random error codes, and when they are unified the error code
clearly changes for one or the other.

Does anything depend on a particular error code returned?

Thanks

Michal

> you could improve them a little.
> 
> Thanks
> Philipp
> 
> On Thu, 25 Nov 2021 19:02:38 +0100
> Michal Suchanek <msuchanek@suse.de> wrote:
> 
> > Hello,
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> > 
> > The first two patches can be applied separately without the rest.
> > 
> > Thanks
> > 
> > Michal
> > 
> > Michal Suchanek (6):
> >   s390/kexec_file: Don't opencode appended signature check.
> >   powerpc/kexec_file: Add KEXEC_SIG support.
> >   kexec_file: Don't opencode appended signature verification.
> >   module: strip the signature marker in the verification function.
> >   module: Use key_being_used_for for log messages in
> >     verify_appended_signature
> >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > 
> >  arch/powerpc/Kconfig                     | 11 +++++
> >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> >  include/linux/module_signature.h         |  1 +
> >  include/linux/verification.h             |  4 ++
> >  kernel/module-internal.h                 |  2 -
> >  kernel/module.c                          | 12 +++--
> >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> >  kernel/module_signing.c                  | 33 +++++++-------
> >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> >  11 files changed, 113 insertions(+), 85 deletions(-)
> > 
>
Philipp Rudo Dec. 8, 2021, 9:54 a.m. UTC | #6
Hi Michal,

On Tue, 7 Dec 2021 18:32:21 +0100
Michal Suchánek <msuchanek@suse.de> wrote:

> On Tue, Dec 07, 2021 at 05:10:14PM +0100, Philipp Rudo wrote:
> > Hi Michal,
> > 
> > i finally had the time to take a closer look at the series. Except for
> > the nit in patch 4 and my personal preference in patch 6 the code looks
> > good to me.
> > 
> > What I don't like are the commit messages on the first commits. In my
> > opinion they are so short that they are almost useless. For example in
> > patch 2 there is absolutely no explanation why you can simply copy the
> > s390 over to ppc.  
> 
> They use the same signature format. I suppose I can add a note saying
> that.

The note is what I was asking for. For me the commit message is an
important piece of documentation for other developers (or yourself in a
year). That's why in my opinion it's important to describe _why_ you do
something in it as you cannot get the _why_ by reading the code.

> > Or in patch 3 you are silently changing the error
> > code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if  
> 
> Not sure what I should do about this. The different implementations use
> different random error codes, and when they are unified the error code
> clearly changes for one or the other.

My complaint wasn't that you change the return code. There's no way to
avoid choosing one over the other. It's again that you don't document
the change in the commit message for others.

> Does anything depend on a particular error code returned?

Not that I know of. At least in the kexec-tools ENODATA and EKEYREJECT
are handled the same way.

Thanks
Philipp


> Thanks
> 
> Michal
> 
> > you could improve them a little.
> > 
> > Thanks
> > Philipp
> > 
> > On Thu, 25 Nov 2021 19:02:38 +0100
> > Michal Suchanek <msuchanek@suse.de> wrote:
> >   
> > > Hello,
> > > 
> > > This is resend of the KEXEC_SIG patchset.
> > > 
> > > The first patch is new because it'a a cleanup that does not require any
> > > change to the module verification code.
> > > 
> > > The second patch is the only one that is intended to change any
> > > functionality.
> > > 
> > > The rest only deduplicates code but I did not receive any review on that
> > > part so I don't know if it's desirable as implemented.
> > > 
> > > The first two patches can be applied separately without the rest.
> > > 
> > > Thanks
> > > 
> > > Michal
> > > 
> > > Michal Suchanek (6):
> > >   s390/kexec_file: Don't opencode appended signature check.
> > >   powerpc/kexec_file: Add KEXEC_SIG support.
> > >   kexec_file: Don't opencode appended signature verification.
> > >   module: strip the signature marker in the verification function.
> > >   module: Use key_being_used_for for log messages in
> > >     verify_appended_signature
> > >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > > 
> > >  arch/powerpc/Kconfig                     | 11 +++++
> > >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> > >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> > >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> > >  include/linux/module_signature.h         |  1 +
> > >  include/linux/verification.h             |  4 ++
> > >  kernel/module-internal.h                 |  2 -
> > >  kernel/module.c                          | 12 +++--
> > >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> > >  kernel/module_signing.c                  | 33 +++++++-------
> > >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> > >  11 files changed, 113 insertions(+), 85 deletions(-)
> > >   
> >   
>
Nayna Dec. 9, 2021, 1:50 a.m. UTC | #7
On 11/25/21 13:02, Michal Suchanek wrote:
> Hello,

Hi Michael,

>
> This is resend of the KEXEC_SIG patchset.
>
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
>
> The second patch is the only one that is intended to change any
> functionality.
>
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
>
> The first two patches can be applied separately without the rest.

Patch 2 fails to apply on v5.16-rc4. Can you please also include git 
tree/branch while posting the patches ?

Secondly, I see that you add the powerpc support in Patch 2 and then 
modify it again in Patch 5 after cleanup. Why not add the support for 
powerpc after the clean up ? This will reduce some rework and also 
probably simplify patches.

Thanks & Regards,

      - Nayna
Michal Suchanek Dec. 9, 2021, 2:57 p.m. UTC | #8
Hello,

On Wed, Dec 08, 2021 at 08:50:54PM -0500, Nayna wrote:
> 
> On 11/25/21 13:02, Michal Suchanek wrote:
> > Hello,
> 
> Hi Michael,
> 
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> > 
> > The first two patches can be applied separately without the rest.
> 
> Patch 2 fails to apply on v5.16-rc4. Can you please also include git
> tree/branch while posting the patches ?

Sorry, I did not have a clean base and the Kconfig had another change.

Here is a tree with the changes applied:
https://github.com/hramrach/kernel/tree/kexec_sig

> 
> Secondly, I see that you add the powerpc support in Patch 2 and then modify
> it again in Patch 5 after cleanup. Why not add the support for powerpc after
> the clean up ? This will reduce some rework and also probably simplify
> patches.

That's because I don't know if the later patches will be accepted. By
queueing this patch first it can be applied standalone to ppc tree
without regard for the other patches. It's a copy of the s390 code so it
needs the same rework - not really adding complexity.

Thanks

Michal