Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Mon, 2017-09-25 at 12:56 +0800, Herbert Xu wrote:
> On Sun, Sep 24, 2017 at 07:42:46PM +0200, Johannes Berg wrote:
> > 
> > Unrelated to this, I'm not sure whose tree this should go through -
> > probably Herbert's (or DaveM's with his ACK? not sure if there's a
> > crypto tree?) or so?
> 
> Since you're just rearranging code invoking the crypto API, rather
> than touching actual crypto API code, I think you should handle it
> as you do with any other wireless patch.

The code moves to crypto/ though, and I'm not even sure I can vouch for
the Makefile choice there.

johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Herbert Xu]

On Sun, Sep 24, 2017 at 07:42:46PM +0200, Johannes Berg wrote:
>
> Unrelated to this, I'm not sure whose tree this should go through -
> probably Herbert's (or DaveM's with his ACK? not sure if there's a
> crypto tree?) or so?

Since you're just rearranging code invoking the crypto API, rather
than touching actual crypto API code, I think you should handle it
as you do with any other wireless patch.

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH 3.18 11/42] [PATCH - RESEND] crypto: AF_ALG - remove SGL terminator indicator when chaining

[Greg Kroah-Hartman]

3.18-stable review patch.  If anyone has any objections, please let me know.

--

From: Stephan Mueller 

Fixed differently upstream as commit 2d97591ef43d ("crypto: af_alg - 
consolidation of duplicate code")

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.

The patch only applies to all kernels up to and including 4.13. The
patch 2d97591ef43d0587be22ad1b0d758d6df4999a0b added to 4.14-rc1
introduced a complete new code base which addresses this bug in
a different way. Yet, that patch is too invasive for stable kernels
and was therefore not marked for stable.

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

--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -92,8 +92,10 @@ static int skcipher_alloc_sgl(struct soc
sg_init_table(sgl->sg, MAX_SGL_ENTS + 1);
sgl->cur = 0;
 
-   if (sg)
+   if (sg) {
scatterwalk_sg_chain(sg, MAX_SGL_ENTS + 1, sgl->sg);
+   sg_unmark_end(sg + (MAX_SGL_ENTS - 1));
+   }
 
list_add_tail(>list, >tsgl);
}

[PATCH 4.4 27/66] [PATCH - RESEND] crypto: AF_ALG - remove SGL terminator indicator when chaining

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: Stephan Mueller 

Fixed differently upstream as commit 2d97591ef43d ("crypto: af_alg - 
consolidation of duplicate code")

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.

The patch only applies to all kernels up to and including 4.13. The
patch 2d97591ef43d0587be22ad1b0d758d6df4999a0b added to 4.14-rc1
introduced a complete new code base which addresses this bug in
a different way. Yet, that patch is too invasive for stable kernels
and was therefore not marked for stable.

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

--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -143,8 +143,10 @@ static int skcipher_alloc_sgl(struct soc
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);
}

[PATCH 4.9 27/77] [PATCH - RESEND] crypto: AF_ALG - remove SGL terminator indicator when chaining

[Greg Kroah-Hartman]

4.9-stable review patch.  If anyone has any objections, please let me know.

--

From: Stephan Mueller 

Fixed differently upstream as commit 2d97591ef43d ("crypto: af_alg - 
consolidation of duplicate code")

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.

The patch only applies to all kernels up to and including 4.13. The
patch 2d97591ef43d0587be22ad1b0d758d6df4999a0b added to 4.14-rc1
introduced a complete new code base which addresses this bug in
a different way. Yet, that patch is too invasive for stable kernels
and was therefore not marked for stable.

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

--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -143,8 +143,10 @@ static int skcipher_alloc_sgl(struct soc
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);
}

[PATCH 4.13 034/109] [PATCH - RESEND] crypto: AF_ALG - remove SGL terminator indicator when chaining

[Greg Kroah-Hartman]

4.13-stable review patch.  If anyone has any objections, please let me know.

--

From: Stephan Mueller 

Fixed differently upstream as commit 2d97591ef43d ("crypto: af_alg - 
consolidation of duplicate code")

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.

The patch only applies to all kernels up to and including 4.13. The
patch 2d97591ef43d0587be22ad1b0d758d6df4999a0b added to 4.14-rc1
introduced a complete new code base which addresses this bug in
a different way. Yet, that patch is too invasive for stable kernels
and was therefore not marked for stable.

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

--- a/crypto/algif_skcipher.c
+++ b/crypto/algif_skcipher.c
@@ -144,8 +144,10 @@ static int skcipher_alloc_sgl(struct soc
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);
}

Re: [RFC PATCH 0/2] let the crypto subsystem generate the ecc privkey

[Marcel Holtmann]

Hi Tudor,

> That Bluetooth SMP knows about the private key is pointless, since the
> detection of debug key usage is actually via the public key portion.
> With this patch set, the Bluetooth SMP will stop keeping a copy of the
> ecdh private key, except when using debug keys. This way we let the
> crypto subsystem to generate and handle the ecdh private key,
> potentially benefiting of hardware ecc private key generation and
> retention.
> 
> Tested with selftest and with btmon and smp-tester on top of hci_vhci,
> with ecdh done in both software and hardware (through atmel-ecc driver).
> All tests passed.
> 
> Tudor Ambarus (2):
>  Bluetooth: move ecdh allocation outside of ecdh_helper
>  Bluetooth: let the crypto subsystem generate the ecc privkey
> 
> net/bluetooth/ecdh_helper.c | 138 ++--
> net/bluetooth/ecdh_helper.h |   8 ++-
> net/bluetooth/selftest.c|  29 +++---
> net/bluetooth/smp.c | 120 --
> 4 files changed, 159 insertions(+), 136 deletions(-)

I only saw the cover letter and the patches never made it to the mailing list.

Regards

Marcel

Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

2017-09-24 13:42 GMT-04:00 Johannes Berg :
> On Sun, 2017-09-24 at 13:21 -0400, Xiang Gao wrote:
>>
>> Do you mean to put more characters each line in the description
>>
> Huh, sorry, no - my bad. I was thinking of the code, not the
> description at all.

Oh yes, these indentation do looks ugly. Thank you for figuring this
out. The tab
width of my editor was set to 4. It should be 8... I will fix these
problems and resend
the patch soon, maybe after receiving a bit more feedback.

>
> For example here:
>
>> -int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
>> - u8 *data, size_t data_len, u8 *mic)
>> +int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
>> +u8 *data, size_t data_len, u8 *auth)
>>
>
> I think you should adjust the indentation to match - or did it just get
> mangled in my mail? It looks *further* indented now, when it should be
> less (to after the opening parenthesis). Similarly in various other
> places.
>
> And perhaps for long things like
>
>> +static inline struct crypto_aead *ieee80211_aes_key_setup_encrypt(
>> +   const u8 key[], size_t key_len,
>> size_t mic_len)
>
>> +struct crypto_aead *aead_key_setup_encrypt(const char *alg,
>> +   const u8 key[], size_t key_len, size_t authsize);
>
> it might be better to write
>
> static inline struct crypto_aead *
> ieee80211_aes_key_setup_encrypt(const u8 key[], ...)
>
> and
>
> struct crypto_aead *
> aead_key_setup_encrypt(const char *alg, ...)
>
>
> respectively, depending on how far you have to indent to break lines
> etc.
>
> Anyway, I'm nitpicking.
>
> Unrelated to this, I'm not sure whose tree this should go through -
> probably Herbert's (or DaveM's with his ACK? not sure if there's a
> crypto tree?) or so?

Yes, there is one at
https://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git/
I'm not sure which tree to go either. I'm also not sure about the
beginning of the patch title,
should it be "mac80211:" or "crypto:"?

Options are:
1. This whole patch goes to either mac80211 tree or crypto tree. I
don't know which is better.
2. Make the higher level api only for internal usage in mac80211, i.e.
move the aead_api.c and aead_api.h to net/mac80211, does not export
the symbol. And of course, this will go to the mac80211 tree. I
personally don't want this to be the final solution because I happen
to be writing a loadable kernel module that uses these higher level
api.
3. Maybe split this patch, one for changes in crypto, which will go to
crypto tree, and the other for mac80211 part, which goes to the
mac80211 tree?

>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Sun, 2017-09-24 at 13:21 -0400, Xiang Gao wrote:
> 
> Do you mean to put more characters each line in the description
> 
Huh, sorry, no - my bad. I was thinking of the code, not the
description at all.

For example here:

> -int ieee80211_aes_gcm_encrypt(struct crypto_aead *tfm, u8 *j_0, u8 *aad,
> - u8 *data, size_t data_len, u8 *mic)
> +int aead_encrypt(struct crypto_aead *tfm, u8 *b_0, u8 *aad, size_t aad_len,
> +u8 *data, size_t data_len, u8 *auth)
> 

I think you should adjust the indentation to match - or did it just get
mangled in my mail? It looks *further* indented now, when it should be
less (to after the opening parenthesis). Similarly in various other
places.

And perhaps for long things like

> +static inline struct crypto_aead *ieee80211_aes_key_setup_encrypt(
> +   const u8 key[], size_t key_len,
> size_t mic_len)

> +struct crypto_aead *aead_key_setup_encrypt(const char *alg,
> +   const u8 key[], size_t key_len, size_t authsize);

it might be better to write

static inline struct crypto_aead *
ieee80211_aes_key_setup_encrypt(const u8 key[], ...)

and

struct crypto_aead *
aead_key_setup_encrypt(const char *alg, ...)


respectively, depending on how far you have to indent to break lines
etc.

Anyway, I'm nitpicking.

Unrelated to this, I'm not sure whose tree this should go through -
probably Herbert's (or DaveM's with his ACK? not sure if there's a
crypto tree?) or so?

johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Xiang Gao]

2017-09-24 11:05 GMT-04:00 Johannes Berg :
> On Sun, 2017-09-24 at 01:40 -0400, Xiang Gao wrote:
>> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
>> copy of each other. This patch reduce code redundancy by moving
>> the code in these two files to crypto/aead_api.c to make it a
>> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
>> and all the functions are now implemented in their headers using
>> the newly added aead api.
>>
> No objection from me, though I'd ask you to respin with the indentation
> fixed up a bit.

Hi Johannes,

Thank you for you time for the suggestion. I'm not sure if I correctly
understand you point.
Do you mean to put more characters each line in the description? Something like:

> Currently, the aes_ccm.c and aes_gcm.c are almost line by line copy of
> each other. This patch reduce code redundancy by moving the code in these
> two files to crypto/aead_api.c to make it a higher level aead api. The
> file aes_ccm.c and aes_gcm.c are removed and all the functions there are
> now implemented in their headers using the newly added aead api.

instead of

> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
> copy of each other. This patch reduce code redundancy by moving
> the code in these two files to crypto/aead_api.c to make it a
> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
> and all the functions are now implemented in their headers using
> the newly added aead api.

Xiang Gao

>
> johannes

Re: [PATCH] mac80211: aead api to reduce redundancy

[Johannes Berg]

On Sun, 2017-09-24 at 01:40 -0400, Xiang Gao wrote:
> Currently, the aes_ccm.c and aes_gcm.c are almost line by line
> copy of each other. This patch reduce code redundancy by moving
> the code in these two files to crypto/aead_api.c to make it a
> higher level aead api. The aes_ccm.c and aes_gcm.c are removed
> and all the functions are now implemented in their headers using
> the newly added aead api.
> 
No objection from me, though I'd ask you to respin with the indentation
fixed up a bit.

johannes

Re: [PATCH] staging:ccree Fix avoid externs in .c files

[Gilad Ben-Yossef]

On Thu, Sep 21, 2017 at 9:43 AM, Janani Sankara Babu
 wrote:
> This patch solves the warning shown by the checkpatch script
> WARNING: externs should be avoided in .c files

I'm afraid here too the feedback is the same - you are working on the
Linus master tree, therefore you are seeing
things that are already gone from the downstream staging tree.


Thanks,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH] staging:ccree Fix use BIT macro

[Gilad Ben-Yossef]

Hi Janani,

On Thu, Sep 21, 2017 at 9:39 AM, Janani Sankara Babu
 wrote:
> This patch is created to solve the following warning shown by the checkpatch
> script Warning: Replace all occurences of (1<
> Signed-off-by: Janani Sankara Babu 

Thank you for the patch - unfortunately, such a fix s already present
in the staging-next tree.

I suggest you base your patches to ccree on that tree as it is the
most current for this driver.

Thanks again,
Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru

Re: [PATCH 0/2] fix authenc() kernel crash

[Stephan Müller]

Am Sonntag, 24. September 2017, 08:22:54 CEST schrieb Stephan Müller:

Hi Herbert,

(private)

> Hi Herbert,
> 
> The two patches together fix a kernel crash that can be triggered via
> AF_ALG when using authenc() with zero plaintext.
> 
> The changes are also tested to verify that the hashing on null data
> is still supported.

You can test / verify the patch with the HEAD code of libkcapi found in [1].

Please execute:

./configure --enable-kcapi-test
make
bin/kcapi -h -x 2 -c "authenc(hmac(sha256),cbc(aes))" -d 2

Note, the last command may take a couple of seconds.

[1] https://github.com/smuellerDD/libkcapi


Ciao
Stephan

[PATCH 0/2] fix authenc() kernel crash

[Stephan Müller]

Hi Herbert,

The two patches together fix a kernel crash that can be triggered via
AF_ALG when using authenc() with zero plaintext.

The changes are also tested to verify that the hashing on null data
is still supported.

I suspect that the vulnerability fixed with patch 1 is present in
abklcipher that was used before the switch to skcipher. Thus, I would
suspect in older kernels that this vulnerability is still present.
Could you please provide guidance on how to address that issue in such
older kernels?

Stephan Mueller (2):
  crypto: skcipher - noop for enc/dec with NULL data
  crypto: shash - no kmap of zero SG

 crypto/shash.c| 4 +++-
 include/crypto/skcipher.h | 6 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

-- 
2.13.5

[PATCH 2/2] crypto: shash - no kmap of zero SG

[Stephan Müller]

In case the caller provides an SG with zero data, prevent a kmap of the
page pointed to by the SG. In this case, it is possible that the page
does not exist.

This fixes a crash in authenc() when the plaintext is zero and thus the
encryption operation is a noop. In this case, no input data exists that
can be hashed. The crash is triggerable via AF_ALG from unprivileged
user space.

Fixes: 3b2f6df08258e ("crypto: hash - Export shash through ahash")
CC: Herbert Xu 
CC: 
Signed-off-by: Stephan Mueller 
---
 crypto/shash.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/shash.c b/crypto/shash.c
index 5e31c8d776df..32d0e1806bf4 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -278,9 +278,11 @@ int shash_ahash_digest(struct ahash_request *req, struct 
shash_desc *desc)
struct scatterlist *sg = req->src;
unsigned int offset = sg->offset;
unsigned int nbytes = req->nbytes;
+   unsigned int process = min(sg->length,
+  ((unsigned int)(PAGE_SIZE)) - offset);
int err;
 
-   if (nbytes < min(sg->length, ((unsigned int)(PAGE_SIZE)) - offset)) {
+   if (process && nbytes < process) {
void *data;
 
data = kmap_atomic(sg_page(sg));
-- 
2.13.5