crypto: Add 0 walk-offset check in scatterwalk_pagedone()
diff mbox

Message ID 1531127419-47014-1-git-send-email-liuchao741@huawei.com
State Changes Requested
Delegated to: Herbert Xu
Headers show

Commit Message

Liu Chao July 9, 2018, 9:10 a.m. UTC
From: Luo Xinqiang <luoxinqiang2@huawei.com>

In function scatterwalk_pagedone(), a kernel panic of invalid
page will occur if walk->offset equals 0. This patch fixes the
problem by setting the page addresswith sg_page(walk->sg)
directly if walk->offset equals 0.

Panic call stack:
[<ffffff9632b4f418>] blkcipher_walk_done+0x430/0x8dc
[<ffffff9632b4ed50>] blkcipher_walk_next+0x750/0x9e8
[<ffffff9632b4fe08>] blkcipher_walk_first+0x110/0x2c0
[<ffffff9632b50084>] blkcipher_walk_virt+0xcc/0xe0
[<ffffff96324b3680>] cbc_decrypt+0xdc/0x1a8
[<ffffff9632ba3f90>] ablk_decrypt+0x138/0x224
[<ffffff9632b50a90>] skcipher_decrypt_ablkcipher+0x130/0x150
[<ffffff9632b9d6d4>] skcipher_recvmsg_sync.isra.17+0x270/0x404
[<ffffff9632b9d900>] skcipher_recvmsg+0x98/0xb8
[<ffffff9634044908>] SyS_recvfrom+0x2ac/0x2fc
[<ffffff96324839c0>] el0_svc_naked+0x34/0x38

Test: do syskaller fuzz test on 4.9 & 4.4

Signed-off-by: Gao Kui <gaokui1@huawei.com>
Signed-off-by: Luo Xinqiang <luoxinqiang2@huawei.com>
---
 crypto/scatterwalk.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Eric Biggers July 15, 2018, 2:04 p.m. UTC | #1
Hi Liu,

On Mon, Jul 09, 2018 at 05:10:19PM +0800, Liu Chao wrote:
> From: Luo Xinqiang <luoxinqiang2@huawei.com>
> 
> In function scatterwalk_pagedone(), a kernel panic of invalid
> page will occur if walk->offset equals 0. This patch fixes the
> problem by setting the page addresswith sg_page(walk->sg)
> directly if walk->offset equals 0.
> 
> Panic call stack:
> [<ffffff9632b4f418>] blkcipher_walk_done+0x430/0x8dc
> [<ffffff9632b4ed50>] blkcipher_walk_next+0x750/0x9e8
> [<ffffff9632b4fe08>] blkcipher_walk_first+0x110/0x2c0
> [<ffffff9632b50084>] blkcipher_walk_virt+0xcc/0xe0
> [<ffffff96324b3680>] cbc_decrypt+0xdc/0x1a8
> [<ffffff9632ba3f90>] ablk_decrypt+0x138/0x224
> [<ffffff9632b50a90>] skcipher_decrypt_ablkcipher+0x130/0x150
> [<ffffff9632b9d6d4>] skcipher_recvmsg_sync.isra.17+0x270/0x404
> [<ffffff9632b9d900>] skcipher_recvmsg+0x98/0xb8
> [<ffffff9634044908>] SyS_recvfrom+0x2ac/0x2fc
> [<ffffff96324839c0>] el0_svc_naked+0x34/0x38
> 
> Test: do syskaller fuzz test on 4.9 & 4.4
> 
> Signed-off-by: Gao Kui <gaokui1@huawei.com>
> Signed-off-by: Luo Xinqiang <luoxinqiang2@huawei.com>
> ---
>  crypto/scatterwalk.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
> index bc769c4..a265907 100644
> --- a/crypto/scatterwalk.c
> +++ b/crypto/scatterwalk.c
> @@ -53,7 +53,11 @@ static void scatterwalk_pagedone(struct scatter_walk *walk, int out,
>  	if (out) {
>  		struct page *page;
>  
> -		page = sg_page(walk->sg) + ((walk->offset - 1) >> PAGE_SHIFT);
> +		if (likely(walk->offset))
> +			page = sg_page(walk->sg) +
> +				((walk->offset - 1) >> PAGE_SHIFT);
> +		else
> +			page = sg_page(walk->sg);
>  		/* Test ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE first as
>  		 * PageSlab cannot be optimised away per se due to
>  		 * use of volatile pointer.

Interesting, I guess the reason this wasn't found by syzbot yet is that syzbot
currently only runs on x86, where ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE isn't
defined.  Otherwise this crash reproduces on the latest kernel by running the
following program:

	#include <linux/if_alg.h>
	#include <sys/socket.h>
	#include <unistd.h>

	int main()
	{
		struct sockaddr_alg addr = {
			.salg_type = "skcipher",
			.salg_name = "cbc(aes)",
		};
		int algfd, reqfd;
		char buffer[4096] __attribute__((aligned(4096))) = { 0 };

		algfd = socket(AF_ALG, SOCK_SEQPACKET, 0);
		bind(algfd, (void *)&addr, sizeof(addr));
		setsockopt(algfd, SOL_ALG, ALG_SET_KEY, buffer, 32);
		reqfd = accept(algfd, NULL, NULL);
		write(reqfd, buffer, 15);
		read(reqfd, buffer, 15);
	}

I don't think your fix makes sense though, because if walk->offset = 0 then no
data was processed, so there would be no need to flush any page at all.  I think
the original intent was that scatterwalk_pagedone() only be called when a
nonzero length was processed.  So a better fix is probably to update
blkcipher_walk_done() (and skcipher_walk_done() and ablkcipher_walk_done()) to
avoid calling scatterwalk_pagedone() in the error case where no bytes were
processed.  I'm working on that fix but it's not ready quite yet.

Thanks!

- Eric

Patch
diff mbox

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index bc769c4..a265907 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -53,7 +53,11 @@  static void scatterwalk_pagedone(struct scatter_walk *walk, int out,
 	if (out) {
 		struct page *page;
 
-		page = sg_page(walk->sg) + ((walk->offset - 1) >> PAGE_SHIFT);
+		if (likely(walk->offset))
+			page = sg_page(walk->sg) +
+				((walk->offset - 1) >> PAGE_SHIFT);
+		else
+			page = sg_page(walk->sg);
 		/* Test ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE first as
 		 * PageSlab cannot be optimised away per se due to
 		 * use of volatile pointer.