20 Dec, 2019

1 commit

  • Some of the algorithm unregistration functions return -ENOENT when asked
    to unregister a non-registered algorithm, while others always return 0
    or always return void. But no users check the return value, except for
    two of the bulk unregistration functions which print a message on error
    but still always return 0 to their caller, and crypto_del_alg() which
    calls crypto_unregister_instance() which always returns 0.

    Since unregistering a non-registered algorithm is always a kernel bug
    but there isn't anything callers should do to handle this situation at
    runtime, let's simplify things by making all the unregistration
    functions return void, and moving the error message into
    crypto_unregister_alg() and upgrading it to a WARN().

    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     

10 Oct, 2019

1 commit


26 Jul, 2019

1 commit

  • Currently, NETLINK_CRYPTO works only in the init network namespace. It
    doesn't make much sense to cut it out of the other network namespaces,
    so do the minor plumbing work necessary to make it work in any network
    namespace. Code inspired by net/core/sock_diag.c.

    Tested using kcapi-dgst from libkcapi [1]:
    Before:
    # unshare -n kcapi-dgst -c sha256
    Signed-off-by: Herbert Xu

    Ondrej Mosnacek
     

05 Jul, 2019

1 commit


03 Jul, 2019

1 commit

  • 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
    (though it doesn't look like the other cases can cause a crash).

    Fix this by making crypto_alg_match() exclude larval algorithms.

    [1] https://lkml.kernel.org/r/20190625071624.27039-1-msuchanek@suse.de
    [2] https://github.com/linux-test-project/ltp/blob/20190517/testcases/kernel/crypto/pcrypt_aead01.c

    Reported-by: Michal Suchanek
    Fixes: a38f7907b926 ("crypto: Add userspace configuration API")
    Cc: # v3.2+
    Cc: Steffen Klassert
    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     

05 Jun, 2019

1 commit

  • Based on 1 normalized pattern(s):

    this program is free software you can redistribute it and or modify
    it under the terms and conditions of the gnu general public license
    version 2 as published by the free software foundation this program
    is distributed in the hope it will be useful but without 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

    extracted by the scancode license scanner the SPDX license identifier

    GPL-2.0-only

    has been chosen to replace the boilerplate/reference in 111 file(s).

    Signed-off-by: Thomas Gleixner
    Reviewed-by: Alexios Zavras
    Reviewed-by: Allison Randal
    Cc: linux-spdx@vger.kernel.org
    Link: https://lkml.kernel.org/r/20190530000436.567572064@linutronix.de
    Signed-off-by: Greg Kroah-Hartman

    Thomas Gleixner
     

28 Apr, 2019

1 commit

  • We currently have two levels of strict validation:

    1) liberal (default)
    - undefined (type >= max) & NLA_UNSPEC attributes accepted
    - attribute length >= expected accepted
    - garbage at end of message accepted
    2) strict (opt-in)
    - NLA_UNSPEC attributes accepted
    - attribute length >= expected accepted

    Split out parsing strictness into four different options:
    * TRAILING - check that there's no trailing data after parsing
    attributes (in message or nested)
    * MAXTYPE - reject attrs > max known type
    * UNSPEC - reject attributes with NLA_UNSPEC policy entries
    * STRICT_ATTRS - strictly validate attribute size

    The default for future things should be *everything*.
    The current *_strict() is a combination of TRAILING and MAXTYPE,
    and is renamed to _deprecated_strict().
    The current regular parsing has none of this, and is renamed to
    *_parse_deprecated().

    Additionally it allows us to selectively set one of the new flags
    even on old policies. Notably, the UNSPEC flag could be useful in
    this case, since it can be arranged (by filling in the policy) to
    not be an incompatible userspace ABI change, but would then going
    forward prevent forgetting attribute entries. Similar can apply
    to the POLICY flag.

    We end up with the following renames:
    * nla_parse -> nla_parse_deprecated
    * nla_parse_strict -> nla_parse_deprecated_strict
    * nlmsg_parse -> nlmsg_parse_deprecated
    * nlmsg_parse_strict -> nlmsg_parse_deprecated_strict
    * nla_parse_nested -> nla_parse_nested_deprecated
    * nla_validate_nested -> nla_validate_nested_deprecated

    Using spatch, of course:
    @@
    expression TB, MAX, HEAD, LEN, POL, EXT;
    @@
    -nla_parse(TB, MAX, HEAD, LEN, POL, EXT)
    +nla_parse_deprecated(TB, MAX, HEAD, LEN, POL, EXT)

    @@
    expression NLH, HDRLEN, TB, MAX, POL, EXT;
    @@
    -nlmsg_parse(NLH, HDRLEN, TB, MAX, POL, EXT)
    +nlmsg_parse_deprecated(NLH, HDRLEN, TB, MAX, POL, EXT)

    @@
    expression NLH, HDRLEN, TB, MAX, POL, EXT;
    @@
    -nlmsg_parse_strict(NLH, HDRLEN, TB, MAX, POL, EXT)
    +nlmsg_parse_deprecated_strict(NLH, HDRLEN, TB, MAX, POL, EXT)

    @@
    expression TB, MAX, NLA, POL, EXT;
    @@
    -nla_parse_nested(TB, MAX, NLA, POL, EXT)
    +nla_parse_nested_deprecated(TB, MAX, NLA, POL, EXT)

    @@
    expression START, MAX, POL, EXT;
    @@
    -nla_validate_nested(START, MAX, POL, EXT)
    +nla_validate_nested_deprecated(START, MAX, POL, EXT)

    @@
    expression NLH, HDRLEN, MAX, POL, EXT;
    @@
    -nlmsg_validate(NLH, HDRLEN, MAX, POL, EXT)
    +nlmsg_validate_deprecated(NLH, HDRLEN, MAX, POL, EXT)

    For this patch, don't actually add the strict, non-renamed versions
    yet so that it breaks compile if I get it wrong.

    Also, while at it, make nla_validate and nla_parse go down to a
    common __nla_validate_parse() function to avoid code duplication.

    Ultimately, this allows us to have very strict validation for every
    new caller of nla_parse()/nlmsg_parse() etc as re-introduced in the
    next patch, while existing things will continue to work as is.

    In effect then, this adds fully strict validation for any new command.

    Signed-off-by: Johannes Berg
    Signed-off-by: David S. Miller

    Johannes Berg
     

23 Dec, 2018

1 commit


13 Dec, 2018

1 commit

  • CRYPTO_MSG_GETALG in NLM_F_DUMP mode sometimes doesn't return all
    registered crypto algorithms, because it doesn't support incremental
    dumps. crypto_dump_report() only permits itself to be called once, yet
    the netlink subsystem allocates at most ~64 KiB for the skb being dumped
    to. Thus only the first recvmsg() returns data, and it may only include
    a subset of the crypto algorithms even if the user buffer passed to
    recvmsg() is large enough to hold all of them.

    Fix this by using one of the arguments in the netlink_callback structure
    to keep track of the current position in the algorithm list. Then
    userspace can do multiple recvmsg() on the socket after sending the dump
    request. This is the way netlink dumps work elsewhere in the kernel;
    it's unclear why this was different (probably just an oversight).

    Also fix an integer overflow when calculating the dump buffer size hint.

    Fixes: a38f7907b926 ("crypto: Add userspace configuration API")
    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     

09 Nov, 2018

3 commits

  • There have been a pretty ridiculous number of issues with initializing
    the report structures that are copied to userspace by NETLINK_CRYPTO.
    Commit 4473710df1f8 ("crypto: user - Prepare for CRYPTO_MAX_ALG_NAME
    expansion") replaced some strncpy()s with strlcpy()s, thereby
    introducing information leaks. Later two other people tried to replace
    other strncpy()s with strlcpy() too, which would have introduced even
    more information leaks:

    - https://lore.kernel.org/patchwork/patch/954991/
    - https://patchwork.kernel.org/patch/10434351/

    Commit cac5818c25d0 ("crypto: user - Implement a generic crypto
    statistics") also uses the buggy strlcpy() approach and therefore leaks
    uninitialized memory to userspace. A fix was proposed, but it was
    originally incomplete.

    Seeing as how apparently no one can get this right with the current
    approach, change all the reporting functions to:

    - Start by memsetting the report structure to 0. This guarantees it's
    always initialized, regardless of what happens later.
    - Initialize all strings using strscpy(). This is safe after the
    memset, ensures null termination of long strings, avoids unnecessary
    work, and avoids the -Wstringop-truncation warnings from gcc.
    - Use sizeof(var) instead of sizeof(type). This is more robust against
    copy+paste errors.

    For simplicity, also reuse the -EMSGSIZE return value from nla_put().

    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     
  • The acomp, akcipher, and kpp algorithm types already have .report
    methods defined, so there's no need to duplicate this functionality in
    crypto_user itself; the duplicate functions are actually never executed.
    Remove the unused code.

    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     
  • All bytes of the NETLINK_CRYPTO report structures must be initialized,
    since they are copied to userspace. The change from strncpy() to
    strlcpy() broke this. As a minimal fix, change it back.

    Fixes: 4473710df1f8 ("crypto: user - Prepare for CRYPTO_MAX_ALG_NAME expansion")
    Cc: # v4.12+
    Signed-off-by: Eric Biggers
    Signed-off-by: Herbert Xu

    Eric Biggers
     

28 Sep, 2018

1 commit