Re: [PATCH v2] crypto: AF_ALG - limit mask and type

2017-12-21 Thread Herbert Xu 
On Fri, Dec 22, 2017 at 08:41:10AM +0100, Stephan Mueller wrote:
>
> Shouldn't we then rather use a white list instead of a black list?
> > 
> > Most other problems however would be bugs in the template code.
> > The first thing a template does when it creates an instance is
> > to check whether the resulting algorithm would fulfil the requested
> > type/mask using crypto_check_attr_type.  So if that's not working
> > then we should fix it there as it may also be triggered via other
> > code paths that can create instances.
> 
> I think I understand that, but does user space need to utilize this logic?

I'm open to a white list as well.  But we should certainly fix
the underlying bugs rather than just papering over them as they
could come back via other channels.

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: Fix race around ctx->rcvused by making it atomic_t

2017-12-21 Thread Herbert Xu 
On Fri, Dec 22, 2017 at 08:50:01AM +0100, Stephan Mueller wrote:
> Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu:
> 
> Hi Herbert,
> 
> > On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> > > This variable was increased and decreased without any protection.
> > > Result was an occasional misscount and negative wrap around resulting
> > > in false resource allocation failures.
> > > 
> > > Signed-off-by: Jonathan Cameron 
> > > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> > 
> > Actually I think it used to be fine because we held the socket
> > lock in the async callback.  It only got broken when we removed
> > the socket lock from the callback.
> 
> But we cannot hold the lock in the async callback since it may be called in 
> interrupt context.

I know.  I'm just saying that the bug was introduced in the commit
that removed the socket lock rather than the commit that introduced
this originally.

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: Fix race around ctx->rcvused by making it atomic_t

2017-12-21 Thread Stephan Mueller 
Am Freitag, 22. Dezember 2017, 08:48:03 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> > This variable was increased and decreased without any protection.
> > Result was an occasional misscount and negative wrap around resulting
> > in false resource allocation failures.
> > 
> > Signed-off-by: Jonathan Cameron 
> > Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")
> 
> Actually I think it used to be fine because we held the socket
> lock in the async callback.  It only got broken when we removed
> the socket lock from the callback.

But we cannot hold the lock in the async callback since it may be called in 
interrupt context.

Ciao
Stephan

Re: [PATCH] crypto: Fix race around ctx->rcvused by making it atomic_t

2017-12-21 Thread Herbert Xu 
On Tue, Dec 19, 2017 at 10:31:22AM +, Jonathan Cameron wrote:
> This variable was increased and decreased without any protection.
> Result was an occasional misscount and negative wrap around resulting
> in false resource allocation failures.
> 
> Signed-off-by: Jonathan Cameron 
> Fixes: 2d97591ef43d ("crypto: af_alg - consolidation of duplicate code")

Actually I think it used to be fine because we held the socket
lock in the async callback.  It only got broken when we removed
the socket lock from the callback.

commit 7d2c3f54e6f646887d019faa45f35d6fe9fe82ce
Author: Stephan Mueller 
Date:   Fri Nov 10 13:20:55 2017 +0100

crypto: af_alg - remove locking in async callback

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

Re: [PATCH v2] crypto: AF_ALG - limit mask and type

2017-12-21 Thread Stephan Mueller 
Am Freitag, 22. Dezember 2017, 08:36:07 CET schrieb Herbert Xu:

Hi Herbert,

> On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> > The user space interface allows specifying the type and the mask field
> > used to allocate the cipher. As user space can precisely select the
> > desired cipher by using either the name or the driver name, additional
> > selection options for cipher are not considered necessary and relevant
> > for user space.
> > 
> > This fixes a bug where user space is able to cause one cipher to be
> > registered multiple times potentially exhausting kernel memory.
> > 
> > Reported-by: syzbot 
> > Cc: 
> > Signed-off-by: Stephan Mueller 
> 
> This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY.  I think
> we should add CRYPTO_ALG_TESTED to the blacklist since there is
> no sane reason to use it here.

Shouldn't we then rather use a white list instead of a black list?
> 
> Most other problems however would be bugs in the template code.
> The first thing a template does when it creates an instance is
> to check whether the resulting algorithm would fulfil the requested
> type/mask using crypto_check_attr_type.  So if that's not working
> then we should fix it there as it may also be triggered via other
> code paths that can create instances.

I think I understand that, but does user space need to utilize this logic?
> 
> Thanks,



Ciao
Stephan

Re: [PATCH v2] crypto: AF_ALG - limit mask and type

2017-12-21 Thread Herbert Xu 
On Tue, Dec 19, 2017 at 07:25:04AM +0100, Stephan Müller wrote:
> The user space interface allows specifying the type and the mask field
> used to allocate the cipher. As user space can precisely select the
> desired cipher by using either the name or the driver name, additional
> selection options for cipher are not considered necessary and relevant
> for user space.
> 
> This fixes a bug where user space is able to cause one cipher to be
> registered multiple times potentially exhausting kernel memory.
> 
> Reported-by: syzbot 
> Cc: 
> Signed-off-by: Stephan Mueller 

This will break users of CRYPTO_ALG_KERN_DRIVER_ONLY.  I think
we should add CRYPTO_ALG_TESTED to the blacklist since there is
no sane reason to use it here.

Most other problems however would be bugs in the template code.
The first thing a template does when it creates an instance is
to check whether the resulting algorithm would fulfil the requested
type/mask using crypto_check_attr_type.  So if that's not working
then we should fix it there as it may also be triggered via other
code paths that can create instances.

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

Re: [PATCH] lib/mpi: Fix umul_ppmm() for MIPS64r6

2017-12-21 Thread Herbert Xu 
On Tue, Dec 05, 2017 at 11:31:35PM +, James Hogan wrote:
> From: James Hogan 
> 
> Current MIPS64r6 toolchains aren't able to generate efficient
> DMULU/DMUHU based code for the C implementation of umul_ppmm(), which
> performs an unsigned 64 x 64 bit multiply and returns the upper and
> lower 64-bit halves of the 128-bit result. Instead it widens the 64-bit
> inputs to 128-bits and emits a __multi3 intrinsic call to perform a 128
> x 128 multiply. This is both inefficient, and it results in a link error
> since we don't include __multi3 in MIPS linux.
> 
> For example commit 90a53e4432b1 ("cfg80211: implement regdb signature
> checking") merged in v4.15-rc1 recently broke the 64r6_defconfig and
> 64r6el_defconfig builds by indirectly selecting MPILIB. The same build
> errors can be reproduced on older kernels by enabling e.g. CRYPTO_RSA:
> 
> lib/mpi/generic_mpih-mul1.o: In function `mpihelp_mul_1':
> lib/mpi/generic_mpih-mul1.c:50: undefined reference to `__multi3'
> lib/mpi/generic_mpih-mul2.o: In function `mpihelp_addmul_1':
> lib/mpi/generic_mpih-mul2.c:49: undefined reference to `__multi3'
> lib/mpi/generic_mpih-mul3.o: In function `mpihelp_submul_1':
> lib/mpi/generic_mpih-mul3.c:49: undefined reference to `__multi3'
> lib/mpi/mpih-div.o In function `mpihelp_divrem':
> lib/mpi/mpih-div.c:205: undefined reference to `__multi3'
> lib/mpi/mpih-div.c:142: undefined reference to `__multi3'
> 
> Therefore add an efficient MIPS64r6 implementation of umul_ppmm() using
> inline assembly and the DMULU/DMUHU instructions, to prevent __multi3
> calls being emitted.
> 
> Fixes: 7fd08ca58ae6 ("MIPS: Add build support for the MIPS R6 ISA")
> Signed-off-by: James Hogan 
> Cc: Ralf Baechle 
> Cc: Herbert Xu 
> Cc: "David S. Miller" 
> Cc: linux-m...@linux-mips.org
> Cc: linux-crypto@vger.kernel.org
> ---
> Please can somebody apply this fix for v4.15, as the MIPS 64r6_defconfig
> and 64r6el_defconfig builds are broken without it.

I can take this but I'd like to see an ack from someone on the
MIPS side.

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

Crypto Fixes for 4.15

2017-12-21 Thread Herbert Xu 
Hi Linus: 

This push fixes the following issues:

- Fix chacha20 crash on zero-length input due to unset IV.
- Fix potential race conditions in mcryptd with spinlock.
- Only wait once at top of algif recvmsg to avoid inconsistencies.
- Fix potential use-after-free in algif_aead/algif_skcipher.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Eric Biggers (1):
  crypto: skcipher - set walk.iv for zero-length inputs

Sebastian Andrzej Siewior (1):
  crypto: mcryptd - protect the per-CPU queue with a lock

Stephan Mueller (2):
  crypto: af_alg - wait for data at beginning of recvmsg
  crypto: af_alg - fix race accessing cipher request

 crypto/af_alg.c  |6 --
 crypto/algif_aead.c  |   16 +++-
 crypto/algif_skcipher.c  |   16 +++-
 crypto/mcryptd.c |   23 ++-
 crypto/skcipher.c|   10 --
 include/crypto/mcryptd.h |1 +
 6 files changed, 37 insertions(+), 35 deletions(-)

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

Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

2017-12-21 Thread Herbert Xu 
On Wed, Dec 20, 2017 at 08:09:26PM +, Corentin Labbe wrote:
> This patch implement a generic way to get statistics about all crypto
> usages.
> 
> Signed-off-by: Corentin Labbe 

Please don't use sysfs.  We already have crypto_user and this
should be exposed through that.

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

Re: BUG: unable to handle kernel paging request in hmac_init_tfm

2017-12-21 Thread Eric Biggers 
On Thu, Dec 21, 2017 at 08:44:03AM +0100, 'Dmitry Vyukov' via syzkaller-bugs 
wrote:
> On Thu, Dec 21, 2017 at 12:09 AM, Eric Biggers <ebigge...@gmail.com> wrote:
> > On Mon, Dec 18, 2017 at 11:36:01AM -0800, syzbot wrote:
> >> Hello,
> >>
> >> syzkaller hit the following crash on
> >> 6084b576dca2e898f5c101baef151f7bfdbb606d
> >> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/master
> >> compiler: gcc (GCC) 7.1.1 20170620
> >> .config is attached
> >
> > FYI, in linux-next KASAN and other memory debugging options are now behind
> > CONFIG_DEBUG_MEMORY.  So, I think KASAN isn't getting turned on anymore, 
> > despite
> > the CONFIG_KASAN=y.  (Patch was "lib/: make "Memory Debugging" a menuconfig 
> > to
> > ease disabling it all".)
> 
> Ouch! That would be pretty bad.
> 
> But I've tried both linux-next HEAD at:
> 
> commit 0e08c463db387a2adcb0243b15ab868a73f87807 (HEAD, tag:
> next-20171221, linux-next/master)
> Author: Stephen Rothwell <s...@canb.auug.org.au>
> Date:   Thu Dec 21 15:37:39 2017 +1100
> Add linux-next specific files for 20171221
> 
> and mmots HEAD at:
> 
> commit 75aa5540627fdb3d8f86229776ea87f995275351 (HEAD, tag:
> v4.15-rc4-mmots-2017-12-20-19-10, mmots/master)
> Author: Linus Torvalds <torva...@linux-foundation.org>
> Date:   Thu Dec 21 04:02:17 2017 +
> pci: test for unexpectedly disabled bridges
> 
> and after running make olddefconfig I still see CONFIG_KASAN=y in
> .config, nor I can find CONFIG_DEBUG_MEMORY in menuconfig.
> 
> What am I missing? What commit has added the config?
> 

Ah, I see the patch was added to -mm on 2017-12-12 but removed two days later:

https://www.spinics.net/lists/mm-commits/msg129685.html
https://www.spinics.net/lists/mm-commits/msg129696.html

So it was in linux-next temporarily (including the kernel version used for this
particular bug report) but now it's not.  Just keep an eye out for it coming
back, I guess.

Eric

Re: [PATCH RFC 1/3] crypto: Prevent to register duplicate cra_driver_name

2017-12-21 Thread LABBE Corentin 
On Thu, Dec 21, 2017 at 01:35:27PM +0100, LABBE Corentin wrote:
> On Thu, Dec 21, 2017 at 05:35:22PM +1100, Herbert Xu wrote:
> > On Wed, Dec 20, 2017 at 08:09:25PM +, Corentin Labbe wrote:
> > > Each crypto algorithm "cra_name" can have multiple implementation called
> > > "cra_driver_name".
> > > If two different implementation have the same cra_driver_name, nothing
> > > can easily differentiate them.
> > > Furthermore the mechanism for getting a crypto algorithm with its
> > > implementation name (crypto_alg_match() in crypto/crypto_user.c) will
> > > get only the first one found.
> > > 
> > > So this patch prevent the registration of two implementation with the
> > > same cra_driver_name.
> > > 
> > > Signed-off-by: Corentin Labbe 
> > 
> > No this is intentional.  The idea is that you can hot-replace
> > an implementation by registering a new version of it while the
> > old one is still in use.  The new one will be used for all new
> > allocations.
> > 
> 
> But the new implementation is different from the first so should have a new 
> name.
> The only case I found is ctr-aes-ce, and both are different (use/dontuse 
> simd) so qualifying for different name.
> 
> Anyway, any advice on how to populate properly /sys/crypto with unique name ?
> I have two idea:
> - A number which increment after each register
> - cra_driver_name-priority
> 
> Or does I use /sys/crypto/cra_driver_name/priority ? (which need to use some 
> usage count on cra_driver_name node)

I just see that kobject already have reference counting so this solution is the 
better.

Regards

Re: [PATCH RFC 2/3] crypto: Implement a generic crypto statistics

2017-12-21 Thread LABBE Corentin 
On Thu, Dec 21, 2017 at 07:38:35AM +0100, Stephan Mueller wrote:
> Am Mittwoch, 20. Dezember 2017, 21:09:26 CET schrieb Corentin Labbe:
> 
> Hi Corentin,
> 
> > This patch implement a generic way to get statistics about all crypto
> > usages.
> > 
> > Signed-off-by: Corentin Labbe 
> > ---
> >  crypto/Kconfig |  11 +++
> >  crypto/ahash.c |  18 +
> >  crypto/algapi.c| 186
> > + crypto/rng.c   | 
> >  3 +
> >  include/crypto/acompress.h |  10 +++
> >  include/crypto/akcipher.h  |  12 +++
> >  include/crypto/kpp.h   |   9 +++
> >  include/crypto/rng.h   |   5 ++
> >  include/crypto/skcipher.h  |   8 ++
> >  include/linux/crypto.h |  22 ++
> >  10 files changed, 284 insertions(+)
> > 
> > diff --git a/crypto/Kconfig b/crypto/Kconfig
> > index d6e9b60fc063..69f1822a026b 100644
> > --- a/crypto/Kconfig
> > +++ b/crypto/Kconfig
> > @@ -1781,6 +1781,17 @@ config CRYPTO_USER_API_AEAD
> >   This option enables the user-spaces interface for AEAD
> >   cipher algorithms.
> > 
> > +config CRYPTO_STATS
> > +   bool "Crypto usage statistics for User-space"
> > +   help
> > + This option enables the gathering of crypto stats.
> > + This will collect:
> > + - encrypt/decrypt size and numbers of symmeric operations
> > + - compress/decompress size and numbers of compress operations
> > + - size and numbers of hash operations
> > + - encrypt/decrypt/sign/verify numbers for asymmetric operations
> > + - generate/seed numbers for rng operations
> > +
> >  config CRYPTO_HASH_INFO
> > bool
> > 
> > diff --git a/crypto/ahash.c b/crypto/ahash.c
> > index 3a35d67de7d9..93b56892f1b8 100644
> > --- a/crypto/ahash.c
> > +++ b/crypto/ahash.c
> > @@ -356,18 +356,36 @@ static int crypto_ahash_op(struct ahash_request *req,
> > 
> >  int crypto_ahash_final(struct ahash_request *req)
> >  {
> > +#ifdef CONFIG_CRYPTO_STATS
> > +   struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > +
> > +   tfm->base.__crt_alg->enc_cnt++;
> > +   tfm->base.__crt_alg->enc_tlen += req->nbytes;
> > +#endif
> > return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->final);
> >  }
> >  EXPORT_SYMBOL_GPL(crypto_ahash_final);
> > 
> >  int crypto_ahash_finup(struct ahash_request *req)
> >  {
> > +#ifdef CONFIG_CRYPTO_STATS
> > +   struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > +
> > +   tfm->base.__crt_alg->enc_cnt++;
> > +   tfm->base.__crt_alg->enc_tlen += req->nbytes;
> > +#endif
> > return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->finup);
> >  }
> >  EXPORT_SYMBOL_GPL(crypto_ahash_finup);
> > 
> >  int crypto_ahash_digest(struct ahash_request *req)
> >  {
> > +#ifdef CONFIG_CRYPTO_STATS
> > +   struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
> > +
> > +   tfm->base.__crt_alg->enc_cnt++;
> > +   tfm->base.__crt_alg->enc_tlen += req->nbytes;
> > +#endif
> 
> This is ugly. May I ask that one inlne function is made that has these ifdefs 
> instead of adding them throughout multiple functions? This comment would 
> apply 
> to the other instrumentation code below as well. The issue is that these 
> ifdefs should not be spread around through the code.
> 
> Besides, I would think that the use of normal integers is not thread-safe. 
> What about using atomic_t?

I will do all your suggestions.

> 
> > return crypto_ahash_op(req, crypto_ahash_reqtfm(req)->digest);
> >  }
> >  EXPORT_SYMBOL_GPL(crypto_ahash_digest);
> > diff --git a/crypto/algapi.c b/crypto/algapi.c
> > index b8f6122f37e9..4fca4576af78 100644
> > --- a/crypto/algapi.c
> > +++ b/crypto/algapi.c
> > @@ -20,11 +20,158 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> > 
> >  #include "internal.h"
> > 
> >  static LIST_HEAD(crypto_template_list);
> > 
> > +#ifdef CONFIG_CRYPTO_STATS
> 
> Instead of ifdefing in the code, may I suggest adding a new file that is 
> compiled / not compiled via the Makefile?

I agree, it will be better.

> 
> > +static struct kobject *crypto_root_kobj;
> > +
> > +static struct kobj_type dynamic_kobj_ktype = {
> > +   .sysfs_ops  = _sysfs_ops,
> > +};
> > +
> > +static ssize_t fcrypto_priority(struct kobject *kobj,
> > +   struct kobj_attribute *attr, char *buf)
> > +{
> > +   struct crypto_alg *alg;
> > +
> > +   alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> > +   return snprintf(buf, 9, "%d\n", alg->cra_priority);
> > +}
> > +
> > +static ssize_t fcrypto_stat_enc_cnt(struct kobject *kobj,
> > +   struct kobj_attribute *attr, char *buf)
> > +{
> > +   struct crypto_alg *alg;
> > +
> > +   alg = container_of(kobj, struct crypto_alg, cra_stat_obj);
> > +   return snprintf(buf, 9, "%lu\n", alg->enc_cnt);
> > +}
> > +
> > +static ssize_t fcrypto_stat_enc_tlen(struct kobject *kobj,
> > +struct kobj_attribute *attr, char *buf)
> > +{
> > +   struct crypto_alg

Re: [PATCH] staging: ccree: fix type mismatch warning

2017-12-21 Thread Greg Kroah-Hartman 
On Thu, Dec 21, 2017 at 02:31:20PM +0100, Arnd Bergmann wrote:
> __dump_byte_array used to be hidden, but is now visible to the compiler
> and causes a harmless warning:
> 
> drivers/staging/ccree/ssi_driver.c:82:6: error: conflicting types for 
> '__dump_byte_array'
> drivers/staging/ccree/ssi_driver.c: In function '__dump_byte_array':
> drivers/staging/ccree/ssi_driver.c:89:41: error: format '%lu' expects 
> argument of type 'long unsigned int', but argument 5 has type 'size_t {aka 
> unsigned int}' [-Werror=format=]
> 
> This changes the prototype in the header to match that of the actual
> function on all architectures, and the format string to match the
> argument.
> 
> Fixes: 3f268f5d6669 ("staging: ccree: turn compile time debug log to params")
> Signed-off-by: Arnd Bergmann 
> ---
>  drivers/staging/ccree/ssi_driver.c | 2 +-
>  drivers/staging/ccree/ssi_driver.h | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Should already be fixed, sorry.

greg k-h

Re: [PATCH] staging: ccree: fix __dump_byte_array() declaration mismatch

2017-12-21 Thread Greg KH 
On Wed, Dec 20, 2017 at 06:16:08PM +, Corentin Labbe wrote:
> This patch corrects the type of the size argument in __dump_byte_array()
> from unsigned long to size_t as done only in 
> drivers/staging/ccree/ssi_driver.c
> 
> This fix also a build error:
> drivers/staging/ccree/ssi_driver.c:82:6: error: conflicting types for 
> '__dump_byte_array'
> 
> Fixes: 3f268f5d6669 ("staging: ccree: turn compile time debug log to params")
> Signed-off-by: Corentin Labbe 
> ---
>  drivers/staging/ccree/ssi_driver.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

There's another warning here that Arnd's patches fixes, yet yours
didn't, so I'm going to take his patch, sorry.

greg k-h

Re: [Part2 PATCH v9 00/38] x86: Secure Encrypted Virtualization (AMD)

2017-12-21 Thread Brijesh Singh 


On 12/21/17 9:51 AM, Brijesh Singh wrote:
>
> On 12/21/17 7:06 AM, Paolo Bonzini wrote:
> 
>
> Hi Paolo,
>  
>
>> Hi Brijesh,
>>
>> I have a couple comments:
>>
>> 1) how is MSR_AMD64_SEV's value passed to the guest, and where is it in
>> the manual?
> It is a non interceptable read-only MSR set by the HW when SEV feature
> is enabled in VMRUN instruction.

I just checked both PPR and Family 17 manual and it seems both are still
missing this MSR definition. I will ping doc team to get it updated. thanks
>> 2) ECX should be 0 in the guest's 0x8000_001f leaf, because we don't
>> support nested SEV guests.  Likewise, EAX bit 2 should be 0 since you
>> don't emulate the page flush MSR.
> IIRC, I do clear both EAX Page_Flush and nested virtualization case from
> Qemu SEV feature is enabled.
>> Both can be fixed on top (and I can do the second myself of course), so
>> there should be no need for a v10.  
> Thanks
>

Re: [Part2 PATCH v9 00/38] x86: Secure Encrypted Virtualization (AMD)

2017-12-21 Thread Brijesh Singh 


On 12/21/17 7:06 AM, Paolo Bonzini wrote:


Hi Paolo,
 

> Hi Brijesh,
>
> I have a couple comments:
>
> 1) how is MSR_AMD64_SEV's value passed to the guest, and where is it in
> the manual?

It is a non interceptable read-only MSR set by the HW when SEV feature
is enabled in VMRUN instruction.
>
> 2) ECX should be 0 in the guest's 0x8000_001f leaf, because we don't
> support nested SEV guests.  Likewise, EAX bit 2 should be 0 since you
> don't emulate the page flush MSR.

IIRC, I do clear both EAX Page_Flush and nested virtualization case from
Qemu SEV feature is enabled.
> Both can be fixed on top (and I can do the second myself of course), so
> there should be no need for a v10.  

Thanks

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

2017-12-21 Thread PrasannaKumar Muralidharan 
Hi Ard,

On 21 December 2017 at 17:52, Ard Biesheuvel  wrote:
> On 21 December 2017 at 10:20, Arnd Bergmann  wrote:
>> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
>>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
 diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
 index ca554d57d01e..35f973ba9878 100644
 --- a/crypto/aes_generic.c
 +++ b/crypto/aes_generic.c
 @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
   f_rl(bo, bi, 3, k); \
  } while (0)

 +#if __GNUC__ >= 7
 +/*
 + * Newer compilers try to optimize integer arithmetic more aggressively,
 + * which generally improves code quality a lot, but in this specific case
 + * ends up hurting more than it helps, in some configurations drastically
 + * so. This turns off two optimization steps that have been shown to
 + * lead to rather badly optimized code with gcc-7.
 + *
 + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
 + */
 +#pragma GCC optimize("-fno-tree-pre")
 +#pragma GCC optimize("-fno-tree-sra")
>>>
>>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>>> predefined macro for those (only for asan and tsan), but either the kernel
>>> does have something already, or could have something added in the
>>> corresponding Makefile.
>>
>> My original interpretation of the resulting object code suggested that 
>> disabling
>> those two optimizations produced better results for this particular
>> file even without
>> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
>> been better, but I did some measurements now as Ard suggested, showing
>> cycles/byte for AES256/CBC with 8KB blocks:
>>
>>
>>default  ubsan patchedpatched+ubsan
>> gcc-4.3.614.9   14.9 
>> gcc-4.6.415.0   15.8 
>> gcc-4.9.415.520.7   15.9 20.9
>> gcc-5.5.015.647.3   86.4 48.8
>> gcc-6.3.114.649.4   94.3 50.9
>> gcc-7.1.113.554.6   15.2 52.0
>> gcc-7.2.116.8   124.7   92.0 52.2
>> gcc-8.0.015.0  no boot  15.3no boot
>>
>> I checked that there are actually three significant digits on the 
>> measurements,
>> detailed output is available at https://pastebin.com/eFsWYjQp
>>
>> It seems that I was wrong about the interpretation that disabling
>> the optimization would be a win on gcc-7 and higher, it almost
>> always makes things worse even with UBSAN. Making that
>> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
>> would help here, or we could list the file as an exception for
>> UBSAN and never sanitize it.
>>
>> Looking at the 'default' column, I wonder if anyone would be interested
>> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>>
>
> Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
> appears the UBSAN code inserts runtime checks to ensure that certain
> u8 variables don't assume values <0 or >255, which seems like a rather
> pointless exercise to me. But even if it didn't, I think it is
> justified to disable UBSAN on all of the low-level cipher
> implementations, given that they are hardly ever modified, and
> typically don't suffer from the issues UBSAN tries to identify.
>
> So my vote is to disable UBSAN for all such cipher implementations:
> aes_generic, but also aes_ti, which has a similar 256 byte lookup
> table [although it does not seem to be affected by the same issue as
> aes_generic], and possibly others as well.
>
> Perhaps it makes sense to move core cipher code into a separate
> sub-directory, and disable UBSAN at the directory level?
>
> It would involve the following files
>
> crypto/aes_generic.c
> crypto/aes_ti.c
> crypto/anubis.c
> crypto/arc4.c
> crypto/blowfish_generic.c
> crypto/camellia_generic.c
> crypto/cast5_generic.c
> crypto/cast6_generic.c
> crypto/des_generic.c
> crypto/fcrypt.c
> crypto/khazad.c
> crypto/seed.c
> crypto/serpent_generic.c
> crypto/tea.c
> crypto/twofish_generic.c

As *SAN is enabled only on developer setup, is such a change required?
Looks like I am missing something here. Can you explain what value it
provides?

Regards,
PrasannaKumar

[PATCH] staging: ccree: fix type mismatch warning

2017-12-21 Thread Arnd Bergmann 
__dump_byte_array used to be hidden, but is now visible to the compiler
and causes a harmless warning:

drivers/staging/ccree/ssi_driver.c:82:6: error: conflicting types for 
'__dump_byte_array'
drivers/staging/ccree/ssi_driver.c: In function '__dump_byte_array':
drivers/staging/ccree/ssi_driver.c:89:41: error: format '%lu' expects argument 
of type 'long unsigned int', but argument 5 has type 'size_t {aka unsigned 
int}' [-Werror=format=]

This changes the prototype in the header to match that of the actual
function on all architectures, and the format string to match the
argument.

Fixes: 3f268f5d6669 ("staging: ccree: turn compile time debug log to params")
Signed-off-by: Arnd Bergmann 
---
 drivers/staging/ccree/ssi_driver.c | 2 +-
 drivers/staging/ccree/ssi_driver.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/ccree/ssi_driver.c 
b/drivers/staging/ccree/ssi_driver.c
index 56b5d45a205c..1254c6922d50 100644
--- a/drivers/staging/ccree/ssi_driver.c
+++ b/drivers/staging/ccree/ssi_driver.c
@@ -86,7 +86,7 @@ void __dump_byte_array(const char *name, const u8 *buf, 
size_t len)
if (!buf)
return;
 
-   snprintf(prefix, sizeof(prefix), "%s[%lu]: ", name, len);
+   snprintf(prefix, sizeof(prefix), "%s[%zu]: ", name, len);
 
print_hex_dump(KERN_DEBUG, prefix, DUMP_PREFIX_ADDRESS, 16, 1, buf,
   len, false);
diff --git a/drivers/staging/ccree/ssi_driver.h 
b/drivers/staging/ccree/ssi_driver.h
index 5a56f7a76b71..e1406d6e1ab2 100644
--- a/drivers/staging/ccree/ssi_driver.h
+++ b/drivers/staging/ccree/ssi_driver.h
@@ -175,9 +175,9 @@ static inline struct device *drvdata_to_dev(struct 
cc_drvdata *drvdata)
 }
 
 void __dump_byte_array(const char *name, const u8 *the_array,
-  unsigned long size);
+  size_t size);
 static inline void dump_byte_array(const char *name, const u8 *the_array,
-  unsigned long size)
+  size_t size)
 {
if (cc_dump_bytes)
__dump_byte_array(name, the_array, size);
-- 
2.9.0

Re: [Part2 PATCH v9 00/38] x86: Secure Encrypted Virtualization (AMD)

2017-12-21 Thread Paolo Bonzini 
On 05/12/2017 02:04, Brijesh Singh wrote:
> This part of Secure Encrypted Virtualization (SEV) patch series focuses on KVM
> changes required to create and manage SEV guests.
> 
> SEV is an extension to the AMD-V architecture which supports running encrypted
> virtual machine (VMs) under the control of a hypervisor. Encrypted VMs have 
> their
> pages (code and data) secured such that only the guest itself has access to
> unencrypted version. Each encrypted VM is associated with a unique encryption 
> key;
> if its data is accessed to a different entity using a different key the 
> encrypted
> guest's data will be incorrectly decrypted, leading to unintelligible data.
> This security model ensures that hypervisor will no longer able to inspect or
> alter any guest code or data.
> 
> The key management of this feature is handled by a separate processor known as
> the AMD Secure Processor (AMD-SP) which is present on AMD SOCs. The SEV Key
> Management Specification (see below) provides a set of commands which can be
> used by hypervisor to load virtual machine keys through the AMD-SP driver.
> 
> The patch series adds a new ioctl in KVM driver (KVM_MEMORY_ENCRYPT_OP). The
> ioctl will be used by qemu to issue SEV guest-specific commands defined in Key
> Management Specification.

Hi Brijesh,

I have a couple comments:

1) how is MSR_AMD64_SEV's value passed to the guest, and where is it in
the manual?

2) ECX should be 0 in the guest's 0x8000_001f leaf, because we don't
support nested SEV guests.  Likewise, EAX bit 2 should be 0 since you
don't emulate the page flush MSR.

Both can be fixed on top (and I can do the second myself of course), so
there should be no need for a v10.  But MSR_AMD64_SEV is leaving me
quite puzzled.

Thanks,

Paolo

> The following links provide additional details:
> 
> AMD Memory Encryption white paper:
> http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memory_Encryption_Whitepaper_v7-Public.pdf
> 
> AMD64 Architecture Programmer's Manual:
> http://support.amd.com/TechDocs/24593.pdf
> SME is section 7.10
> SEV is section 15.34
> 
> SEV Key Management:
> http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf
> 
> KVM Forum Presentation:
> http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtualizatoin_Memory_Encryption_Technology.pdf
> 
> SEV Guest BIOS support:
>   SEV support has been add to EDKII/OVMF BIOS
>   https://github.com/tianocore/edk2
> 
> --
> 
> The series applies on kvm/next commit : 4fbd8d194f06 (Linux 4.15-rc1)
> 
> Complete tree is available at:
> repo: https://github.com/codomania/kvm.git
> branch: sev-v9-p2
> 
> TODO:
> * Add SEV guest migration command support
> 
> Cc: Thomas Gleixner 
> Cc: Ingo Molnar 
> Cc: "H. Peter Anvin" 
> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Joerg Roedel 
> Cc: Borislav Petkov 
> Cc: Tom Lendacky 
> Cc: Herbert Xu 
> Cc: David S. Miller 
> Cc: Gary Hook 
> Cc: x...@kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-crypto@vger.kernel.org
> 
> Changes since v8:
>  * Rebase the series to kvm/next branch
>  * Update SEV asid allocation to limit the ASID between SEV_MIN_ASID to 
> SEV_MAX_ASID
>(EPYC BIOS provide option to change the SEV_MIN_ASID -- which can be used 
> to
>limit the number of SEV-enable guest)
> 
> Changes since v7:
>  * Rebase the series to kvm/next branch
>  * move the FW error enum definition in include/uapi/linux/psp-sev.h so that
>both userspace and kernel can share it.
>  * (ccp) drop cmd_buf arg from sev_platform_init()
>  * (ccp) apply some cleanup/fixup from Boris
>  * (ccp) add some comments in FACTORY_RESET command handling
>  * (kvm) some fixup/cleanup from Boris
>  * (kvm) acquire the kvm->lock when modifying the sev->regions_list
> 
> Changes since v6:
>  * (ccp): Extend psp_device structure to track the FW INIT and SHUTDOWN 
> states.
>  * (ccp): Init and Uninit SEV FW during module load/unload
>  * (ccp): Avoid repeated k*alloc() for init and status command buffer
>  * (kvm): Rework DBG command to fix the compilation warning seen with gcc7.x
>  * (kvm): Convert the SEV doc in rst format
> 
> Changes since v5:
>  * split the PSP driver support into multiple patches
>  * multiple improvements from Boris
>  * remove mem_enc_enabled() ops
> 
> Changes since v4:
>  * Fixes to address kbuild robot errors
>  * Add 'sev' module params to allow enable/disable SEV feature
>  * Update documentation
>  * Multiple fixes to address v4 feedbacks
>  * Some coding style changes to address checkpatch reports
> 
> Changes since v3:
>  * Re-design the PSP interface support patch
>  * Rename the ioctls based on the feedbacks
>  * Improve documentation
>  * Fix i386 build issues
>  * Add

Re: [PATCH RFC 1/3] crypto: Prevent to register duplicate cra_driver_name

2017-12-21 Thread LABBE Corentin 
On Thu, Dec 21, 2017 at 05:35:22PM +1100, Herbert Xu wrote:
> On Wed, Dec 20, 2017 at 08:09:25PM +, Corentin Labbe wrote:
> > Each crypto algorithm "cra_name" can have multiple implementation called
> > "cra_driver_name".
> > If two different implementation have the same cra_driver_name, nothing
> > can easily differentiate them.
> > Furthermore the mechanism for getting a crypto algorithm with its
> > implementation name (crypto_alg_match() in crypto/crypto_user.c) will
> > get only the first one found.
> > 
> > So this patch prevent the registration of two implementation with the
> > same cra_driver_name.
> > 
> > Signed-off-by: Corentin Labbe 
> 
> No this is intentional.  The idea is that you can hot-replace
> an implementation by registering a new version of it while the
> old one is still in use.  The new one will be used for all new
> allocations.
> 

But the new implementation is different from the first so should have a new 
name.
The only case I found is ctr-aes-ce, and both are different (use/dontuse simd) 
so qualifying for different name.

Anyway, any advice on how to populate properly /sys/crypto with unique name ?
I have two idea:
- A number which increment after each register
- cra_driver_name-priority

Or does I use /sys/crypto/cra_driver_name/priority ? (which need to use some 
usage count on cra_driver_name node)

Regards

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

2017-12-21 Thread Ard Biesheuvel 
On 21 December 2017 at 10:20, Arnd Bergmann  wrote:
> On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
>> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
>>> diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
>>> index ca554d57d01e..35f973ba9878 100644
>>> --- a/crypto/aes_generic.c
>>> +++ b/crypto/aes_generic.c
>>> @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
>>>   f_rl(bo, bi, 3, k); \
>>>  } while (0)
>>>
>>> +#if __GNUC__ >= 7
>>> +/*
>>> + * Newer compilers try to optimize integer arithmetic more aggressively,
>>> + * which generally improves code quality a lot, but in this specific case
>>> + * ends up hurting more than it helps, in some configurations drastically
>>> + * so. This turns off two optimization steps that have been shown to
>>> + * lead to rather badly optimized code with gcc-7.
>>> + *
>>> + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>>> + */
>>> +#pragma GCC optimize("-fno-tree-pre")
>>> +#pragma GCC optimize("-fno-tree-sra")
>>
>> So do it only when UBSAN is enabled?  GCC doesn't have a particular
>> predefined macro for those (only for asan and tsan), but either the kernel
>> does have something already, or could have something added in the
>> corresponding Makefile.
>
> My original interpretation of the resulting object code suggested that 
> disabling
> those two optimizations produced better results for this particular
> file even without
> UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
> been better, but I did some measurements now as Ard suggested, showing
> cycles/byte for AES256/CBC with 8KB blocks:
>
>
>default  ubsan patchedpatched+ubsan
> gcc-4.3.614.9   14.9 
> gcc-4.6.415.0   15.8 
> gcc-4.9.415.520.7   15.9 20.9
> gcc-5.5.015.647.3   86.4 48.8
> gcc-6.3.114.649.4   94.3 50.9
> gcc-7.1.113.554.6   15.2 52.0
> gcc-7.2.116.8   124.7   92.0 52.2
> gcc-8.0.015.0  no boot  15.3no boot
>
> I checked that there are actually three significant digits on the 
> measurements,
> detailed output is available at https://pastebin.com/eFsWYjQp
>
> It seems that I was wrong about the interpretation that disabling
> the optimization would be a win on gcc-7 and higher, it almost
> always makes things worse even with UBSAN. Making that
> check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
> would help here, or we could list the file as an exception for
> UBSAN and never sanitize it.
>
> Looking at the 'default' column, I wonder if anyone would be interested
> in looking at why the throughput regressed with gcc-7.2 and gcc-8.
>

Thanks for the elaborate benchmarks. Looking at the bugzilla entry, it
appears the UBSAN code inserts runtime checks to ensure that certain
u8 variables don't assume values <0 or >255, which seems like a rather
pointless exercise to me. But even if it didn't, I think it is
justified to disable UBSAN on all of the low-level cipher
implementations, given that they are hardly ever modified, and
typically don't suffer from the issues UBSAN tries to identify.

So my vote is to disable UBSAN for all such cipher implementations:
aes_generic, but also aes_ti, which has a similar 256 byte lookup
table [although it does not seem to be affected by the same issue as
aes_generic], and possibly others as well.

Perhaps it makes sense to move core cipher code into a separate
sub-directory, and disable UBSAN at the directory level?

It would involve the following files

crypto/aes_generic.c
crypto/aes_ti.c
crypto/anubis.c
crypto/arc4.c
crypto/blowfish_generic.c
crypto/camellia_generic.c
crypto/cast5_generic.c
crypto/cast6_generic.c
crypto/des_generic.c
crypto/fcrypt.c
crypto/khazad.c
crypto/seed.c
crypto/serpent_generic.c
crypto/tea.c
crypto/twofish_generic.c

[PATCH] padata: add SPDX identifier

2017-12-21 Thread Cheah Kok Cheong 
Add SPDX license identifier according to the type of license text found
in the file.

Cc: Philippe Ombredanne 
Signed-off-by: Cheah Kok Cheong 
---
 kernel/padata.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/padata.c b/kernel/padata.c
index e265953..eb9a9d9 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * padata.c - generic interface to process data streams in parallel
  *
-- 
2.7.4

Re: [PATCH 1/2] padata: Remove FSF address

2017-12-21 Thread Cheah Kok Cheong 
On Thu, Dec 21, 2017 at 08:34:37AM +0100, Philippe Ombredanne wrote:
> Dear CheahKC,
> 
> On Wed, Dec 20, 2017 at 10:17 PM, Cheah Kok Cheong  wrote:
> > On Wed, Dec 20, 2017 at 09:20:48PM +0100, Philippe Ombredanne wrote:
> >> On Wed, Dec 20, 2017 at 9:15 PM, Cheah Kok Cheong  
> >> wrote:
> >> > Remove FSF address otherwise checkpatch will flag my next patch.
> >> >
> >> > Signed-off-by: Cheah Kok Cheong 
> >> > ---
> >> >  kernel/padata.c | 4 
> >> >  1 file changed, 4 deletions(-)
> >> >
> >> > diff --git a/kernel/padata.c b/kernel/padata.c
> >> > index 57c0074..9d91909 100644
> >> > --- a/kernel/padata.c
> >> > +++ b/kernel/padata.c
> >> > @@ -14,10 +14,6 @@
> >> >   * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> >   * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
> >> > License for
> >> >   * more details.
> >> > - *
> >> > - * You should have received a copy of the GNU General Public License 
> >> > along with
> >> > - * this program; if not, write to the Free Software Foundation, Inc.,
> >> > - * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> >> >   */
> >> >
> >> >  #include 
> >> > --
> >> > 2.7.4
> >> >
> >>
> >>
> >> Why not use instead the simpler SPDX ids?
> >> --
> >> Cordially
> >> Philippe Ombredanne
> >
> > Hi Philippe,
> >   Adding the SPDX id can be the subject of a separate patch.
> > Believe you are part of the team doing an audit in this matter.
> > I shall leave adding the SPDX id to you guys the professionals.
> > Looks like you guys are using script to do this en masse.
> >
> > But if you insist and our maintainer is agreeable, I can send
> > another patch to add the SPDX id.
> 
> If you could work it out directly that would be best. There are a lot
> of files in the kernel and each one needs a careful review even with
> best license detection script (which . So tagging files on the go when
> they are updated is the best way to help!
> 
> Thank you.
> 
> -- 
> Cordially
> Philippe Ombredanne
>
Ok I'll send a separate patch to add the SPDX id. Glad to help.

Thank you.

Brgds,
CheahKC

Re: [PATCH] [RFT] crypto: aes-generic - turn off -ftree-pre and -ftree-sra

2017-12-21 Thread Arnd Bergmann 
On Wed, Dec 20, 2017 at 10:46 PM, Jakub Jelinek  wrote:
> On Wed, Dec 20, 2017 at 09:52:05PM +0100, Arnd Bergmann wrote:
>> diff --git a/crypto/aes_generic.c b/crypto/aes_generic.c
>> index ca554d57d01e..35f973ba9878 100644
>> --- a/crypto/aes_generic.c
>> +++ b/crypto/aes_generic.c
>> @@ -1331,6 +1331,20 @@ EXPORT_SYMBOL_GPL(crypto_aes_set_key);
>>   f_rl(bo, bi, 3, k); \
>>  } while (0)
>>
>> +#if __GNUC__ >= 7
>> +/*
>> + * Newer compilers try to optimize integer arithmetic more aggressively,
>> + * which generally improves code quality a lot, but in this specific case
>> + * ends up hurting more than it helps, in some configurations drastically
>> + * so. This turns off two optimization steps that have been shown to
>> + * lead to rather badly optimized code with gcc-7.
>> + *
>> + * See also https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
>> + */
>> +#pragma GCC optimize("-fno-tree-pre")
>> +#pragma GCC optimize("-fno-tree-sra")
>
> So do it only when UBSAN is enabled?  GCC doesn't have a particular
> predefined macro for those (only for asan and tsan), but either the kernel
> does have something already, or could have something added in the
> corresponding Makefile.

My original interpretation of the resulting object code suggested that disabling
those two optimizations produced better results for this particular
file even without
UBSAN, on both gcc-7 and gcc-8 (but not gcc-6), so my patch might have
been better, but I did some measurements now as Ard suggested, showing
cycles/byte for AES256/CBC with 8KB blocks:


   default  ubsan patchedpatched+ubsan
gcc-4.3.614.9   14.9 
gcc-4.6.415.0   15.8 
gcc-4.9.415.520.7   15.9 20.9
gcc-5.5.015.647.3   86.4 48.8
gcc-6.3.114.649.4   94.3 50.9
gcc-7.1.113.554.6   15.2 52.0
gcc-7.2.116.8   124.7   92.0 52.2
gcc-8.0.015.0  no boot  15.3no boot

I checked that there are actually three significant digits on the measurements,
detailed output is available at https://pastebin.com/eFsWYjQp

It seems that I was wrong about the interpretation that disabling
the optimization would be a win on gcc-7 and higher, it almost
always makes things worse even with UBSAN. Making that
check "#if __GNUC__ == 7 && IS_ENABLED(CONFIG_UBSAN_SANITIZE_ALL)"
would help here, or we could list the file as an exception for
UBSAN and never sanitize it.

Looking at the 'default' column, I wonder if anyone would be interested
in looking at why the throughput regressed with gcc-7.2 and gcc-8.

   Arnd

Re: [PATCH] crypto: pcrypt - fix freeing pcrypt instances

2017-12-21 Thread Dmitry Vyukov 
On Wed, Dec 20, 2017 at 11:28 PM, Eric Biggers  wrote:
> From: Eric Biggers 
>
> pcrypt is using the old way of freeing instances, where the ->free()
> method specified in the 'struct crypto_template' is passed a pointer to
> the 'struct crypto_instance'.  But the crypto_instance is being
> kfree()'d directly, which is incorrect because the memory was actually
> allocated as an aead_instance, which contains the crypto_instance at a
> nonzero offset.  Thus, the wrong pointer was being kfree()'d.


That's interesting. KASAN does not detect frees of invalid pointers
(e.g. into a middle of an object). It should.
I've requested a component for KASAN in kernel bugzilla to file this
(not sure if anybody is actually reading these emails), and so far
filed it in an internal bug tracker.


> Fix it by switching to the new way to free aead_instance's where the
> ->free() method is specified in the aead_instance itself.
>
> Reported-by: syzbot 
> Fixes: 0496f56065e0 ("crypto: pcrypt - Add support for new AEAD interface")
> Cc:  # v4.2+
> Signed-off-by: Eric Biggers 
> ---
>  crypto/pcrypt.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
> index ee9cfb99fe25..f8ec3d4ba4a8 100644
> --- a/crypto/pcrypt.c
> +++ b/crypto/pcrypt.c
> @@ -254,6 +254,14 @@ static void pcrypt_aead_exit_tfm(struct crypto_aead *tfm)
> crypto_free_aead(ctx->child);
>  }
>
> +static void pcrypt_free(struct aead_instance *inst)
> +{
> +   struct pcrypt_instance_ctx *ctx = aead_instance_ctx(inst);
> +
> +   crypto_drop_aead(>spawn);
> +   kfree(inst);
> +}
> +
>  static int pcrypt_init_instance(struct crypto_instance *inst,
> struct crypto_alg *alg)
>  {
> @@ -319,6 +327,8 @@ static int pcrypt_create_aead(struct crypto_template 
> *tmpl, struct rtattr **tb,
> inst->alg.encrypt = pcrypt_aead_encrypt;
> inst->alg.decrypt = pcrypt_aead_decrypt;
>
> +   inst->free = pcrypt_free;
> +
> err = aead_register_instance(tmpl, inst);
> if (err)
> goto out_drop_aead;
> @@ -349,14 +359,6 @@ static int pcrypt_create(struct crypto_template *tmpl, 
> struct rtattr **tb)
> return -EINVAL;
>  }
>
> -static void pcrypt_free(struct crypto_instance *inst)
> -{
> -   struct pcrypt_instance_ctx *ctx = crypto_instance_ctx(inst);
> -
> -   crypto_drop_aead(>spawn);
> -   kfree(inst);
> -}
> -
>  static int pcrypt_cpumask_change_notify(struct notifier_block *self,
> unsigned long val, void *data)
>  {
> @@ -469,7 +471,6 @@ static void pcrypt_fini_padata(struct padata_pcrypt 
> *pcrypt)
>  static struct crypto_template pcrypt_tmpl = {
> .name = "pcrypt",
> .create = pcrypt_create,
> -   .free = pcrypt_free,
> .module = THIS_MODULE,
>  };
>
> --
> 2.15.1.620.gb9897f4670-goog
>
> --
> You received this message because you are subscribed to the Google Groups 
> "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to syzkaller-bugs+unsubscr...@googlegroups.com.
> To view this discussion on the web visit 
> https://groups.google.com/d/msgid/syzkaller-bugs/20171220222825.207321-1-ebiggers3%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.