Re: [PATCH 2/2] crypto: DRBG - use caller buffer if suitable
> Am 20.07.2018 um 05:54 schrieb Herbert Xu : > >> On Thu, Jul 19, 2018 at 10:57:16PM +0200, Stephan Müller wrote: >> >> Therefore, I am not sure that either having an SGL interface for the RNG API >> or a virtual address interface for the sync skcipher would be helpful. > > Could you please explain again why a virtual address interface to > sync skcipher algorithms won't be useful? It's something that others > have asked for in the past too. Maybe I have a different understanding of how such interface should look like. Can you give me some more detail on how you envision such virtual address interface should work? Thanks a lot Stephan
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 2/2] crypto: DRBG - use caller buffer if suitable
On Thu, Jul 19, 2018 at 10:57:16PM +0200, Stephan Müller wrote: > > Therefore, I am not sure that either having an SGL interface for the RNG API > or a virtual address interface for the sync skcipher would be helpful. Could you please explain again why a virtual address interface to sync skcipher algorithms won't be useful? It's something that others have asked for in the past too. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 2/2] crypto: DRBG - use caller buffer if suitable
Am Donnerstag, 19. Juli 2018, 11:34:33 CEST schrieb Herbert Xu: Hi Herbert, > I think this is an abuse of virt_addr_valid. It's meant to catch > bogus uses of SG lists, it's not meant to be a guarantee that an > address can be used on an SG list. Thanks for your insights. > > A better solution would be either an SG-list interface for rng, > or alternatively a virtual address interface for sync skcipher. My goal is to implement an in-place cipher operation drbg_kcapi_sym_ctr operating directly on the output buffer if possible. Without knowing whether the output buffer is valid for SGLs, I would need to use the scratchpad buffer as input/output buffer. That means that the data must be memcpy'ed from the scratchpad buffer to the output buffer. To provide the fastest DRBG implementation, we should eliminate the memcpy in drbg_kcapi_sym_ctr and with it the restriction of generating DRBG_OUTSCRATCHLEN with one cipher invocation. I.e. the ultimate goal is to operate directly on the output buffer if at all possible. So far, all RNGs generate data for one buffer only. Thus, I would consider converting the RNG interface to use SGLs as overkill. Furthermore, if the RNG uses the SHASH ciphers (like the DRBG use case), the input data is expected to be a straight buffer. Also, even the caller of the RNG may not know whether the destination buffer is valid for an SGL in case it is an intermediate layer. Therefore, I am not sure that either having an SGL interface for the RNG API or a virtual address interface for the sync skcipher would be helpful. Is there no way of identifying whether a buffer is valid for SGL operation? PS: I experimented with the in-place cipher operation in drbg_kcapi_sym_ctr while operating directly on the output buffer. The result was a more than 3- times increase in performance. Current implementation: 16 bytes| 12.267661 MB/s|61338304 bytes | 500213 ns 32 bytes| 23.603770 MB/s| 118018848 bytes | 500073 ns 64 bytes| 46.732262 MB/s| 233661312 bytes | 500241 ns 128 bytes| 90.038042 MB/s| 450190208 bytes | 500244 ns 256 bytes| 160.399616 MB/s| 801998080 bytes | 500393 ns 512 bytes| 259.878400 MB/s| 1299392000 bytes | 501675 ns 1024 bytes| 386.050662 MB/s| 1930253312 bytes | 501661 ns 2048 bytes| 493.641728 MB/s| 2468208640 bytes | 501598 ns 4096 bytes| 581.835981 MB/s| 2909179904 bytes | 503426 ns In-place cipher operation directly on output buffer: 16 bytes| 16.017830 MB/s|80089152 bytes | 500752 ns 32 bytes| 30.775686 MB/s| 153878432 bytes | 500701 ns 64 bytes| 58.299430 MB/s| 291497152 bytes | 500466 ns 128 bytes| 114.847462 MB/s| 574237312 bytes | 500385 ns 256 bytes| 218.359859 MB/s| 1091799296 bytes | 500476 ns 512 bytes| 416.003379 MB/s| 2080016896 bytes | 501105 ns 1024 bytes| 714.792346 MB/s| 3573961728 bytes | 5000718637 ns 2048 bytes|1.143480 GB/s| 5717401600 bytes | 500094 ns 4096 bytes|1.609953 GB/s| 8049766400 bytes | 501687 ns Ciao Stephan
Re: [PATCH 2/2] crypto: DRBG - use caller buffer if suitable
On Tue, Jul 10, 2018 at 05:57:00PM +0200, Stephan Müller wrote: > The SGL can directly operate caller-provided memory with the exception > of stack memory. The DRBG detects whether the caller provided > non-suitable memory and uses the scratchpad only on those circumstances. > > This patch increases the speed of the CTR DRBG by 1 to 3 percent > depending on the buffer size of the output buffer. > > Signed-off-by: Stephan Mueller I think this is an abuse of virt_addr_valid. It's meant to catch bogus uses of SG lists, it's not meant to be a guarantee that an address can be used on an SG list. A better solution would be either an SG-list interface for rng, or alternatively a virtual address interface for sync skcipher. Cheers, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: dh - fix calculating encoded key size
On Wed, Jul 11, 2018 at 09:27:56AM -0700, Eric Biggers wrote: > > The callers do check for errors, but at the point of the proposed BUG_ON() a > buffer overflow may have already occurred, so I think a BUG_ON() would be more > appropriate than a WARN_ON(). Of course, it would be better to prevent any > buffer overflow from occurring in the first place, but that's already the > purpose of the 'len != crypto_dh_key_len(params)' check; the issue was that > 'crypto_dh_key_len()' calculated the wrong length. How about adding an end argument to dh_pack_data so that it can check that the buffer isn't overflown each time it's called? Then if you cared you could also have one final check at the end of the function to ensure the buffer is fully used. As we can easily avoid having a BUG_ON here I think we should since a BUG_ON would leave the machine unresponsive which should be a last resort. Thanks, -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
