[PATCH] crypto: AF_ALG - remove SGL end indicator when chaining

[Stephan Müller]

The SGL is MAX_SGL_ENTS + 1 in size. The last SG entry is used for the
chaining and is properly updated with the sg_chain invocation. During
the filling-in of the initial SG entries, sg_mark_end is called for each
SG entry. This is appropriate as long as no additional SGL is chained
with the current SGL. However, when a new SGL is chained and the last
SG entry is updated with sg_chain, the last but one entry still contains
the end marker from the sg_mark_end. This end marker must be removed as
otherwise a walk of the chained SGLs will cause a NULL pointer
dereference at the last but one SG entry, because sg_next will return
NULL.

Fixes: 8ff590903d5fc ("crypto: algif_skcipher - User-space interface
for skcipher operations")
CC: 
CC: Herbert Xu 
Signed-off-by: Stephan Mueller 
---
 crypto/algif_skcipher.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/algif_skcipher.c b/crypto/algif_skcipher.c
index 43839b00fe6c..62449a8f14ce 100644
--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -139,8 +139,10 @@ static int skcipher_alloc_sgl(struct sock *sk)
sg_init_table(sgl->sg, MAX_SGL_ENTS + 1);
sgl->cur = 0;
 
-   if (sg)
+   if (sg) {
sg_chain(sg, MAX_SGL_ENTS + 1, sgl->sg);
+   sg_unmark_end(sg + (MAX_SGL_ENTS - 1));
+   }
 
list_add_tail(>list, >tsgl);
}
-- 
2.13.5

Re: [PATCH] crypto: MPI - kunmap after finishing accessing buffer

[Stephan Mueller]

Am Dienstag, 22. August 2017, 08:33:15 CEST schrieb Herbert Xu:

Hi Herbert,

> On Thu, Aug 10, 2017 at 08:06:18AM +0200, Stephan Müller wrote:
> > Hi Herbert,
> > 
> > I found that issue while playing around with edge conditions in my
> > algif_akcipher implementation. This issue only manifests in a
> > segmentation violation on 32 bit machines and with an SGL where each
> > SG points to one byte. SGLs with larger buffers seem to be not
> > affected by this issue.
> > 
> > Yet this access-after-unmap should be a candidate for stable, IMHO.
> 
> Good catch.  Thanks!
> 
> Fixes: 4816c9406430 ("lib/mpi: Fix SG miter leak")
> Cc: 

Just to confirm: stable is not CCed in this email -- did you send it 
separately to stable? (dto for "[PATCH v4] crypto: only call put_page on 
referenced and used pages")

Thanks a lot
Stephan

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Tudor,

>> you still need to get the public key out of the kernel if you want to use it 
>> from user space. Or feed the remote public key if you plan to use some sort 
>> of key derivation function.
> 
> The crypto hardware that I'm working on, generates the private key
> internally within the device and never reveals it to software and
> immediately returns the public key pair. The user can retrieve the
> public key from hardware.

and don’t we want some sort of access control here. Only the user / process 
that requested the private key and has access to the public key is allowed to 
keep using the private key?

>> I am saying this again, if you only have a hammer, everything looks like a 
>> nail. What about actually looking at how this would be used from user space 
>> in real crypto cases.
>> My point is that the usages here are key generation, some sort of 
>> key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
>> Frankly the focus with asymmetric ciphers are the keys and the key 
>> derivation. They are not encryption and decryption of massive amounts of 
>> data.
> 
> The hardware uses it's own private key and the public key received from
> the other end and computes the ecdh shared secret. The hardware computed
> shared secret can then be used for key derivation.

And that is normally the case. Get your local public key, send it to the other 
side, get the other sides public key, give it to the hardware and get shared 
secret.

So how is AF_ALG a good fit here?

Regards

Marcel

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Tudor Ambarus]

Hi, Marcel,

On 08/30/2017 10:21 AM, Marcel Holtmann wrote:

you still need to get the public key out of the kernel if you want to use it 
from user space. Or feed the remote public key if you plan to use some sort of 
key derivation function.



The crypto hardware that I'm working on, generates the private key
internally within the device and never reveals it to software and
immediately returns the public key pair. The user can retrieve the
public key from hardware.


I am saying this again, if you only have a hammer, everything looks like a 
nail. What about actually looking at how this would be used from user space in 
real crypto cases.

My point is that the usages here are key generation, some sort of 
key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
Frankly the focus with asymmetric ciphers are the keys and the key derivation. 
They are not encryption and decryption of massive amounts of data.


The hardware uses it's own private key and the public key received from
the other end and computes the ecdh shared secret. The hardware computed
shared secret can then be used for key derivation.

Cheers,
ta

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Marcel Holtmann]

Hi Tudor,

> akcipher can work with its own internal keys, now that we have crypto
> accelerators that can generate keys that never leave the hardware. Going
> through the kernel's key subsystem seems superfluous in this case.
> 
> I also understand the need of going through the kernel's key subsystem
> when the user wants to refer to a key which exists elsewhere, such as in
> TPM or within an SGX software enclave, but this seems orthogonal with
> crypto accelerators with key generation and retention support.
> 
> How should we interface akcipher/kpp with user-space?

you still need to get the public key out of the kernel if you want to use it 
from user space. Or feed the remote public key if you plan to use some sort of 
key derivation function.

I am saying this again, if you only have a hammer, everything looks like a 
nail. What about actually looking at how this would be used from user space in 
real crypto cases.

My point is that the usages here are key generation, some sort of 
key-exchange-agreement (aka DH) and key derivation into a symmetric key. 
Frankly the focus with asymmetric ciphers are the keys and the key derivation. 
They are not encryption and decryption of massive amounts of data.

Regards

Marcel

[PATCH] crypto: AF_ALG - update correct dst SGL entry

[Stephan Müller]

When two adjacent TX SGL are processed and parts of both TX SGLs
are pulled into the per-request TX SGL, the wrong per-request
TX SGL entries were updated.

This fixes a NULL pointer dereference when a cipher implementation walks
the TX SGL where some of the SGL entries were NULL.

Signed-off-by: Stephan Mueller 
---
 crypto/af_alg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crypto/af_alg.c b/crypto/af_alg.c
index ffa9f4ccd9b4..337cf382718e 100644
--- a/crypto/af_alg.c
+++ b/crypto/af_alg.c
@@ -619,14 +619,14 @@ void af_alg_pull_tsgl(struct sock *sk, size_t used, 
struct scatterlist *dst,
struct af_alg_ctx *ctx = ask->private;
struct af_alg_tsgl *sgl;
struct scatterlist *sg;
-   unsigned int i, j;
+   unsigned int i, j = 0;
 
while (!list_empty(>tsgl_list)) {
sgl = list_first_entry(>tsgl_list, struct af_alg_tsgl,
   list);
sg = sgl->sg;
 
-   for (i = 0, j = 0; i < sgl->cur; i++) {
+   for (i = 0; i < sgl->cur; i++) {
size_t plen = min_t(size_t, used, sg[i].length);
struct page *page = sg_page(sg + i);
 
-- 
2.13.5

Re: [PATCH v8 0/4] crypto: add algif_akcipher user space API

[Tudor Ambarus]

Hi, Herbert, all,

akcipher can work with its own internal keys, now that we have crypto
accelerators that can generate keys that never leave the hardware. Going
through the kernel's key subsystem seems superfluous in this case.

I also understand the need of going through the kernel's key subsystem
when the user wants to refer to a key which exists elsewhere, such as in
TPM or within an SGX software enclave, but this seems orthogonal with
crypto accelerators with key generation and retention support.

How should we interface akcipher/kpp with user-space?

Thanks,
ta

On 08/17/2017 04:17 PM, Tudor Ambarus wrote:

Hi, all,

On 08/11/2017 07:05 PM, Marcel Holtmann wrote:

Hi Stephan,

AF_ALG is best suited for crypto use cases where a socket is set up 
once

and there are lots of reads and writes to justify the setup cost. With
asymmetric crypto, the setup cost is high when you might only use the
socket for a brief time to do one verify or encrypt operation.


To me, the entire AF_ALG purpose is solely to export hardware support 
to user
space. That said, if user space wants an accelerator, a socket would 
be opened

once followed by numerous read/write requests.

Besides, I am aware of Tadeusz' patch to link algif_akcipher to the 
keyring
and I planned to port it to the current implementation. But I thought 
I offer

a small patch focusing on the externalization of the akcipher API first.

I think the keyctl and AF_ALG are no opponents, but rather are 
orthogonal to
each other. The statement I made for the KPP AF_ALG RFC applies here 
too:


"""
I am aware and in fact supported development of the DH support in the 
keys
subsystem. The question will be raised whether the AF_ALG KPP 
interface is
superfluous in light of the keys DH support. My answer is that AF_ALG 
KPP is

orthogonal to the keys DH support. The keys DH support use case is that
the keys are managed inside the kernel and thus has the focus on the
key management. Contrary, AF_ALG KPP does not really focus on key 
management
but simply externalizes the DH/ECDH support found in the kernel 
including
hardware acceleration. User space is in full control of the key life 
cycle.

This way, AF_ALG could be used to complement user-space network protocol
implementations like TLS or IKE to offload the resource intense DH
calculations to accelerators.
“""


we do not need two interfaces for doing the same thing. Especially not 
one that can not handle hardware backed keys. And more important if 
you can not abstract an accelerator that doesn’t expose the private 
key at all.


I'm working with a crypto accelerator (find it at [1]) that is capable
of generating random ecc private keys internally within the device that
are never revealed outside of it. The keys can be further used for ECDH
and ECDSA.

The simplest way to access my device from user-space is to go through
af_alg. We can permit the users to provide NULL keys, and if so, we can
generate the keys inside the kernel/hardware. If hardware supports
key generation and retention, it will use it, else the keys
will be generated inside the kernel. Either way it's a win, we don't
reveal the private keys to user-space. Going through the keyring
subsystem seems superfluous in this case.

My use case compared with the one of using keyring subsystem to access
keys from TPMs or Intel's sgx enclave seem orthogonal. What do you
think?

Cheers,
ta

[1] http://www.microchip.com/wwwproducts/en/ATECC508A
The driver can be found in drivers/crypto/atmel-ecc* in Herbert's tree.