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

2022-01-25 Thread Luis Chamberlain
On Tue, Jan 11, 2022 at 12:37:48PM +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.
> 
> This changes the error from ENOENT to ENODATA for ima_read_modsig in the
> case the signature marker is missing.
> 
> This also changes the buffer length in ima_read_modsig from size_t to
> unsigned long. This reduces the possible value range on 32bit but the
> length refers to kernel in-memory buffer which cannot be longer than
> ULONG_MAX.
> 
> Also change mod_check_sig to unsigned long while at it.
> 
> Signed-off-by: Michal Suchanek 

Reviewed-by: Luis Chamberlain 

  Luis


[PATCH v5 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig

2022-01-11 Thread Michal Suchanek
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.

This changes the error from ENOENT to ENODATA for ima_read_modsig in the
case the signature marker is missing.

This also changes the buffer length in ima_read_modsig from size_t to
unsigned long. This reduces the possible value range on 32bit but the
length refers to kernel in-memory buffer which cannot be longer than
ULONG_MAX.

Also change mod_check_sig to unsigned long while at it.

Signed-off-by: Michal Suchanek 
---
v3: - Philipp Rudo : Update the commit with note about
  change of raturn value
- Preserve the EBADMSG error code while moving code araound
v4: - remove unused variable ms in module_signing.c
- note about buffer length
v5: - also change the functions in module_signature.c to unsigned long
---
 include/linux/module_signature.h|  4 +-
 kernel/module_signature.c   | 58 -
 kernel/module_signing.c | 27 ++
 security/integrity/ima/ima_modsig.c | 22 ++-
 4 files changed, 66 insertions(+), 45 deletions(-)

diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
index 7eb4b00381ac..e5fb157c085c 100644
--- a/include/linux/module_signature.h
+++ b/include/linux/module_signature.h
@@ -40,7 +40,9 @@ struct module_signature {
__be32  sig_len;/* Length of signature data */
 };
 
-int mod_check_sig(const struct module_signature *ms, size_t file_len,
+int mod_check_sig(const struct module_signature *ms, unsigned long file_len,
+ const char *name);
+int mod_parse_sig(const void *data, unsigned long *len, unsigned long *sig_len,
  const char *name);
 
 #endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
index 00132d12487c..4a36405ecd08 100644
--- a/kernel/module_signature.c
+++ b/kernel/module_signature.c
@@ -8,17 +8,39 @@
 
 #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, unsigned long *len)
+{
+   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,
+int mod_check_sig(const struct module_signature *ms, unsigned long file_len,
  const char *name)
 {
if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
@@ -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, unsigned long *len, unsigned long 
*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 -EBADMSG;
+
+   sig = 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 20857d2a15ca..1d4cb03cce21 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -25,35 +25,16 @@ int verify_appended_signature(const void *data, unsigned 
long *len,
  struct key *trusted_keys,
  enum key_being_used_for purpose)
 {
-   const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
-   struct module_signature *ms;
-   unsigned long sig_len, modlen = *len;
+   unsigned long sig_len;
int ret;
 
-   pr_devel("==>%s %s(,%lu)\n", __func__,