Re: [V2][PATCH 1/2] PKCS#7: Fix kernel panic when referring to the empty AuthorityKeyIdentifier

[Lans Zhang]

On 07/15/2016 10:48 PM, David Howells wrote:

Lans Zhang  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



--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH] KEYS: add SP800-56A KDF support for DH

[Mat Martineau]

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 
---
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=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(, 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, 

Re: [RFC PATCH] KEYS: add SP800-56A KDF support for DH

[Stephan Mueller]

Am Donnerstag, 14. Juli 2016, 17:45:59 schrieb Mat Martineau:

Hi Mat,

> > Signed-off-by: Stephan Mueller 
> > ---
> > 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=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(, 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,
> > + 

Re: [V2][PATCH 1/2] PKCS#7: Fix kernel panic when referring to the empty AuthorityKeyIdentifier

[David Howells]

Lans Zhang  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
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[patch] crypto: nx - off by one bug in nx_of_update_msc()

[Dan Carpenter]

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 

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);
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html