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:ebiggers3@xxxxxxxxx] 发送时间: 2018年7月15日 22:05 收件人: Liuchao (H) <liuchao741@xxxxxxxxxx> 抄送: herbert@xxxxxxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; linux-crypto@xxxxxxxxxxxxxxx; dongjinguang <dongjinguang@xxxxxxxxxx>; ebiggers@xxxxxxxxxx; 罗新强 <luoxinqiang2@xxxxxxxxxx>; gaokui (A) <gaokui1@xxxxxxxxxx> 主题: 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 <luoxinqiang2@xxxxxxxxxx> > > 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@xxxxxxxxxx> > Signed-off-by: Luo Xinqiang <luoxinqiang2@xxxxxxxxxx> > --- > 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
