Re: 答复: [PATCH 1/3] crypto: skcipher - fix crash flushing dcache in error path
Hi GaoKui, On Thu, Jul 26, 2018 at 02:44:30AM +, gaokui (A) wrote: > Hi, Eric, > Thanks for your reply. > > I have run your program on an original kernel and it reproduced the > crash. And I also run the program on a kernel with our patch, but there was > no crash. > > I think the reason of the crash is the parameter buffer is aligned > with the page . So the address of the parameter buffer starts at the > beginning of the page, which making "walk->offset = 0" and generating the > crash. I add some logs in "scatterwalk_pagedone()" to print the value of > walk->offset, and the log before the crash shows that "walk->offset = 0". > > And I do not understand 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, and the member length represents the > length of the buffer. In function "af_alg_make_sg()", if a buffer occupies > more than one pages, the offset will also 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 when > "walk->offset = 0" the page needs to be flushed as well. > > BRs > GaoKui > Did you test my patches or just yours? Your patch fixes the crash, but I don't agree that it's the best fix. What you're missing is that walk->offset has already been increased by scatterwalk_advance() to the offset of the *end* of the data region processed. Hence, walk->offset = 0 implies that 0 bytes were processed (as walk->offset must have been 0 initially, then had 0 added to it), which I think isn't meant to be a valid case. And in particular it does *not* make sense to flush any page when 0 bytes were processed. Note that this could also be a problem for empty scatterlist elements, but AFAICS the scatterlist walk code doesn't actually support those when the total length isn't 0. I think that needs improvement too, but AFAICS other changes would be needed to properly fix that limitation, and you apparently cannot generate empty scatterlist elements via AF_ALG anyway so only in-kernel users would be affected. - Eric
答复: [PATCH 1/3] crypto: skcipher - fix crash flushing dcache in error path
Hi, Eric,
Thanks for your reply.
I have run your program on an original kernel and it reproduced the
crash. And I also run the program on a kernel with our patch, but there was no
crash.
I think the reason of the crash is the parameter buffer is aligned
with the page . So the address of the parameter buffer starts at the beginning
of the page, which making "walk->offset = 0" and generating the crash. I add
some logs in "scatterwalk_pagedone()" to print the value of walk->offset, and
the log before the crash shows that "walk->offset = 0".
And I do not understand 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, and the member length represents the length
of the buffer. In function "af_alg_make_sg()", if a buffer occupies more than
one pages, the offset will also 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 when "walk->offset = 0" the page
needs to be flushed as well.
BRs
GaoKui
> -邮件原件-
> 发件人: linux-crypto-ow...@vger.kernel.org [mailto:linux-crypto-
> ow...@vger.kernel.org] 代表 Eric Biggers
> 发送时间: 2018年7月24日 1:55
> 收件人: linux-crypto@vger.kernel.org; Herbert Xu
>
> 抄送: Liuchao (H) ; 罗新强
> ; gaokui (A) ; Eric
> Biggers
> 主题: [PATCH 1/3] crypto: skcipher - fix crash flushing dcache in error path
>
> From: Eric Biggers
>
> scatterwalk_done() is only meant to be called after a nonzero number of
> bytes have been processed, since scatterwalk_pagedone() will flush the
> dcache of the *previous* page. But in the error case of
> skcipher_walk_done(), e.g. if the input wasn't an integer number of blocks,
> scatterwalk_done() was actually called after advancing 0 bytes.
> This caused a crash ("BUG: unable to handle kernel paging request") during
> '!PageSlab(page)' on architectures like arm and arm64 that define
> ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
> page-aligned as in that case walk->offset == 0.
>
> Fix it by reorganizing skcipher_walk_done() to skip the
> scatterwalk_advance() and scatterwalk_done() if an error has occurred.
>
> This bug was found by syzkaller fuzzing.
>
> Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
>
> #include
> #include
> #include
>
> int main()
> {
> struct sockaddr_alg addr = {
> .salg_type = "skcipher",
> .salg_name = "cbc(aes-generic)",
> };
> char buffer[4096] __attribute__((aligned(4096))) = { 0 };
> int fd;
>
> fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
> bind(fd, (void *), sizeof(addr));
> setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
> fd = accept(fd, NULL, NULL);
> write(fd, buffer, 15);
> read(fd, buffer, 15);
> }
>
> Reported-by: Liu Chao
> Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface")
> Cc: # v4.10+
> Signed-off-by: Eric Biggers
> ---
> crypto/skcipher.c | 53 ---
> 1 file changed, 27 insertions(+), 26 deletions(-)
>
> diff --git a/crypto/skcipher.c b/crypto/skcipher.c index
> 7d6a49fe3047..5f7017b36d75 100644
> --- a/crypto/skcipher.c
> +++ b/crypto/skcipher.c
> @@ -95,7 +95,7 @@ static inline u8 *skcipher_get_spot(u8 *start,
> unsigned int len)
> return max(start, end_page);
> }
>
> -static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int
> bsize)
> +static void skcipher_done_slow(struct skcipher_walk *walk, unsigned int
> +bsize)
> {
> u8 *addr;
>
> @@ -103,23 +103,24 @@ static int skcipher_done_slow(struct
> skcipher_walk *walk, unsigned int bsize)
> addr = skcipher_get_spot(addr, bsize);
> scatterwalk_copychunks(addr, >out, bsize,
> (walk->flags & SKCIPHER_WALK_PHYS) ? 2 : 1);
> - return 0;
> }
>
> int skcipher_walk_done(struct skcipher_walk *walk, int err) {
> - unsigned int n = walk->nbytes - err;
> - unsigned int nbytes;
> -
> - nbytes = walk->total - n;
> -
> - if (unlikely(err < 0)) {
> - nbytes = 0;
> - n = 0;
> - } else if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
> -SKCIPHER_WALK_SLOW |
> -
[PATCH 1/3] crypto: skcipher - fix crash flushing dcache in error path
From: Eric Biggers
scatterwalk_done() is only meant to be called after a nonzero number of
bytes have been processed, since scatterwalk_pagedone() will flush the
dcache of the *previous* page. But in the error case of
skcipher_walk_done(), e.g. if the input wasn't an integer number of
blocks, scatterwalk_done() was actually called after advancing 0 bytes.
This caused a crash ("BUG: unable to handle kernel paging request")
during '!PageSlab(page)' on architectures like arm and arm64 that define
ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
page-aligned as in that case walk->offset == 0.
Fix it by reorganizing skcipher_walk_done() to skip the
scatterwalk_advance() and scatterwalk_done() if an error has occurred.
This bug was found by syzkaller fuzzing.
Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
#include
#include
#include
int main()
{
struct sockaddr_alg addr = {
.salg_type = "skcipher",
.salg_name = "cbc(aes-generic)",
};
char buffer[4096] __attribute__((aligned(4096))) = { 0 };
int fd;
fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(fd, (void *), sizeof(addr));
setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
fd = accept(fd, NULL, NULL);
write(fd, buffer, 15);
read(fd, buffer, 15);
}
Reported-by: Liu Chao
Fixes: b286d8b1a690 ("crypto: skcipher - Add skcipher walk interface")
Cc: # v4.10+
Signed-off-by: Eric Biggers
---
crypto/skcipher.c | 53 ---
1 file changed, 27 insertions(+), 26 deletions(-)
diff --git a/crypto/skcipher.c b/crypto/skcipher.c
index 7d6a49fe3047..5f7017b36d75 100644
--- a/crypto/skcipher.c
+++ b/crypto/skcipher.c
@@ -95,7 +95,7 @@ static inline u8 *skcipher_get_spot(u8 *start, unsigned int
len)
return max(start, end_page);
}
-static int skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
+static void skcipher_done_slow(struct skcipher_walk *walk, unsigned int bsize)
{
u8 *addr;
@@ -103,23 +103,24 @@ static int skcipher_done_slow(struct skcipher_walk *walk,
unsigned int bsize)
addr = skcipher_get_spot(addr, bsize);
scatterwalk_copychunks(addr, >out, bsize,
(walk->flags & SKCIPHER_WALK_PHYS) ? 2 : 1);
- return 0;
}
int skcipher_walk_done(struct skcipher_walk *walk, int err)
{
- unsigned int n = walk->nbytes - err;
- unsigned int nbytes;
-
- nbytes = walk->total - n;
-
- if (unlikely(err < 0)) {
- nbytes = 0;
- n = 0;
- } else if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
- SKCIPHER_WALK_SLOW |
- SKCIPHER_WALK_COPY |
- SKCIPHER_WALK_DIFF {
+ unsigned int n; /* bytes processed */
+ bool more;
+
+ if (unlikely(err < 0))
+ goto finish;
+
+ n = walk->nbytes - err;
+ walk->total -= n;
+ more = (walk->total != 0);
+
+ if (likely(!(walk->flags & (SKCIPHER_WALK_PHYS |
+ SKCIPHER_WALK_SLOW |
+ SKCIPHER_WALK_COPY |
+ SKCIPHER_WALK_DIFF {
unmap_src:
skcipher_unmap_src(walk);
} else if (walk->flags & SKCIPHER_WALK_DIFF) {
@@ -131,28 +132,28 @@ int skcipher_walk_done(struct skcipher_walk *walk, int
err)
skcipher_unmap_dst(walk);
} else if (unlikely(walk->flags & SKCIPHER_WALK_SLOW)) {
if (WARN_ON(err)) {
+ /* unexpected case; didn't process all bytes */
err = -EINVAL;
- nbytes = 0;
- } else
- n = skcipher_done_slow(walk, n);
+ goto finish;
+ }
+ skcipher_done_slow(walk, n);
+ goto already_advanced;
}
- if (err > 0)
- err = 0;
-
- walk->total = nbytes;
- walk->nbytes = nbytes;
-
scatterwalk_advance(>in, n);
scatterwalk_advance(>out, n);
- scatterwalk_done(>in, 0, nbytes);
- scatterwalk_done(>out, 1, nbytes);
+already_advanced:
+ scatterwalk_done(>in, 0, more);
+ scatterwalk_done(>out, 1, more);
- if (nbytes) {
+ if (more) {
crypto_yield(walk->flags & SKCIPHER_WALK_SLEEP ?
CRYPTO_TFM_REQ_MAY_SLEEP : 0);
return skcipher_walk_next(walk);
}
+ err = 0;
+finish:
+ walk->nbytes = 0;
/* Short-circuit for the common/fast path. */
if (!((unsigned long)walk->buffer | (unsigned long)walk->page))
--
