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 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 > > --- > > 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 > > #include > > +#include > > #include > > #include > > > > +/** > > + * 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 (
Re: [PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig
Hi Michal, On Thu, 25 Nov 2021 19:02:44 +0100 Michal Suchanek 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 > --- > 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 > #include > +#include > #include > #include > > +/** > + * 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 mod_has_sig_marker is much more precise. I would use that instead. 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,