Re: [PATCH] crypto: Add 0 walk-offset check in scatterwalk_pagedone()
Hi, Eric,
Thanks for your reply. I had tried your program on a original kernel and it
reproduced the crash. And I also tried the program on a kernel with our patch,
but there was no crash occur.
I think the crash reason of the program is that, the parameter buffer is
aligned with the page and its address starts at the beginning of the page,
making walk->offset = 0 and generating the crash. I had added log in
scatterwalk_pagedone() to print value of walk->offset, and the log before the
crash showed that walk->offset = 0.
And what still confuses me is that why walk->offset = 0 means no data to be
processed. In the structure scatterlist, the member offset represents the
offset of the buffer in the page, while the member length represents the length
of the buffer. In function af_alg_make_sg(), we also can see that, if a buffer
occupies more than one pages, the offset will be set to 0 in the second and
following pages. And In function scatterwalk_done(), walk->offset = 0 will also
allow to call scatterwalk_pagedone(). So I think that walk->offset = 0 needs to
flush the page as well.
Thanks.
-邮件原件-
发件人: Eric Biggers [mailto:ebigge...@gmail.com]
发送时间: 2018年7月15日 22:05
收件人: Liuchao (H)
抄送: herb...@gondor.apana.org.au; da...@davemloft.net;
linux-crypto@vger.kernel.org; dongjinguang ;
ebigg...@google.com; 罗新强 ; gaokui (A)
主题: Re: [PATCH] crypto: Add 0 walk-offset check in scatterwalk_pagedone()
Hi Liu,
On Mon, Jul 09, 2018 at 05:10:19PM +0800, Liu Chao wrote:
> From: Luo Xinqiang
>
> 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:
> [] blkcipher_walk_done+0x430/0x8dc
> [] blkcipher_walk_next+0x750/0x9e8
> [] blkcipher_walk_first+0x110/0x2c0
> [] blkcipher_walk_virt+0xcc/0xe0
> [] cbc_decrypt+0xdc/0x1a8 []
> ablk_decrypt+0x138/0x224 []
> skcipher_decrypt_ablkcipher+0x130/0x150
> [] skcipher_recvmsg_sync.isra.17+0x270/0x404
> [] skcipher_recvmsg+0x98/0xb8 []
> SyS_recvfrom+0x2ac/0x2fc [] el0_svc_naked+0x34/0x38
>
> Test: do syskaller fuzz test on 4.9 & 4.4
>
> Signed-off-by: Gao Kui
> Signed-off-by: Luo Xinqiang
> ---
> 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
#include
#include
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 *), 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
Re: [PATCH] crypto: Add 0 walk-offset check in scatterwalk_pagedone()
Hi Liu,
On Mon, Jul 09, 2018 at 05:10:19PM +0800, Liu Chao wrote:
> From: Luo Xinqiang
>
> 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:
> [] blkcipher_walk_done+0x430/0x8dc
> [] blkcipher_walk_next+0x750/0x9e8
> [] blkcipher_walk_first+0x110/0x2c0
> [] blkcipher_walk_virt+0xcc/0xe0
> [] cbc_decrypt+0xdc/0x1a8
> [] ablk_decrypt+0x138/0x224
> [] skcipher_decrypt_ablkcipher+0x130/0x150
> [] skcipher_recvmsg_sync.isra.17+0x270/0x404
> [] skcipher_recvmsg+0x98/0xb8
> [] SyS_recvfrom+0x2ac/0x2fc
> [] el0_svc_naked+0x34/0x38
>
> Test: do syskaller fuzz test on 4.9 & 4.4
>
> Signed-off-by: Gao Kui
> Signed-off-by: Luo Xinqiang
> ---
> 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
#include
#include
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 *), 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] crypto: Add 0 walk-offset check in scatterwalk_pagedone()
From: Luo Xinqiang
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:
[] blkcipher_walk_done+0x430/0x8dc
[] blkcipher_walk_next+0x750/0x9e8
[] blkcipher_walk_first+0x110/0x2c0
[] blkcipher_walk_virt+0xcc/0xe0
[] cbc_decrypt+0xdc/0x1a8
[] ablk_decrypt+0x138/0x224
[] skcipher_decrypt_ablkcipher+0x130/0x150
[] skcipher_recvmsg_sync.isra.17+0x270/0x404
[] skcipher_recvmsg+0x98/0xb8
[] SyS_recvfrom+0x2ac/0x2fc
[] el0_svc_naked+0x34/0x38
Test: do syskaller fuzz test on 4.9 & 4.4
Signed-off-by: Gao Kui
Signed-off-by: Luo Xinqiang
---
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.
--
2.7.4
