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
