1. 10 Jul, 2019 2 commits
    • Vincent Whitchurch's avatar
      crypto: cryptd - Fix skcipher instance memory leak · a86c45d2
      Vincent Whitchurch authored
      commit 1a0fad63 upstream.
      
      cryptd_skcipher_free() fails to free the struct skcipher_instance
      allocated in cryptd_create_skcipher(), leading to a memory leak.  This
      is detected by kmemleak on bootup on ARM64 platforms:
      
       unreferenced object 0xffff80003377b180 (size 1024):
         comm "cryptomgr_probe", pid 822, jiffies 4294894830 (age 52.760s)
         backtrace:
           kmem_cache_alloc_trace+0x270/0x2d0
           cryptd_create+0x990/0x124c
           cryptomgr_probe+0x5c/0x1e8
           kthread+0x258/0x318
           ret_from_fork+0x10/0x1c
      
      Fixes: 4e0958d1
      
       ("crypto: cryptd - Add support for skcipher")
      Cc: <stable@vger.kernel.org>
      Signed-off-by: default avatarVincent Whitchurch <vincent.whitchurch@axis.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      a86c45d2
    • Eric Biggers's avatar
      crypto: user - prevent operating on larval algorithms · d06037bc
      Eric Biggers authored
      commit 21d4120e upstream.
      
      Michal Suchanek reported [1] that running the pcrypt_aead01 test from
      LTP [2] in a loop and holding Ctrl-C causes a NULL dereference of
      alg->cra_users.next in crypto_remove_spawns(), via crypto_del_alg().
      The test repeatedly uses CRYPTO_MSG_NEWALG and CRYPTO_MSG_DELALG.
      
      The crash occurs when the instance that CRYPTO_MSG_DELALG is trying to
      unregister isn't a real registered algorithm, but rather is a "test
      larval", which is a special "algorithm" added to the algorithms list
      while the real algorithm is still being tested.  Larvals don't have
      initialized cra_users, so that causes the crash.  Normally pcrypt_aead01
      doesn't trigger this because CRYPTO_MSG_NEWALG waits for the algorithm
      to be tested; however, CRYPTO_MSG_NEWALG returns early when interrupted.
      
      Everything else in the "crypto user configuration" API has this same bug
      too, i.e. it inappropriately allows operating on larval algorithms
      (...
      d06037bc
  2. 21 May, 2019 6 commits
    • Eric Biggers's avatar
      crypto: ccm - fix incompatibility between "ccm" and "ccm_base" · 654a509d
      Eric Biggers authored
      commit 6a1faa4a upstream.
      
      CCM instances can be created by either the "ccm" template, which only
      allows choosing the block cipher, e.g. "ccm(aes)"; or by "ccm_base",
      which allows choosing the ctr and cbcmac implementations, e.g.
      "ccm_base(ctr(aes-generic),cbcmac(aes-generic))".
      
      However, a "ccm_base" instance prevents a "ccm" instance from being
      registered using the same implementations.  Nor will the instance be
      found by lookups of "ccm".  This can be used as a denial of service.
      Moreover, "ccm_base" instances are never tested by the crypto
      self-tests, even if there are compatible "ccm" tests.
      
      The root cause of these problems is that instances of the two templates
      use different cra_names.  Therefore, fix these problems by making
      "ccm_base" instances set the same cra_name as "ccm" instances, e.g.
      "ccm(aes)" instead of "ccm_base(ctr(aes-generic),cbcmac(aes-generic))".
      
      This requires extracting the block cipher n...
      654a509d
    • Eric Biggers's avatar
      crypto: salsa20 - don't access already-freed walk.iv · c4cf8de7
      Eric Biggers authored
      commit edaf28e9 upstream.
      
      If the user-provided IV needs to be aligned to the algorithm's
      alignmask, then skcipher_walk_virt() copies the IV into a new aligned
      buffer walk.iv.  But skcipher_walk_virt() can fail afterwards, and then
      if the caller unconditionally accesses walk.iv, it's a use-after-free.
      
      salsa20-generic doesn't set an alignmask, so currently it isn't affected
      by this despite unconditionally accessing walk.iv.  However this is more
      subtle than desired, and it was actually broken prior to the alignmask
      being removed by commit b62b3db7 ("crypto: salsa20-generic - cleanup
      and convert to skcipher API").
      
      Since salsa20-generic does not update the IV and does not need any IV
      alignment, update it to use req->iv instead of walk.iv.
      
      Fixes: 2407d608
      
       ("[CRYPTO] salsa20: Salsa20 stream cipher")
      Cc: stable@vger.kernel.org
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: Herbert Xu <herbert@gondor...
      c4cf8de7
    • Eric Biggers's avatar
      crypto: gcm - fix incompatibility between "gcm" and "gcm_base" · b5344a0e
      Eric Biggers authored
      commit f699594d upstream.
      
      GCM instances can be created by either the "gcm" template, which only
      allows choosing the block cipher, e.g. "gcm(aes)"; or by "gcm_base",
      which allows choosing the ctr and ghash implementations, e.g.
      "gcm_base(ctr(aes-generic),ghash-generic)".
      
      However, a "gcm_base" instance prevents a "gcm" instance from being
      registered using the same implementations.  Nor will the instance be
      found by lookups of "gcm".  This can be used as a denial of service.
      Moreover, "gcm_base" instances are never tested by the crypto
      self-tests, even if there are compatible "gcm" tests.
      
      The root cause of these problems is that instances of the two templates
      use different cra_names.  Therefore, fix these problems by making
      "gcm_base" instances set the same cra_name as "gcm" instances, e.g.
      "gcm(aes)" instead of "gcm_base(ctr(aes-generic),ghash-generic)".
      
      This requires extracting the block cipher name from the ...
      b5344a0e
    • Eric Biggers's avatar
      crypto: crct10dif-generic - fix use via crypto_shash_digest() · e4bd7d1d
      Eric Biggers authored
      commit 307508d1 upstream.
      
      The ->digest() method of crct10dif-generic reads the current CRC value
      from the shash_desc context.  But this value is uninitialized, causing
      crypto_shash_digest() to compute the wrong result.  Fix it.
      
      Probably this wasn't noticed before because lib/crc-t10dif.c only uses
      crypto_shash_update(), not crypto_shash_digest().  Likewise,
      crypto_shash_digest() is not yet tested by the crypto self-tests because
      those only test the ahash API which only uses shash init/update/final.
      
      This bug was detected by my patches that improve testmgr to fuzz
      algorithms against their generic implementation.
      
      Fixes: 2d31e518
      
       ("crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework")
      Cc: <stable@vger.kernel.org> # v3.11+
      Cc: Tim Chen <tim.c.chen@linux.intel.com>
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-...
      e4bd7d1d
    • Eric Biggers's avatar
      crypto: skcipher - don't WARN on unprocessed data after slow walk step · d418ee2c
      Eric Biggers authored
      commit dcaca01a upstream.
      
      skcipher_walk_done() assumes it's a bug if, after the "slow" path is
      executed where the next chunk of data is processed via a bounce buffer,
      the algorithm says it didn't process all bytes.  Thus it WARNs on this.
      
      However, this can happen legitimately when the message needs to be
      evenly divisible into "blocks" but isn't, and the algorithm has a
      'walksize' greater than the block size.  For example, ecb-aes-neonbs
      sets 'walksize' to 128 bytes and only supports messages evenly divisible
      into 16-byte blocks.  If, say, 17 message bytes remain but they straddle
      scatterlist elements, the skcipher_walk code will take the "slow" path
      and pass the algorithm all 17 bytes in the bounce buffer.  But the
      algorithm will only be able to process 16 bytes, triggering the WARN.
      
      Fix this by just removing the WARN_ON().  Returning -EINVAL, as the code
      already does, is the right behavior.
      
      This bug...
      d418ee2c
    • Eric Biggers's avatar
      crypto: chacha20poly1305 - set cra_name correctly · 559c1730
      Eric Biggers authored
      commit 5e27f38f upstream.
      
      If the rfc7539 template is instantiated with specific implementations,
      e.g. "rfc7539(chacha20-generic,poly1305-generic)" rather than
      "rfc7539(chacha20,poly1305)", then the implementation names end up
      included in the instance's cra_name.  This is incorrect because it then
      prevents all users from allocating "rfc7539(chacha20,poly1305)", if the
      highest priority implementations of chacha20 and poly1305 were selected.
      Also, the self-tests aren't run on an instance allocated in this way.
      
      Fix it by setting the instance's cra_name from the underlying
      algorithms' actual cra_names, rather than from the requested names.
      This matches what other templates do.
      
      Fixes: 71ebc4d1
      
       ("crypto: chacha20poly1305 - Add a ChaCha20-Poly1305 AEAD construction, RFC7539")
      Cc: <stable@vger.kernel.org> # v4.2+
      Cc: Martin Willi <martin@strongswan.org>
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Reviewed-by: Martin Will...
      559c1730
  3. 27 Apr, 2019 1 commit
    • Eric Biggers's avatar
      crypto: x86/poly1305 - fix overflow during partial reduction · 4de25ac0
      Eric Biggers authored
      commit 678cce40 upstream.
      
      The x86_64 implementation of Poly1305 produces the wrong result on some
      inputs because poly1305_4block_avx2() incorrectly assumes that when
      partially reducing the accumulator, the bits carried from limb 'd4' to
      limb 'h0' fit in a 32-bit integer.  This is true for poly1305-generic
      which processes only one block at a time.  However, it's not true for
      the AVX2 implementation, which processes 4 blocks at a time and
      therefore can produce intermediate limbs about 4x larger.
      
      Fix it by making the relevant calculations use 64-bit arithmetic rather
      than 32-bit.  Note that most of the carries already used 64-bit
      arithmetic, but the d4 -> h0 carry was different for some reason.
      
      To be safe I also made the same change to the corresponding SSE2 code,
      though that only operates on 1 or 2 blocks at a time.  I don't think
      it's really needed for poly1305_block_sse2(), but it doesn't hurt
      because it's alre...
      4de25ac0
  4. 23 Mar, 2019 4 commits
    • Eric Biggers's avatar
      crypto: pcbc - remove bogus memcpy()s with src == dest · 05c02839
      Eric Biggers authored
      commit 251b7aea upstream.
      
      The memcpy()s in the PCBC implementation use walk->iv as both the source
      and destination, which has undefined behavior.  These memcpy()'s are
      actually unneeded, because walk->iv is already used to hold the previous
      plaintext block XOR'd with the previous ciphertext block.  Thus,
      walk->iv is already updated to its final value.
      
      So remove the broken and unnecessary memcpy()s.
      
      Fixes: 91652be5
      
       ("[CRYPTO] pcbc: Add Propagated CBC template")
      Cc: <stable@vger.kernel.org> # v2.6.21+
      Cc: David Howells <dhowells@redhat.com>
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarMaxim Zhukov <mussitantesmortem@gmail.com>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      05c02839
    • Eric Biggers's avatar
      crypto: testmgr - skip crc32c context test for ahash algorithms · 29ef5d0f
      Eric Biggers authored
      commit eb5e6730 upstream.
      
      Instantiating "cryptd(crc32c)" causes a crypto self-test failure because
      the crypto_alloc_shash() in alg_test_crc32c() fails.  This is because
      cryptd(crc32c) is an ahash algorithm, not a shash algorithm; so it can
      only be accessed through the ahash API, unlike shash algorithms which
      can be accessed through both the ahash and shash APIs.
      
      As the test is testing the shash descriptor format which is only
      applicable to shash algorithms, skip it for ahash algorithms.
      
      (Note that it's still important to fix crypto self-test failures even
       for weird algorithm instantiations like cryptd(crc32c) that no one
       would really use; in fips_enabled mode unprivileged users can use them
       to panic the kernel, and also they prevent treating a crypto self-test
       failure as a bug when fuzzing the kernel.)
      
      Fixes: 8e3ee85e ("crypto: crc32c - Test descriptor context format")
      Cc: stable@vger.kernel.org
      Sig...
      29ef5d0f
    • Eric Biggers's avatar
      crypto: hash - set CRYPTO_TFM_NEED_KEY if ->setkey() fails · 3a48ec70
      Eric Biggers authored
      commit ba7d7433 upstream.
      
      Some algorithms have a ->setkey() method that is not atomic, in the
      sense that setting a key can fail after changes were already made to the
      tfm context.  In this case, if a key was already set the tfm can end up
      in a state that corresponds to neither the old key nor the new key.
      
      It's not feasible to make all ->setkey() methods atomic, especially ones
      that have to key multiple sub-tfms.  Therefore, make the crypto API set
      CRYPTO_TFM_NEED_KEY if ->setkey() fails and the algorithm requires a
      key, to prevent the tfm from being used until a new key is set.
      
      Note: we can't set CRYPTO_TFM_NEED_KEY for OPTIONAL_KEY algorithms, so
      ->setkey() for those must nevertheless be atomic.  That's fine for now
      since only the crc32 and crc32c algorithms set OPTIONAL_KEY, and it's
      not intended that OPTIONAL_KEY be used much.
      
      [Cc stable mainly because when introducing the NEED_KEY flag I changed
       AF_ALG to rely on it; and unlike in-kernel crypto API users, AF_ALG
       previously didn't have this problem.  So these "incompletely keyed"
       states became theoretically accessible via AF_ALG -- though, the
       opportunities for causing real mischief seem pretty limited.]
      
      Fixes: 9fa68f62
      
       ("crypto: hash - prevent using keyed hashes without setting key")
      Cc: stable@vger.kernel.org
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      3a48ec70
    • Eric Biggers's avatar
      crypto: ahash - fix another early termination in hash walk · 09024fed
      Eric Biggers authored
      commit 77568e53 upstream.
      
      Hash algorithms with an alignmask set, e.g. "xcbc(aes-aesni)" and
      "michael_mic", fail the improved hash tests because they sometimes
      produce the wrong digest.  The bug is that in the case where a
      scatterlist element crosses pages, not all the data is actually hashed
      because the scatterlist walk terminates too early.  This happens because
      the 'nbytes' variable in crypto_hash_walk_done() is assigned the number
      of bytes remaining in the page, then later interpreted as the number of
      bytes remaining in the scatterlist element.  Fix it.
      
      Fixes: 900a081f
      
       ("crypto: ahash - Fix early termination in hash walk")
      Cc: stable@vger.kernel.org
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      09024fed
  5. 23 Feb, 2019 1 commit
    • Mao Wenan's avatar
      net: crypto set sk to NULL when af_alg_release. · 6e4c01ee
      Mao Wenan authored
      [ Upstream commit 9060cb71 ]
      
      KASAN has found use-after-free in sockfs_setattr.
      The existed commit 6d8c50dc ("socket: close race condition between sock_close()
      and sockfs_setattr()") is to fix this simillar issue, but it seems to ignore
      that crypto module forgets to set the sk to NULL after af_alg_release.
      
      KASAN report details as below:
      BUG: KASAN: use-after-free in sockfs_setattr+0x120/0x150
      Write of size 4 at addr ffff88837b956128 by task syz-executor0/4186
      
      CPU: 2 PID: 4186 Comm: syz-executor0 Not tainted xxx + #1
      Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
      1.10.2-1ubuntu1 04/01/2014
      Call Trace:
       dump_stack+0xca/0x13e
       print_address_description+0x79/0x330
       ? vprintk_func+0x5e/0xf0
       kasan_report+0x18a/0x2e0
       ? sockfs_setattr+0x120/0x150
       sockfs_setattr+0x120/0x150
       ? sock_register+0x2d0/0x2d0
       notify_change+0x90c/0xd40
       ? chown_common+0x2ef/0x510
       chown_common+0x2ef/0x510
       ? chmod_common+0x3b0/0x3b0
       ? _...
      6e4c01ee
  6. 12 Feb, 2019 1 commit
    • Eric Biggers's avatar
      crypto: aes_ti - disable interrupts while accessing S-box · e867d756
      Eric Biggers authored
      [ Upstream commit 0a6a40c2 ]
      
      In the "aes-fixed-time" AES implementation, disable interrupts while
      accessing the S-box, in order to make cache-timing attacks more
      difficult.  Previously it was possible for the CPU to be interrupted
      while the S-box was loaded into L1 cache, potentially evicting the
      cachelines and causing later table lookups to be time-variant.
      
      In tests I did on x86 and ARM, this doesn't affect performance
      significantly.  Responsiveness is potentially a concern, but interrupts
      are only disabled for a single AES block.
      
      Note that even after this change, the implementation still isn't
      necessarily guaranteed to be constant-time; see
      https://cr.yp.to/antiforgery/cachetiming-20050414.pdf
      
       for a discussion
      of the many difficulties involved in writing truly constant-time AES
      software.  But it's valuable to make such attacks more difficult.
      Reviewed-by: default avatarArd Biesheuvel <ard.biesheuvel@linaro.org>
      Signed-off-by...
      e867d756
  7. 23 Jan, 2019 2 commits
    • Eric Biggers's avatar
      crypto: authenc - fix parsing key with misaligned rta_len · b9119fd2
      Eric Biggers authored
      commit 8f9c4693 upstream.
      
      Keys for "authenc" AEADs are formatted as an rtattr containing a 4-byte
      'enckeylen', followed by an authentication key and an encryption key.
      crypto_authenc_extractkeys() parses the key to find the inner keys.
      
      However, it fails to consider the case where the rtattr's payload is
      longer than 4 bytes but not 4-byte aligned, and where the key ends
      before the next 4-byte aligned boundary.  In this case, 'keylen -=
      RTA_ALIGN(rta->rta_len);' underflows to a value near UINT_MAX.  This
      causes a buffer overread and crash during crypto_ahash_setkey().
      
      Fix it by restricting the rtattr payload to the expected size.
      
      Reproducer using AF_ALG:
      
      	#include <linux/if_alg.h>
      	#include <linux/rtnetlink.h>
      	#include <sys/socket.h>
      
      	int main()
      	{
      		int fd;
      		struct sockaddr_alg addr = {
      			.salg_type = "aead",
      			.salg_name = "authenc(hmac(sha256),cbc(aes))",
      		};
      		struct {
      			struct rtattr attr;
      			__be32 en...
      b9119fd2
    • Harsh Jain's avatar
      crypto: authencesn - Avoid twice completion call in decrypt path · d196d2fd
      Harsh Jain authored
      commit a7773363 upstream.
      
      Authencesn template in decrypt path unconditionally calls aead_request_complete
      after ahash_verify which leads to following kernel panic in after decryption.
      
      [  338.539800] BUG: unable to handle kernel NULL pointer dereference at 0000000000000004
      [  338.548372] PGD 0 P4D 0
      [  338.551157] Oops: 0000 [#1] SMP PTI
      [  338.554919] CPU: 0 PID: 0 Comm: swapper/0 Kdump: loaded Tainted: G        W I       4.19.7+ #13
      [  338.564431] Hardware name: Supermicro X8ST3/X8ST3, BIOS 2.0        07/29/10
      [  338.572212] RIP: 0010:esp_input_done2+0x350/0x410 [esp4]
      [  338.578030] Code: ff 0f b6 68 10 48 8b 83 c8 00 00 00 e9 8e fe ff ff 8b 04 25 04 00 00 00 83 e8 01 48 98 48 8b 3c c5 10 00 00 00 e9 f7 fd ff ff <8b> 04 25 04 00 00 00 83 e8 01 48 98 4c 8b 24 c5 10 00 00 00 e9 3b
      [  338.598547] RSP: 0018:ffff911c97803c00 EFLAGS: 00010246
      [  338.604268] RAX: 0000000000000002 RBX: ffff911c4469ee00 RCX: 000000...
      d196d2fd
  8. 01 Dec, 2018 1 commit
  9. 21 Nov, 2018 1 commit
  10. 13 Nov, 2018 2 commits
  11. 04 Oct, 2018 1 commit
    • Stafford Horne's avatar
      crypto: skcipher - Fix -Wstringop-truncation warnings · 29db2772
      Stafford Horne authored
      [ Upstream commit cefd769f
      
       ]
      
      As of GCC 9.0.0 the build is reporting warnings like:
      
          crypto/ablkcipher.c: In function ‘crypto_ablkcipher_report’:
          crypto/ablkcipher.c:374:2: warning: ‘strncpy’ specified bound 64 equals destination size [-Wstringop-truncation]
            strncpy(rblkcipher.geniv, alg->cra_ablkcipher.geniv ?: "<default>",
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
             sizeof(rblkcipher.geniv));
             ~~~~~~~~~~~~~~~~~~~~~~~~~
      
      This means the strnycpy might create a non null terminated string.  Fix this by
      explicitly performing '\0' termination.
      
      Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
      Cc: Arnd Bergmann <arnd@arndb.de>
      Cc: Max Filippov <jcmvbkbc@gmail.com>
      Cc: Eric Biggers <ebiggers3@gmail.com>
      Cc: Nick Desaulniers <nick.desaulniers@gmail.com>
      Signed-off-by: default avatarStafford Horne <shorne@gmail.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Sign...
      29db2772
  12. 26 Sep, 2018 1 commit
  13. 19 Sep, 2018 1 commit
    • Arnd Bergmann's avatar
      crypto: aes-generic - fix aes-generic regression on powerpc · 64def6f3
      Arnd Bergmann authored
      commit 6e36719f
      
       upstream.
      
      My last bugfix added -Os on the command line, which unfortunately caused
      a build regression on powerpc in some configurations.
      
      I've done some more analysis of the original problem and found slightly
      different workaround that avoids this regression and also results in
      better performance on gcc-7.0: -fcode-hoisting is an optimization step
      that got added in gcc-7 and that for all gcc-7 versions causes worse
      performance.
      
      This disables -fcode-hoisting on all compilers that understand the option.
      For gcc-7.1 and 7.2 I found the same performance as my previous patch
      (using -Os), in gcc-7.0 it was even better. On gcc-8 I could see no
      change in performance from this patch. In theory, code hoisting should
      not be able make things better for the AES cipher, so leaving it
      disabled for gcc-8 only serves to simplify the Makefile change.
      Reported-by: default avatarkbuild test robot <fengguang.wu@intel.com>
      Link: ht...
      64def6f3
  14. 09 Sep, 2018 1 commit
  15. 17 Aug, 2018 6 commits
    • Eric Biggers's avatar
      crypto: skcipher - fix crash flushing dcache in error path · 7e179bff
      Eric Biggers authored
      commit 8088d3dd
      
       upstream.
      
      scatterwalk_done() is only meant to be called after a nonzero number of
      bytes have been processed, since scatterwalk_pagedone() will flush the
      dcache of the *previous* page.  But in the error case of
      skcipher_walk_done(), e.g. if the input wasn't an integer number of
      blocks, scatterwalk_done() was actually called after advancing 0 bytes.
      This caused a crash ("BUG: unable to handle kernel paging request")
      during '!PageSlab(page)' on architectures like arm and arm64 that define
      ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
      page-aligned as in that case walk->offset == 0.
      
      Fix it by reorganizing skcipher_walk_done() to skip the
      scatterwalk_advance() and scatterwalk_done() if an error has occurred.
      
      This bug was found by syzkaller fuzzing.
      
      Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
      
      	#include <linux/if_alg.h>
      	#include <sys/socket.h>
      	#include <unistd.h>
      
      	int main()
      	{
      		struct sockaddr_alg addr = {
      			.salg_type = "skcipher",
      			.salg_name = "cbc(aes-generic)",
      		};
      		char buffer[4096] __attribute__((aligned(4096))) = { 0 };
      		int fd;
      
      		fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
      		bind(fd, (void *)&addr, sizeof(addr));
      		setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
      		fd = accept(fd, NULL, NULL);
      		write(fd, buffer, 15);
      		read(fd, buffer, 15);
      	}
      Reported-by: default avatarLiu Chao <liuchao741@huawei.com>
      Fixes: b286d8b1
      
       ("crypto: skcipher - Add skcipher walk interface")
      Cc: <stable@vger.kernel.org> # v4.10+
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      7e179bff
    • Eric Biggers's avatar
      crypto: skcipher - fix aligning block size in skcipher_copy_iv() · 0f2981ee
      Eric Biggers authored
      commit 0567fc9e upstream.
      
      The ALIGN() macro needs to be passed the alignment, not the alignmask
      (which is the alignment minus 1).
      
      Fixes: b286d8b1
      
       ("crypto: skcipher - Add skcipher walk interface")
      Cc: <stable@vger.kernel.org> # v4.10+
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      0f2981ee
    • Eric Biggers's avatar
      crypto: ablkcipher - fix crash flushing dcache in error path · 68432fd1
      Eric Biggers authored
      commit 318abdfb
      
       upstream.
      
      Like the skcipher_walk and blkcipher_walk cases:
      
      scatterwalk_done() is only meant to be called after a nonzero number of
      bytes have been processed, since scatterwalk_pagedone() will flush the
      dcache of the *previous* page.  But in the error case of
      ablkcipher_walk_done(), e.g. if the input wasn't an integer number of
      blocks, scatterwalk_done() was actually called after advancing 0 bytes.
      This caused a crash ("BUG: unable to handle kernel paging request")
      during '!PageSlab(page)' on architectures like arm and arm64 that define
      ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
      page-aligned as in that case walk->offset == 0.
      
      Fix it by reorganizing ablkcipher_walk_done() to skip the
      scatterwalk_advance() and scatterwalk_done() if an error has occurred.
      Reported-by: default avatarLiu Chao <liuchao741@huawei.com>
      Fixes: bf06099d
      
       ("crypto: skcipher - Add ablkcipher_walk interfaces")
      Cc: <stable@vger.kernel.org> # v2.6.35+
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      68432fd1
    • Eric Biggers's avatar
      crypto: blkcipher - fix crash flushing dcache in error path · 2cde72d9
      Eric Biggers authored
      commit 0868def3
      
       upstream.
      
      Like the skcipher_walk case:
      
      scatterwalk_done() is only meant to be called after a nonzero number of
      bytes have been processed, since scatterwalk_pagedone() will flush the
      dcache of the *previous* page.  But in the error case of
      blkcipher_walk_done(), e.g. if the input wasn't an integer number of
      blocks, scatterwalk_done() was actually called after advancing 0 bytes.
      This caused a crash ("BUG: unable to handle kernel paging request")
      during '!PageSlab(page)' on architectures like arm and arm64 that define
      ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE, provided that the input was
      page-aligned as in that case walk->offset == 0.
      
      Fix it by reorganizing blkcipher_walk_done() to skip the
      scatterwalk_advance() and scatterwalk_done() if an error has occurred.
      
      This bug was found by syzkaller fuzzing.
      
      Reproducer, assuming ARCH_IMPLEMENTS_FLUSH_DCACHE_PAGE:
      
      	#include <linux/if_alg.h>
      	#include <sys/socket.h>
      	#include <unistd.h>
      
      	int main()
      	{
      		struct sockaddr_alg addr = {
      			.salg_type = "skcipher",
      			.salg_name = "ecb(aes-generic)",
      		};
      		char buffer[4096] __attribute__((aligned(4096))) = { 0 };
      		int fd;
      
      		fd = socket(AF_ALG, SOCK_SEQPACKET, 0);
      		bind(fd, (void *)&addr, sizeof(addr));
      		setsockopt(fd, SOL_ALG, ALG_SET_KEY, buffer, 16);
      		fd = accept(fd, NULL, NULL);
      		write(fd, buffer, 15);
      		read(fd, buffer, 15);
      	}
      Reported-by: default avatarLiu Chao <liuchao741@huawei.com>
      Fixes: 5cde0af2
      
       ("[CRYPTO] cipher: Added block cipher type")
      Cc: <stable@vger.kernel.org> # v2.6.19+
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      2cde72d9
    • Eric Biggers's avatar
      crypto: vmac - separate tfm and request context · e7aefb13
      Eric Biggers authored
      commit bb296481 upstream.
      
      syzbot reported a crash in vmac_final() when multiple threads
      concurrently use the same "vmac(aes)" transform through AF_ALG.  The bug
      is pretty fundamental: the VMAC template doesn't separate per-request
      state from per-tfm (per-key) state like the other hash algorithms do,
      but rather stores it all in the tfm context.  That's wrong.
      
      Also, vmac_final() incorrectly zeroes most of the state including the
      derived keys and cached pseudorandom pad.  Therefore, only the first
      VMAC invocation with a given key calculates the correct digest.
      
      Fix these bugs by splitting the per-tfm state from the per-request state
      and using the proper init/update/final sequencing for requests.
      
      Reproducer for the crash:
      
          #include <linux/if_alg.h>
          #include <sys/socket.h>
          #include <unistd.h>
      
          int main()
          {
                  int fd;
                  struct sockaddr_alg addr = {
                          .salg_type = "hash",
        ...
      e7aefb13
    • Eric Biggers's avatar
      crypto: vmac - require a block cipher with 128-bit block size · ef70d145
      Eric Biggers authored
      commit 73bf20ef upstream.
      
      The VMAC template assumes the block cipher has a 128-bit block size, but
      it failed to check for that.  Thus it was possible to instantiate it
      using a 64-bit block size cipher, e.g. "vmac(cast5)", causing
      uninitialized memory to be used.
      
      Add the needed check when instantiating the template.
      
      Fixes: f1939f7c
      
       ("crypto: vmac - New hash algorithm for intel_txt support")
      Cc: <stable@vger.kernel.org> # v2.6.32+
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      ef70d145
  16. 03 Aug, 2018 2 commits
  17. 22 Jul, 2018 1 commit
  18. 17 Jul, 2018 1 commit
    • Eric Biggers's avatar
      crypto: x86/salsa20 - remove x86 salsa20 implementations · 19f39eff
      Eric Biggers authored
      commit b7b73cd5
      
       upstream.
      
      The x86 assembly implementations of Salsa20 use the frame base pointer
      register (%ebp or %rbp), which breaks frame pointer convention and
      breaks stack traces when unwinding from an interrupt in the crypto code.
      Recent (v4.10+) kernels will warn about this, e.g.
      
      WARNING: kernel stack regs at 00000000a8291e69 in syzkaller047086:4677 has bad 'bp' value 000000001077994c
      [...]
      
      But after looking into it, I believe there's very little reason to still
      retain the x86 Salsa20 code.  First, these are *not* vectorized
      (SSE2/SSSE3/AVX2) implementations, which would be needed to get anywhere
      close to the best Salsa20 performance on any remotely modern x86
      processor; they're just regular x86 assembly.  Second, it's still
      unclear that anyone is actually using the kernel's Salsa20 at all,
      especially given that now ChaCha20 is supported too, and with much more
      efficient SSSE3 and AVX2 implementations.  Finally, in benchmarks I did
      on both Intel and AMD processors with both gcc 8.1.0 and gcc 4.9.4, the
      x86_64 salsa20-asm is actually slightly *slower* than salsa20-generic
      (~3% slower on Skylake, ~10% slower on Zen), while the i686 salsa20-asm
      is only slightly faster than salsa20-generic (~15% faster on Skylake,
      ~20% faster on Zen).  The gcc version made little difference.
      
      So, the x86_64 salsa20-asm is pretty clearly useless.  That leaves just
      the i686 salsa20-asm, which based on my tests provides a 15-20% speed
      boost.  But that's without updating the code to not use %ebp.  And given
      the maintenance cost, the small speed difference vs. salsa20-generic,
      the fact that few people still use i686 kernels, the doubt that anyone
      is even using the kernel's Salsa20 at all, and the fact that a SSE2
      implementation would almost certainly be much faster on any remotely
      modern x86 processor yet no one has cared enough to add one yet, I don't
      think it's worthwhile to keep.
      
      Thus, just remove both the x86_64 and i686 salsa20-asm implementations.
      
      Reported-by: syzbot+ffa3a158337bbc01ff09@syzkaller.appspotmail.com
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      19f39eff
  19. 03 Jul, 2018 1 commit
    • Maciej S. Szmigiero's avatar
      X.509: unpack RSA signatureValue field from BIT STRING · af20e4ec
      Maciej S. Szmigiero authored
      commit b65c32ec
      
       upstream.
      
      The signatureValue field of a X.509 certificate is encoded as a BIT STRING.
      For RSA signatures this BIT STRING is of so-called primitive subtype, which
      contains a u8 prefix indicating a count of unused bits in the encoding.
      
      We have to strip this prefix from signature data, just as we already do for
      key data in x509_extract_key_data() function.
      
      This wasn't noticed earlier because this prefix byte is zero for RSA key
      sizes divisible by 8. Since BIT STRING is a big-endian encoding adding zero
      prefixes has no bearing on its value.
      
      The signature length, however was incorrect, which is a problem for RSA
      implementations that need it to be exactly correct (like AMD CCP).
      Signed-off-by: default avatarMaciej S. Szmigiero <mail@maciej.szmigiero.name>
      Fixes: c26fd69f ("X.509: Add a crypto key parser for binary (DER) X.509 certificates")
      Cc: stable@vger.kernel.org
      Signed-off-by: James Morris <james.morris@micros...
      af20e4ec
  20. 30 May, 2018 1 commit
    • Eric Biggers's avatar
      PKCS#7: fix direct verification of SignerInfo signature · e47c1bf9
      Eric Biggers authored
      [ Upstream commit 6459ae38 ]
      
      If none of the certificates in a SignerInfo's certificate chain match a
      trusted key, nor is the last certificate signed by a trusted key, then
      pkcs7_validate_trust_one() tries to check whether the SignerInfo's
      signature was made directly by a trusted key.  But, it actually fails to
      set the 'sig' variable correctly, so it actually verifies the last
      signature seen.  That will only be the SignerInfo's signature if the
      certificate chain is empty; otherwise it will actually be the last
      certificate's signature.
      
      This is not by itself a security problem, since verifying any of the
      certificates in the chain should be sufficient to verify the SignerInfo.
      Still, it's not working as intended so it should be fixed.
      
      Fix it by setting 'sig' correctly for the direct verification case.
      
      Fixes: 757932e6 ("PKCS#7: Handle PKCS#7 messages that contain no X.509 certs")
      Signed-off-by: Eric Biggers <ebigger...
      e47c1bf9
  21. 16 May, 2018 1 commit
  22. 01 May, 2018 1 commit
  23. 12 Apr, 2018 1 commit
    • Arnd Bergmann's avatar
      crypto: aes-generic - build with -Os on gcc-7+ · 7cae67e3
      Arnd Bergmann authored
      
      [ Upstream commit 148b974d ]
      
      While testing other changes, I discovered that gcc-7.2.1 produces badly
      optimized code for aes_encrypt/aes_decrypt. This is especially true when
      CONFIG_UBSAN_SANITIZE_ALL is enabled, where it leads to extremely
      large stack usage that in turn might cause kernel stack overflows:
      
      crypto/aes_generic.c: In function 'aes_encrypt':
      crypto/aes_generic.c:1371:1: warning: the frame size of 4880 bytes is larger than 2048 bytes [-Wframe-larger-than=]
      crypto/aes_generic.c: In function 'aes_decrypt':
      crypto/aes_generic.c:1441:1: warning: the frame size of 4864 bytes is larger than 2048 bytes [-Wframe-larger-than=]
      
      I verified that this problem exists on all architectures that are
      supported by gcc-7.2, though arm64 in particular is less affected than
      the others. I also found that gcc-7.1 and gcc-8 do not show the extreme
      stack usage but still produce worse code than earlier versions for this
      file, apparently because of optimization passes that generally provide
      a substantial improvement in object code quality but understandably fail
      to find any shortcuts in the AES algorithm.
      
      Possible workarounds include
      
      a) disabling -ftree-pre and -ftree-sra optimizations, this was an earlier
         patch I tried, which reliably fixed the stack usage, but caused a
         serious performance regression in some versions, as later testing
         found.
      
      b) disabling UBSAN on this file or all ciphers, as suggested by Ard
         Biesheuvel. This would lead to massively better crypto performance in
         UBSAN-enabled kernels and avoid the stack usage, but there is a concern
         over whether we should exclude arbitrary files from UBSAN at all.
      
      c) Forcing the optimization level in a different way. Similar to a),
         but rather than deselecting specific optimization stages,
         this now uses "gcc -Os" for this file, regardless of the
         CONFIG_CC_OPTIMIZE_FOR_PERFORMANCE/SIZE option. This is a reliable
         workaround for the stack consumption on all architecture, and I've
         retested the performance results now on x86, cycles/byte (lower is
         better) for cbc(aes-generic) with 256 bit keys:
      
      			-O2     -Os
      	gcc-6.3.1	14.9	15.1
      	gcc-7.0.1	14.7	15.3
      	gcc-7.1.1	15.3	14.7
      	gcc-7.2.1	16.8	15.9
      	gcc-8.0.0	15.5	15.6
      
      This implements the option c) by enabling forcing -Os on all compiler
      versions starting with gcc-7.1. As a workaround for PR83356, it would
      only be needed for gcc-7.2+ with UBSAN enabled, but since it also shows
      better performance on gcc-7.1 without UBSAN, it seems appropriate to
      use the faster version here as well.
      
      Side note: during testing, I also played with the AES code in libressl,
      which had a similar performance regression from gcc-6 to gcc-7.2,
      but was three times slower overall. It might be interesting to
      investigate that further and possibly port the Linux implementation
      into that.
      
      Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83356
      Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83651
      
      
      Cc: Richard Biener <rguenther@suse.de>
      Cc: Jakub Jelinek <jakub@gcc.gnu.org>
      Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
      Signed-off-by: default avatarArnd Bergmann <arnd@arndb.de>
      Acked-by: default avatarArd Biesheuvel <ard.biesheuvel@linaro.org>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      Signed-off-by: default avatarSasha Levin <alexander.levin@microsoft.com>
      Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
      7cae67e3