* Re: [PATCH] DH support: add KDF handling support
From: Stephan Mueller @ 2016-07-14 14:19 UTC (permalink / raw)
To: noloader; +Cc: keyrings, linux-crypto
In-Reply-To: <CAH8yC8=GUqm5w069_42SWpjTwcuQxvmCtrAfzX9BnoyW_PEYwA@mail.gmail.com>
Am Donnerstag, 14. Juli 2016, 04:00:57 schrieb Jeffrey Walton:
Hi Jeffrey,
> > Note, as shared secrets potentially post-processed by a KDF usually are
> > again used as key or data encryption keys, they need to be
> > truncated/expanded to a specific length anyway. A KDF inherently provides
> > the truncation support to any arbitrary length. Thus, I would think that
> > the caller needs to provide that length but does not need to truncate the
> > output itself.
>
> As far as I know, there's no reduction in proof that a truncated hash
> is as secure as the non-truncated one. One of the reasons to provide
> the output length as a security parameter is to help avoid truncation
> and related hash output attacks.
>
> Also see Kelsey's work on the subject;
> http://www.google.com/search?q=nist+kelsey+truncated+hash.
I understand that point. However, a KDF is not just a simple hash or
truncation thereof. Furthermore, Kelsey is part of the NIST team that also
developed SP800-108 (the KDF definition). Furthermore, the authors of
SP800-56A (in particular Allen Roginsky who I met personally a number of
times) work closely with Kelsey too.
So, I would not think that applying the standard KDF operation which is
intended to "right-size" the DH output is nothing we should worry about.
I would rather worry about the size of the private key in the DH operation.
The private key should be as small as cryptographically possible (e.g. 224 or
256 bits) instead of half of the DH public key minus one (what TLS does) due
to the reduced number of Sopie-Germain primes that are available in the latter
case.
Ciao
Stephan
^ permalink raw reply
* Re: [PATCH] DH support: add KDF handling support
From: Mat Martineau @ 2016-07-14 23:47 UTC (permalink / raw)
To: Stephan Mueller; +Cc: Mat Martineau, David Howells, keyrings, linux-crypto
In-Reply-To: <1954976.nIU2Zma9Rk@tauon.atsec.com>
On Thu, 14 Jul 2016, Stephan Mueller wrote:
> Am Mittwoch, 13. Juli 2016, 16:17:12 schrieb Mat Martineau:
>
> Hi Mat,
>>
>>> ---8<----
>>>
>>> Add the interface logic to support DH with KDF handling support.
>>>
>>> The dh_compute code now allows the following options:
>>>
>>> - no KDF support / output of raw DH shared secret:
>>> dh_compute <private> <prime> <base>
>>>
>>> - KDF support without "other information" string:
>>> dh_compute <private> <prime> <base> <output length> <KDF type>
>>
>> Why is <output length> needed? Can it be determined from <KDF type>?
>
> The KDF can be considered as a random number generator that is seeded by the
> shared secret. I.e. it can produce arbitrary string lengths. There is no
> default string length for any given KDF.
Makes sense, thanks for the explanation.
> Note, as shared secrets potentially post-processed by a KDF usually are again
> used as key or data encryption keys, they need to be truncated/expanded to a
> specific length anyway. A KDF inherently provides the truncation support to
> any arbitrary length. Thus, I would think that the caller needs to provide
> that length but does not need to truncate the output itself.
>>
>>> - KDF support with "other information string:
>>> dh_compute <private> <prime> <base> <output length> <KDF type> <OI
>>> string>
>>>
>>> The test to verify the code is based on a test vector used for the CAVS
>>> testing of SP800-56A.
>>>
>>> Signed-off-by: Stephan Mueller <smueller@chronox.de>
>>> ---
>>> keyctl.c | 14 +++++-
>>> keyutils.c | 48 ++++++++++++++++++
>>> keyutils.h | 13 +++++
>>> tests/keyctl/dh_compute/valid/runtest.sh | 83
>>> ++++++++++++++++++++++++++++++++ 4 files changed, 156 insertions(+), 2
>>> deletions(-)
>>>
>>> diff --git a/keyctl.c b/keyctl.c
>>> index edb03de..32478b3 100644
>>> --- a/keyctl.c
>>> +++ b/keyctl.c
>>> @@ -1638,14 +1638,24 @@ static void act_keyctl_dh_compute(int argc, char
>>> *argv[])>
>>> char *p;
>>> int ret, sep, col;
>>>
>>> - if (argc != 4)
>>> + if (argc != 4 && argc != 6 && argc != 7)
>>>
>>> format();
>>>
>>> private = get_key_id(argv[1]);
>>> prime = get_key_id(argv[2]);
>>> base = get_key_id(argv[3]);
>>>
>>> - ret = keyctl_dh_compute_alloc(private, prime, base, &buffer);
>>> + if (argc == 4)
>>> + ret = keyctl_dh_compute_alloc(private, prime, base, &buffer);
>>> + else if (argc == 6)
>>> + ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
>>> + argv[5], NULL, &buffer);
>>> + else if (argc == 7)
>>> + ret = keyctl_dh_compute_kdf(private, prime, base, argv[4],
>>> + argv[5], argv[6], &buffer);
>>> + else
>>> + error("dh_compute: unknown number of arguments");
>>> +
>>>
>>> if (ret < 0)
>>>
>>> error("keyctl_dh_compute_alloc");
>>>
>>> diff --git a/keyutils.c b/keyutils.c
>>> index 2a69304..ffdd622 100644
>>> --- a/keyutils.c
>>> +++ b/keyutils.c
>>> @@ -386,6 +386,54 @@ int keyctl_dh_compute_alloc(key_serial_t private,
>>> key_serial_t prime, }
>>>
>>> /*
>>> + * fetch DH computation results processed by a KDF into an
>>> + * allocated buffer
>>> + * - resulting buffer has an extra NUL added to the end
>>> + * - returns count (not including extraneous NUL)
>>> + */
>>> +int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
>>> + key_serial_t base, char *len, char *kdfname,
>>> + char *otherinfo, void **_buffer)
>>
>> All of the other keyctl_* wrappers (without the _alloc) just do the
>> syscall, with some packing/unpacking of structs where needed. The
>> allocation behavior below is only found in the *_alloc functions (in the
>> "utilities" section of keyutils.h) - I think it's worthwhile to have both
>> keyctl_dh_compute_kdf() (for pre-allocated buffers) and
>> keyctl_dh_compute_kdf_alloc().
>
> Thank you for the note. I will change the code accordingly.
>
> Though, shall I stuff the wrapper code back into the existing dh_compute
> functions or can I leave them as separate functions?
I'm not sure. In the existing code there's one keyctl wrapper per keyctl
command. A combined wrapper would need some extra logic to decide
whether kdfparams is passed in or not, which is different from existing
code.
>>
>>> +{
>>> + char *buf;
>>> + unsigned long buflen;
>>> + int ret;
>>> + struct keyctl_dh_params params = { .private = private,
>>> + .prime = prime,
>>> + .base = base };
>>> + struct keyctl_kdf_params kdfparams;
>>> +
>>> + buflen = strtoul(len, NULL, 10);
>>
>> The string to integer conversion needs to be done in
>> act_keyctl_dh_compute(). Length can be passed in as a size_t if it's
>> needed.
>>
>
> Ok, will do.
>
>>> + if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN)
>>> + return -1;
>>> +
>>> + buf = malloc(buflen + 1);
>>
>> The other _alloc functions exist because the buffer length isn't known to
>> the caller in advance. If the buffer length is known in advance, the
>> caller should be allocating the buffer.
>
> With the implementation of the _alloc and "non-alloc" function, I would assume
> that this comment should be covered.
>>
>> keyctl_dh_compute makes two syscalls: one to determine the buffer length,
>> and one to do the DH calculations.
>
> I am aware of that, but as explained above, in case of a KDF, there is no
> specifically required buffer size. Any buffer size the caller provides is
> supported and will be filled with data. Thus, in the KDF case, we can skip the
> first call.
Ok.
>>
>>> + if (!buf)
>>> + return -1;
>>> +
>>> + if (otherinfo) {
>>> + kdfparams.kdfname = kdfname;
>>> + kdfparams.kdfnamelen = strlen(kdfname);
>>
>> If kdfname is a null-terminated string, then kdfnamelen seems
>> unneccessary. I haven't reviewed the kernel side yet, but may comment more
>> there. There are other examples of null-terminated string usage in the
>> keyctl API, I'll comment more on this in the kernel patches.
>
> Please let me know on the kernel side, how you would like to handle it. Note,
> we only need that length information to ensure copy_from_user copies a maximum
> buffer length anyway.
I'll comment on that code as well, but look for use of strndup_user() in
the kernel's keyctl.c for examples.
>
> The string is used to find the proper crypto implementation via the kernel
> crypto API. The kernel crypto API will limit the maximum number of parsed
> bytes to 64 already.
>>
>>> + kdfparams.otherinfo = otherinfo;
>>> + kdfparams.otherinfolen = strlen(otherinfo);
>>
>> Same for otherinfolen.
>
> Sure. But note, otherinfo must support binary data. Thus, I think that the
> kernel side must support a length parameter.
>
> Here, for user space keyctl support, surely we have some limitation regarding
> the support for binary data (i.e. the binary data must not contain a \0 as
> strlen would return the wrong size).
If there may be binary data, then a length is definitely required. Keep in
mind that this code base is both for the keyctl command line tool and
libkeyutils. This function may be called directly by other code, so binary
data is just an array of bytes (including \0). To deal with binary data
from the command line, look at "keyctl add" vs "keyctl padd". The first
takes the key payload from a command line arg, the second accepts binary
data from a pipe or redirect.
>>
>>> + } else {
>>> + kdfparams.kdfname = kdfname;
>>> + kdfparams.kdfnamelen = strlen(kdfname);
>>> + kdfparams.otherinfo = NULL;
>>> + kdfparams.otherinfolen = 0;
>>> + }
>>> + ret = keyctl(KEYCTL_DH_COMPUTE, ¶ms, buf, buflen, &kdfparams);
>>> + if (ret < 0) {
>>> + free(buf);
>>> + return -1;
>>> + }
>>> +
>>> + buf[ret] = 0;
>>> + *_buffer = buf;
>>> + return ret;
>>> +}
>>> +
>>> +/*
>>>
>>> * Depth-first recursively apply a function over a keyring tree
>>> */
>>>
>>> static int recursive_key_scan_aux(key_serial_t parent, key_serial_t key,
>>> diff --git a/keyutils.h b/keyutils.h
>>> index b321aa8..5026270 100644
>>> --- a/keyutils.h
>>> +++ b/keyutils.h
>>> @@ -108,6 +108,16 @@ struct keyctl_dh_params {
>>>
>>> key_serial_t base;
>>>
>>> };
>>>
>>> +struct keyctl_kdf_params {
>>> +#define KEYCTL_KDF_MAX_OUTPUTLEN 1024 /* max length of KDF
>>> output */ +#define KEYCTL_KDF_MAX_STRING_LEN 64 /* maximum
>>> length of strings */
>> It's better to leave this information out of the userspace as it may
>> change depending on the kernel version. Let the kernel return an error if
>> the lengths exceed limits.
>
> Will do.
>>
>>> + char *kdfname;
>>> + uint32_t kdfnamelen;
>>> + char *otherinfo;
>>> + uint32_t otherinfolen;
>>> + uint32_t flags;
>>> +};
>>> +
>>> /*
>>>
>>> * syscall wrappers
>>> */
>>>
>>> @@ -172,6 +182,9 @@ extern int keyctl_read_alloc(key_serial_t id, void
>>> **_buffer); extern int keyctl_get_security_alloc(key_serial_t id, char
>>> **_buffer); extern int keyctl_dh_compute_alloc(key_serial_t private,
>>> key_serial_t prime,>
>>> key_serial_t base, void **_buffer);
>>>
>>> +int keyctl_dh_compute_kdf(key_serial_t private, key_serial_t prime,
>>> + key_serial_t base, char *len, char *kdfname,
>>> + char *otherinfo, void **_buffer);
>>>
>>> typedef int (*recursive_key_scanner_t)(key_serial_t parent, key_serial_t
>>> key,>
>>> char *desc, int desc_len, void *data);
>>>
>>> diff --git a/tests/keyctl/dh_compute/valid/runtest.sh
>>> b/tests/keyctl/dh_compute/valid/runtest.sh index 40ec387..1c77268 100644
>>> --- a/tests/keyctl/dh_compute/valid/runtest.sh
>>> +++ b/tests/keyctl/dh_compute/valid/runtest.sh
>>> @@ -80,6 +80,89 @@ marker "COMPUTE DH PUBLIC KEY"
>>> dh_compute $privateid $primeid $generatorid
>>> expect_payload payload $public
>>>
>>> +
>>> +################################################################
>>> +# Testing DH compute with KDF according to SP800-56A
>>> +#
>>> +# test vectors from
>>> http://csrc.nist.gov/groups/STM/cavp/documents/keymgmt/KASTestVectorsFFC2
>>> 014.zip +################################################################
>>> +
>>> +# SHA-256
>>> +
>>> +# XephemCAVS
>>> +private="\x81\xb2\xc6\x5f\x5c\xba\xc0\x0b\x13\x53\xac\x38\xbd\x77\xa2\x5a
>>> "
>>> +private+="\x86\x50\xed\x48\x5e\x41\x3e\xac\x1d\x6c\x48\x85"
>>> +
>>> +# P
>>> +prime="\xa3\xcc\x62\x23\xe5\x0c\x6e\x3f\x7b\xb0\x58\x1d\xcb\x9e\x9f\xf0"
>>> +prime+="\x2c\x58\x07\x68\x32\x8a\x15\x20\x7b\x1c\x32\x31\x7f\xb7\x84\x96"
>>> +prime+="\x81\x5e\x3c\xf7\xf9\xd0\x9c\xcb\x9f\xa8\x40\xff\x47\x98\x51\x1a"
>>> +prime+="\x17\xb5\x59\x28\x72\x1e\x5d\xfb\xcc\xc5\x41\x47\xe0\xf0\x5f\x85"
>>> +prime+="\xb3\xac\x41\x0b\x6a\xe3\xf5\x9b\x79\x6f\x3f\xea\xc7\xfc\x52\x49"
>>> +prime+="\x21\x7e\xb2\xa0\x45\x88\x29\x3a\x5a\xde\x22\x78\x79\xf4\x6c\xeb"
>>> +prime+="\x56\x45\x7b\x5c\x43\x12\x93\xe5\xe1\x04\xd1\xb9\x64\xbd\x2c\xdf"
>>> +prime+="\xde\xff\xa0\x40\x49\xa9\x1e\x67\xee\x8c\x86\xe9\x44\xf0\x4f\x94"
>>> +prime+="\x4a\x30\xe3\x61\xf8\xd1\x5d\x17\xe5\x01\x0c\xab\xb4\xef\x40\xc0"
>>> +prime+="\xeb\xa5\xf4\xa2\x52\xd4\xfd\x6c\xf9\xda\xe6\x0e\x86\xe4\xb3\x00"
>>> +prime+="\x9b\x1d\xfc\x92\x66\x70\x35\x72\x61\x58\x7a\xd0\x5c\x00\xa6\xc6"
>>> +prime+="\xf0\x10\x6c\xec\x8f\xc5\x91\x31\x51\x50\x84\xa8\x70\x59\x41\x65"
>>> +prime+="\xb4\x93\x90\xdb\x2d\x00\xe7\x53\x8f\x23\x0d\x53\x2f\x4a\x4e\xca"
>>> +prime+="\x83\x09\xd7\x07\xc0\xb3\x83\x5c\xee\x04\xf3\xca\x55\x8a\x22\xc6"
>>> +prime+="\xb5\x20\xfe\x25\xde\x6f\xfa\x90\xef\xda\x49\x27\xd0\x18\x59\x4c"
>>> +prime+="\x0c\x0b\x77\x06\x73\x93\xb7\xf1\xe0\xfc\x7c\xf2\x16\xaf\xf3\x9f"
>>> +
>>> +# YephemIUT
>>> +xa="\x9a\x70\x82\x2d\x3f\x06\x12\x3d\x0e\x51\x8e\xe1\x16\x51\xe5\xf6"
>>> +xa+="\xb1\x19\xdc\x3b\x97\xd5\xb1\xc0\xa2\xa6\xf6\xde\x94\x25\x64\xba"
>>> +xa+="\x10\x06\x1e\xec\xde\xb7\x36\x9c\xa5\x37\x49\x9e\x04\xb0\x36\xe9"
>>> +xa+="\x7f\x44\x5a\x95\x6f\x63\x69\xae\x6e\x63\xfd\x27\xea\xe3\xe3\x47"
>>> +xa+="\x85\x54\x47\xd3\xba\xc1\xc6\x0c\x10\xe7\x35\x07\x72\xc6\xc0\xc6"
>>> +xa+="\xfb\xf9\xca\x3e\x38\xf0\xe8\x65\x88\x25\xd3\xb2\x0f\x1f\x02\x8f"
>>> +xa+="\x35\xe3\x4d\x12\x35\x10\x3d\xf2\x33\x9b\x5b\x09\x9d\x3f\xe3\xe5"
>>> +xa+="\x34\x6a\x69\x16\x42\xba\xc5\xb0\xbb\x03\xcd\x5d\x04\xd7\x56\x26"
>>> +xa+="\x21\x49\x3f\xf1\xc4\x27\x3b\x6a\x45\xc5\xec\xb0\xb5\xe9\x08\xa0"
>>> +xa+="\xf9\xf5\x62\x28\x2e\x85\x3e\xfc\x9a\x7e\xa1\x12\xe9\x47\x4f\xf6"
>>> +xa+="\x94\x18\xf7\xc4\x7a\xe9\x66\xd4\x52\x4c\xa1\x70\x1b\x60\xa4\xbe"
>>> +xa+="\x15\xc7\x5e\x27\xb4\x05\x80\x64\x68\x15\x6e\x02\xcb\xc5\x8f\xf4"
>>> +xa+="\x66\x3c\x96\xac\x0c\x87\x36\x81\x35\xfa\x9b\x0b\xb6\x33\x7a\xe2"
>>> +xa+="\x58\x52\x1d\x7d\x60\xc2\xa9\x1b\x4e\xd7\x72\xad\x65\x03\x40\x49"
>>> +xa+="\x97\xf6\x79\x9d\xf6\x63\xa8\x99\x9c\xfd\x74\x7f\xa0\x67\xb9\x05"
>>> +xa+="\x8a\xb3\x3b\xc1\x45\x94\x36\x6f\x28\xf5\xa2\xd9\x00\xb6\x46\x7a"
>>> +
>>> +# Z
>>> +shared="0fdbd9a2 ebf50cba 489b4e4d 7cd6924a 42ee6324 a26988b2 22bc38e6
>>> 9cc445f1\n" +shared+="eb47c1a4 62eca39f 39bcd7b8 19dede51 30bc38da
>>> ec99c16f 40a4e5c1 9c97b796\n" +shared+="8b41823d a0650e37 13c73e6f
>>> 5f2a9dff 2e67dbf5 40ee66f4 e694c28f ba1d604b\n" +shared+="71b57b8a
>>> eeb67a35 ba425a38 490b6fb9 f713db22 6f893b7a 8962f426 ba3046fb\n"
>>> +shared+="cff8538c 16f583e8 ae947672 0ba55ff9 75b440d0 c4565cc7 5837d23a
>>> fea61a39\n" +shared+="e0b7f6c4 e24c2154 7eb19fce f8dbed10 b06a9cce
>>> 971c0f0f ba7c1d5c b5035eaa\n" +shared+="4fddd3ba fe757339 e3321e3e
>>> 4ebfe9e7 9c6c0401 4df63cf9 28d0a2c0 5b2d5521\n" +shared+="030c35f1
>>> c84c97fe 64cad509 8012a003 d52d24c4 1a1f9348 b7575251 3facb02f\n" +
>>> +# OI
>>> +otherinfo="\xa1\xb2\xc3\xd4\xe5\x43\x41\x56\x53\x69\x64\x0d\x64\xc1\xb2"
>>> +otherinfo+="\x33\x61\xb2\x61\xde\x78\x68\x8e\xa8\x65\xfc\xff\x11\x3c\x84"
>>> +
>>> +# DKM
>>> +derived="8284e313 02c8a26b 393ec52d 9f9e0882\n"
>>> +
>>> +pcreate_key "-e $prime" user dh:prime @s
>>> +expect_keyid primeid
>>> +
>>> +pcreate_key "-e $xa" user dh:xa @s
>>> +expect_keyid xaid
>>> +
>>> +pcreate_key "-e $private" user dh:private @s
>>> +expect_keyid privateid
>>> +
>>> +marker "COMPUTE DH SHARED SECRET"
>>> +dh_compute $privateid $primeid $xaid
>>> +expect_payload payload $shared
>>
>> As I mentioned at the top, we'll need an 'expect_multiline' function that
>> handles values like $shared.
>
> Ok.
>>
>>> +
>>> +marker "COMPUTE DERIVED KEY FROM DH SHARED SECRET (SHA-256)"
>>> +dh_compute $privateid $primeid $xaid 16 "kdf_ctr(sha256)" "$(echo -e -n
>>> $otherinfo)" +expect_payload payload $derived
>>
>> Have you checked that expect_payload works correctly in this case? Seems
>> like it should, since the output fits on one line.
>
> I just tested it and the code does NOT catch the error. I.e. when changing
> $derived, the harness still returns a PASS even though the returned data does
> not match the expected data.
When I was implementing expect_multiline, I realized I also had a quoting
problem in my use of expect_payload. Look over the patchset I posted to
the keyrings list today, with that you should use:
expect_multiline payload "$shared"
Note that you'll also have to update your assignment to the 'shared'
variable to make sure the newlines are preserved, like my change to the
'public' variable assignment.
>>
>>> +
>>> echo "++++ FINISHED TEST: $result" >>$OUTPUTFILE
>>>
>>> # --- then report the results in the database ---
Regards,
--
Mat Martineau
Intel OTC
^ permalink raw reply
* Re: [RFC PATCH] KEYS: add SP800-56A KDF support for DH
From: Mat Martineau @ 2016-07-15 0:45 UTC (permalink / raw)
To: Stephan Mueller; +Cc: Mat Martineau, David Howells, keyrings, linux-crypto
In-Reply-To: <4161793.TTVXSVQtZL@positron.chronox.de>
On Tue, 12 Jul 2016, Stephan Mueller wrote:
> Hi Mat, David,
>
> This patch is based on the cryptodev-2.6 tree with the patch
> 4693fc734d675c5518ea9bd4c9623db45bc37402 ("KEYS: Add placeholder
> for KDF usage with DH") from Linus' tree added on top.
>
> I am aware of the fact that the compat code is not present. This
> patch is intended for review to verify that the user space
> interface follows the general approach for the keys subsystem.
>
> The patch to add KDF to the kernel crypto API will be appended to
> this patch.
>
> The patch for the keyutils user space extension will also be added.
>
> Ciao
> Stephan
>
> ---8<---
>
> SP800-56A define the use of DH with key derivation function based on a
> counter. The input to the KDF is defined as (DH shared secret || other
> information). The value for the "other information" is to be provided by
> the caller.
>
> The KDF is provided by the kernel crypto API. The SP800-56A KDF is equal
> to the SP800-108 counter KDF. However, the caller is allowed to specify
> the KDF type that he wants to use to derive the key material allowing
> the use of the other KDFs provided with the kernel crypto API.
>
> As the KDF implements the proper truncation of the DH shared secret to
> the requested size, this patch fills the caller buffer up to its size.
>
> The patch is tested with a new test added to the keyutils user space
> code which uses a CAVS test vector testing the compliance with
> SP800-56A.
>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
> include/uapi/linux/keyctl.h | 10 +++++
> security/keys/Kconfig | 1 +
> security/keys/dh.c | 98 ++++++++++++++++++++++++++++++++++++++++-----
> security/keys/internal.h | 5 ++-
> security/keys/keyctl.c | 2 +-
> 5 files changed, 103 insertions(+), 13 deletions(-)
Be sure to update Documentation/security/keys.txt once the interface is
settled on.
>
> diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
> index 86eddd6..cc4ce7c 100644
> --- a/include/uapi/linux/keyctl.h
> +++ b/include/uapi/linux/keyctl.h
> @@ -68,4 +68,14 @@ struct keyctl_dh_params {
> __s32 base;
> };
>
> +struct keyctl_kdf_params {
> +#define KEYCTL_KDF_MAX_OUTPUTLEN 1024 /* max length of KDF output */
> +#define KEYCTL_KDF_MAX_STRING_LEN 64 /* maximum length of strings */
I think these limits should be in the internal headers rather than uapi.
> + char *kdfname;
> + __u32 kdfnamelen;
As noted in the userspace patch, if kdfname is a null-terminated string
then kdfnamelen isn't needed.
> + char *otherinfo;
> + __u32 otherinfolen;
> + __u32 flags;
Looks like flags aren't used anywhere. Do you have a use planned? You
could add some spare capacity like the keyctl_pkey_* structs instead (see
https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-next&id=dd7e2ea418b4f8851a4eb976b9431fbc03d2ebaf
)
> +};
> +
> #endif /* _LINUX_KEYCTL_H */
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index f826e87..56491fe 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -90,6 +90,7 @@ config KEY_DH_OPERATIONS
> bool "Diffie-Hellman operations on retained keys"
> depends on KEYS
> select MPILIB
> + select CRYPTO_KDF
> help
> This option provides support for calculating Diffie-Hellman
> public keys and shared secrets using values stored as keys
> diff --git a/security/keys/dh.c b/security/keys/dh.c
> index 531ed2e..4c93969 100644
> --- a/security/keys/dh.c
> +++ b/security/keys/dh.c
> @@ -77,14 +77,74 @@ error:
> return ret;
> }
>
> +static int keyctl_dh_compute_kdf(struct keyctl_kdf_params *kdfcopy,
> + char __user *buffer, size_t buflen,
> + uint8_t *kbuf, size_t resultlen)
> +{
Minor point: this function name made me think it was a replacement for
keyctl_dh_compute at first (like the userspace counterpart).
> + char kdfname[CRYPTO_MAX_ALG_NAME] = { 0 };
> + struct crypto_rng *tfm;
> + uint8_t *outbuf = NULL;
> + int ret;
> +
> + BUILD_BUG_ON(CRYPTO_MAX_ALG_NAME != KEYCTL_KDF_MAX_STRING_LEN);
If this is a requirement, why define KEYCTL_KDF_MAX_STRING_LEN? Use
CRYPTO_MAX_ALG_NAME directly.
> + if (!kdfcopy->kdfnamelen)
> + return -EFAULT;
> + if (copy_from_user(&kdfname, kdfcopy->kdfname,
> + kdfcopy->kdfnamelen) != 0)
strndup_user works nicely for strings.
> + return -EFAULT;
> +
It would be best to validate all of the userspace input before the DH
computation is done.
> + /*
> + * Concatenate otherinfo past DH shared secret -- the
> + * input to the KDF is (DH shared secret || otherinfo)
> + */
> + if (kdfcopy->otherinfo &&
> + copy_from_user(kbuf + resultlen, kdfcopy->otherinfo,
> + kdfcopy->otherinfolen)
> + != 0)
> + return -EFAULT;
> +
> + tfm = crypto_alloc_rng(kdfname, 0, 0);
> + if (IS_ERR(tfm))
> + return PTR_ERR(tfm);
> +
> +#if 0
> + /* we do not support HMAC currently */
> + ret = crypto_rng_reset(tfm, xx, xxlen);
> + if (ret) {
> + crypto_free_rng(tfm);
> + goto error5;
> + }
> +#endif
> +
> + outbuf = kmalloc(buflen, GFP_KERNEL);
> + if (!outbuf) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = crypto_rng_generate(tfm, kbuf, resultlen + kdfcopy->otherinfolen,
> + outbuf, buflen);
> + if (ret)
> + goto err;
> +
> + ret = buflen;
> + if (copy_to_user(buffer, outbuf, buflen) != 0)
> + ret = -EFAULT;
> +
> +err:
> + kzfree(outbuf);
> + crypto_free_rng(tfm);
> + return ret;
> +}
> long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> char __user *buffer, size_t buflen,
> - void __user *reserved)
> + struct keyctl_kdf_params __user *kdf)
> {
> long ret;
> MPI base, private, prime, result;
> unsigned nbytes;
> struct keyctl_dh_params pcopy;
> + struct keyctl_kdf_params kdfcopy;
> uint8_t *kbuf;
> ssize_t keylen;
> size_t resultlen;
> @@ -98,12 +158,24 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> goto out;
> }
>
> - if (reserved) {
> - ret = -EINVAL;
> - goto out;
> + if (kdf) {
> + if (copy_from_user(&kdfcopy, kdf, sizeof(kdfcopy)) != 0) {
> + ret = -EFAULT;
> + goto out;
> + }
> + if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN ||
> + kdfcopy.otherinfolen > KEYCTL_KDF_MAX_STRING_LEN ||
> + kdfcopy.kdfnamelen > KEYCTL_KDF_MAX_STRING_LEN) {
> + ret = -EMSGSIZE;
> + goto out;
> + }
> }
>
> - keylen = mpi_from_key(pcopy.prime, buflen, &prime);
> + /*
> + * If the caller requests postprocessing with a KDF, allow an
> + * arbitrary output buffer size since the KDF ensures proper truncation.
> + */
> + keylen = mpi_from_key(pcopy.prime, kdf ? SIZE_MAX : buflen, &prime);
> if (keylen < 0 || !prime) {
> /* buflen == 0 may be used to query the required buffer size,
> * which is the prime key length.
> @@ -133,7 +205,8 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> goto error3;
> }
>
> - kbuf = kmalloc(resultlen, GFP_KERNEL);
> + kbuf = kmalloc(kdf ? (resultlen + kdfcopy.otherinfolen) : resultlen,
> + GFP_KERNEL);
> if (!kbuf) {
> ret = -ENOMEM;
> goto error4;
> @@ -147,12 +220,17 @@ long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> if (ret != 0)
> goto error5;
>
> - ret = nbytes;
> - if (copy_to_user(buffer, kbuf, nbytes) != 0)
> - ret = -EFAULT;
> + if (kdf) {
> + ret = keyctl_dh_compute_kdf(&kdfcopy, buffer, buflen,
> + kbuf, resultlen);
> + } else {
> + ret = nbytes;
> + if (copy_to_user(buffer, kbuf, nbytes) != 0)
> + ret = -EFAULT;
> + }
>
> error5:
> - kfree(kbuf);
> + kzfree(kbuf);
Thanks for adjusting this.
> error4:
> mpi_free(result);
> error3:
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index a705a7d..35a8d11 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -259,12 +259,13 @@ static inline long keyctl_get_persistent(uid_t uid, key_serial_t destring)
> #endif
>
> #ifdef CONFIG_KEY_DH_OPERATIONS
> +#include <crypto/rng.h>
> extern long keyctl_dh_compute(struct keyctl_dh_params __user *, char __user *,
> - size_t, void __user *);
> + size_t, struct keyctl_kdf_params __user *);
> #else
> static inline long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> char __user *buffer, size_t buflen,
> - void __user *reserved)
> + struct keyctl_kdf_params __user *kdf)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> index d580ad0..b106898 100644
> --- a/security/keys/keyctl.c
> +++ b/security/keys/keyctl.c
> @@ -1689,7 +1689,7 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long, arg2, unsigned long, arg3,
> case KEYCTL_DH_COMPUTE:
> return keyctl_dh_compute((struct keyctl_dh_params __user *) arg2,
> (char __user *) arg3, (size_t) arg4,
> - (void __user *) arg5);
> + (struct keyctl_kdf_params __user *) arg5);
>
> default:
> return -EOPNOTSUPP;
Regards,
--
Mat Martineau
Intel OTC
^ permalink raw reply
* Please quote urgently
From: RAHAMAN BIN ABD HAMID TRADING @ 2016-07-15 2:41 UTC (permalink / raw)
[-- Attachment #1: Type: text/plain, Size: 345 bytes --]
Dear sir/madam,
I am RAHAMAN BIN, I am the Managing Director/CEO of ABD RAHAMAN BIN
ABD HAMID
Trading UK, I am introduced to you by a friend and an associate.
Please we need this items from you as we have been told you supply
them.
Kindly view the PDF file attached for copy of the items we need I
shall wait
you urgent reply
regards
RAHAMAN
[-- Attachment #2: ORDER.pdf --]
[-- Type: application/pdf, Size: 34531 bytes --]
^ permalink raw reply
* Re: [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation
From: Tadeusz Struk @ 2016-07-15 3:39 UTC (permalink / raw)
To: Salvatore Benedetto, herbert, davem, andrew.zaborowski; +Cc: linux-crypto
In-Reply-To: <1468491905-2597-1-git-send-email-salvatore.benedetto@intel.com>
Hi Salvatore,
On 07/14/2016 03:25 AM, Salvatore Benedetto wrote:
> Embedding the akcipher_request in pkcs1pad_request don't take
> into account the context space required by the akcipher object.
I think we do take into account the sub request context. See line 675.
The only thing that is wrong is that the child_req should be at
the end of the structure. This is build tested only.
---8<---
From: Tadeusz Struk <tadeusz.struk@intel.com>
Subject: [PATCH] crypto: rsa-pkcs1pad - fix rsa-pkcs1pad request struct
To allow for child request context the struct akcipher_request child_req
needs to be at the end of the structure.
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
crypto/rsa-pkcs1pad.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/crypto/rsa-pkcs1pad.c b/crypto/rsa-pkcs1pad.c
index 880d3db..877019a 100644
--- a/crypto/rsa-pkcs1pad.c
+++ b/crypto/rsa-pkcs1pad.c
@@ -101,10 +101,9 @@ struct pkcs1pad_inst_ctx {
};
struct pkcs1pad_request {
- struct akcipher_request child_req;
-
struct scatterlist in_sg[2], out_sg[1];
uint8_t *in_buf, *out_buf;
+ struct akcipher_request child_req;
};
static int pkcs1pad_set_pub_key(struct crypto_akcipher *tfm, const void *key,
^ permalink raw reply related
* [patch] crypto: nx - off by one bug in nx_of_update_msc()
From: Dan Carpenter @ 2016-07-15 11:09 UTC (permalink / raw)
To: Leonidas S. Barbosa, Kent Yoder
Cc: Paulo Flabiano Smorigo, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman, Herbert Xu, David S. Miller, linux-crypto,
linuxppc-dev, kernel-janitors
The props->ap[] array is defined like this:
struct alg_props ap[NX_MAX_FC][NX_MAX_MODE][3];
So we can see that if msc->fc and msc->mode are == to NX_MAX_FC or
NX_MAX_MODE then we're off by one.
Fixes: ae0222b7289d ('powerpc/crypto: nx driver code supporting nx encryption')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/crypto/nx/nx.c b/drivers/crypto/nx/nx.c
index 0794f1c..42f0f22 100644
--- a/drivers/crypto/nx/nx.c
+++ b/drivers/crypto/nx/nx.c
@@ -392,7 +392,7 @@ static void nx_of_update_msc(struct device *dev,
((bytes_so_far + sizeof(struct msc_triplet)) <= lenp) &&
i < msc->triplets;
i++) {
- if (msc->fc > NX_MAX_FC || msc->mode > NX_MAX_MODE) {
+ if (msc->fc >= NX_MAX_FC || msc->mode >= NX_MAX_MODE) {
dev_err(dev, "unknown function code/mode "
"combo: %d/%d (ignored)\n", msc->fc,
msc->mode);
^ permalink raw reply related
* Re: [V2][PATCH 1/2] PKCS#7: Fix kernel panic when referring to the empty AuthorityKeyIdentifier
From: David Howells @ 2016-07-15 14:48 UTC (permalink / raw)
To: Lans Zhang; +Cc: dhowells, bhe, vgoyal, kexec, linux-crypto
In-Reply-To: <1468416937-21237-1-git-send-email-jia.zhang@windriver.com>
Lans Zhang <jia.zhang@windriver.com> wrote:
> This fix resolves the following kernel panic if the empty
> AuthorityKeyIdentifier employed.
It should be noted that this is only an issue if DEBUG is #defined at the top
of pkcs7_verify.c as the crash happens in a pr_debug() statement.
David
^ permalink raw reply
* Re: [RFC PATCH] KEYS: add SP800-56A KDF support for DH
From: Stephan Mueller @ 2016-07-15 16:38 UTC (permalink / raw)
To: Mat Martineau; +Cc: David Howells, keyrings, linux-crypto
In-Reply-To: <alpine.OSX.2.20.1607141654250.69287@istotlan-mobl1.amr.corp.intel.com>
Am Donnerstag, 14. Juli 2016, 17:45:59 schrieb Mat Martineau:
Hi Mat,
> > Signed-off-by: Stephan Mueller <smueller@chronox.de>
> > ---
> > include/uapi/linux/keyctl.h | 10 +++++
> > security/keys/Kconfig | 1 +
> > security/keys/dh.c | 98
> > ++++++++++++++++++++++++++++++++++++++++----- security/keys/internal.h
> > | 5 ++-
> > security/keys/keyctl.c | 2 +-
> > 5 files changed, 103 insertions(+), 13 deletions(-)
>
> Be sure to update Documentation/security/keys.txt once the interface is
> settled on.
Thanks for the reminder
>
> > diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
> > index 86eddd6..cc4ce7c 100644
> > --- a/include/uapi/linux/keyctl.h
> > +++ b/include/uapi/linux/keyctl.h
> > @@ -68,4 +68,14 @@ struct keyctl_dh_params {
> >
> > __s32 base;
> >
> > };
> >
> > +struct keyctl_kdf_params {
> > +#define KEYCTL_KDF_MAX_OUTPUTLEN 1024 /* max length of KDF output */
> > +#define KEYCTL_KDF_MAX_STRING_LEN 64 /* maximum length of strings
*/
>
> I think these limits should be in the internal headers rather than uapi.
Ok
>
> > + char *kdfname;
> > + __u32 kdfnamelen;
>
> As noted in the userspace patch, if kdfname is a null-terminated string
> then kdfnamelen isn't needed.
Ok
>
> > + char *otherinfo;
> > + __u32 otherinfolen;
> > + __u32 flags;
>
> Looks like flags aren't used anywhere. Do you have a use planned? You
> could add some spare capacity like the keyctl_pkey_* structs instead (see
> https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h
> =keys-next&id=dd7e2ea418b4f8851a4eb976b9431fbc03d2ebaf )
I am not sure what to do here: I see the profileration of new syscalls which
just differ from existing syscalls by a new flags field because the initial
implementation simply missed such thing.
I want to avoid something like this happening here.
I am open for any suggestions.
>
> > +};
> > +
> > #endif /* _LINUX_KEYCTL_H */
> > diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> > index f826e87..56491fe 100644
> > --- a/security/keys/Kconfig
> > +++ b/security/keys/Kconfig
> > @@ -90,6 +90,7 @@ config KEY_DH_OPERATIONS
> >
> > bool "Diffie-Hellman operations on retained keys"
> > depends on KEYS
> > select MPILIB
> >
> > + select CRYPTO_KDF
> >
> > help
> >
> > This option provides support for calculating Diffie-Hellman
> > public keys and shared secrets using values stored as keys
> >
> > diff --git a/security/keys/dh.c b/security/keys/dh.c
> > index 531ed2e..4c93969 100644
> > --- a/security/keys/dh.c
> > +++ b/security/keys/dh.c
> >
> > @@ -77,14 +77,74 @@ error:
> > return ret;
> >
> > }
> >
> > +static int keyctl_dh_compute_kdf(struct keyctl_kdf_params *kdfcopy,
> > + char __user *buffer, size_t buflen,
> > + uint8_t *kbuf, size_t resultlen)
> > +{
>
> Minor point: this function name made me think it was a replacement for
> keyctl_dh_compute at first (like the userspace counterpart).
Well, initially I had it part of dh_compute, but then extracted it to make the
code nicer and less distracting.
>
> > + char kdfname[CRYPTO_MAX_ALG_NAME] = { 0 };
> > + struct crypto_rng *tfm;
> > + uint8_t *outbuf = NULL;
> > + int ret;
> > +
> > + BUILD_BUG_ON(CRYPTO_MAX_ALG_NAME != KEYCTL_KDF_MAX_STRING_LEN);
>
> If this is a requirement, why define KEYCTL_KDF_MAX_STRING_LEN? Use
> CRYPTO_MAX_ALG_NAME directly.
Ok, I was not sure if I am allowed to add a crypto API header to key header
files.
>
> > + if (!kdfcopy->kdfnamelen)
> > + return -EFAULT;
> > + if (copy_from_user(&kdfname, kdfcopy->kdfname,
> > + kdfcopy->kdfnamelen) != 0)
>
> strndup_user works nicely for strings.
yes.
>
> > + return -EFAULT;
> > +
>
> It would be best to validate all of the userspace input before the DH
> computation is done.
Uh, that means I cannot have a separate dh_compute_kdf function. But, ok, no
problem.
>
> > + /*
> > + * Concatenate otherinfo past DH shared secret -- the
> > + * input to the KDF is (DH shared secret || otherinfo)
> > + */
> > + if (kdfcopy->otherinfo &&
> > + copy_from_user(kbuf + resultlen, kdfcopy->otherinfo,
> > + kdfcopy->otherinfolen)
> > + != 0)
> > + return -EFAULT;
> > +
> > + tfm = crypto_alloc_rng(kdfname, 0, 0);
> > + if (IS_ERR(tfm))
> > + return PTR_ERR(tfm);
> > +
> > +#if 0
> > + /* we do not support HMAC currently */
> > + ret = crypto_rng_reset(tfm, xx, xxlen);
> > + if (ret) {
> > + crypto_free_rng(tfm);
> > + goto error5;
> > + }
> > +#endif
> > +
> > + outbuf = kmalloc(buflen, GFP_KERNEL);
> > + if (!outbuf) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + ret = crypto_rng_generate(tfm, kbuf, resultlen + kdfcopy-
>otherinfolen,
> > + outbuf, buflen);
> > + if (ret)
> > + goto err;
> > +
> > + ret = buflen;
> > + if (copy_to_user(buffer, outbuf, buflen) != 0)
> > + ret = -EFAULT;
> > +
> > +err:
> > + kzfree(outbuf);
> > + crypto_free_rng(tfm);
> > + return ret;
> > +}
> > long keyctl_dh_compute(struct keyctl_dh_params __user *params,
> >
> > char __user *buffer, size_t buflen,
> >
> > - void __user *reserved)
> > + struct keyctl_kdf_params __user *kdf)
> > {
> >
> > long ret;
> > MPI base, private, prime, result;
> > unsigned nbytes;
> > struct keyctl_dh_params pcopy;
> >
> > + struct keyctl_kdf_params kdfcopy;
> >
> > uint8_t *kbuf;
> > ssize_t keylen;
> > size_t resultlen;
> >
> > @@ -98,12 +158,24 @@ long keyctl_dh_compute(struct keyctl_dh_params __user
> > *params,>
> > goto out;
> >
> > }
> >
> > - if (reserved) {
> > - ret = -EINVAL;
> > - goto out;
> > + if (kdf) {
> > + if (copy_from_user(&kdfcopy, kdf, sizeof(kdfcopy)) != 0) {
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > + if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN ||
> > + kdfcopy.otherinfolen > KEYCTL_KDF_MAX_STRING_LEN ||
> > + kdfcopy.kdfnamelen > KEYCTL_KDF_MAX_STRING_LEN) {
> > + ret = -EMSGSIZE;
> > + goto out;
> > + }
> >
> > }
> >
> > - keylen = mpi_from_key(pcopy.prime, buflen, &prime);
> > + /*
> > + * If the caller requests postprocessing with a KDF, allow an
> > + * arbitrary output buffer size since the KDF ensures proper
truncation.
> > + */
> > + keylen = mpi_from_key(pcopy.prime, kdf ? SIZE_MAX : buflen, &prime);
> >
> > if (keylen < 0 || !prime) {
> >
> > /* buflen == 0 may be used to query the required buffer size,
> >
> > * which is the prime key length.
> >
> > @@ -133,7 +205,8 @@ long keyctl_dh_compute(struct keyctl_dh_params __user
> > *params,>
> > goto error3;
> >
> > }
> >
> > - kbuf = kmalloc(resultlen, GFP_KERNEL);
> > + kbuf = kmalloc(kdf ? (resultlen + kdfcopy.otherinfolen) : resultlen,
> > + GFP_KERNEL);
> >
> > if (!kbuf) {
> >
> > ret = -ENOMEM;
> > goto error4;
> >
> > @@ -147,12 +220,17 @@ long keyctl_dh_compute(struct keyctl_dh_params
> > __user *params,>
> > if (ret != 0)
> >
> > goto error5;
> >
> > - ret = nbytes;
> > - if (copy_to_user(buffer, kbuf, nbytes) != 0)
> > - ret = -EFAULT;
> > + if (kdf) {
> > + ret = keyctl_dh_compute_kdf(&kdfcopy, buffer, buflen,
> > + kbuf, resultlen);
> > + } else {
> > + ret = nbytes;
> > + if (copy_to_user(buffer, kbuf, nbytes) != 0)
> > + ret = -EFAULT;
> > + }
> >
> > error5:
> > - kfree(kbuf);
> > + kzfree(kbuf);
>
> Thanks for adjusting this.
I hope it is ok to not have it in a separate patch.
>
> > error4:
> > mpi_free(result);
> >
> > error3:
> > diff --git a/security/keys/internal.h b/security/keys/internal.h
> > index a705a7d..35a8d11 100644
> > --- a/security/keys/internal.h
> > +++ b/security/keys/internal.h
> > @@ -259,12 +259,13 @@ static inline long keyctl_get_persistent(uid_t uid,
> > key_serial_t destring) #endif
> >
> > #ifdef CONFIG_KEY_DH_OPERATIONS
> > +#include <crypto/rng.h>
> > extern long keyctl_dh_compute(struct keyctl_dh_params __user *, char
> > __user *, - size_t, void __user *);
> > + size_t, struct keyctl_kdf_params __user *);
> > #else
> > static inline long keyctl_dh_compute(struct keyctl_dh_params __user
> > *params,>
> > char __user *buffer, size_t buflen,
> >
> > - void __user *reserved)
> > + struct keyctl_kdf_params __user *kdf)
> > {
> >
> > return -EOPNOTSUPP;
> >
> > }
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index d580ad0..b106898 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -1689,7 +1689,7 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long,
> > arg2, unsigned long, arg3,>
> > case KEYCTL_DH_COMPUTE:
> > return keyctl_dh_compute((struct keyctl_dh_params __user *)
arg2,
> >
> > (char __user *) arg3, (size_t) arg4,
> >
> > - (void __user *) arg5);
> > + (struct keyctl_kdf_params __user *)
arg5);
> >
> > default:
> > return -EOPNOTSUPP;
>
> Regards,
>
> --
> Mat Martineau
> Intel OTC
Ciao
Stephan
^ permalink raw reply
* Re: [RFC PATCH] KEYS: add SP800-56A KDF support for DH
From: Mat Martineau @ 2016-07-15 18:45 UTC (permalink / raw)
To: Stephan Mueller; +Cc: Mat Martineau, David Howells, keyrings, linux-crypto
In-Reply-To: <25767136.yGpxH6HqEs@tauon.atsec.com>
Stephan,
On Fri, 15 Jul 2016, Stephan Mueller wrote:
> Am Donnerstag, 14. Juli 2016, 17:45:59 schrieb Mat Martineau:
>
> Hi Mat,
>
>>> Signed-off-by: Stephan Mueller <smueller@chronox.de>
>>> ---
>>> include/uapi/linux/keyctl.h | 10 +++++
>>> security/keys/Kconfig | 1 +
>>> security/keys/dh.c | 98
>>> ++++++++++++++++++++++++++++++++++++++++----- security/keys/internal.h
>>> | 5 ++-
>>> security/keys/keyctl.c | 2 +-
>>> 5 files changed, 103 insertions(+), 13 deletions(-)
>>
>> Be sure to update Documentation/security/keys.txt once the interface is
>> settled on.
>
> Thanks for the reminder
>>
>>> diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
>>> index 86eddd6..cc4ce7c 100644
>>> --- a/include/uapi/linux/keyctl.h
>>> +++ b/include/uapi/linux/keyctl.h
>>> @@ -68,4 +68,14 @@ struct keyctl_dh_params {
>>>
>>> __s32 base;
>>>
>>> };
>>>
>>> +struct keyctl_kdf_params {
>>> +#define KEYCTL_KDF_MAX_OUTPUTLEN 1024 /* max length of KDF output */
>>> +#define KEYCTL_KDF_MAX_STRING_LEN 64 /* maximum length of strings
> */
>>
>> I think these limits should be in the internal headers rather than uapi.
>
> Ok
>>
>>> + char *kdfname;
>>> + __u32 kdfnamelen;
>>
>> As noted in the userspace patch, if kdfname is a null-terminated string
>> then kdfnamelen isn't needed.
>
> Ok
>>
>>> + char *otherinfo;
>>> + __u32 otherinfolen;
>>> + __u32 flags;
>>
>> Looks like flags aren't used anywhere. Do you have a use planned? You
>> could add some spare capacity like the keyctl_pkey_* structs instead (see
>> https://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/commit/?h
>> =keys-next&id=dd7e2ea418b4f8851a4eb976b9431fbc03d2ebaf )
>
> I am not sure what to do here: I see the profileration of new syscalls which
> just differ from existing syscalls by a new flags field because the initial
> implementation simply missed such thing.
>
> I want to avoid something like this happening here.
>
> I am open for any suggestions.
David's approach in the structs I referenced is to add a spare array of
__u32's:
__u32 __spare[8];
That way future additions can be added to the space currently used by
__spare, and that array is made smaller so the overall struct size stays
the same and the older members are not moved around.
>>
>>> +};
>>> +
>>> #endif /* _LINUX_KEYCTL_H */
>>> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
>>> index f826e87..56491fe 100644
>>> --- a/security/keys/Kconfig
>>> +++ b/security/keys/Kconfig
>>> @@ -90,6 +90,7 @@ config KEY_DH_OPERATIONS
>>>
>>> bool "Diffie-Hellman operations on retained keys"
>>> depends on KEYS
>>> select MPILIB
>>>
>>> + select CRYPTO_KDF
>>>
>>> help
>>>
>>> This option provides support for calculating Diffie-Hellman
>>> public keys and shared secrets using values stored as keys
>>>
>>> diff --git a/security/keys/dh.c b/security/keys/dh.c
>>> index 531ed2e..4c93969 100644
>>> --- a/security/keys/dh.c
>>> +++ b/security/keys/dh.c
>>>
>>> @@ -77,14 +77,74 @@ error:
>>> return ret;
>>>
>>> }
>>>
>>> +static int keyctl_dh_compute_kdf(struct keyctl_kdf_params *kdfcopy,
>>> + char __user *buffer, size_t buflen,
>>> + uint8_t *kbuf, size_t resultlen)
>>> +{
>>
>> Minor point: this function name made me think it was a replacement for
>> keyctl_dh_compute at first (like the userspace counterpart).
>
> Well, initially I had it part of dh_compute, but then extracted it to make the
> code nicer and less distracting.
Extracting it is good - my comment was only regarding the name.
>
>>
>>> + char kdfname[CRYPTO_MAX_ALG_NAME] = { 0 };
>>> + struct crypto_rng *tfm;
>>> + uint8_t *outbuf = NULL;
>>> + int ret;
>>> +
>>> + BUILD_BUG_ON(CRYPTO_MAX_ALG_NAME != KEYCTL_KDF_MAX_STRING_LEN);
>>
>> If this is a requirement, why define KEYCTL_KDF_MAX_STRING_LEN? Use
>> CRYPTO_MAX_ALG_NAME directly.
>
> Ok, I was not sure if I am allowed to add a crypto API header to key header
> files.
I don't think you need to include the crypto header in any key headers,
just here in dh.c. dh.c will be converted to use the DH implementation
recently added to the crypto subsystem, by the way.
>>
>>> + if (!kdfcopy->kdfnamelen)
>>> + return -EFAULT;
>>> + if (copy_from_user(&kdfname, kdfcopy->kdfname,
>>> + kdfcopy->kdfnamelen) != 0)
>>
>> strndup_user works nicely for strings.
>
> yes.
>>
>>> + return -EFAULT;
>>> +
>>
>> It would be best to validate all of the userspace input before the DH
>> computation is done.
>
> Uh, that means I cannot have a separate dh_compute_kdf function. But, ok, no
> problem.
>>
>>> + /*
>>> + * Concatenate otherinfo past DH shared secret -- the
>>> + * input to the KDF is (DH shared secret || otherinfo)
>>> + */
>>> + if (kdfcopy->otherinfo &&
>>> + copy_from_user(kbuf + resultlen, kdfcopy->otherinfo,
>>> + kdfcopy->otherinfolen)
>>> + != 0)
>>> + return -EFAULT;
>>> +
>>> + tfm = crypto_alloc_rng(kdfname, 0, 0);
>>> + if (IS_ERR(tfm))
>>> + return PTR_ERR(tfm);
>>> +
>>> +#if 0
>>> + /* we do not support HMAC currently */
>>> + ret = crypto_rng_reset(tfm, xx, xxlen);
>>> + if (ret) {
>>> + crypto_free_rng(tfm);
>>> + goto error5;
>>> + }
>>> +#endif
>>> +
>>> + outbuf = kmalloc(buflen, GFP_KERNEL);
>>> + if (!outbuf) {
>>> + ret = -ENOMEM;
>>> + goto err;
>>> + }
>>> +
>>> + ret = crypto_rng_generate(tfm, kbuf, resultlen + kdfcopy-
>> otherinfolen,
>>> + outbuf, buflen);
>>> + if (ret)
>>> + goto err;
>>> +
>>> + ret = buflen;
>>> + if (copy_to_user(buffer, outbuf, buflen) != 0)
>>> + ret = -EFAULT;
>>> +
>>> +err:
>>> + kzfree(outbuf);
>>> + crypto_free_rng(tfm);
>>> + return ret;
>>> +}
>>> long keyctl_dh_compute(struct keyctl_dh_params __user *params,
>>>
>>> char __user *buffer, size_t buflen,
>>>
>>> - void __user *reserved)
>>> + struct keyctl_kdf_params __user *kdf)
>>> {
>>>
>>> long ret;
>>> MPI base, private, prime, result;
>>> unsigned nbytes;
>>> struct keyctl_dh_params pcopy;
>>>
>>> + struct keyctl_kdf_params kdfcopy;
>>>
>>> uint8_t *kbuf;
>>> ssize_t keylen;
>>> size_t resultlen;
>>>
>>> @@ -98,12 +158,24 @@ long keyctl_dh_compute(struct keyctl_dh_params __user
>>> *params,>
>>> goto out;
>>>
>>> }
>>>
>>> - if (reserved) {
>>> - ret = -EINVAL;
>>> - goto out;
>>> + if (kdf) {
>>> + if (copy_from_user(&kdfcopy, kdf, sizeof(kdfcopy)) != 0) {
>>> + ret = -EFAULT;
>>> + goto out;
>>> + }
>>> + if (buflen > KEYCTL_KDF_MAX_OUTPUTLEN ||
>>> + kdfcopy.otherinfolen > KEYCTL_KDF_MAX_STRING_LEN ||
>>> + kdfcopy.kdfnamelen > KEYCTL_KDF_MAX_STRING_LEN) {
>>> + ret = -EMSGSIZE;
>>> + goto out;
>>> + }
>>>
>>> }
>>>
>>> - keylen = mpi_from_key(pcopy.prime, buflen, &prime);
>>> + /*
>>> + * If the caller requests postprocessing with a KDF, allow an
>>> + * arbitrary output buffer size since the KDF ensures proper truncation.
>>> + */
>>> + keylen = mpi_from_key(pcopy.prime, kdf ? SIZE_MAX : buflen, &prime);
>>>
>>> if (keylen < 0 || !prime) {
>>>
>>> /* buflen == 0 may be used to query the required buffer size,
>>>
>>> * which is the prime key length.
>>>
>>> @@ -133,7 +205,8 @@ long keyctl_dh_compute(struct keyctl_dh_params __user
>>> *params,>
>>> goto error3;
>>>
>>> }
>>>
>>> - kbuf = kmalloc(resultlen, GFP_KERNEL);
>>> + kbuf = kmalloc(kdf ? (resultlen + kdfcopy.otherinfolen) : resultlen,
>>> + GFP_KERNEL);
>>>
>>> if (!kbuf) {
>>>
>>> ret = -ENOMEM;
>>> goto error4;
>>>
>>> @@ -147,12 +220,17 @@ long keyctl_dh_compute(struct keyctl_dh_params
>>> __user *params,>
>>> if (ret != 0)
>>>
>>> goto error5;
>>>
>>> - ret = nbytes;
>>> - if (copy_to_user(buffer, kbuf, nbytes) != 0)
>>> - ret = -EFAULT;
>>> + if (kdf) {
>>> + ret = keyctl_dh_compute_kdf(&kdfcopy, buffer, buflen,
>>> + kbuf, resultlen);
>>> + } else {
>>> + ret = nbytes;
>>> + if (copy_to_user(buffer, kbuf, nbytes) != 0)
>>> + ret = -EFAULT;
>>> + }
>>>
>>> error5:
>>> - kfree(kbuf);
>>> + kzfree(kbuf);
>>
>> Thanks for adjusting this.
>
> I hope it is ok to not have it in a separate patch.
>>
>>> error4:
>>> mpi_free(result);
>>>
>>> error3:
>>> diff --git a/security/keys/internal.h b/security/keys/internal.h
>>> index a705a7d..35a8d11 100644
>>> --- a/security/keys/internal.h
>>> +++ b/security/keys/internal.h
>>> @@ -259,12 +259,13 @@ static inline long keyctl_get_persistent(uid_t uid,
>>> key_serial_t destring) #endif
>>>
>>> #ifdef CONFIG_KEY_DH_OPERATIONS
>>> +#include <crypto/rng.h>
>>> extern long keyctl_dh_compute(struct keyctl_dh_params __user *, char
>>> __user *, - size_t, void __user *);
>>> + size_t, struct keyctl_kdf_params __user *);
>>> #else
>>> static inline long keyctl_dh_compute(struct keyctl_dh_params __user
>>> *params,>
>>> char __user *buffer, size_t buflen,
>>>
>>> - void __user *reserved)
>>> + struct keyctl_kdf_params __user *kdf)
>>> {
>>>
>>> return -EOPNOTSUPP;
>>>
>>> }
>>> diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
>>> index d580ad0..b106898 100644
>>> --- a/security/keys/keyctl.c
>>> +++ b/security/keys/keyctl.c
>>> @@ -1689,7 +1689,7 @@ SYSCALL_DEFINE5(keyctl, int, option, unsigned long,
>>> arg2, unsigned long, arg3,>
>>> case KEYCTL_DH_COMPUTE:
>>> return keyctl_dh_compute((struct keyctl_dh_params __user *) arg2,
>>>
>>> (char __user *) arg3, (size_t) arg4,
>>>
>>> - (void __user *) arg5);
>>> + (struct keyctl_kdf_params __user *) arg5);
>>>
>>> default:
>>> return -EOPNOTSUPP;
--
Mat Martineau
Intel OTC
^ permalink raw reply
* Re: [V2][PATCH 1/2] PKCS#7: Fix kernel panic when referring to the empty AuthorityKeyIdentifier
From: Lans Zhang @ 2016-07-16 1:31 UTC (permalink / raw)
To: David Howells; +Cc: bhe, vgoyal, kexec, linux-crypto
In-Reply-To: <3760.1468594137@warthog.procyon.org.uk>
On 07/15/2016 10:48 PM, David Howells wrote:
> Lans Zhang<jia.zhang@windriver.com> wrote:
>
>> This fix resolves the following kernel panic if the empty
>> AuthorityKeyIdentifier employed.
>
> It should be noted that this is only an issue if DEBUG is #defined at the top
> of pkcs7_verify.c as the crash happens in a pr_debug() statement.
>
Yep and your previous analysis is correct.
Let me know if I need to add this comment to commit header.
Cheers,
Jia
> David
>
^ permalink raw reply
* Re: [V2][PATCH 1/2] PKCS#7: Fix kernel panic when referring to the empty AuthorityKeyIdentifier
From: David Howells @ 2016-07-17 22:58 UTC (permalink / raw)
To: Lans Zhang; +Cc: dhowells, bhe, vgoyal, kexec, linux-crypto
In-Reply-To: <57898E6A.5080101@windriver.com>
Lans Zhang <jia.zhang@windriver.com> wrote:
> Let me know if I need to add this comment to commit header.
I've done that.
David
^ permalink raw reply
* [PATCH 0/3] KEYS: Miscellaneous fixes
From: David Howells @ 2016-07-17 23:10 UTC (permalink / raw)
To: jmorris
Cc: dhowells, keyring, linux-security-module, linux-kernel,
linux-crypto
Hi James,
Here are three miscellaneous fixes:
(1) Fix a panic in some debugging code in PKCS#7. This can only happen by
explicitly inserting a #define DEBUG into the code.
(2) Fix the calculation of the digest length in the PE file parser. This
causes a failure where there should be a success.
(3) Fix the case where an X.509 cert can be added as an asymmetric key to
a trusted keyring with no trust restriction if no AKID is supplied.
Bugs (1) and (2) aren't particularly problematic, but (3) allows a security
check to be bypassed. Bug (3) is added since the 4.6 kernel.
The patches can be found here also:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=keys-fixes
at tag:
keys-fixes-20160718
David
---
Lans Zhang (2):
PKCS#7: Fix panic when referring to the empty AKID when DEBUG defined
pefile: Fix the failure of calculation for digest
Mat Martineau (1):
KEYS: Fix for erroneous trust of incorrectly signed X.509 certs
crypto/asymmetric_keys/mscode_parser.c | 7 ++++++-
crypto/asymmetric_keys/pkcs7_verify.c | 2 +-
crypto/asymmetric_keys/restrict.c | 2 +-
3 files changed, 8 insertions(+), 3 deletions(-)
^ permalink raw reply
* [PATCH 1/3] PKCS#7: Fix panic when referring to the empty AKID when DEBUG defined
From: David Howells @ 2016-07-17 23:10 UTC (permalink / raw)
To: jmorris
Cc: keyring, Lans Zhang, Baoquan He, kexec, linux-kernel, dhowells,
linux-security-module, linux-crypto, Dave Young, Vivek Goyal
In-Reply-To: <146879703192.32133.3670984393495441516.stgit@warthog.procyon.org.uk>
From: Lans Zhang <jia.zhang@windriver.com>
This fix resolves the following kernel panic if an empty or missing
AuthorityKeyIdentifier is encountered and DEBUG is defined in
pkcs7_verify.c.
[ 459.041989] PKEY: <==public_key_verify_signature() = 0
[ 459.041993] PKCS7: Verified signature 1
[ 459.041995] PKCS7: ==> pkcs7_verify_sig_chain()
[ 459.041999] PKCS7: verify Sample DB Certificate for SCP: 01
[ 459.042002] PKCS7: - issuer Sample KEK Certificate for SCP
[ 459.042014] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 459.042135] IP: [<ffffffff813e7b4c>] pkcs7_verify+0x72c/0x7f0
[ 459.042217] PGD 739e6067 PUD 77719067 PMD 0
[ 459.042286] Oops: 0000 [#1] PREEMPT SMP
[ 459.042328] Modules linked in:
[ 459.042368] CPU: 0 PID: 474 Comm: kexec Not tainted 4.7.0-rc7-WR8.0.0.0_standard+ #18
[ 459.042462] Hardware name: To be filled by O.E.M. To be filled by O.E.M./Aptio CRB, BIOS 5.6.5 10/09/2014
[ 459.042586] task: ffff880073a50000 ti: ffff8800738e8000 task.ti: ffff8800738e8000
[ 459.042675] RIP: 0010:[<ffffffff813e7b4c>] [<ffffffff813e7b4c>] pkcs7_verify+0x72c/0x7f0
[ 459.042784] RSP: 0018:ffff8800738ebd58 EFLAGS: 00010246
[ 459.042845] RAX: 0000000000000000 RBX: ffff880076b7da80 RCX: 0000000000000006
[ 459.042929] RDX: 0000000000000001 RSI: ffffffff81c85001 RDI: ffffffff81ca00a9
[ 459.043014] RBP: ffff8800738ebd98 R08: 0000000000000400 R09: ffff8800788a304c
[ 459.043098] R10: 0000000000000000 R11: 00000000000060ca R12: ffff8800769a2bc0
[ 459.043182] R13: ffff880077358300 R14: 0000000000000000 R15: ffff8800769a2dc0
[ 459.043268] FS: 00007f24cc741700(0000) GS:ffff880074e00000(0000) knlGS:0000000000000000
[ 459.043365] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 459.043431] CR2: 0000000000000000 CR3: 0000000073a36000 CR4: 00000000001006f0
[ 459.043514] Stack:
[ 459.043530] 0000000000000000 ffffffbf00000020 31ffffff813e68b0 0000000000000002
[ 459.043644] ffff8800769a2bc0 0000000000000000 00000000007197b8 0000000000000002
[ 459.043756] ffff8800738ebdd8 ffffffff81153fb1 0000000000000000 0000000000000000
[ 459.043869] Call Trace:
[ 459.043898] [<ffffffff81153fb1>] verify_pkcs7_signature+0x61/0x140
[ 459.043974] [<ffffffff813e7f0b>] verify_pefile_signature+0x2cb/0x830
[ 459.044052] [<ffffffff813e8470>] ? verify_pefile_signature+0x830/0x830
[ 459.044134] [<ffffffff81048e25>] bzImage64_verify_sig+0x15/0x20
[ 459.046332] [<ffffffff81046e09>] arch_kexec_kernel_verify_sig+0x29/0x40
[ 459.048552] [<ffffffff810f10e4>] SyS_kexec_file_load+0x1f4/0x6c0
[ 459.050768] [<ffffffff81050e36>] ? __do_page_fault+0x1b6/0x550
[ 459.052996] [<ffffffff8199241f>] entry_SYSCALL_64_fastpath+0x17/0x93
[ 459.055242] Code: e8 0a d6 ff ff 85 c0 0f 88 7a fb ff ff 4d 39 fd 4d 89 7d 08 74 45 4d 89 fd e9 14 fe ff ff 4d 8b 76 08 31 c0 48 c7 c7 a9 00 ca 81 <41> 0f b7 36 49 8d 56 02 e8 d0 91 d6 ff 4d 8b 3c 24 4d 85 ff 0f
[ 459.060535] RIP [<ffffffff813e7b4c>] pkcs7_verify+0x72c/0x7f0
[ 459.063040] RSP <ffff8800738ebd58>
[ 459.065456] CR2: 0000000000000000
[ 459.075998] ---[ end trace c15f0e897cda28dc ]---
Signed-off-by: Lans Zhang <jia.zhang@windriver.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Cc: Dave Young <dyoung@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
cc: linux-crypto@vger.kernel.org
cc: kexec@lists.infradead.org
---
crypto/asymmetric_keys/pkcs7_verify.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 44b746e9df1b..2ffd69769466 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -227,7 +227,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
if (asymmetric_key_id_same(p->id, auth))
goto found_issuer_check_skid;
}
- } else {
+ } else if (sig->auth_ids[1]) {
auth = sig->auth_ids[1];
pr_debug("- want %*phN\n", auth->len, auth->data);
for (p = pkcs7->certs; p; p = p->next) {
^ permalink raw reply related
* [PATCH 2/3] pefile: Fix the failure of calculation for digest
From: David Howells @ 2016-07-17 23:10 UTC (permalink / raw)
To: jmorris
Cc: keyring, Lans Zhang, Baoquan He, kexec, linux-kernel, dhowells,
linux-security-module, linux-crypto, Dave Young, Vivek Goyal
In-Reply-To: <146879703192.32133.3670984393495441516.stgit@warthog.procyon.org.uk>
From: Lans Zhang <jia.zhang@windriver.com>
Commit e68503bd68 forgot to set digest_len and thus cause the following
error reported by kexec when launching a crash kernel:
kexec_file_load failed: Bad message
Fixes: e68503bd68 (KEYS: Generalise system_verify_data() to provide access to internal content)
Signed-off-by: Lans Zhang <jia.zhang@windriver.com>
Tested-by: Dave Young <dyoung@redhat.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Cc: Baoquan He <bhe@redhat.com>
Cc: Vivek Goyal <vgoyal@redhat.com>
cc: kexec@lists.infradead.org
cc: linux-crypto@vger.kernel.org
---
crypto/asymmetric_keys/mscode_parser.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/crypto/asymmetric_keys/mscode_parser.c b/crypto/asymmetric_keys/mscode_parser.c
index 6a76d5c70ef6..9492e1c22d38 100644
--- a/crypto/asymmetric_keys/mscode_parser.c
+++ b/crypto/asymmetric_keys/mscode_parser.c
@@ -124,5 +124,10 @@ int mscode_note_digest(void *context, size_t hdrlen,
struct pefile_context *ctx = context;
ctx->digest = kmemdup(value, vlen, GFP_KERNEL);
- return ctx->digest ? 0 : -ENOMEM;
+ if (!ctx->digest)
+ return -ENOMEM;
+
+ ctx->digest_len = vlen;
+
+ return 0;
}
^ permalink raw reply related
* [PATCH 3/3] KEYS: Fix for erroneous trust of incorrectly signed X.509 certs
From: David Howells @ 2016-07-17 23:10 UTC (permalink / raw)
To: jmorris
Cc: keyring, Petko Manolov, Mat Martineau, linux-kernel, dhowells,
linux-security-module, linux-crypto
In-Reply-To: <146879703192.32133.3670984393495441516.stgit@warthog.procyon.org.uk>
From: Mat Martineau <mathew.j.martineau@linux.intel.com>
Arbitrary X.509 certificates without authority key identifiers (AKIs)
can be added to "trusted" keyrings, including IMA or EVM certs loaded
from the filesystem. Signature verification is currently bypassed for
certs without AKIs.
Trusted keys were recently refactored, and this bug is not present in
4.6.
restrict_link_by_signature should return -ENOKEY (no matching parent
certificate found) if the certificate being evaluated has no AKIs,
instead of bypassing signature checks and returning 0 (new certificate
accepted).
Reported-by: Petko Manolov <petkan@mip-labs.com>
Signed-off-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---
crypto/asymmetric_keys/restrict.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index ac4bddf669de..19d1afb9890f 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -87,7 +87,7 @@ int restrict_link_by_signature(struct key *trust_keyring,
sig = payload->data[asym_auth];
if (!sig->auth_ids[0] && !sig->auth_ids[1])
- return 0;
+ return -ENOKEY;
if (ca_keyid && !asymmetric_key_id_partial(sig->auth_ids[1], ca_keyid))
return -EPERM;
^ permalink raw reply related
* Re: [PATCH 5/6] crypto: img-hash - Add support for export and import
From: Herbert Xu @ 2016-07-18 6:13 UTC (permalink / raw)
To: Will Thomas; +Cc: linux-crypto, james.hartley
In-Reply-To: <578649C2.801@imgtec.com>
On Wed, Jul 13, 2016 at 03:01:38PM +0100, Will Thomas wrote:
>
> I don't see any other drivers explicitly requesting a fallback
> driver by name. Anyways, should this be done using "sha1-generic"
The problem is that you're relying on the fallback to implement
import/export. So if the fallback changes, then your algorithm
parameters will change which is not allowed.
> in the crypto_alloc_ahash call when setting up the fallback tfm?
Yes that should be sufficient.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3 1/4] crypto: add template handling for RNGs
From: Herbert Xu @ 2016-07-18 7:14 UTC (permalink / raw)
To: Stephan Mueller; +Cc: mathew.j.martineau, dhowells, keyrings, linux-crypto
In-Reply-To: <2944776.2qGGKJgDkv@positron.chronox.de>
Stephan Mueller <smueller@chronox.de> wrote:
> This patch adds the ability to register templates for RNGs. RNGs are
> "meta" mechanisms using raw cipher primitives. Thus, RNGs can now be
> implemented as templates to allow the complete flexibility the kernel
> crypto API provides.
>
> Signed-off-by: Stephan Mueller <smueller@chronox.de>
> ---
> crypto/rng.c | 31 +++++++++++++++++++++++++++++++
> include/crypto/rng.h | 39 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 70 insertions(+)
>
> diff --git a/crypto/rng.c b/crypto/rng.c
> index b81cffb..92cc02a 100644
> --- a/crypto/rng.c
> +++ b/crypto/rng.c
> @@ -232,5 +232,36 @@ void crypto_unregister_rngs(struct rng_alg *algs, int count)
> }
> EXPORT_SYMBOL_GPL(crypto_unregister_rngs);
>
> +void rng_free_instance(struct crypto_instance *inst)
> +{
> + crypto_drop_spawn(crypto_instance_ctx(inst));
> + kfree(rng_instance(inst));
> +}
Please use the new free interface, i.e.,
void rng_free_instance(struct rng_instance *inst)
and then
inst->free = rng_free_instance;
> +static inline struct rng_alg *__crypto_rng_alg(struct crypto_alg *alg)
> +{
> + return container_of(alg, struct rng_alg, base);
> +}
> +
> +static inline struct rng_instance *rng_instance(
> + struct crypto_instance *inst)
> +{
> + return container_of(__crypto_rng_alg(&inst->alg),
> + struct rng_instance, alg);
> +}
These two can then be deleted.
Thanks,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH v3 1/4] crypto: add template handling for RNGs
From: Stephan Mueller @ 2016-07-18 7:18 UTC (permalink / raw)
To: Herbert Xu; +Cc: mathew.j.martineau, dhowells, keyrings, linux-crypto
In-Reply-To: <20160718071417.GA12600@gondor.apana.org.au>
Am Montag, 18. Juli 2016, 15:14:17 schrieb Herbert Xu:
Hi Herbert,
> >
> > diff --git a/crypto/rng.c b/crypto/rng.c
> > index b81cffb..92cc02a 100644
> > --- a/crypto/rng.c
> > +++ b/crypto/rng.c
> > @@ -232,5 +232,36 @@ void crypto_unregister_rngs(struct rng_alg *algs, int
> > count) }
> > EXPORT_SYMBOL_GPL(crypto_unregister_rngs);
> >
> > +void rng_free_instance(struct crypto_instance *inst)
> > +{
> > + crypto_drop_spawn(crypto_instance_ctx(inst));
> > + kfree(rng_instance(inst));
> > +}
>
> Please use the new free interface, i.e.,
>
> void rng_free_instance(struct rng_instance *inst)
>
> and then
>
> inst->free = rng_free_instance;
>
> > +static inline struct rng_alg *__crypto_rng_alg(struct crypto_alg *alg)
> > +{
> > + return container_of(alg, struct rng_alg, base);
> > +}
> > +
> > +static inline struct rng_instance *rng_instance(
> > + struct crypto_instance *inst)
> > +{
> > + return container_of(__crypto_rng_alg(&inst->alg),
> > + struct rng_instance, alg);
> > +}
>
> These two can then be deleted.
Thanks. I will add that to the next round.
>
> Thanks,
Ciao
Stephan
^ permalink raw reply
* Re: [patch] crypto: sha256-mb - cleanup a || vs | typo
From: Pavel Machek @ 2016-07-18 8:59 UTC (permalink / raw)
To: Linus Torvalds
Cc: Herbert Xu, Tim Chen, Ingo Molnar, H. Peter Anvin, Dan Carpenter,
David S. Miller, Thomas Gleixner, Ingo Molnar,
the arch/x86 maintainers, Megha Dey, Wang, Rui Y, Denys Vlasenko,
Xiaodong Liu, Linux Crypto Mailing List,
Linux Kernel Mailing List, kernel-janitors, Andrew Morton,
Peter Zijlstra
In-Reply-To: <CA+55aFyQYJerovMsSoSKS7PessZBr4vNp-3QUUwhqk4A4_jcbg@mail.gmail.com>
On Fri 2016-07-08 10:19:26, Linus Torvalds wrote:
> [ rare comment rant. I think I'll do this once, and then ignore the discussion ]
>
> On Fri, Jul 8, 2016 at 9:45 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> >
> > Nack. As I said the commenting style in the crypto API is the
> > same as the network stack. So unless we decide to change both
> > please stick to the current style.
>
> Can we please get rid of the brain-damaged stupid networking comment
> syntax style, PLEASE?
Please? :-). Having different comment styles between networking and
the rest is confusing.
And yes, this uglyness is documented for net/ and drivers/net/, but
not for crypto/, so at the very least Documentation/CodingStyle should
be updated.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* [PATCH] crypto: marvell: Fix wrong flag used for GFP in mv_cesa_dma_add_iv_op
From: Romain Perier @ 2016-07-18 9:32 UTC (permalink / raw)
To: Boris Brezillon, Arnaud Ebalard
Cc: David S. Miller, linux-crypto, Thomas Petazzoni, Jason Cooper,
Andrew Lunn, Sebastian Hesselbarth, Gregory Clement
Use the parameter 'gfp_flags' instead of 'flag' as second argument of
dma_pool_alloc(). The parameter 'flag' is for the TDMA descriptor, its
content has no sense for the allocator.
Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
---
drivers/crypto/marvell/tdma.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c
index 942f4b6..51a631d 100644
--- a/drivers/crypto/marvell/tdma.c
+++ b/drivers/crypto/marvell/tdma.c
@@ -223,7 +223,7 @@ int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src,
if (IS_ERR(tdma))
return PTR_ERR(tdma);
- iv = dma_pool_alloc(cesa_dev->dma->iv_pool, flags, &dma_handle);
+ iv = dma_pool_alloc(cesa_dev->dma->iv_pool, gfp_flags, &dma_handle);
if (!iv)
return -ENOMEM;
--
2.8.1
^ permalink raw reply related
* Re: [PATCH] crypto: marvell: Fix wrong flag used for GFP in mv_cesa_dma_add_iv_op
From: Boris Brezillon @ 2016-07-18 10:00 UTC (permalink / raw)
To: Romain Perier
Cc: Arnaud Ebalard, David S. Miller, linux-crypto, Thomas Petazzoni,
Jason Cooper, Andrew Lunn, Sebastian Hesselbarth, Gregory Clement
In-Reply-To: <1468834344-14160-1-git-send-email-romain.perier@free-electrons.com>
On Mon, 18 Jul 2016 11:32:24 +0200
Romain Perier <romain.perier@free-electrons.com> wrote:
> Use the parameter 'gfp_flags' instead of 'flag' as second argument of
> dma_pool_alloc(). The parameter 'flag' is for the TDMA descriptor, its
> content has no sense for the allocator.
>
> Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Herbert, can you add the following line when applying then patch?
Fixes: bac8e805a30d ("crypto: marvell - Copy IV vectors by DMA transfers for acipher requests")
> ---
> drivers/crypto/marvell/tdma.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/crypto/marvell/tdma.c b/drivers/crypto/marvell/tdma.c
> index 942f4b6..51a631d 100644
> --- a/drivers/crypto/marvell/tdma.c
> +++ b/drivers/crypto/marvell/tdma.c
> @@ -223,7 +223,7 @@ int mv_cesa_dma_add_iv_op(struct mv_cesa_tdma_chain *chain, dma_addr_t src,
> if (IS_ERR(tdma))
> return PTR_ERR(tdma);
>
> - iv = dma_pool_alloc(cesa_dev->dma->iv_pool, flags, &dma_handle);
> + iv = dma_pool_alloc(cesa_dev->dma->iv_pool, gfp_flags, &dma_handle);
> if (!iv)
> return -ENOMEM;
>
^ permalink raw reply
* [PATCH] crypto: testmgr - Print akcipher algorithm name
From: Herbert Xu @ 2016-07-18 10:20 UTC (permalink / raw)
To: Linux Crypto Mailing List
When an akcipher test fails, we don't know which algorithm failed
because the name is not printed. This patch fixes this.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 769cc2a..5c9d5a5 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -2034,6 +2034,8 @@ free_xbuf:
static int test_akcipher(struct crypto_akcipher *tfm, const char *alg,
struct akcipher_testvec *vecs, unsigned int tcount)
{
+ const char *algo =
+ crypto_tfm_alg_driver_name(crypto_akcipher_tfm(tfm));
int ret, i;
for (i = 0; i < tcount; i++) {
@@ -2041,8 +2043,8 @@ static int test_akcipher(struct crypto_akcipher *tfm, const char *alg,
if (!ret)
continue;
- pr_err("alg: akcipher: test failed on vector %d, err=%d\n",
- i + 1, ret);
+ pr_err("alg: akcipher: test %d failed for %s, err=%d\n",
+ i + 1, algo, ret);
return ret;
}
return 0;
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply related
* Re: [PATCH] crypto: rsa-pkcs1pad - Fix akcipher request allocation
From: Herbert Xu @ 2016-07-18 10:23 UTC (permalink / raw)
To: Tadeusz Struk; +Cc: Salvatore Benedetto, davem, andrew.zaborowski, linux-crypto
In-Reply-To: <bed68e2b-6397-1663-606c-6f19695d94ed@gmail.com>
On Thu, Jul 14, 2016 at 08:39:18PM -0700, Tadeusz Struk wrote:
> Hi Salvatore,
> On 07/14/2016 03:25 AM, Salvatore Benedetto wrote:
> > Embedding the akcipher_request in pkcs1pad_request don't take
> > into account the context space required by the akcipher object.
>
> I think we do take into account the sub request context. See line 675.
> The only thing that is wrong is that the child_req should be at
> the end of the structure. This is build tested only.
>
> ---8<---
> From: Tadeusz Struk <tadeusz.struk@intel.com>
> Subject: [PATCH] crypto: rsa-pkcs1pad - fix rsa-pkcs1pad request struct
>
> To allow for child request context the struct akcipher_request child_req
> needs to be at the end of the structure.
>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [patch] crypto: nx - off by one bug in nx_of_update_msc()
From: Herbert Xu @ 2016-07-18 10:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: Leonidas S. Barbosa, Kent Yoder, Paulo Flabiano Smorigo,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
David S. Miller, linux-crypto, linuxppc-dev, kernel-janitors
In-Reply-To: <20160715110912.GE9258@mwanda>
On Fri, Jul 15, 2016 at 02:09:13PM +0300, Dan Carpenter wrote:
> The props->ap[] array is defined like this:
>
> struct alg_props ap[NX_MAX_FC][NX_MAX_MODE][3];
>
> So we can see that if msc->fc and msc->mode are == to NX_MAX_FC or
> NX_MAX_MODE then we're off by one.
>
> Fixes: ae0222b7289d ('powerpc/crypto: nx driver code supporting nx encryption')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Patch applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 0/2] Automatically load the vmx_crypto module if supported
From: Herbert Xu @ 2016-07-18 10:22 UTC (permalink / raw)
To: alastair
Cc: mpe, benh, paulus, davem, linuxppc-dev, linux-crypto,
linux-kernel, Alastair D'Silva
In-Reply-To: <1468388840-12068-1-git-send-email-alastair@au1.ibm.com>
On Wed, Jul 13, 2016 at 03:47:16PM +1000, alastair@au1.ibm.com wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> This series allows the vmx_crypto module to be detected and automatically
> loaded via UDEV if the CPU supports the vector crypto feature.
>
> Alastair D'Silva (2):
> powerpc: Add module autoloading based on CPU features
> crypto: vmx - Convert to CPU feature based module autoloading
Both patches applied. Thanks.
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox