unaligned access in pkcs7_verify

Commit Message

Sowmini Varadhan Oct. 13, 2015, 1:29 p.m. UTC

On (10/12/15 21:32), Herbert Xu wrote:
> 
> .. pkcs7_verify definitely
> shouldn't place the structure after the digest without aligning the
> pointer.  So something like your patch is needed (but please use
> alignof instead of sizeof).  Also don't put in digest_size but
> instead align the pointer like
> 
> 	desc = PTR_ALIGN(digest + digest_size, ...)

I tried the patch below for 24+ hours, and it ran without mishaps. 
But given that the panics I saw earlier (page faults typically
triggered  by openswan's pluto) occurred unpredictably, it would be 
useful to have an expert-review of this code.

Thanks,
--Sowmini

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Herbert Xu Oct. 13, 2015, 1:36 p.m. UTC | #1

On Tue, Oct 13, 2015 at 09:29:28AM -0400, Sowmini Varadhan wrote:
> 
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index d20c0b4..958ac01 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -46,14 +46,15 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
>  		return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
>  
>  	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> -	sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> -
> +	sinfo->sig.digest_size = crypto_shash_digestsize(tfm);
> +	digest_size = crypto_shash_digestsize(tfm) +
> +		ALIGN(crypto_shash_digestsize(tfm), __alignof__(*desc));

You should leave digest_size as is but round it up when you use
it in kzalloc.  Otherwise you are essentially rounding it up
twice which happens to work but looks rather odd.  It may also
lead to bugs later on if somebody uses digest_size as the digest
size.

>  	ret = -ENOMEM;
>  	digest = kzalloc(digest_size + desc_size, GFP_KERNEL);

So I would replace this with

	digest = kzalloc(ALIGN(digest_size, desc_align) + desc_size, GFP_KERNEL);

>  	if (!digest)
>  		goto error_no_desc;
>  
> -	desc = digest + digest_size;
> +	desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc));

This looks good.

BTW when you submit this please add a sign-off as specified by
Documentation/SubmittingPatches.

Thanks,

Patch

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index d20c0b4..958ac01 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -46,14 +46,15 @@  static int pkcs7_digest(struct pkcs7_message *pkcs7,
 		return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
 
 	desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
-	sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
-
+	sinfo->sig.digest_size = crypto_shash_digestsize(tfm);
+	digest_size = crypto_shash_digestsize(tfm) +
+		ALIGN(crypto_shash_digestsize(tfm), __alignof__(*desc));
 	ret = -ENOMEM;
 	digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
 	if (!digest)
 		goto error_no_desc;
 
-	desc = digest + digest_size;
+	desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc));
 	desc->tfm   = tfm;
 	desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;