Re: [sparc64] cryptomgr_test OOPS kernel 4.9.0+

2016-12-26 Thread David Miller 
From: Anatoly Pugachev 
Date: Sun, 25 Dec 2016 20:56:08 +0300

> Disabling kernel config option
> CRYPTO_MANAGER_DISABLE_TESTS
> i.e. enable run-time self tests, makes kernel unbootable:
> 
> tested with git kernels v4.9-8648-g5cc60aeedf31 and v4.9-12259-g7c0f6ba682b9

I think the testing code for the new synchronous compression module is
putting kernel image pointers into scatterlists, which in turn we
attempt to transform to and from page structs.

That doesn't work.

It's coming from the test input buffers:

static int test_acomp(struct crypto_acomp *tfm, struct comp_testvec *ctemplate,
  struct comp_testvec *dtemplate, int ctcount, int dtcount)
{
 ...
sg_init_one(, ctemplate[i].input, ilen);

These have to be copied into kmalloc() buffers or similar, just like
the skchiper tests do.

The crash on sparc64 shows that we try to dereference a page struct at
a bogus vmemmap address for a page that doesn't exist.

I hacked up the following and this makes the crashes go away:

diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index f616ad7..117bb33 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1449,22 +1449,31 @@ static int test_acomp(struct crypto_acomp *tfm, struct 
comp_testvec *ctemplate,
const char *algo = crypto_tfm_alg_driver_name(crypto_acomp_tfm(tfm));
unsigned int i;
char *output;
+   char *input;
int ret;
struct scatterlist src, dst;
struct acomp_req *req;
struct tcrypt_result result;
 
+   pr_info("test_acomp: COMP_BUF_SIZE %d\n", (int) COMP_BUF_SIZE);
+
output = kmalloc(COMP_BUF_SIZE, GFP_KERNEL);
if (!output)
return -ENOMEM;
+   input = kmalloc(COMP_BUF_SIZE, GFP_KERNEL);
+   if (!input) {
+   kfree(output);
+   return -ENOMEM;
+   }
 
for (i = 0; i < ctcount; i++) {
unsigned int dlen = COMP_BUF_SIZE;
int ilen = ctemplate[i].inlen;
 
memset(output, 0, dlen);
+   memcpy(input, ctemplate[i].input, ilen);
init_completion();
-   sg_init_one(, ctemplate[i].input, ilen);
+   sg_init_one(, input, ilen);
sg_init_one(, output, dlen);
 
req = acomp_request_alloc(tfm);
@@ -1512,8 +1521,9 @@ static int test_acomp(struct crypto_acomp *tfm, struct 
comp_testvec *ctemplate,
int ilen = dtemplate[i].inlen;
 
memset(output, 0, dlen);
+   memcpy(input, dtemplate[i].input, ilen);
init_completion();
-   sg_init_one(, dtemplate[i].input, ilen);
+   sg_init_one(, input, ilen);
sg_init_one(, output, dlen);
 
req = acomp_request_alloc(tfm);
@@ -1559,6 +1569,7 @@ static int test_acomp(struct crypto_acomp *tfm, struct 
comp_testvec *ctemplate,
ret = 0;
 
 out:
+   kfree(input);
kfree(output);
return ret;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4.10 3/6] bpf: Use SHA256 instead of SHA1 for bpf digests

2016-12-26 Thread Alexei Starovoitov 
On Sat, Dec 24, 2016 at 08:59:53PM +0100, Daniel Borkmann wrote:
> On 12/24/2016 03:22 AM, Andy Lutomirski wrote:
> >BPF digests are intended to be used to avoid reloading programs that
> >are already loaded.  For use cases (CRIU?) where untrusted programs
> >are involved, intentional hash collisions could cause the wrong BPF
> >program to execute.  Additionally, if BPF digests are ever used
> >in-kernel to skip verification, a hash collision could give privilege
> >escalation directly.
> 
> Just for the record, digests will never ever be used to skip the
> verification step, so I don't know why this idea even comes up
> here (?) or is part of the changelog? As this will never be done
> anyway, rather drop that part so we can avoid confusion on this?

+1 to what Daniel said above.

For the others let me explain what this patch set is actually
trying to accomplish.
Andy had an idea that sha256 of the program can somehow be used
to bypass kernel verifier during program loading. Furthemore
he thinks that such 'bypass' would be useful for criu of bpf programs,
hence see vigorously attacking existing prog_digest (sha1) because
it's not as secure as sha256 and hence cannot be used for such 'bypass'.

The problem with criu of bpf programs is same as criu of kernel modules.
For the main tracing and networking use cases, we cannot stop the kernel,
so criu is out of question already.
Even if we could stop all the events that trigger bpf program execution,
the sha256 or memcmp() of the full program is not enough to guarantee
that two programs are the same.
Ex. bpf_map_lookup() may be safe for one program and not for another
depending on how map was created. Two programs of different types
are not comparable either. Etc, etc.
Therefore the idea of using sha256 for such purpose is bogus,
and I have an obvious NACK for bpf related patches 3,4,5,6.

For the questions raised in other threads:
I'm not ok with making BPF depend on CRYPTO, since it's the same as
requiring CRYPTO to select BPF for no good reason.

And 0/6 commit log:
> Since there are plenty of uses for the new-in-4.10 BPF digest feature
> that would be problematic if malicious users could produce collisions,
> the BPF digest should be collision-resistant. 

This statement is also bogus. The only reason we added prog_digest is
to improve debuggability and introspection of bpf programs.
As I said in the previous thread "collisions are ok" and we could have
used jhash here to avoid patches like this ever appearing
and wasting everyones time.

sha1 is 20 bytes which is already a bit long to print and copy paste by humans.
whereas 4 byte jhash is a bit too short, since collisions are not that rare
and may lead to incorrect assumptions from the users that develop the programs.
I would prefer something in 6-10 byte range that prevents collisions most of
the time and short to print as hex, but I don't know of anything like this
in the existing kernel and inventing bpf specific hash is not great.
Another requirement for debugging (and prog_digest) that user space
should be able to produce the same hash without asking kernel, so
sha1 fits that as well, since it's well known and easy to put into library.

sha256 doesn't fit either of these requirements. 32-bytes are too long to print
and when we use it as a substitue for the prog name for jited ksym, looking
at long function names will screw up all tools like perf, which we don't
want. sha256 is equally not easy for user space app like iproute2,
so not an acceptable choice from that pov either.

--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4.10 3/6] bpf: Use SHA256 instead of SHA1 for bpf digests

2016-12-26 Thread Andy Lutomirski 
On Mon, Dec 26, 2016 at 5:36 PM, Alexei Starovoitov
 wrote:
> On Sat, Dec 24, 2016 at 08:59:53PM +0100, Daniel Borkmann wrote:
>> On 12/24/2016 03:22 AM, Andy Lutomirski wrote:
>> >BPF digests are intended to be used to avoid reloading programs that
>> >are already loaded.  For use cases (CRIU?) where untrusted programs
>> >are involved, intentional hash collisions could cause the wrong BPF
>> >program to execute.  Additionally, if BPF digests are ever used
>> >in-kernel to skip verification, a hash collision could give privilege
>> >escalation directly.
>>
>> Just for the record, digests will never ever be used to skip the
>> verification step, so I don't know why this idea even comes up
>> here (?) or is part of the changelog? As this will never be done
>> anyway, rather drop that part so we can avoid confusion on this?
>
> +1 to what Daniel said above.
>
> For the others let me explain what this patch set is actually
> trying to accomplish.

The patch:

a) cleans up the code and

b) uses a cryptographic hash that is actually believed to satisfy the
definition of a cryptographic hash.

There's no excuse for not doing b.

> and I have an obvious NACK for bpf related patches 3,4,5,6.

Did you *read* the ones that were pure cleanups?

>
> sha1 is 20 bytes which is already a bit long to print and copy paste by 
> humans.
> whereas 4 byte jhash is a bit too short, since collisions are not that rare
> and may lead to incorrect assumptions from the users that develop the 
> programs.
> I would prefer something in 6-10 byte range that prevents collisions most of
> the time and short to print as hex, but I don't know of anything like this
> in the existing kernel and inventing bpf specific hash is not great.
> Another requirement for debugging (and prog_digest) that user space
> should be able to produce the same hash without asking kernel, so
> sha1 fits that as well, since it's well known and easy to put into library.

Then truncate them in user space.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] virtio-crypto: support crypto engine framework

2016-12-26 Thread Gonglei 
crypto engine was introduced since 'commit 735d37b5424b ("crypto: engine
- Introduce the block request crypto engine framework")' which uses work
queue to realize the asynchronous processing for ablk_cipher and ahash.

For virtio-crypto device, I register an engine for each
data virtqueue so that we can use the capability of
multiple data queues in future.

Cc: Baolin Wang 
Cc: Herbert Xu 
Cc: Michael S. Tsirkin 
Signed-off-by: Gonglei 
---
 drivers/crypto/virtio/Kconfig|  1 +
 drivers/crypto/virtio/virtio_crypto_algs.c   | 52 ---
 drivers/crypto/virtio/virtio_crypto_common.h | 16 ++
 drivers/crypto/virtio/virtio_crypto_core.c   | 74 ++--
 4 files changed, 121 insertions(+), 22 deletions(-)

diff --git a/drivers/crypto/virtio/Kconfig b/drivers/crypto/virtio/Kconfig
index d80f733..5db0749 100644
--- a/drivers/crypto/virtio/Kconfig
+++ b/drivers/crypto/virtio/Kconfig
@@ -4,6 +4,7 @@ config CRYPTO_DEV_VIRTIO
select CRYPTO_AEAD
select CRYPTO_AUTHENC
select CRYPTO_BLKCIPHER
+   select CRYPTO_ENGINE
default m
help
  This driver provides support for virtio crypto device. If you
diff --git a/drivers/crypto/virtio/virtio_crypto_algs.c 
b/drivers/crypto/virtio/virtio_crypto_algs.c
index c2374df..970d0ca 100644
--- a/drivers/crypto/virtio/virtio_crypto_algs.c
+++ b/drivers/crypto/virtio/virtio_crypto_algs.c
@@ -288,8 +288,7 @@ static int virtio_crypto_ablkcipher_setkey(struct 
crypto_ablkcipher *tfm,
 static int
 __virtio_crypto_ablkcipher_do_req(struct virtio_crypto_request *vc_req,
struct ablkcipher_request *req,
-   struct data_queue *data_vq,
-   __u8 op)
+   struct data_queue *data_vq)
 {
struct crypto_ablkcipher *tfm = crypto_ablkcipher_reqtfm(req);
unsigned int ivsize = crypto_ablkcipher_ivsize(tfm);
@@ -329,7 +328,7 @@ static int virtio_crypto_ablkcipher_setkey(struct 
crypto_ablkcipher *tfm,
vc_req->req_data = req_data;
vc_req->type = VIRTIO_CRYPTO_SYM_OP_CIPHER;
/* Head of operation */
-   if (op) {
+   if (vc_req->encrypt) {
req_data->header.session_id =
cpu_to_le64(ctx->enc_sess_info.session_id);
req_data->header.opcode =
@@ -424,19 +423,15 @@ static int virtio_crypto_ablkcipher_encrypt(struct 
ablkcipher_request *req)
struct virtio_crypto_ablkcipher_ctx *ctx = crypto_ablkcipher_ctx(atfm);
struct virtio_crypto_request *vc_req = ablkcipher_request_ctx(req);
struct virtio_crypto *vcrypto = ctx->vcrypto;
-   int ret;
/* Use the first data virtqueue as default */
struct data_queue *data_vq = >data_vq[0];
 
vc_req->ablkcipher_ctx = ctx;
vc_req->ablkcipher_req = req;
-   ret = __virtio_crypto_ablkcipher_do_req(vc_req, req, data_vq, 1);
-   if (ret < 0) {
-   pr_err("virtio_crypto: Encryption failed!\n");
-   return ret;
-   }
+   vc_req->encrypt = true;
+   vc_req->dataq = data_vq;
 
-   return -EINPROGRESS;
+   return crypto_transfer_cipher_request_to_engine(data_vq->engine, req);
 }
 
 static int virtio_crypto_ablkcipher_decrypt(struct ablkcipher_request *req)
@@ -445,20 +440,16 @@ static int virtio_crypto_ablkcipher_decrypt(struct 
ablkcipher_request *req)
struct virtio_crypto_ablkcipher_ctx *ctx = crypto_ablkcipher_ctx(atfm);
struct virtio_crypto_request *vc_req = ablkcipher_request_ctx(req);
struct virtio_crypto *vcrypto = ctx->vcrypto;
-   int ret;
/* Use the first data virtqueue as default */
struct data_queue *data_vq = >data_vq[0];
 
vc_req->ablkcipher_ctx = ctx;
vc_req->ablkcipher_req = req;
 
-   ret = __virtio_crypto_ablkcipher_do_req(vc_req, req, data_vq, 0);
-   if (ret < 0) {
-   pr_err("virtio_crypto: Decryption failed!\n");
-   return ret;
-   }
+   vc_req->encrypt = false;
+   vc_req->dataq = data_vq;
 
-   return -EINPROGRESS;
+   return crypto_transfer_cipher_request_to_engine(data_vq->engine, req);
 }
 
 static int virtio_crypto_ablkcipher_init(struct crypto_tfm *tfm)
@@ -484,6 +475,33 @@ static void virtio_crypto_ablkcipher_exit(struct 
crypto_tfm *tfm)
ctx->vcrypto = NULL;
 }
 
+int virtio_crypto_ablkcipher_crypt_req(
+   struct crypto_engine *engine,
+   struct ablkcipher_request *req)
+{
+   struct virtio_crypto_request *vc_req = ablkcipher_request_ctx(req);
+   struct data_queue *data_vq = vc_req->dataq;
+   int ret;
+
+   ret = __virtio_crypto_ablkcipher_do_req(vc_req, req, data_vq);
+   if (ret < 0)
+   return ret;
+
+   virtqueue_kick(data_vq->vq);
+
+   return 0;
+}
+
+void virtio_crypto_ablkcipher_finalize_req(
+   struct virtio_crypto_request

Re: [RFC PATCH 4.10 0/6] Switch BPF's digest to SHA256

2016-12-26 Thread Herbert Xu 
Andy Lutomirski  wrote:
> Since there are plenty of uses for the new-in-4.10 BPF digest feature
> that would be problematic if malicious users could produce collisions,
> the BPF digest should be collision-resistant.  SHA-1 is no longer
> considered collision-resistant, so switch it to SHA-256.
> 
> The actual switchover is trivial.  Most of this series consists of
> cleanups to the SHA256 code to make it usable as a standalone library
> (since BPF should not depend on crypto).
> 
> The cleaned up library is much more user-friendly than the SHA-1 code,
> so this also significantly tidies up the BPF digest code.
> 
> This is intended for 4.10.  If this series misses 4.10 and nothing
> takes its place, then we'll have an unpleasant ABI stability
> situation.

Can you please explain why BPF needs to be able to use SHA directly
rather than through the crypto API?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash

2016-12-26 Thread Andy Lutomirski 
On Mon, Dec 26, 2016 at 9:51 AM, Ard Biesheuvel
 wrote:
> On 26 December 2016 at 07:57, Herbert Xu  wrote:
>> On Sat, Dec 24, 2016 at 09:57:53AM -0800, Andy Lutomirski wrote:
>>>
>>> I actually do use incremental hashing later on.   BPF currently
>>> vmallocs() a big temporary buffer just so it can fill it and hash it.
>>> I change it to hash as it goes.
>>
>> How much data is this supposed to hash on average? If it's a large
>> amount then perhaps using the existing crypto API would be a better
>> option than adding this.
>>
>
> This is a good point actually: you didn't explain *why* BPF shouldn't
> depend on the crypto API.

According to Daniel, the networking folks want to let embedded systems
include BPF without requiring the crypto core.

At some point, I'd also like to use modern hash functions for module
verification.  If doing so would require the crypto core to be
available when modules are loaded, then the crypto core couldn't be
modular.  (Although it occurs to me that my patches get that wrong --
if this change happens, I need to split the code so that the library
functions can be built in even if CRYPTO=m.)

Daniel, would you be okay with BPF selecting CRYPTO and CRYPTO_HASH?

Also, as a bikeshed thought: I could call the functions
sha256_init_direct(), etc.  Then there wouldn't be namespace
collisions and the fact that they bypass accelerated drivers would be
more obvious.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash

2016-12-26 Thread Ard Biesheuvel 
On 26 December 2016 at 07:57, Herbert Xu  wrote:
> On Sat, Dec 24, 2016 at 09:57:53AM -0800, Andy Lutomirski wrote:
>>
>> I actually do use incremental hashing later on.   BPF currently
>> vmallocs() a big temporary buffer just so it can fill it and hash it.
>> I change it to hash as it goes.
>
> How much data is this supposed to hash on average? If it's a large
> amount then perhaps using the existing crypto API would be a better
> option than adding this.
>

This is a good point actually: you didn't explain *why* BPF shouldn't
depend on the crypto API.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [sparc64] cryptomgr_test OOPS kernel 4.9.0+

2016-12-26 Thread David Miller 
From: Anatoly Pugachev 
Date: Sun, 25 Dec 2016 20:56:08 +0300

> Disabling kernel config option
> CRYPTO_MANAGER_DISABLE_TESTS
> i.e. enable run-time self tests, makes kernel unbootable:
> 
> tested with git kernels v4.9-8648-g5cc60aeedf31 and v4.9-12259-g7c0f6ba682b9

I'm getting this with the current GIT tree too, will try to see
what's going wrong.
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html