libkmod-signature: Fix crash when module signature is not present.
diff mbox

Message ID 20180308181426.5617-1-msuchanek@suse.de
State New
Headers show

Commit Message

Michal Suchanek March 8, 2018, 6:14 p.m. UTC
The mod_sig is allocated on stack and when no signature is present it is not
initialized and contains garbage. Later when freeing mod_sig garbage pointer is
dereferenced.

Make mod_sig pointer initialized to NULL instead so it has meaningful value
only when a signature was stored in it. This somewhat straightens the interface
to kmod_module_signature_info as well.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 libkmod/libkmod-internal.h  |  2 +-
 libkmod/libkmod-module.c    | 21 +++++++++---------
 libkmod/libkmod-signature.c | 53 +++++++++++++++++++++++++--------------------
 3 files changed, 42 insertions(+), 34 deletions(-)

Comments

Yauheni Kaliuta March 12, 2018, 8:41 p.m. UTC | #1
Hi, Michal!

>>>>> On Thu,  8 Mar 2018 19:14:26 +0100, Michal Suchanek  wrote:

 > The mod_sig is allocated on stack and when no signature is present it is not
 > initialized and contains garbage. Later when freeing mod_sig garbage pointer is
 > dereferenced.

I guess, it is enough to init the structure.

 > Make mod_sig pointer initialized to NULL instead so it has meaningful value
 > only when a signature was stored in it. This somewhat straightens the interface
 > to kmod_module_signature_info as well.

 > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
 > ---
 >  libkmod/libkmod-internal.h  |  2 +-
 >  libkmod/libkmod-module.c    | 21 +++++++++---------
 >  libkmod/libkmod-signature.c | 53 +++++++++++++++++++++++++--------------------
 >  3 files changed, 42 insertions(+), 34 deletions(-)

 > diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
 > index 2ad74c7a33a5..8bc9ecfb906e 100644
 > --- a/libkmod/libkmod-internal.h
 > +++ b/libkmod/libkmod-internal.h
 > @@ -192,5 +192,5 @@ struct kmod_signature_info {
 >  	void (*free)(void *);
 >  	void *private;
 >  };
 > -bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info) _must_check_ __attribute__((nonnull(1, 2)));
 > +struct kmod_signature_info *kmod_module_signature_info(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 >  void kmod_module_signature_info_free(struct kmod_signature_info *sig_info) __attribute__((nonnull));
 > diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
 > index 7b00d526edec..cbe35819932e 100644
 > --- a/libkmod/libkmod-module.c
 > +++ b/libkmod/libkmod-module.c
 > @@ -2304,7 +2304,7 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 >  	struct kmod_elf *elf;
 >  	char **strings;
 >  	int i, count, ret = -ENOMEM;
 > -	struct kmod_signature_info sig_info;
 > +	struct kmod_signature_info *sig_info = NULL;
 
 >  	if (mod == NULL || list == NULL)
 >  		return -ENOENT;
 > @@ -2341,32 +2341,32 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 >  			goto list_error;
 >  	}
 
 > -	if (kmod_module_signature_info(mod->file, &sig_info)) {
 > +	if (sig_info = kmod_module_signature_info(mod->file)) {
 >  		struct kmod_list *n;
 
 >  		n = kmod_module_info_append(list, "sig_id", strlen("sig_id"),
 > -				sig_info.id_type, strlen(sig_info.id_type));
 > +				sig_info->id_type, strlen(sig_info->id_type));
 >  		if (n == NULL)
 >  			goto list_error;
 >  		count++;
 
 >  		n = kmod_module_info_append(list, "signer", strlen("signer"),
 > -				sig_info.signer, sig_info.signer_len);
 > +				sig_info->signer, sig_info->signer_len);
 >  		if (n == NULL)
 >  			goto list_error;
 >  		count++;
 
 
 >  		n = kmod_module_info_append_hex(list, "sig_key", strlen("sig_key"),
 > -						sig_info.key_id,
 > -						sig_info.key_id_len);
 > +						sig_info->key_id,
 > +						sig_info->key_id_len);
 >  		if (n == NULL)
 >  			goto list_error;
 >  		count++;
 
 >  		n = kmod_module_info_append(list,
 >  				"sig_hashalgo", strlen("sig_hashalgo"),
 > -				sig_info.hash_algo, strlen(sig_info.hash_algo));
 > +				sig_info->hash_algo, strlen(sig_info->hash_algo));
 >  		if (n == NULL)
 >  			goto list_error;
 >  		count++;
 > @@ -2377,8 +2377,8 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 >  		 */
 >  		n = kmod_module_info_append_hex(list, "signature",
 >  						strlen("signature"),
 > -						sig_info.sig,
 > -						sig_info.sig_len);
 > +						sig_info->sig,
 > +						sig_info->sig_len);
 
 >  		if (n == NULL)
 >  			goto list_error;
 > @@ -2389,7 +2389,8 @@ KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 
 >  list_error:
 >  	/* aux structures freed in normal case also */
 > -	kmod_module_signature_info_free(&sig_info);
 > +	if (sig_info)
 > +		kmod_module_signature_info_free(sig_info);
 
 >  	if (ret < 0) {
 >  		kmod_module_info_free_list(*list);
 > diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
 > index bdf84cb14718..fae074e6dd1d 100644
 > --- a/libkmod/libkmod-signature.c
 > +++ b/libkmod/libkmod-signature.c
 > @@ -93,13 +93,17 @@ struct module_signature {
 >  	uint32_t sig_len;    /* Length of signature data (big endian) */
 >  };
 
 > -static bool
 > +static struct kmod_signature_info *
 >  kmod_module_signature_info_default(const char *mem,
 >  				   off_t size,
 >  				   const struct module_signature *modsig,
 > -				   size_t sig_len,
 > -				   struct kmod_signature_info *sig_info)
 > +				   size_t sig_len)
 >  {
 > +	struct kmod_signature_info *sig_info = malloc(sizeof *sig_info);
 > +
 > +	if (!sig_info)
 > +		return NULL;
 > +
 >  	size -= sig_len;
 sig_info-> sig = mem + size;
 sig_info-> sig_len = sig_len;
 > @@ -119,7 +123,7 @@ kmod_module_signature_info_default(const char *mem,
 sig_info-> free = NULL;
 sig_info-> private = NULL;
 
 > -	return true;
 > +	return sig_info;
 >  }
 
 >  static void kmod_module_signature_info_pkcs7_free(void *s)
 > @@ -129,13 +133,13 @@ static void kmod_module_signature_info_pkcs7_free(void *s)
 >  	pkcs7_free_cert(si->private);
 >  }
 
 > -static bool
 > +static struct kmod_signature_info *
 >  kmod_module_signature_info_pkcs7(const char *mem,
 >  				 off_t size,
 >  				 const struct module_signature *modsig,
 > -				 size_t sig_len,
 > -				 struct kmod_signature_info *sig_info)
 > +				 size_t sig_len)
 >  {
 > +	struct kmod_signature_info *sig_info = NULL;
 >  	const char *pkcs7_raw;
 >  	struct pkcs7_cert *cert;
 
 > @@ -144,7 +148,13 @@ kmod_module_signature_info_pkcs7(const char *mem,
 
 >  	cert = pkcs7_parse_cert(pkcs7_raw, sig_len);
 >  	if (cert == NULL)
 > -		return false;
 > +		return NULL;
 > +
 > +	sig_info = malloc(sizeof *sig_info);
 > +	if (!sig_info) {
 > +		free(cert);
 > +		return NULL;
 > +	}
 
 sig_info-> private = cert;
 sig_info-> free = kmod_module_signature_info_pkcs7_free;
 > @@ -162,7 +172,7 @@ kmod_module_signature_info_pkcs7(const char *mem,
 sig_info-> hash_algo = cert->hash_algo;
 sig_info-> id_type = pkey_id_type[modsig->id_type];
 
 > -	return true;
 > +	return sig_info;
 >  }
 
 >  #define SIG_MAGIC "~Module signature appended~\n"
 > @@ -178,48 +188,45 @@ kmod_module_signature_info_pkcs7(const char *mem,
 >   * [ SIG_MAGIC               ]
 >   */
 
 > -bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info)
 > +struct kmod_signature_info *kmod_module_signature_info(const struct kmod_file *file)
 >  {
 >  	const char *mem;
 >  	off_t size;
 >  	const struct module_signature *modsig;
 >  	size_t sig_len;
 > -	bool ret;
 
 >  	size = kmod_file_get_size(file);
 >  	mem = kmod_file_get_contents(file);
 >  	if (size < (off_t)strlen(SIG_MAGIC))
 > -		return false;
 > +		return NULL;
 >  	size -= strlen(SIG_MAGIC);
 >  	if (memcmp(SIG_MAGIC, mem + size, strlen(SIG_MAGIC)) != 0)
 > -		return false;
 > +		return NULL;
 
 >  	if (size < (off_t)sizeof(struct module_signature))
 > -		return false;
 > +		return NULL;
 >  	size -= sizeof(struct module_signature);
 >  	modsig = (struct module_signature *)(mem + size);
 >  	if (modsig->algo >= PKEY_ALGO__LAST ||
 modsig-> hash >= PKEY_HASH__LAST ||
 modsig-> id_type >= PKEY_ID_TYPE__LAST)
 > -		return false;
 > +		return NULL;
 >  	sig_len = be32toh(get_unaligned(&modsig->sig_len));
 >  	if (sig_len == 0 ||
 >  	    size < (int64_t)(modsig->signer_len + modsig->key_id_len + sig_len))
 > -		return false;
 > +		return NULL;
 
 >  	if (modsig->id_type == PKEY_ID_PKCS7)
 > -		ret = kmod_module_signature_info_pkcs7(mem, size,
 > -						       modsig, sig_len,
 > -						       sig_info);
 > +		return kmod_module_signature_info_pkcs7(mem, size,
 > +						        modsig, sig_len);
 >  	else
 > -		ret = kmod_module_signature_info_default(mem, size,
 > -							 modsig, sig_len,
 > -							 sig_info);
 > -	return ret;
 > +		return kmod_module_signature_info_default(mem, size,
 > +							  modsig, sig_len);
 >  }
 
 >  void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
 >  {
 >  	if (sig_info->free)
 sig_info-> free(sig_info);
 > +	free(sig_info);
 >  }
 > -- 
 > 2.13.6

 > --
 > To unsubscribe from this list: send the line "unsubscribe linux-modules" in
 > the body of a message to majordomo@vger.kernel.org
 > More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michal Suchanek March 13, 2018, 9:57 a.m. UTC | #2
On Mon, 12 Mar 2018 22:41:19 +0200
Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote:

> Hi, Michal!
> 
> >>>>> On Thu,  8 Mar 2018 19:14:26 +0100, Michal Suchanek  wrote:  
> 
>  > The mod_sig is allocated on stack and when no signature is present
>  > it is not initialized and contains garbage. Later when freeing
>  > mod_sig garbage pointer is dereferenced.  
> 
> I guess, it is enough to init the structure.

Yes, it is enough to initialize the structure. However, this issue is
caused by the awkward interface to the kmod_module_signature_info
function so changing the interface should prevent such errors in the
future.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-modules" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yauheni Kaliuta March 13, 2018, 10:03 a.m. UTC | #3
Hi, Michal!

(Leaving in Cc only the list and maintainer)

>>>>> On Tue, 13 Mar 2018 10:57:39 +0100, Michal Suchánek  wrote:

 > On Mon, 12 Mar 2018 22:41:19 +0200
 > Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote:

 >> Hi, Michal!
 >> 
 >> >>>>> On Thu,  8 Mar 2018 19:14:26 +0100, Michal Suchanek  wrote:  
 >> 
 >> > The mod_sig is allocated on stack and when no signature is present
 >> > it is not initialized and contains garbage. Later when freeing
 >> > mod_sig garbage pointer is dereferenced.  
 >> 
 >> I guess, it is enough to init the structure.

 > Yes, it is enough to initialize the structure. However, this issue is
 > caused by the awkward interface to the kmod_module_signature_info
 > function so changing the interface should prevent such errors in the
 > future.

Frankly, I made the same change of the interface in my initial
implementation before the functionality :) But it's up to Lucas, I guess,
you can send the patch on top of the master already now ready for
applying.

Patch
diff mbox

diff --git a/libkmod/libkmod-internal.h b/libkmod/libkmod-internal.h
index 2ad74c7a33a5..8bc9ecfb906e 100644
--- a/libkmod/libkmod-internal.h
+++ b/libkmod/libkmod-internal.h
@@ -192,5 +192,5 @@  struct kmod_signature_info {
 	void (*free)(void *);
 	void *private;
 };
-bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info) _must_check_ __attribute__((nonnull(1, 2)));
+struct kmod_signature_info *kmod_module_signature_info(const struct kmod_file *file) _must_check_ __attribute__((nonnull(1)));
 void kmod_module_signature_info_free(struct kmod_signature_info *sig_info) __attribute__((nonnull));
diff --git a/libkmod/libkmod-module.c b/libkmod/libkmod-module.c
index 7b00d526edec..cbe35819932e 100644
--- a/libkmod/libkmod-module.c
+++ b/libkmod/libkmod-module.c
@@ -2304,7 +2304,7 @@  KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 	struct kmod_elf *elf;
 	char **strings;
 	int i, count, ret = -ENOMEM;
-	struct kmod_signature_info sig_info;
+	struct kmod_signature_info *sig_info = NULL;
 
 	if (mod == NULL || list == NULL)
 		return -ENOENT;
@@ -2341,32 +2341,32 @@  KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 			goto list_error;
 	}
 
-	if (kmod_module_signature_info(mod->file, &sig_info)) {
+	if (sig_info = kmod_module_signature_info(mod->file)) {
 		struct kmod_list *n;
 
 		n = kmod_module_info_append(list, "sig_id", strlen("sig_id"),
-				sig_info.id_type, strlen(sig_info.id_type));
+				sig_info->id_type, strlen(sig_info->id_type));
 		if (n == NULL)
 			goto list_error;
 		count++;
 
 		n = kmod_module_info_append(list, "signer", strlen("signer"),
-				sig_info.signer, sig_info.signer_len);
+				sig_info->signer, sig_info->signer_len);
 		if (n == NULL)
 			goto list_error;
 		count++;
 
 
 		n = kmod_module_info_append_hex(list, "sig_key", strlen("sig_key"),
-						sig_info.key_id,
-						sig_info.key_id_len);
+						sig_info->key_id,
+						sig_info->key_id_len);
 		if (n == NULL)
 			goto list_error;
 		count++;
 
 		n = kmod_module_info_append(list,
 				"sig_hashalgo", strlen("sig_hashalgo"),
-				sig_info.hash_algo, strlen(sig_info.hash_algo));
+				sig_info->hash_algo, strlen(sig_info->hash_algo));
 		if (n == NULL)
 			goto list_error;
 		count++;
@@ -2377,8 +2377,8 @@  KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 		 */
 		n = kmod_module_info_append_hex(list, "signature",
 						strlen("signature"),
-						sig_info.sig,
-						sig_info.sig_len);
+						sig_info->sig,
+						sig_info->sig_len);
 
 		if (n == NULL)
 			goto list_error;
@@ -2389,7 +2389,8 @@  KMOD_EXPORT int kmod_module_get_info(const struct kmod_module *mod, struct kmod_
 
 list_error:
 	/* aux structures freed in normal case also */
-	kmod_module_signature_info_free(&sig_info);
+	if (sig_info)
+		kmod_module_signature_info_free(sig_info);
 
 	if (ret < 0) {
 		kmod_module_info_free_list(*list);
diff --git a/libkmod/libkmod-signature.c b/libkmod/libkmod-signature.c
index bdf84cb14718..fae074e6dd1d 100644
--- a/libkmod/libkmod-signature.c
+++ b/libkmod/libkmod-signature.c
@@ -93,13 +93,17 @@  struct module_signature {
 	uint32_t sig_len;    /* Length of signature data (big endian) */
 };
 
-static bool
+static struct kmod_signature_info *
 kmod_module_signature_info_default(const char *mem,
 				   off_t size,
 				   const struct module_signature *modsig,
-				   size_t sig_len,
-				   struct kmod_signature_info *sig_info)
+				   size_t sig_len)
 {
+	struct kmod_signature_info *sig_info = malloc(sizeof *sig_info);
+
+	if (!sig_info)
+		return NULL;
+
 	size -= sig_len;
 	sig_info->sig = mem + size;
 	sig_info->sig_len = sig_len;
@@ -119,7 +123,7 @@  kmod_module_signature_info_default(const char *mem,
 	sig_info->free = NULL;
 	sig_info->private = NULL;
 
-	return true;
+	return sig_info;
 }
 
 static void kmod_module_signature_info_pkcs7_free(void *s)
@@ -129,13 +133,13 @@  static void kmod_module_signature_info_pkcs7_free(void *s)
 	pkcs7_free_cert(si->private);
 }
 
-static bool
+static struct kmod_signature_info *
 kmod_module_signature_info_pkcs7(const char *mem,
 				 off_t size,
 				 const struct module_signature *modsig,
-				 size_t sig_len,
-				 struct kmod_signature_info *sig_info)
+				 size_t sig_len)
 {
+	struct kmod_signature_info *sig_info = NULL;
 	const char *pkcs7_raw;
 	struct pkcs7_cert *cert;
 
@@ -144,7 +148,13 @@  kmod_module_signature_info_pkcs7(const char *mem,
 
 	cert = pkcs7_parse_cert(pkcs7_raw, sig_len);
 	if (cert == NULL)
-		return false;
+		return NULL;
+
+	sig_info = malloc(sizeof *sig_info);
+	if (!sig_info) {
+		free(cert);
+		return NULL;
+	}
 
 	sig_info->private = cert;
 	sig_info->free = kmod_module_signature_info_pkcs7_free;
@@ -162,7 +172,7 @@  kmod_module_signature_info_pkcs7(const char *mem,
 	sig_info->hash_algo = cert->hash_algo;
 	sig_info->id_type = pkey_id_type[modsig->id_type];
 
-	return true;
+	return sig_info;
 }
 
 #define SIG_MAGIC "~Module signature appended~\n"
@@ -178,48 +188,45 @@  kmod_module_signature_info_pkcs7(const char *mem,
  * [ SIG_MAGIC               ]
  */
 
-bool kmod_module_signature_info(const struct kmod_file *file, struct kmod_signature_info *sig_info)
+struct kmod_signature_info *kmod_module_signature_info(const struct kmod_file *file)
 {
 	const char *mem;
 	off_t size;
 	const struct module_signature *modsig;
 	size_t sig_len;
-	bool ret;
 
 	size = kmod_file_get_size(file);
 	mem = kmod_file_get_contents(file);
 	if (size < (off_t)strlen(SIG_MAGIC))
-		return false;
+		return NULL;
 	size -= strlen(SIG_MAGIC);
 	if (memcmp(SIG_MAGIC, mem + size, strlen(SIG_MAGIC)) != 0)
-		return false;
+		return NULL;
 
 	if (size < (off_t)sizeof(struct module_signature))
-		return false;
+		return NULL;
 	size -= sizeof(struct module_signature);
 	modsig = (struct module_signature *)(mem + size);
 	if (modsig->algo >= PKEY_ALGO__LAST ||
 			modsig->hash >= PKEY_HASH__LAST ||
 			modsig->id_type >= PKEY_ID_TYPE__LAST)
-		return false;
+		return NULL;
 	sig_len = be32toh(get_unaligned(&modsig->sig_len));
 	if (sig_len == 0 ||
 	    size < (int64_t)(modsig->signer_len + modsig->key_id_len + sig_len))
-		return false;
+		return NULL;
 
 	if (modsig->id_type == PKEY_ID_PKCS7)
-		ret = kmod_module_signature_info_pkcs7(mem, size,
-						       modsig, sig_len,
-						       sig_info);
+		return kmod_module_signature_info_pkcs7(mem, size,
+						        modsig, sig_len);
 	else
-		ret = kmod_module_signature_info_default(mem, size,
-							 modsig, sig_len,
-							 sig_info);
-	return ret;
+		return kmod_module_signature_info_default(mem, size,
+							  modsig, sig_len);
 }
 
 void kmod_module_signature_info_free(struct kmod_signature_info *sig_info)
 {
 	if (sig_info->free)
 		sig_info->free(sig_info);
+	free(sig_info);
 }