by Michal Suchánek

[permalink] [raw]

Subject: Re: [PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig

Hello,

On Tue, Dec 07, 2021 at 05:10:34PM +0100, Philipp Rudo wrote:
> Hi Michal,
>
> On Thu, 25 Nov 2021 19:02:44 +0100
> Michal Suchanek <[email protected]> wrote:
>
> > Multiple users of mod_check_sig check for the marker, then call
> > mod_check_sig, extract signature length, and remove the signature.
> >
> > Put this code in one place together with mod_check_sig.
> >
> > Signed-off-by: Michal Suchanek <[email protected]>
> > ---
> > include/linux/module_signature.h | 1 +
> > kernel/module_signature.c | 56 ++++++++++++++++++++++++++++-
> > kernel/module_signing.c | 26 +++-----------
> > security/integrity/ima/ima_modsig.c | 22 ++----------
> > 4 files changed, 63 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> > index 7eb4b00381ac..1343879b72b3 100644
> > --- a/include/linux/module_signature.h
> > +++ b/include/linux/module_signature.h
> > @@ -42,5 +42,6 @@ struct module_signature {
> >
> > int mod_check_sig(const struct module_signature *ms, size_t file_len,
> > const char *name);
> > +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name);
> >
> > #endif /* _LINUX_MODULE_SIGNATURE_H */
> > diff --git a/kernel/module_signature.c b/kernel/module_signature.c
> > index 00132d12487c..784b40575ee4 100644
> > --- a/kernel/module_signature.c
> > +++ b/kernel/module_signature.c
> > @@ -8,14 +8,36 @@
> >
> > #include <linux/errno.h>
> > #include <linux/printk.h>
> > +#include <linux/string.h>
> > #include <linux/module_signature.h>
> > #include <asm/byteorder.h>
> >
> > +/**
> > + * mod_check_sig_marker - check that the given data has signature marker at the end
> > + *
> > + * @data: Data with appended signature
> > + * @len: Length of data. Signature marker length is subtracted on success.
> > + */
> > +static inline int mod_check_sig_marker(const void *data, size_t *len)
>
> I personally don't like it when a function has a "check" in it's name
> as it doesn't describe what the function is checking for. For me

It is consistent with mod_check_sig

> mod_has_sig_marker is much more precise. I would use that instead.

It actually would not because it does more than that.

Thanks

Michal

>
> Thanks
> Philipp
>
> > +{
> > + const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > +
> > + if (markerlen > *len)
> > + return -ENODATA;
> > +
> > + if (memcmp(data + *len - markerlen, MODULE_SIG_STRING,
> > + markerlen))
> > + return -ENODATA;
> > +
> > + *len -= markerlen;
> > + return 0;
> > +}
> > +
> > /**
> > * mod_check_sig - check that the given signature is sane
> > *
> > * @ms: Signature to check.
> > - * @file_len: Size of the file to which @ms is appended.
> > + * @file_len: Size of the file to which @ms is appended (without the marker).
> > * @name: What is being checked. Used for error messages.
> > */
> > int mod_check_sig(const struct module_signature *ms, size_t file_len,
> > @@ -44,3 +66,35 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
> >
> > return 0;
> > }
> > +
> > +/**
> > + * mod_parse_sig - check that the given signature is sane and determine signature length
> > + *
> > + * @data: Data with appended signature.
> > + * @len: Length of data. Signature and marker length is subtracted on success.
> > + * @sig_len: Length of signature. Filled on success.
> > + * @name: What is being checked. Used for error messages.
> > + */
> > +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name)
> > +{
> > + const struct module_signature *sig;
> > + int rc;
> > +
> > + rc = mod_check_sig_marker(data, len);
> > + if (rc)
> > + return rc;
> > +
> > + if (*len < sizeof(*sig))
> > + return -ENODATA;
> > +
> > + sig = (const struct module_signature *)(data + (*len - sizeof(*sig)));
> > +
> > + rc = mod_check_sig(sig, *len, name);
> > + if (rc)
> > + return rc;
> > +
> > + *sig_len = be32_to_cpu(sig->sig_len);
> > + *len -= *sig_len + sizeof(*sig);
> > +
> > + return 0;
> > +}
> > diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> > index cef72a6f6b5d..02bbca90f467 100644
> > --- a/kernel/module_signing.c
> > +++ b/kernel/module_signing.c
> > @@ -25,35 +25,17 @@ int verify_appended_signature(const void *data, size_t *len,
> > struct key *trusted_keys,
> > enum key_being_used_for purpose)
> > {
> > - const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > struct module_signature ms;
> > - size_t sig_len, modlen = *len;
> > + size_t sig_len;
> > int ret;
> >
> > - pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);
> > + pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], *len);
> >
> > - if (markerlen > modlen)
> > - return -ENODATA;
> > -
> > - if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> > - markerlen))
> > - return -ENODATA;
> > - modlen -= markerlen;
> > -
> > - if (modlen <= sizeof(ms))
> > - return -EBADMSG;
> > -
> > - memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
> > -
> > - ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
> > + ret = mod_parse_sig(data, len, &sig_len, key_being_used_for[purpose]);
> > if (ret)
> > return ret;
> >
> > - sig_len = be32_to_cpu(ms.sig_len);
> > - modlen -= sig_len + sizeof(ms);
> > - *len = modlen;
> > -
> > - return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
> > + return verify_pkcs7_signature(data, *len, data + *len, sig_len,
> > trusted_keys,
> > purpose,
> > NULL, NULL);
> > diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> > index fb25723c65bc..46917eb37fd8 100644
> > --- a/security/integrity/ima/ima_modsig.c
> > +++ b/security/integrity/ima/ima_modsig.c
> > @@ -37,33 +37,17 @@ struct modsig {
> > *
> > * Return: 0 on success, error code otherwise.
> > */
> > -int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
> > +int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t len,
> > struct modsig **modsig)
> > {
> > - const size_t marker_len = strlen(MODULE_SIG_STRING);
> > - const struct module_signature *sig;
> > struct modsig *hdr;
> > - size_t sig_len;
> > - const void *p;
> > + size_t sig_len, buf_len = len;
> > int rc;
> >
> > - if (buf_len <= marker_len + sizeof(*sig))
> > - return -ENOENT;
> > -
> > - p = buf + buf_len - marker_len;
> > - if (memcmp(p, MODULE_SIG_STRING, marker_len))
> > - return -ENOENT;
> > -
> > - buf_len -= marker_len;
> > - sig = (const struct module_signature *)(p - sizeof(*sig));
> > -
> > - rc = mod_check_sig(sig, buf_len, func_tokens[func]);
> > + rc = mod_parse_sig(buf, &buf_len, &sig_len, func_tokens[func]);
> > if (rc)
> > return rc;
> >
> > - sig_len = be32_to_cpu(sig->sig_len);
> > - buf_len -= sig_len + sizeof(*sig);
> > -
> > /* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
> > hdr = kzalloc(sizeof(*hdr) + sig_len, GFP_KERNEL);
> > if (!hdr)
>

2021-12-13 18:18:22

by Michal Suchánek

[permalink] [raw]

Subject: Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

Hello,

On Sun, Dec 12, 2021 at 07:46:53PM -0500, Nayna wrote:
>
> On 11/25/21 13:02, Michal Suchanek wrote:
> > Copy the code from s390x
> >
> > Signed-off-by: Michal Suchanek <[email protected]>
> > ---
> > arch/powerpc/Kconfig | 11 +++++++++++
> > arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 47 insertions(+)
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index ac0c515552fd..ecc1227a77f1 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -561,6 +561,17 @@ config KEXEC_FILE
> > config ARCH_HAS_KEXEC_PURGATORY
> > def_bool KEXEC_FILE
> >
> > +config KEXEC_SIG
> > + bool "Verify kernel signature during kexec_file_load() syscall"
> > + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > + help
> > + This option makes kernel signature verification mandatory for
> > + the kexec_file_load() syscall.
> > +
>
> Resending my last response as looks like it didn't go through mailing list
> because of some wrong formatting. My apologies to those who are receiving it
> twice.
>
> Since powerpc also supports IMA_ARCH_POLICY for kernel image signature
> verification, please include the following:
>
> "An alternative implementation for the powerpc arch is IMA_ARCH_POLICY. It
> verifies the appended kernel image signature and additionally includes both
> the signed and unsigned file hashes in the IMA measurement list, extends the
> IMA PCR in the TPM, and prevents blacklisted binary kernel images from being
> kexec'd."

It also does blacklist based on the file hash?

There is a downstream patch that adds the support for the module
signatures, and when the code is reused for KEXEC_SIG the blacklist
also applies to it.

Which kind of shows that people really want to use the IMA features but
with no support on some major architectures it's not going to work.

Thanks

Michal