unaligned access in pkcs7_verify

PATCH Sowmini Varadhan 3 participants 9 messages Oct 2, 2015 – Oct 13, 2015

Subsystems (2)

Crypto API Asymmetric keys

lore

Share & tools

lore

Sowmini Varadhan

author

2015-10-02 14:00 UTC #0 lore raw

Hi,

I'm getting a lot of unaligned access messages each time I do "modprobe [-r] <module>" on sparc:

Kernel unaligned access at TPC[6ad9b4] pkcs7_verify+0x1ec/0x5e0
Kernel unaligned access at TPC[6a5484] crypto_shash_finup+0xc/0x5c Kernel unaligned access at TPC[6a5390] crypto_shash_update+0xc/0x54 Kernel unaligned access at TPC[10150308] sha1_sparc64_update+0x14/0x5c [sha1_sparc64] Kernel unaligned access at TPC[101501ac] __sha1_sparc64_update+0xc/0x98 [sha1_sparc64]

Looks like these are being caused by an unaligned desc at

        desc = digest + digest_size;

Doing this:

crypto/asymmetric_keys/pkcs7_verify.c +2 -1

@@ -46,7 +46,8 @@ 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 = digest_size = 
+               ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
 
        ret = -ENOMEM;
        digest = kzalloc(digest_size + desc_size, GFP_KERNEL);

makes the unaliagned message go away, but I dont know if sinfo->sig.digest_size needs to be set to the (unaligned) raw value of crypto_shash_digestsize() itself, and how to verify that this doesnt break something else in crypto

--Sowmini

Herbert Xu

maintainer

2015-10-08 13:15 UTC #1 lore raw

Quoted 29 lines · Sowmini Varadhan

I'm getting a lot of unaligned access messages each time I do "modprobe [-r] <module>" on sparc:
Kernel unaligned access at TPC[6ad9b4] pkcs7_verify+0x1ec/0x5e0
Kernel unaligned access at TPC[6a5484] crypto_shash_finup+0xc/0x5c Kernel unaligned access at TPC[6a5390] crypto_shash_update+0xc/0x54 Kernel unaligned access at TPC[10150308] sha1_sparc64_update+0x14/0x5c [sha1_sparc64] Kernel unaligned access at TPC[101501ac] __sha1_sparc64_update+0xc/0x98 [sha1_sparc64]
Looks like these are being caused by an unaligned desc at
        desc = digest + digest_size;
Doing this:
crypto/asymmetric_keys/pkcs7_verify.c +2 -1
@@ -46,7 +46,8 @@ 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 = digest_size = 
+               ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
 
        ret = -ENOMEM;
        digest = kzalloc(digest_size + desc_size, GFP_KERNEL);

Quoted 4 lines

makes the unaliagned message go away, but I dont know if sinfo->sig.digest_size needs to be set to the (unaligned) raw value of crypto_shash_digestsize() itself, and how to verify that this doesnt break something else in crypto

What hash algorithm were you using?

Thanks,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sowmini Varadhan

author

2015-10-08 14:43 UTC #2 lore raw

Quoted 4 lines · Herbert Xu

crypto/asymmetric_keys/pkcs7_verify.c static int pkcs7_digest(struct pkcs7_message *pkcs7, · line 50
Quoted 4 lines
        desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
crypto/asymmetric_keys/pkcs7_verify.c +2 -1
-       sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
+       sinfo->sig.digest_size = digest_size = 
+               ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));

Quoted 1 line

What hash algorithm were you using?

Algorithm is sha1. From printk, crypto_shash_descsize(tfm) comes out to 0x60, digest_size to 0x14. Stack trace (for each modprobe [-r]) is

	pkcs7_verify+0x1d0/0x5e0
	system_verify_data+0x54/0xb4
	mod_verify_sig+0xa0/0xc4
	load_module+0x48/0x16a0
	SyS_init_module+0x114/0x128
	linux_sparc_syscall+0x34/0x44

--Sowmini

Herbert Xu

maintainer

2015-10-12 13:32 UTC #3 lore raw

Quoted 11 lines · Sowmini Varadhan

Quoted 4 lines · Herbert Xu
crypto/asymmetric_keys/pkcs7_verify.c static int pkcs7_digest(struct pkcs7_message *pkcs7, · line 50
Quoted 4 lines
        desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
crypto/asymmetric_keys/pkcs7_verify.c +2 -1
-       sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
+       sinfo->sig.digest_size = digest_size = 
+               ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
:
Quoted 1 line
What hash algorithm were you using?
Algorithm is sha1. From printk, crypto_shash_descsize(tfm) comes out to 0x60, digest_size to 0x14. Stack trace (for each modprobe [-r]) is

Quoted 6 lines

	pkcs7_verify+0x1d0/0x5e0
	system_verify_data+0x54/0xb4
	mod_verify_sig+0xa0/0xc4
	load_module+0x48/0x16a0
	SyS_init_module+0x114/0x128
	linux_sparc_syscall+0x34/0x44

Thanks. We have two bugs here. First of all 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, ...)

The sparc sha algorithms themselves need to declare the alignment that they require. Currently they claim to be able to handle any alignment which appears to not be the case.

Cheers,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sowmini Varadhan

author

2015-10-12 13:46 UTC #4 lore raw

Quoted 7 lines · Herbert Xu

Thanks. We have two bugs here. First of all 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, ...)

That patch might not be rock-solid by itself though. I was seeing some panics/crashes when I was running with that patch, so I backed it off temporarily - should sinfo->sig.digest_size be set to the aligned value?

Quoted 3 lines

The sparc sha algorithms themselves need to declare the alignment that they require. Currently they claim to be able to handle any alignment which appears to not be the case.

How would one do that correctly? I'm not a crypto expert, but I can help test the patch..

--Sowmini

David Miller

maintainer

2015-10-12 13:50 UTC #5 lore raw

In reply to Herbert Xu

From: Herbert Xu <[email protected]>
Date: Mon, 12 Oct 2015 21:32:09 +0800

Quoted 3 lines

The sparc sha algorithms themselves need to declare the alignment that they require. Currently they claim to be able to handle any alignment which appears to not be the case.

The sparc SHA assembler can handle arbitrary alignment.

Herbert Xu

maintainer

2015-10-13 13:39 UTC #6 lore raw

Quoted 8 lines · David Miller

From: Herbert Xu <[email protected]>
Date: Mon, 12 Oct 2015 21:32:09 +0800
Quoted 3 lines
The sparc sha algorithms themselves need to declare the alignment that they require. Currently they claim to be able to handle any alignment which appears to not be the case.
The sparc SHA assembler can handle arbitrary alignment.

Right. The warnings that originated in sha1_sparc64 are probably just the result of an unaligned desc structure caused by the bug in pkcs7_verify.

Thanks for the confirmation.

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Sowmini Varadhan

author

2015-10-13 13:29 UTC #7 lore raw

In reply to Herbert Xu

Quoted 8 lines · Herbert Xu

.. 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

crypto/asymmetric_keys/pkcs7_verify.c +4 -3

index d20c0b4..958ac01 100644
@@ -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;

Herbert Xu

maintainer

2015-10-13 13:36 UTC #8 lore raw

Quoted 6 lines · Sowmini Varadhan

crypto/asymmetric_keys/pkcs7_verify.c +0 -0
index d20c0b4..958ac01 100644

crypto/asymmetric_keys/pkcs7_verify.c static int pkcs7_digest(struct pkcs7_message *pkcs7, · line 51

Quoted 8 lines

crypto/asymmetric_keys/pkcs7_verify.c +3 -2

@@ -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.

crypto/asymmetric_keys/pkcs7_verify.c static int pkcs7_digest(struct pkcs7_message *pkcs7, · line 53

Quoted 2 lines

crypto/asymmetric_keys/pkcs7_verify.c +0 -0
 	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);

crypto/asymmetric_keys/pkcs7_verify.c static int pkcs7_digest(struct pkcs7_message *pkcs7, · line 57

Quoted 5 lines

crypto/asymmetric_keys/pkcs7_verify.c +1 -1

 	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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt