Skip to content
Snippets Groups Projects
  1. Mar 08, 2019
  2. Feb 28, 2019
  3. Feb 22, 2019
  4. Feb 18, 2019
    • Mao Wenan's avatar
      net: crypto set sk to NULL when af_alg_release. · 9060cb71
      Mao Wenan authored
      
      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
       ? __lock_is_held+0xbc/0x160
       ? __sb_start_write+0x13d/0x2b0
       ? __mnt_want_write+0x19a/0x250
       do_fchownat+0x15c/0x190
       ? __ia32_sys_chmod+0x80/0x80
       ? trace_hardirqs_on_thunk+0x1a/0x1c
       __x64_sys_fchownat+0xbf/0x160
       ? lockdep_hardirqs_on+0x39a/0x5e0
       do_syscall_64+0xc8/0x580
       entry_SYSCALL_64_after_hwframe+0x49/0xbe
      RIP: 0033:0x462589
      Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89
      f7 48 89 d6 48 89
      ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3
      48 c7 c1 bc ff ff
      ff f7 d8 64 89 01 48
      RSP: 002b:00007fb4b2c83c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000104
      RAX: ffffffffffffffda RBX: 000000000072bfa0 RCX: 0000000000462589
      RDX: 0000000000000000 RSI: 00000000200000c0 RDI: 0000000000000007
      RBP: 0000000000000005 R08: 0000000000001000 R09: 0000000000000000
      R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb4b2c846bc
      R13: 00000000004bc733 R14: 00000000006f5138 R15: 00000000ffffffff
      
      Allocated by task 4185:
       kasan_kmalloc+0xa0/0xd0
       __kmalloc+0x14a/0x350
       sk_prot_alloc+0xf6/0x290
       sk_alloc+0x3d/0xc00
       af_alg_accept+0x9e/0x670
       hash_accept+0x4a3/0x650
       __sys_accept4+0x306/0x5c0
       __x64_sys_accept4+0x98/0x100
       do_syscall_64+0xc8/0x580
       entry_SYSCALL_64_after_hwframe+0x49/0xbe
      
      Freed by task 4184:
       __kasan_slab_free+0x12e/0x180
       kfree+0xeb/0x2f0
       __sk_destruct+0x4e6/0x6a0
       sk_destruct+0x48/0x70
       __sk_free+0xa9/0x270
       sk_free+0x2a/0x30
       af_alg_release+0x5c/0x70
       __sock_release+0xd3/0x280
       sock_close+0x1a/0x20
       __fput+0x27f/0x7f0
       task_work_run+0x136/0x1b0
       exit_to_usermode_loop+0x1a7/0x1d0
       do_syscall_64+0x461/0x580
       entry_SYSCALL_64_after_hwframe+0x49/0xbe
      
      Syzkaller reproducer:
      r0 = perf_event_open(&(0x7f0000000000)={0x0, 0x70, 0x0, 0x0, 0x0, 0x0,
      0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
      0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
      0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, @perf_config_ext}, 0x0, 0x0,
      0xffffffffffffffff, 0x0)
      r1 = socket$alg(0x26, 0x5, 0x0)
      getrusage(0x0, 0x0)
      bind(r1, &(0x7f00000001c0)=@alg={0x26, 'hash\x00', 0x0, 0x0,
      'sha256-ssse3\x00'}, 0x80)
      r2 = accept(r1, 0x0, 0x0)
      r3 = accept4$unix(r2, 0x0, 0x0, 0x0)
      r4 = dup3(r3, r0, 0x0)
      fchownat(r4, &(0x7f00000000c0)='\x00', 0x0, 0x0, 0x1000)
      
      Fixes: 6d8c50dc ("socket: close race condition between sock_close() and sockfs_setattr()")
      Signed-off-by: default avatarMao Wenan <maowenan@huawei.com>
      Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
      9060cb71
  5. Feb 15, 2019
  6. Feb 08, 2019
    • Eric Biggers's avatar
      crypto: testmgr - check for aead_request corruption · a6e5ef9b
      Eric Biggers authored
      
      Check that algorithms do not change the aead_request structure, as users
      may rely on submitting the request again (e.g. after copying new data
      into the same source buffer) without reinitializing everything.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      a6e5ef9b
    • Eric Biggers's avatar
      crypto: testmgr - check for skcipher_request corruption · fa353c99
      Eric Biggers authored
      
      Check that algorithms do not change the skcipher_request structure, as
      users may rely on submitting the request again (e.g. after copying new
      data into the same source buffer) without reinitializing everything.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      fa353c99
    • Eric Biggers's avatar
      crypto: testmgr - convert hash testing to use testvec_configs · 4cc2dcf9
      Eric Biggers authored
      
      Convert alg_test_hash() to use the new test framework, adding a list of
      testvec_configs to test by default.  When the extra self-tests are
      enabled, randomly generated testvec_configs are tested as well.
      
      This improves hash test coverage mainly because now all algorithms have
      a variety of data layouts tested, whereas before each algorithm was
      responsible for declaring its own chunked test cases which were often
      missing or provided poor test coverage.  The new code also tests both
      the MAY_SLEEP and !MAY_SLEEP cases and buffers that cross pages.
      
      This already found bugs in the hash walk code and in the arm32 and arm64
      implementations of crct10dif.
      
      I removed the hash chunked test vectors that were the same as
      non-chunked ones, but left the ones that were unique.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      4cc2dcf9
    • Eric Biggers's avatar
      crypto: testmgr - convert aead testing to use testvec_configs · ed96804f
      Eric Biggers authored
      
      Convert alg_test_aead() to use the new test framework, using the same
      list of testvec_configs that skcipher testing uses.
      
      This significantly improves AEAD test coverage mainly because previously
      there was only very limited test coverage of the possible data layouts.
      Now the data layouts to test are listed in one place for all algorithms
      and optionally are also randomly generated.  In fact, only one AEAD
      algorithm (AES-GCM) even had a chunked test case before.
      
      This already found bugs in all the AEGIS and MORUS implementations, the
      x86 AES-GCM implementation, and the arm64 AES-CCM implementation.
      
      I removed the AEAD chunked test vectors that were the same as
      non-chunked ones, but left the ones that were unique.
      
      Note: the rewritten test code allocates an aead_request just once per
      algorithm rather than once per encryption/decryption, but some AEAD
      algorithms incorrectly change the tfm pointer in the request.  It's
      nontrivial to fix these, so to move forward I'm temporarily working
      around it by resetting the tfm pointer.  But they'll need to be fixed.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      ed96804f
    • Eric Biggers's avatar
      crypto: testmgr - convert skcipher testing to use testvec_configs · 4e7babba
      Eric Biggers authored
      
      Convert alg_test_skcipher() to use the new test framework, adding a list
      of testvec_configs to test by default.  When the extra self-tests are
      enabled, randomly generated testvec_configs are tested as well.
      
      This improves skcipher test coverage mainly because now all algorithms
      have a variety of data layouts tested, whereas before each algorithm was
      responsible for declaring its own chunked test cases which were often
      missing or provided poor test coverage.  The new code also tests both
      the MAY_SLEEP and !MAY_SLEEP cases, different IV alignments, and buffers
      that cross pages.
      
      This has already found a bug in the arm64 ctr-aes-neonbs algorithm.
      It would have easily found many past bugs.
      
      I removed the skcipher chunked test vectors that were the same as
      non-chunked ones, but left the ones that were unique.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      4e7babba
    • Eric Biggers's avatar
      crypto: testmgr - implement random testvec_config generation · 25f9dddb
      Eric Biggers authored
      
      Add functions that generate a random testvec_config, in preparation for
      using it for randomized fuzz tests.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      25f9dddb
    • Eric Biggers's avatar
      crypto: testmgr - introduce CONFIG_CRYPTO_MANAGER_EXTRA_TESTS · 5b2706a4
      Eric Biggers authored
      
      To achieve more comprehensive crypto test coverage, I'd like to add fuzz
      tests that use random data layouts and request flags.
      
      To be most effective these tests should be part of testmgr, so they
      automatically run on every algorithm registered with the crypto API.
      However, they will take much longer to run than the current tests and
      therefore will only really be intended to be run by developers, whereas
      the current tests have a wider audience.
      
      Therefore, add a new kconfig option CONFIG_CRYPTO_MANAGER_EXTRA_TESTS
      that can be set by developers to enable these extra, expensive tests.
      
      Similar to the regular tests, also add a module parameter
      cryptomgr.noextratests to support disabling the tests.
      
      Finally, another module parameter cryptomgr.fuzz_iterations is added to
      control how many iterations the fuzz tests do.  Note: for now setting
      this to 0 will be equivalent to cryptomgr.noextratests=1.  But I opted
      for separate parameters to provide more flexibility to add other types
      of tests under the "extra tests" category in the future.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      5b2706a4
    • Eric Biggers's avatar
      crypto: testmgr - add testvec_config struct and helper functions · 3f47a03d
      Eric Biggers authored
      
      Crypto algorithms must produce the same output for the same input
      regardless of data layout, i.e. how the src and dst scatterlists are
      divided into chunks and how each chunk is aligned.  Request flags such
      as CRYPTO_TFM_REQ_MAY_SLEEP must not affect the result either.
      
      However, testing of this currently has many gaps.  For example,
      individual algorithms are responsible for providing their own chunked
      test vectors.  But many don't bother to do this or test only one or two
      cases, providing poor test coverage.  Also, other things such as
      misaligned IVs and CRYPTO_TFM_REQ_MAY_SLEEP are never tested at all.
      
      Test code is also duplicated between the chunked and non-chunked cases,
      making it difficult to make other improvements.
      
      To improve the situation, this patch series basically moves the chunk
      descriptions into the testmgr itself so that they are shared by all
      algorithms.  However, it's done in an extensible way via a new struct
      'testvec_config', which describes not just the scaled chunk lengths but
      also all other aspects of the crypto operation besides the data itself
      such as the buffer alignments, the request flags, whether the operation
      is in-place or not, the IV alignment, and for hash algorithms when to
      do each update() and when to use finup() vs. final() vs. digest().
      
      Then, this patch series makes skcipher, aead, and hash algorithms be
      tested against a list of default testvec_configs, replacing the current
      test code.  This improves overall test coverage, without reducing test
      performance too much.  Note that the test vectors themselves are not
      changed, except for removing the chunk lists.
      
      This series also adds randomized fuzz tests, enabled by a new kconfig
      option intended for developer use only, where skcipher, aead, and hash
      algorithms are tested against many randomly generated testvec_configs.
      This provides much more comprehensive test coverage.
      
      These improved tests have already exposed many bugs.
      
      To start it off, this initial patch adds the testvec_config and various
      helper functions that will be used by the skcipher, aead, and hash test
      code that will be converted to use the new testvec_config framework.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      3f47a03d
    • Eric Biggers's avatar
      crypto: ahash - fix another early termination in hash walk · 77568e53
      Eric Biggers authored
      
      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>
      77568e53
    • Eric Biggers's avatar
      crypto: morus - fix handling chunked inputs · d644f1c8
      Eric Biggers authored
      
      The generic MORUS implementations all fail the improved AEAD tests
      because they produce the wrong result with some data layouts.  The issue
      is that they assume that if the skcipher_walk API gives 'nbytes' not
      aligned to the walksize (a.k.a. walk.stride), then it is the end of the
      data.  In fact, this can happen before the end.  Fix them.
      
      Fixes: 396be41f ("crypto: morus - Add generic MORUS AEAD implementations")
      Cc: <stable@vger.kernel.org> # v4.18+
      Cc: Ondrej Mosnacek <omosnace@redhat.com>
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Reviewed-by: default avatarOndrej Mosnacek <omosnace@redhat.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      d644f1c8
    • Eric Biggers's avatar
      crypto: aegis - fix handling chunked inputs · 0f533e67
      Eric Biggers authored
      
      The generic AEGIS implementations all fail the improved AEAD tests
      because they produce the wrong result with some data layouts.  The issue
      is that they assume that if the skcipher_walk API gives 'nbytes' not
      aligned to the walksize (a.k.a. walk.stride), then it is the end of the
      data.  In fact, this can happen before the end.  Fix them.
      
      Fixes: f606a88e ("crypto: aegis - Add generic AEGIS AEAD implementations")
      Cc: <stable@vger.kernel.org> # v4.18+
      Cc: Ondrej Mosnacek <omosnace@redhat.com>
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Reviewed-by: default avatarOndrej Mosnacek <omosnace@redhat.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      0f533e67
    • Christopher Diaz Riveros's avatar
      crypto: testmgr - use kmemdup · e3d90e52
      Christopher Diaz Riveros authored
      
      Fixes coccinnelle alerts:
      
      /crypto/testmgr.c:2112:13-20: WARNING opportunity for kmemdup
      /crypto/testmgr.c:2130:13-20: WARNING opportunity for kmemdup
      /crypto/testmgr.c:2152:9-16: WARNING opportunity for kmemdup
      
      Signed-off-by: default avatarChristopher Diaz Riveros <chrisadr@gentoo.org>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      e3d90e52
  7. Feb 01, 2019
  8. Jan 25, 2019
  9. Jan 18, 2019
    • Eric Biggers's avatar
      crypto: testmgr - unify the AEAD encryption and decryption test vectors · a0d608ee
      Eric Biggers authored
      
      Currently testmgr has separate encryption and decryption test vectors
      for AEADs.  That's massively redundant, since usually the decryption
      tests are identical to the encryption tests, just with the input/result
      swapped.  And for some algorithms it was forgotten to add decryption
      test vectors, so for them currently only encryption is being tested.
      
      Therefore, eliminate the redundancy by removing the AEAD decryption test
      vectors and updating testmgr to test both AEAD encryption and decryption
      using what used to be the encryption test vectors.  Naming is adjusted
      accordingly: each aead_testvec now has a 'ptext' (plaintext), 'plen'
      (plaintext length), 'ctext' (ciphertext), and 'clen' (ciphertext length)
      instead of an 'input', 'ilen', 'result', and 'rlen'.  "Ciphertext" here
      refers to the full ciphertext, including the authentication tag.
      
      For now the scatterlist divisions are just given for the plaintext
      length, not also the ciphertext length.  For decryption, the last
      scatterlist element is just extended by the authentication tag length.
      
      In total, this removes over 5000 lines from testmgr.h, with no reduction
      in test coverage since prior patches already copied the few unique
      decryption test vectors into the encryption test vectors.
      
      The testmgr.h portion of this patch was automatically generated using
      the following awk script, except that I also manually updated the
      definition of 'struct aead_testvec' and fixed the location of the
      comment describing the AEGIS-128 test vectors.
      
          BEGIN { OTHER = 0; ENCVEC = 1; DECVEC = 2; DECVEC_TAIL = 3; mode = OTHER }
      
          /^static const struct aead_testvec.*_enc_/ { sub("_enc", ""); mode = ENCVEC }
          /^static const struct aead_testvec.*_dec_/ { mode = DECVEC }
          mode == ENCVEC {
              sub(/\.input[[:space:]]*=/,     ".ptext\t=")
              sub(/\.result[[:space:]]*=/,    ".ctext\t=")
              sub(/\.ilen[[:space:]]*=/,      ".plen\t=")
              sub(/\.rlen[[:space:]]*=/,      ".clen\t=")
              print
          }
          mode == DECVEC_TAIL && /[^[:space:]]/ { mode = OTHER }
          mode == OTHER                         { print }
          mode == ENCVEC && /^};/               { mode = OTHER }
          mode == DECVEC && /^};/               { mode = DECVEC_TAIL }
      
      Note that git's default diff algorithm gets confused by the testmgr.h
      portion of this patch, and reports too many lines added and removed.
      It's better viewed with 'git diff --minimal' (or 'git show --minimal'),
      which reports "2 files changed, 1235 insertions(+), 6491 deletions(-)".
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      a0d608ee
    • Eric Biggers's avatar
      crypto: testmgr - add rfc4543(gcm(aes)) decryption test to encryption tests · d7250b41
      Eric Biggers authored
      
      One "rfc4543(gcm(aes))" decryption test vector doesn't exactly match any of the
      encryption test vectors with input and result swapped.  In preparation
      for removing the AEAD decryption test vectors and testing AEAD
      decryption using the encryption test vectors, add this to the encryption
      test vectors, so we don't lose any test coverage.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      d7250b41
    • Eric Biggers's avatar
      crypto: testmgr - add gcm(aes) decryption tests to encryption tests · f38e8885
      Eric Biggers authored
      
      Some "gcm(aes)" decryption test vectors don't exactly match any of the
      encryption test vectors with input and result swapped.  In preparation
      for removing the AEAD decryption test vectors and testing AEAD
      decryption using the encryption test vectors, add these to the
      encryption test vectors, so we don't lose any test coverage.
      
      In the case of the chunked test vector, I truncated the last scatterlist
      element to the end of the plaintext.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      f38e8885
    • Eric Biggers's avatar
      crypto: testmgr - add ccm(aes) decryption tests to encryption tests · de845da9
      Eric Biggers authored
      
      Some "ccm(aes)" decryption test vectors don't exactly match any of the
      encryption test vectors with input and result swapped.  In preparation
      for removing the AEAD decryption test vectors and testing AEAD
      decryption using the encryption test vectors, add these to the
      encryption test vectors, so we don't lose any test coverage.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      de845da9
    • Eric Biggers's avatar
      crypto: testmgr - skip AEAD encryption test vectors with novrfy set · 5bc3de58
      Eric Biggers authored
      
      In preparation for unifying the AEAD encryption and decryption test
      vectors, skip AEAD test vectors with the 'novrfy' (verification failure
      expected) flag set when testing encryption rather than decryption.
      These test vectors only make sense for decryption.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      5bc3de58
    • Eric Biggers's avatar
      crypto: af_alg - remove redundant initializations of sk_family · 6d0d6cfb
      Eric Biggers authored
      
      sk_alloc() already sets sock::sk_family to PF_ALG which is passed as the
      'family' argument, so there's no need to set it again.
      
      Signed-off-by: default avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: default avatarHerbert Xu <herbert@gondor.apana.org.au>
      6d0d6cfb
Loading