* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 10:13 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905085348.GJ28598@console-pimps.org>
Hi Matt,
First, thanks for your review!
於 四，2013-09-05 於 09:53 +0100，Matt Fleming 提到：
> On Thu, 22 Aug, at 07:01:50PM, Lee, Chun-Yi wrote:
> > +static int efi_status_to_err(efi_status_t status)
> > +{
> > + int err;
> > +
> > + switch (status) {
> > + case EFI_INVALID_PARAMETER:
> > + err = -EINVAL;
> > + break;
> > + case EFI_OUT_OF_RESOURCES:
> > + err = -ENOSPC;
> > + break;
> > + case EFI_DEVICE_ERROR:
> > + err = -EIO;
> > + break;
> > + case EFI_WRITE_PROTECTED:
> > + err = -EROFS;
> > + break;
> > + case EFI_SECURITY_VIOLATION:
> > + err = -EACCES;
> > + break;
> > + case EFI_NOT_FOUND:
> > + err = -ENODATA;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
> Please don't reimplement this. Instead make the existing function
> global.
>
OK, I will make the function to global.
> [...]
>
> > +static void *load_wake_key_data(unsigned long *datasize)
> > +{
> > + u32 attr;
> > + void *wkey_data;
> > + efi_status_t status;
> > +
> > + if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > + return ERR_PTR(-EPERM);
> > +
> > + /* obtain the size */
> > + *datasize = 0;
> > + status = efi.get_variable(EFI_S4_WAKE_KEY_NAME, &EFI_HIBERNATE_GUID,
> > + NULL, datasize, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL) {
> > + wkey_data = ERR_PTR(efi_status_to_err(status));
> > + pr_err("PM: Couldn't get wake key data size: 0x%lx\n", status);
> > + goto error;
> > + }
>
> Is it safe to completely bypass the efivars interface and access
> efi.get_variable() directly? I wouldn't have thought so, unless you can
> guarantee that the kernel isn't going to access any of the EFI runtime
> services while you execute this function.
>
This S4WakeKey is a VOLATILE variable that could not modify by
SetVariable() at runtime. So, it's read only even through efivars.
Does it what your concern?
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 10:13 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905085348.GJ28598@console-pimps.org>
Hi Matt,
First, thanks for your review!
於 四，2013-09-05 於 09:53 +0100，Matt Fleming 提到：
> On Thu, 22 Aug, at 07:01:50PM, Lee, Chun-Yi wrote:
> > +static int efi_status_to_err(efi_status_t status)
> > +{
> > + int err;
> > +
> > + switch (status) {
> > + case EFI_INVALID_PARAMETER:
> > + err = -EINVAL;
> > + break;
> > + case EFI_OUT_OF_RESOURCES:
> > + err = -ENOSPC;
> > + break;
> > + case EFI_DEVICE_ERROR:
> > + err = -EIO;
> > + break;
> > + case EFI_WRITE_PROTECTED:
> > + err = -EROFS;
> > + break;
> > + case EFI_SECURITY_VIOLATION:
> > + err = -EACCES;
> > + break;
> > + case EFI_NOT_FOUND:
> > + err = -ENODATA;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
> Please don't reimplement this. Instead make the existing function
> global.
>
OK, I will make the function to global.
> [...]
>
> > +static void *load_wake_key_data(unsigned long *datasize)
> > +{
> > + u32 attr;
> > + void *wkey_data;
> > + efi_status_t status;
> > +
> > + if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > + return ERR_PTR(-EPERM);
> > +
> > + /* obtain the size */
> > + *datasize = 0;
> > + status = efi.get_variable(EFI_S4_WAKE_KEY_NAME, &EFI_HIBERNATE_GUID,
> > + NULL, datasize, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL) {
> > + wkey_data = ERR_PTR(efi_status_to_err(status));
> > + pr_err("PM: Couldn't get wake key data size: 0x%lx\n", status);
> > + goto error;
> > + }
>
> Is it safe to completely bypass the efivars interface and access
> efi.get_variable() directly? I wouldn't have thought so, unless you can
> guarantee that the kernel isn't going to access any of the EFI runtime
> services while you execute this function.
>
This S4WakeKey is a VOLATILE variable that could not modify by
SetVariable() at runtime. So, it's read only even through efivars.
Does it what your concern?
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 10:13 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
opensuse-kernel-stAJ6ESoqRxg9hUCZPvPmw, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina-IBi9RG/b67k, Rusty Russell, Herbert Xu,
David S. Miller, H. Peter Anvin, Michal Marek, Gary Lin,
Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905085348.GJ28598-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>
Hi Matt,
First, thanks for your review!
於 四，2013-09-05 於 09:53 +0100，Matt Fleming 提到：
> On Thu, 22 Aug, at 07:01:50PM, Lee, Chun-Yi wrote:
> > +static int efi_status_to_err(efi_status_t status)
> > +{
> > + int err;
> > +
> > + switch (status) {
> > + case EFI_INVALID_PARAMETER:
> > + err = -EINVAL;
> > + break;
> > + case EFI_OUT_OF_RESOURCES:
> > + err = -ENOSPC;
> > + break;
> > + case EFI_DEVICE_ERROR:
> > + err = -EIO;
> > + break;
> > + case EFI_WRITE_PROTECTED:
> > + err = -EROFS;
> > + break;
> > + case EFI_SECURITY_VIOLATION:
> > + err = -EACCES;
> > + break;
> > + case EFI_NOT_FOUND:
> > + err = -ENODATA;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
> Please don't reimplement this. Instead make the existing function
> global.
>
OK, I will make the function to global.
> [...]
>
> > +static void *load_wake_key_data(unsigned long *datasize)
> > +{
> > + u32 attr;
> > + void *wkey_data;
> > + efi_status_t status;
> > +
> > + if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > + return ERR_PTR(-EPERM);
> > +
> > + /* obtain the size */
> > + *datasize = 0;
> > + status = efi.get_variable(EFI_S4_WAKE_KEY_NAME, &EFI_HIBERNATE_GUID,
> > + NULL, datasize, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL) {
> > + wkey_data = ERR_PTR(efi_status_to_err(status));
> > + pr_err("PM: Couldn't get wake key data size: 0x%lx\n", status);
> > + goto error;
> > + }
>
> Is it safe to completely bypass the efivars interface and access
> efi.get_variable() directly? I wouldn't have thought so, unless you can
> guarantee that the kernel isn't going to access any of the EFI runtime
> services while you execute this function.
>
This S4WakeKey is a VOLATILE variable that could not modify by
SetVariable() at runtime. So, it's read only even through efivars.
Does it what your concern?
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 10:13 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905085348.GJ28598@console-pimps.org>
Hi Matt,
First, thanks for your review!
於 四，2013-09-05 於 09:53 +0100，Matt Fleming 提到：
> On Thu, 22 Aug, at 07:01:50PM, Lee, Chun-Yi wrote:
> > +static int efi_status_to_err(efi_status_t status)
> > +{
> > + int err;
> > +
> > + switch (status) {
> > + case EFI_INVALID_PARAMETER:
> > + err = -EINVAL;
> > + break;
> > + case EFI_OUT_OF_RESOURCES:
> > + err = -ENOSPC;
> > + break;
> > + case EFI_DEVICE_ERROR:
> > + err = -EIO;
> > + break;
> > + case EFI_WRITE_PROTECTED:
> > + err = -EROFS;
> > + break;
> > + case EFI_SECURITY_VIOLATION:
> > + err = -EACCES;
> > + break;
> > + case EFI_NOT_FOUND:
> > + err = -ENODATA;
> > + break;
> > + default:
> > + err = -EINVAL;
> > + }
> > +
> > + return err;
> > +}
>
> Please don't reimplement this. Instead make the existing function
> global.
>
OK, I will make the function to global.
> [...]
>
> > +static void *load_wake_key_data(unsigned long *datasize)
> > +{
> > + u32 attr;
> > + void *wkey_data;
> > + efi_status_t status;
> > +
> > + if (!efi_enabled(EFI_RUNTIME_SERVICES))
> > + return ERR_PTR(-EPERM);
> > +
> > + /* obtain the size */
> > + *datasize = 0;
> > + status = efi.get_variable(EFI_S4_WAKE_KEY_NAME, &EFI_HIBERNATE_GUID,
> > + NULL, datasize, NULL);
> > + if (status != EFI_BUFFER_TOO_SMALL) {
> > + wkey_data = ERR_PTR(efi_status_to_err(status));
> > + pr_err("PM: Couldn't get wake key data size: 0x%lx\n", status);
> > + goto error;
> > + }
>
> Is it safe to completely bypass the efivars interface and access
> efi.get_variable() directly? I wouldn't have thought so, unless you can
> guarantee that the kernel isn't going to access any of the EFI runtime
> services while you execute this function.
>
This S4WakeKey is a VOLATILE variable that could not modify by
SetVariable() at runtime. So, it's read only even through efivars.
Does it what your concern?
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: Matt Fleming @ 2013-09-05 10:31 UTC (permalink / raw)
To: joeyli
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
opensuse-kernel-stAJ6ESoqRxg9hUCZPvPmw, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina-IBi9RG/b67k, Rusty Russell, Herbert Xu,
David S. Miller, H. Peter Anvin, Michal Marek, Gary Lin,
Vivek Goyal, Takashi Iwai
In-Reply-To: <1378376016.6193.71.camel-ONCj+Eqt86TasUa73XJKwA@public.gmane.org>
On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> This S4WakeKey is a VOLATILE variable that could not modify by
> SetVariable() at runtime. So, it's read only even through efivars.
>
> Does it what your concern?
No, the UEFI spec probibits certain runtime functions from being
executed concurrently on separate cpus and the spinlock used in the
efivars code ensures that we adhere to that restriction. See table 31 in
section 7.1 of the UEFI 2.4 spec for the list of services that are
non-reentrant.
The problem isn't that we want to avoid simultaneous access to
S4WakeKey, it's that we can't invoke any of the variable runtime service
functions concurrently.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 13:28 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905103158.GM28598@console-pimps.org>
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到：
> On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> > This S4WakeKey is a VOLATILE variable that could not modify by
> > SetVariable() at runtime. So, it's read only even through efivars.
> >
> > Does it what your concern?
>
> No, the UEFI spec probibits certain runtime functions from being
> executed concurrently on separate cpus and the spinlock used in the
> efivars code ensures that we adhere to that restriction. See table 31 in
> section 7.1 of the UEFI 2.4 spec for the list of services that are
> non-reentrant.
>
> The problem isn't that we want to avoid simultaneous access to
> S4WakeKey, it's that we can't invoke any of the variable runtime service
> functions concurrently.
>
I see! I will use efivar api to access EFI variable.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 13:28 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905103158.GM28598@console-pimps.org>
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到：
> On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> > This S4WakeKey is a VOLATILE variable that could not modify by
> > SetVariable() at runtime. So, it's read only even through efivars.
> >
> > Does it what your concern?
>
> No, the UEFI spec probibits certain runtime functions from being
> executed concurrently on separate cpus and the spinlock used in the
> efivars code ensures that we adhere to that restriction. See table 31 in
> section 7.1 of the UEFI 2.4 spec for the list of services that are
> non-reentrant.
>
> The problem isn't that we want to avoid simultaneous access to
> S4WakeKey, it's that we can't invoke any of the variable runtime service
> functions concurrently.
>
I see! I will use efivar api to access EFI variable.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 13:28 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905103158.GM28598@console-pimps.org>
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到：
> On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> > This S4WakeKey is a VOLATILE variable that could not modify by
> > SetVariable() at runtime. So, it's read only even through efivars.
> >
> > Does it what your concern?
>
> No, the UEFI spec probibits certain runtime functions from being
> executed concurrently on separate cpus and the spinlock used in the
> efivars code ensures that we adhere to that restriction. See table 31 in
> section 7.1 of the UEFI 2.4 spec for the list of services that are
> non-reentrant.
>
> The problem isn't that we want to avoid simultaneous access to
> S4WakeKey, it's that we can't invoke any of the variable runtime service
> functions concurrently.
>
I see! I will use efivar api to access EFI variable.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 13:28 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-security-module-u79uwXL29TY76Z2rM5mHXA,
linux-efi-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-crypto-u79uwXL29TY76Z2rM5mHXA,
opensuse-kernel-stAJ6ESoqRxg9hUCZPvPmw, David Howells,
Rafael J. Wysocki, Matthew Garrett, Len Brown, Pavel Machek,
Josh Boyer, Vojtech Pavlik, Matt Fleming, James Bottomley,
Greg KH, JKosina-IBi9RG/b67k, Rusty Russell, Herbert Xu,
David S. Miller, H. Peter Anvin, Michal Marek, Gary Lin,
Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905103158.GM28598-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到：
> On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> > This S4WakeKey is a VOLATILE variable that could not modify by
> > SetVariable() at runtime. So, it's read only even through efivars.
> >
> > Does it what your concern?
>
> No, the UEFI spec probibits certain runtime functions from being
> executed concurrently on separate cpus and the spinlock used in the
> efivars code ensures that we adhere to that restriction. See table 31 in
> section 7.1 of the UEFI 2.4 spec for the list of services that are
> non-reentrant.
>
> The problem isn't that we want to avoid simultaneous access to
> S4WakeKey, it's that we can't invoke any of the variable runtime service
> functions concurrently.
>
I see! I will use efivar api to access EFI variable.
Thanks a lot!
Joey Lee
^ permalink raw reply
* Re: [PATCH 11/18] Hibernate: introduced RSA key-pair to verify signature of snapshot
From: joeyli @ 2013-09-05 13:28 UTC (permalink / raw)
To: Matt Fleming
Cc: linux-kernel, linux-security-module, linux-efi, linux-pm,
linux-crypto, opensuse-kernel, David Howells, Rafael J. Wysocki,
Matthew Garrett, Len Brown, Pavel Machek, Josh Boyer,
Vojtech Pavlik, Matt Fleming, James Bottomley, Greg KH, JKosina,
Rusty Russell, Herbert Xu, David S. Miller, H. Peter Anvin,
Michal Marek, Gary Lin, Vivek Goyal, Takashi Iwai
In-Reply-To: <20130905103158.GM28598@console-pimps.org>
於 四，2013-09-05 於 11:31 +0100，Matt Fleming 提到：
> On Thu, 05 Sep, at 06:13:36PM, joeyli wrote:
> > This S4WakeKey is a VOLATILE variable that could not modify by
> > SetVariable() at runtime. So, it's read only even through efivars.
> >
> > Does it what your concern?
>
> No, the UEFI spec probibits certain runtime functions from being
> executed concurrently on separate cpus and the spinlock used in the
> efivars code ensures that we adhere to that restriction. See table 31 in
> section 7.1 of the UEFI 2.4 spec for the list of services that are
> non-reentrant.
>
> The problem isn't that we want to avoid simultaneous access to
> S4WakeKey, it's that we can't invoke any of the variable runtime service
> functions concurrently.
>
I see! I will use efivar api to access EFI variable.
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* race condition in crypto larval handling
From: Kees Cook @ 2013-09-06 23:20 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
Hi,
I've tracked down a race condition and ref counting problem in the
crypto API internals. We've been seeing it under Chrome OS, but it
seems it's not isolated to just us:
https://code.google.com/p/chromium/issues/detail?id=244581
http://marc.info/?l=linux-crypto-vger&m=135429403609373&w=2
https://bugzilla.redhat.com/show_bug.cgi?id=983682
http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html
I think I've found the basic origin of the problem.
crypto_larval_add() has synchronization to make sure only a single
larval can ever be created. That logic seems totally fine. However,
this means that crypto_larval_lookup() from two threads can return the
same larval, which means that crypto_alg_mod_lookup() runs the risk of
calling crypto_larval_kill() on the same larval twice, which does not
appear to be handled sanely.
I can easily crash the kernel by forcing a synchronization point just
before the "return crypt_larval_add(...)" call in
crypto_larval_lookup(). Basically, I added this sloppy sync code (I
feel like there must be a better way to do this):
+static atomic_t global_sync = ATOMIC_INIT(0);
...
struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
...
+ if (strncmp(name, "asdf", 4) == 0) {
+ int value;
+
+ value = atomic_add_return(1, &global_sync);
+ if (value == 1) {
+ /* I was first to stall here, wait for inc. */
+ while (atomic_read(&global_sync) != 2)
+ cpu_relax();
+ } else if (value == 2) {
+ /* I was second to stall here, wait for dec. */
+ while (atomic_read(&global_sync) != 1)
+ cpu_relax();
+ } else {
+ BUG();
+ }
+ atomic_dec(&global_sync);
+ }
return crypto_larval_add(name, type, mask);
}
And then ran code from userspace that did, effectively:
struct sockaddr_alg sa = {
.salg_family = AF_ALG,
.salg_type = "hash",
};
strcpy(sa.salg_name, argv[1]);
fork();
...
sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
bind(sds[0], (struct sockaddr *) &sa, sizeof(sa));
And ran this as "./tickle asdf1" to generate the two threads trying to
find an invalid alg. The race looks possible even with valid algs, but
this was the fastest path I could see to tickle the issue.
With added printks to the kernel, it was clear that crypto_larval_kill
was being called twice on the same larval, and the list_del() call was
doing bad things. When I fixed that, the refcnt bug became very
obvious. Here's the change I made for crypto_larval_kill:
@@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
struct crypto_larval *larval = (void *)alg;
down_write(&crypto_alg_sem);
- list_del(&alg->cra_list);
+ if (!list_empty(&alg->cra_list))
+ list_del_init(&alg->cra_list);
up_write(&crypto_alg_sem);
complete_all(&larval->completion);
crypto_alg_put(alg);
It seems that there are too many "put" calls (mixed between
crypto_alg_put() and crypto_mod_put(), which also calls
crypto_alg_put()). See this annotated portion of
crypto_alg_mod_lookup:
/* This can (correctly) return the same larval for two threads. */
larval = crypto_larval_lookup(name, type, mask);
if (IS_ERR(larval) || !crypto_is_larval(larval))
return larval;
ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);
if (ok == NOTIFY_STOP)
/* This calls crypto_mod_put(), but sometimes also get?? */
alg = crypto_larval_wait(larval);
else {
/* This directly calls crypto_mod_put */
crypto_mod_put(larval);
alg = ERR_PTR(-ENOENT);
}
/* This calls crypto_alg_put */
crypto_larval_kill(larval);
return alg;
In the two-thread situation, the first thread gets a larval with
refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
decrements the ref count twice.
It seems to me like either each call to crypto_larval_lookup() should
result in a refcount bumped by two, or crypto_alg_mod_lookup() should
decrement only once, and the initial refcount should be 1 not 2 from
crypto_larval_add. But it's not clear to me which is sensible here.
What's the right solution here?
Thanks,
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Crypto Update for 3.12
From: Herbert Xu @ 2013-09-07 3:55 UTC (permalink / raw)
To: Linus Torvalds, David S. Miller, Linux Kernel Mailing List,
Linux Crypto Mailing List
In-Reply-To: <20130223023331.GA11002@gondor.apana.org.au>
Hi Linus:
Here is the crypto update for 3.12:
* Added MODULE_SOFTDEP to allow pre-loading of modules.
* Reinstated crct10dif driver using the module softdep feature.
* Allow via rng driver to be auto-loaded.
* Split large input data when necessary in nx.
* Handle zero length messages correctly for GCM/XCBC in nx.
* Handle SHA-2 chunks bigger than block size properly in nx.
* Handle unaligned lengths in omap-aes.
* Added SHA384/SHA512 to omap-sham.
* Added OMAP5/AM43XX SHAM support.
* Added OMAP4 TRNG support.
* Misc fixes.
Please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git
Alex Porosanu (2):
crypto: caam - replace xstr macro with __stringify
crypto: caam - add option for enabling DEBUG mode
Andi Kleen (1):
crypto: make tables used from assembler __visible
Andreas Robinson (1):
modules: add support for soft module dependencies
Ben Hutchings (1):
hwrng: via - Add MODULE_DEVICE_TABLE
Chen Gang (1):
padata - share code between CPU_ONLINE and CPU_DOWN_FAILED, same to CPU_DOWN_PREPARE and CPU_UP_CANCELED
Cristian Stoica (1):
crypto: testmgr - remove double execution of the same test suite
Dan Carpenter (2):
crypto: sahara - checking the wrong variable
crypto: tegra-aes - bitwise vs logical and
Fabio Estevam (1):
hwrng: mxc-rnga - Check the return value from clk_prepare_enable()
Fionnuala Gunter (3):
crypto: nx - saves chaining value from co-processor
crypto: nx - fix limits to sg lists for AES-XCBC
crypto: nx - fix limits to sg lists for AES-CCM
Herbert Xu (2):
Merge git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux
Reinstate "crypto: crct10dif - Wrap crc_t10dif function all to use crypto transform framework"
Jan-Simon Möller (1):
crypto: fcrypt - Fix bitoperation for compilation with clang
Jingoo Han (3):
hwrng: pixocel - Staticize 'rng_dev'
crypto: sahara - Staticize local symbol
crypto: crypto4xx - Staticize local symbols
Joe Perches (1):
crypto: ux500 - Fix logging, make arrays const, neatening
Joel Fernandes (14):
crypto: scatterwalk - Add support for calculating number of SG elements
crypto: omap-aes - Add useful debug macros
crypto: omap-aes - Populate number of SG elements
crypto: omap-aes - Simplify DMA usage by using direct SGs
crypto: omap-aes - Sync SG before DMA operation
crypto: omap-aes - Remove previously used intermediate buffers
crypto: omap-aes - Add IRQ info and helper macros
crypto: omap-aes - PIO mode: Add IRQ handler and walk SGs
crypto: omap-aes - PIO mode: platform data for OMAP4/AM437x and trigger
crypto: omap-aes - Switch to PIO mode during probe
crypto: omap-aes - Add support for cases of unaligned lengths
crypto: omap-aes - Convert kzalloc to devm_kzalloc
crypto: omap-aes - Convert request_irq to devm_request_irq
crypto: omap-aes - Kconfig: Add build support for AM437x
John Haxby (1):
crypto: xor - Check for osxsave as well as avx in crypto/xor
Julia Lawall (3):
hwrng: tx4939 - simplify use of devm_ioremap_resource
crypto: camellia-x86-64 - replace commas by semicolons and adjust code alignment
crypto: camellia_generic - replace commas by semicolons and adjust code alignment
Lokesh Vutla (12):
crypto: omap-sham - Add SHA384 and SHA512 Support
crypto: omap-sham - Add OMAP5/AM43XX SHAM Support
crypto: omap-sham - Convert to devm_request_irq()
crypto: omap-sham - Convert to devm_kzalloc()
hwrng: omap - Use module_platform_driver macro
hwrng: omap - Convert to devm_kzalloc()
hwrng: omap - Remove duplicated function call
hwrng: omap - Add device tree support
ARM: OMAP2+: Only manually add hwmod data when DT not used.
hwrng: omap - Add OMAP4 TRNG support
crypto: omap-sham - Enable Polling mode if DMA fails
crypto: omap-sham - correct dma burst size
Marcelo Cerri (11):
crypto: nx - fix physical addresses added to sg lists
crypto: nx - fix limits to sg lists for SHA-2
crypto: nx - fix concurrency issue
crypto: nx - add offset to nx_build_sg_lists()
crypto: nx - fix limits to sg lists for AES-ECB
crypto: nx - fix limits to sg lists for AES-CBC
crypto: nx - fix limits to sg lists for AES-CTR
crypto: nx - fix limits to sg lists for AES-GCM
crypto: nx - fix XCBC for zero length messages
crypto: nx - fix GCM for zero length messages
crypto: nx - fix SHA-2 for chunks bigger than block size
Olof Johansson (1):
hwrng: omap - reorder OMAP TRNG driver code
Richard Weinberger (1):
padata - Register hotcpu notifier after initialization
Ruchika Gupta (2):
crypto: caam - RNG instantiation by directly programming DECO
crypto: caam - Remove unused functions from Job Ring
Vakul Garg (1):
crypto: caam - Moved macro DESC_JOB_IO_LEN to desc_constr.h
jmlatten@linux.vnet.ibm.com (1):
crypto: nx - fix nx-aes-gcm verification
arch/arm/mach-omap2/devices.c | 2 +-
arch/x86/crypto/Makefile | 2 +
arch/x86/crypto/camellia_glue.c | 64 +--
arch/x86/crypto/crct10dif-pcl-asm_64.S | 643 +++++++++++++++++++++++++++++++
arch/x86/crypto/crct10dif-pclmul_glue.c | 151 ++++++++
arch/x86/include/asm/xor_avx.h | 4 +-
crypto/Kconfig | 19 +
crypto/Makefile | 1 +
crypto/aes_generic.c | 8 +-
crypto/camellia_generic.c | 48 +--
crypto/cast_common.c | 8 +-
crypto/crct10dif.c | 178 +++++++++
crypto/fcrypt.c | 2 +-
crypto/scatterwalk.c | 22 ++
crypto/tcrypt.c | 8 +
crypto/testmgr.c | 12 +-
crypto/testmgr.h | 33 ++
drivers/char/hw_random/Kconfig | 6 +-
drivers/char/hw_random/mxc-rnga.c | 4 +-
drivers/char/hw_random/omap-rng.c | 386 +++++++++++++++----
drivers/char/hw_random/picoxcell-rng.c | 2 +-
drivers/char/hw_random/tx4939-rng.c | 4 +-
drivers/char/hw_random/via-rng.c | 7 +
drivers/crypto/Kconfig | 13 +-
drivers/crypto/amcc/crypto4xx_alg.c | 15 +-
drivers/crypto/caam/Kconfig | 8 +
drivers/crypto/caam/Makefile | 3 +
drivers/crypto/caam/caamalg.c | 80 ++--
drivers/crypto/caam/caamhash.c | 70 ++--
drivers/crypto/caam/ctrl.c | 77 ++--
drivers/crypto/caam/desc_constr.h | 1 +
drivers/crypto/caam/intern.h | 5 -
drivers/crypto/caam/jr.c | 67 ----
drivers/crypto/caam/jr.h | 2 -
drivers/crypto/caam/key_gen.c | 6 +-
drivers/crypto/caam/regs.h | 12 +-
drivers/crypto/nx/nx-aes-cbc.c | 57 ++-
drivers/crypto/nx/nx-aes-ccm.c | 283 ++++++++++----
drivers/crypto/nx/nx-aes-ctr.c | 52 ++-
drivers/crypto/nx/nx-aes-ecb.c | 50 ++-
drivers/crypto/nx/nx-aes-gcm.c | 296 ++++++++++----
drivers/crypto/nx/nx-aes-xcbc.c | 205 +++++++---
drivers/crypto/nx/nx-sha256.c | 124 +++---
drivers/crypto/nx/nx-sha512.c | 131 ++++---
drivers/crypto/nx/nx.c | 35 +-
drivers/crypto/nx/nx.h | 3 +-
drivers/crypto/omap-aes.c | 468 ++++++++++++----------
drivers/crypto/omap-sham.c | 382 ++++++++++++++----
drivers/crypto/sahara.c | 4 +-
drivers/crypto/tegra-aes.c | 6 +-
drivers/crypto/ux500/hash/hash_core.c | 586 ++++++++++++++--------------
include/crypto/scatterwalk.h | 2 +
include/linux/crc-t10dif.h | 4 +
include/linux/module.h | 5 +
kernel/padata.c | 32 +-
lib/Kconfig | 2 +
lib/crc-t10dif.c | 74 ++--
57 files changed, 3410 insertions(+), 1364 deletions(-)
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: race condition in crypto larval handling
From: Neil Horman @ 2013-09-07 14:39 UTC (permalink / raw)
To: Kees Cook; +Cc: Herbert Xu, David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <CAGXu5jJvuAC3paV5k_bmoHHrHVgc0Hi6ShSDk3iQUt1Lf0cioQ@mail.gmail.com>
On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
> Hi,
>
> I've tracked down a race condition and ref counting problem in the
> crypto API internals. We've been seeing it under Chrome OS, but it
> seems it's not isolated to just us:
>
> https://code.google.com/p/chromium/issues/detail?id=244581
> http://marc.info/?l=linux-crypto-vger&m=135429403609373&w=2
> https://bugzilla.redhat.com/show_bug.cgi?id=983682
> http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html
>
> I think I've found the basic origin of the problem.
> crypto_larval_add() has synchronization to make sure only a single
> larval can ever be created. That logic seems totally fine. However,
> this means that crypto_larval_lookup() from two threads can return the
> same larval, which means that crypto_alg_mod_lookup() runs the risk of
> calling crypto_larval_kill() on the same larval twice, which does not
> appear to be handled sanely.
>
> I can easily crash the kernel by forcing a synchronization point just
> before the "return crypt_larval_add(...)" call in
> crypto_larval_lookup(). Basically, I added this sloppy sync code (I
> feel like there must be a better way to do this):
>
> +static atomic_t global_sync = ATOMIC_INIT(0);
> ...
> struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
> ...
> + if (strncmp(name, "asdf", 4) == 0) {
> + int value;
> +
> + value = atomic_add_return(1, &global_sync);
> + if (value == 1) {
> + /* I was first to stall here, wait for inc. */
> + while (atomic_read(&global_sync) != 2)
> + cpu_relax();
> + } else if (value == 2) {
> + /* I was second to stall here, wait for dec. */
> + while (atomic_read(&global_sync) != 1)
> + cpu_relax();
> + } else {
> + BUG();
> + }
> + atomic_dec(&global_sync);
> + }
>
> return crypto_larval_add(name, type, mask);
> }
>
> And then ran code from userspace that did, effectively:
>
> struct sockaddr_alg sa = {
> .salg_family = AF_ALG,
> .salg_type = "hash",
> };
> strcpy(sa.salg_name, argv[1]);
>
> fork();
> ...
> sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
> bind(sds[0], (struct sockaddr *) &sa, sizeof(sa));
>
> And ran this as "./tickle asdf1" to generate the two threads trying to
> find an invalid alg. The race looks possible even with valid algs, but
> this was the fastest path I could see to tickle the issue.
>
> With added printks to the kernel, it was clear that crypto_larval_kill
> was being called twice on the same larval, and the list_del() call was
> doing bad things. When I fixed that, the refcnt bug became very
> obvious. Here's the change I made for crypto_larval_kill:
>
> @@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
> struct crypto_larval *larval = (void *)alg;
>
> down_write(&crypto_alg_sem);
> - list_del(&alg->cra_list);
> + if (!list_empty(&alg->cra_list))
> + list_del_init(&alg->cra_list);
> up_write(&crypto_alg_sem);
> complete_all(&larval->completion);
> crypto_alg_put(alg);
>
> It seems that there are too many "put" calls (mixed between
> crypto_alg_put() and crypto_mod_put(), which also calls
> crypto_alg_put()). See this annotated portion of
> crypto_alg_mod_lookup:
>
> /* This can (correctly) return the same larval for two threads. */
> larval = crypto_larval_lookup(name, type, mask);
> if (IS_ERR(larval) || !crypto_is_larval(larval))
> return larval;
>
> ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);
>
> if (ok == NOTIFY_STOP)
> /* This calls crypto_mod_put(), but sometimes also get?? */
> alg = crypto_larval_wait(larval);
> else {
> /* This directly calls crypto_mod_put */
> crypto_mod_put(larval);
> alg = ERR_PTR(-ENOENT);
> }
> /* This calls crypto_alg_put */
> crypto_larval_kill(larval);
> return alg;
>
> In the two-thread situation, the first thread gets a larval with
> refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
> larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
> the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
> decrements the ref count twice.
>
> It seems to me like either each call to crypto_larval_lookup() should
> result in a refcount bumped by two, or crypto_alg_mod_lookup() should
> decrement only once, and the initial refcount should be 1 not 2 from
> crypto_larval_add. But it's not clear to me which is sensible here.
>
> What's the right solution here?
>
> Thanks,
>
> -Kees
>
> --
> Kees Cook
> Chrome OS Security
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
I've been tracking this problem too:
https://bugzilla.redhat.com/show_bug.cgi?id=1002351
Though I'd just started so I've not gotten anywhere near this far. It seems,
given your analysis above that we really need to rework the refcounting here.
At the very least we probably need to:
1) only modify the module reference count when a larval for a given alg is
created, or when its last refcount is decremented.
2) fix up the cra_refcount to start at one and increment for each lookup, and
decrement for each kill.
Neil
^ permalink raw reply
* Re: race condition in crypto larval handling
From: Kees Cook @ 2013-09-07 17:10 UTC (permalink / raw)
To: Neil Horman; +Cc: Herbert Xu, David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <20130907143922.GA13851@neilslaptop.think-freely.org>
On Sat, Sep 7, 2013 at 7:39 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
>> Hi,
>>
>> I've tracked down a race condition and ref counting problem in the
>> crypto API internals. We've been seeing it under Chrome OS, but it
>> seems it's not isolated to just us:
>>
>> https://code.google.com/p/chromium/issues/detail?id=244581
>> http://marc.info/?l=linux-crypto-vger&m=135429403609373&w=2
>> https://bugzilla.redhat.com/show_bug.cgi?id=983682
>> http://www.mail-archive.com/linux-cifs@vger.kernel.org/msg07933.html
>>
>> I think I've found the basic origin of the problem.
>> crypto_larval_add() has synchronization to make sure only a single
>> larval can ever be created. That logic seems totally fine. However,
>> this means that crypto_larval_lookup() from two threads can return the
>> same larval, which means that crypto_alg_mod_lookup() runs the risk of
>> calling crypto_larval_kill() on the same larval twice, which does not
>> appear to be handled sanely.
>>
>> I can easily crash the kernel by forcing a synchronization point just
>> before the "return crypt_larval_add(...)" call in
>> crypto_larval_lookup(). Basically, I added this sloppy sync code (I
>> feel like there must be a better way to do this):
>>
>> +static atomic_t global_sync = ATOMIC_INIT(0);
>> ...
>> struct crypto_alg *crypto_larval_lookup(const char *name, u32 type, u32 mask)
>> ...
>> + if (strncmp(name, "asdf", 4) == 0) {
>> + int value;
>> +
>> + value = atomic_add_return(1, &global_sync);
>> + if (value == 1) {
>> + /* I was first to stall here, wait for inc. */
>> + while (atomic_read(&global_sync) != 2)
>> + cpu_relax();
>> + } else if (value == 2) {
>> + /* I was second to stall here, wait for dec. */
>> + while (atomic_read(&global_sync) != 1)
>> + cpu_relax();
>> + } else {
>> + BUG();
>> + }
>> + atomic_dec(&global_sync);
>> + }
>>
>> return crypto_larval_add(name, type, mask);
>> }
>>
>> And then ran code from userspace that did, effectively:
>>
>> struct sockaddr_alg sa = {
>> .salg_family = AF_ALG,
>> .salg_type = "hash",
>> };
>> strcpy(sa.salg_name, argv[1]);
>>
>> fork();
>> ...
>> sds[0] = socket(AF_ALG, SOCK_SEQPACKET, 0);
>> bind(sds[0], (struct sockaddr *) &sa, sizeof(sa));
>>
>> And ran this as "./tickle asdf1" to generate the two threads trying to
>> find an invalid alg. The race looks possible even with valid algs, but
>> this was the fastest path I could see to tickle the issue.
>>
>> With added printks to the kernel, it was clear that crypto_larval_kill
>> was being called twice on the same larval, and the list_del() call was
>> doing bad things. When I fixed that, the refcnt bug became very
>> obvious. Here's the change I made for crypto_larval_kill:
>>
>> @@ -161,7 +166,8 @@ void crypto_larval_kill(struct crypto_alg *alg)
>> struct crypto_larval *larval = (void *)alg;
>>
>> down_write(&crypto_alg_sem);
>> - list_del(&alg->cra_list);
>> + if (!list_empty(&alg->cra_list))
>> + list_del_init(&alg->cra_list);
>> up_write(&crypto_alg_sem);
>> complete_all(&larval->completion);
>> crypto_alg_put(alg);
>>
>> It seems that there are too many "put" calls (mixed between
>> crypto_alg_put() and crypto_mod_put(), which also calls
>> crypto_alg_put()). See this annotated portion of
>> crypto_alg_mod_lookup:
>>
>> /* This can (correctly) return the same larval for two threads. */
>> larval = crypto_larval_lookup(name, type, mask);
>> if (IS_ERR(larval) || !crypto_is_larval(larval))
>> return larval;
>>
>> ok = crypto_probing_notify(CRYPTO_MSG_ALG_REQUEST, larval);
>>
>> if (ok == NOTIFY_STOP)
>> /* This calls crypto_mod_put(), but sometimes also get?? */
>> alg = crypto_larval_wait(larval);
>> else {
>> /* This directly calls crypto_mod_put */
>> crypto_mod_put(larval);
>> alg = ERR_PTR(-ENOENT);
>> }
>> /* This calls crypto_alg_put */
>> crypto_larval_kill(larval);
>> return alg;
>>
>> In the two-thread situation, the first thread gets a larval with
>> refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
>> larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
>> the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
>> decrements the ref count twice.
>>
>> It seems to me like either each call to crypto_larval_lookup() should
>> result in a refcount bumped by two, or crypto_alg_mod_lookup() should
>> decrement only once, and the initial refcount should be 1 not 2 from
>> crypto_larval_add. But it's not clear to me which is sensible here.
>>
>> What's the right solution here?
>>
>> Thanks,
>>
>> -Kees
>>
>> --
>> Kees Cook
>> Chrome OS Security
>>
> I've been tracking this problem too:
> https://bugzilla.redhat.com/show_bug.cgi?id=1002351
>
> Though I'd just started so I've not gotten anywhere near this far. It seems,
> given your analysis above that we really need to rework the refcounting here.
> At the very least we probably need to:
>
> 1) only modify the module reference count when a larval for a given alg is
> created, or when its last refcount is decremented.
>
> 2) fix up the cra_refcount to start at one and increment for each lookup, and
> decrement for each kill.
What I haven't been able to figure out is the "expected" behavior of a
larval. crypto_larval_kill() seems to be the only thing that removes
it from the alg list, so that seems like the only place a "put" should
be happening. Though that put should likely be the mod_put not the
alg_put. I don't think larval_wait should be doing a put, but it also
can perform a "get", so I'm baffled by that too. :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: race condition in crypto larval handling
From: Herbert Xu @ 2013-09-08 1:32 UTC (permalink / raw)
To: Kees Cook; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <CAGXu5jJvuAC3paV5k_bmoHHrHVgc0Hi6ShSDk3iQUt1Lf0cioQ@mail.gmail.com>
On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
>
> In the two-thread situation, the first thread gets a larval with
> refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
> larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
> the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
> decrements the ref count twice.
>
> It seems to me like either each call to crypto_larval_lookup() should
> result in a refcount bumped by two, or crypto_alg_mod_lookup() should
> decrement only once, and the initial refcount should be 1 not 2 from
> crypto_larval_add. But it's not clear to me which is sensible here.
>
> What's the right solution here?
First of all thanks a lot for tracking this problem down! It's
been bothering me for months but I was unable to find a good
reproducer.
So now that you've identified the problem, the solution is easy.
crypto_larval_lookup should only ever return a larval if it created
one. Any larval created earlier must be waited on first before we
return.
So does this patch fix the crash for you?
diff --git a/crypto/api.c b/crypto/api.c
index 320ea4d..a2b39c5 100644
--- a/crypto/api.c
+++ b/crypto/api.c
@@ -34,6 +34,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
BLOCKING_NOTIFIER_HEAD(crypto_chain);
EXPORT_SYMBOL_GPL(crypto_chain);
+static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
+
struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
{
return try_module_get(alg->cra_module) ? crypto_alg_get(alg) : NULL;
@@ -144,8 +146,11 @@ static struct crypto_alg *crypto_larval_add(const char *name, u32 type,
}
up_write(&crypto_alg_sem);
- if (alg != &larval->alg)
+ if (alg != &larval->alg) {
kfree(larval);
+ if (crypto_is_larval(alg))
+ alg = crypto_larval_wait(alg);
+ }
return alg;
}
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 related
* Re: race condition in crypto larval handling
From: Kees Cook @ 2013-09-08 3:34 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <20130908013210.GA30627@gondor.apana.org.au>
On Sat, Sep 7, 2013 at 6:32 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
>>
>> In the two-thread situation, the first thread gets a larval with
>> refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
>> larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
>> the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
>> decrements the ref count twice.
>>
>> It seems to me like either each call to crypto_larval_lookup() should
>> result in a refcount bumped by two, or crypto_alg_mod_lookup() should
>> decrement only once, and the initial refcount should be 1 not 2 from
>> crypto_larval_add. But it's not clear to me which is sensible here.
>>
>> What's the right solution here?
>
> First of all thanks a lot for tracking this problem down! It's
> been bothering me for months but I was unable to find a good
> reproducer.
>
> So now that you've identified the problem, the solution is easy.
> crypto_larval_lookup should only ever return a larval if it created
> one. Any larval created earlier must be waited on first before we
> return.
>
> So does this patch fix the crash for you?
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 320ea4d..a2b39c5 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -34,6 +34,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
> BLOCKING_NOTIFIER_HEAD(crypto_chain);
> EXPORT_SYMBOL_GPL(crypto_chain);
>
> +static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
> +
> struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
> {
> return try_module_get(alg->cra_module) ? crypto_alg_get(alg) : NULL;
> @@ -144,8 +146,11 @@ static struct crypto_alg *crypto_larval_add(const char *name, u32 type,
> }
> up_write(&crypto_alg_sem);
>
> - if (alg != &larval->alg)
> + if (alg != &larval->alg) {
> kfree(larval);
> + if (crypto_is_larval(alg))
> + alg = crypto_larval_wait(alg);
> + }
>
> return alg;
> }
Thanks! This does stop the crash I was seeing on the "race with no
valid alg" path, yay!
However, I noticed on the "good" path (even without the above patch),
I sometimes see a double-kfree triggered by the modprobe process. I
can't, however, see how that's happening, since larval_destroy should
only be called when refcnt == 0.
Here's the debugging output I added that noticed this. pid 15797 and
15798 are my "hash" tool that forks before doing identical AF_ALG
calls for "sha512", which has to be modprobed. The trailing "31" and
"32" was an atomic counter I added just to make sure I wasn't going
crazy:
[ 112.495789] crypto_alg_mod_lookup 15797: looking up [sha512]
[ 112.495815] crypto_larval_lookup 15797: looking up [sha512]
[ 112.495839] crypto_larval_lookup 15797: requesting module [sha512]
[ 112.495915] crypto_alg_mod_lookup 15798: looking up [sha512]
[ 112.495937] crypto_larval_lookup 15798: looking up [sha512]
[ 112.495960] crypto_larval_lookup 15798: requesting module [sha512]
[ 112.498691] crypto_larval_destroy 15808(modprobe): larval
kfree(ffff880123e9da00) refcnt:0 (serial:31)
[ 112.498771] crypto_larval_destroy 15808(modprobe): larval
kfree(ffff880123e9da00) refcnt:0 (serial:32)
[ 112.500888] crypto_larval_lookup 15797: after requesting module
[sha512], got alg ffffffffc0299050
[ 112.500904] crypto_larval_lookup 15797: for [sha512], have alg
ffffffffc0299050
[ 112.500934] crypto_larval_lookup 15797: for [sha512], alg
ffffffffc0299050 is NOT larval
[ 112.500953] crypto_alg_mod_lookup 15797: [sha512] is not larval
ffffffffc0299050
[ 112.501384] crypto_larval_lookup 15798: after requesting module
[sha512], got alg ffffffffc0299050
[ 112.501404] crypto_larval_lookup 15798: for [sha512], have alg
ffffffffc0299050
[ 112.501417] crypto_larval_lookup 15798: for [sha512], alg
ffffffffc0299050 is NOT larval
[ 112.501432] crypto_alg_mod_lookup 15798: [sha512] is not larval
ffffffffc0299050
Regardless, this seems to be a separate bug.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: race condition in crypto larval handling
From: Herbert Xu @ 2013-09-08 4:37 UTC (permalink / raw)
To: Kees Cook; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <CAGXu5j+DoXK_9v430xhCzOO1bsuuhe84fjkf7+DZ0r35hUNBBg@mail.gmail.com>
On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
>
> However, I noticed on the "good" path (even without the above patch),
> I sometimes see a double-kfree triggered by the modprobe process. I
> can't, however, see how that's happening, since larval_destroy should
> only be called when refcnt == 0.
Do you still see this double free with this patch? Without the
patch it is completely expected as killing the same lavral twice
will cause memory corruption leading to all sorts of weirdness,
even if you stop it from deleting the list entry twice.
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: race condition in crypto larval handling
From: Herbert Xu @ 2013-09-08 4:54 UTC (permalink / raw)
To: Kees Cook; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <20130908043703.GA31554@gondor.apana.org.au>
On Sun, Sep 08, 2013 at 02:37:03PM +1000, Herbert Xu wrote:
> On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
> >
> > However, I noticed on the "good" path (even without the above patch),
> > I sometimes see a double-kfree triggered by the modprobe process. I
> > can't, however, see how that's happening, since larval_destroy should
> > only be called when refcnt == 0.
>
> Do you still see this double free with this patch? Without the
> patch it is completely expected as killing the same lavral twice
> will cause memory corruption leading to all sorts of weirdness,
> even if you stop it from deleting the list entry twice.
Actually I know what it is. sha512 registers two algorithms.
Therefore, it will create two larvals in sequence and then destroy
them in turn. So it's not a double free at all. If you put a
printk in crypto_larval_alloc that should confirm this.
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: race condition in crypto larval handling
From: Kees Cook @ 2013-09-08 6:01 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <20130908045422.GA31896@gondor.apana.org.au>
On Sat, Sep 7, 2013 at 9:54 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Sun, Sep 08, 2013 at 02:37:03PM +1000, Herbert Xu wrote:
>> On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
>> >
>> > However, I noticed on the "good" path (even without the above patch),
>> > I sometimes see a double-kfree triggered by the modprobe process. I
>> > can't, however, see how that's happening, since larval_destroy should
>> > only be called when refcnt == 0.
>>
>> Do you still see this double free with this patch? Without the
>> patch it is completely expected as killing the same lavral twice
>> will cause memory corruption leading to all sorts of weirdness,
>> even if you stop it from deleting the list entry twice.
I noticed while testing the larval_kill fix, and then tried it again
after reverting the fix -- both showed the behavior.
> Actually I know what it is. sha512 registers two algorithms.
> Therefore, it will create two larvals in sequence and then destroy
> them in turn. So it's not a double free at all. If you put a
> printk in crypto_larval_alloc that should confirm this.
Ah! That would make sense; it just happens to re-allocate to the exact
same location, yes. Whew, that's certainly what's happening. I can
retest to confirm in my morning.
Thanks again for the larval_kill fix! I'll get it rolled out for wider
testing to confirm that it make our crash numbers go down.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: race condition in crypto larval handling
From: Kees Cook @ 2013-09-08 15:52 UTC (permalink / raw)
To: Herbert Xu; +Cc: David S. Miller, LKML, linux-crypto, Tyler Hicks
In-Reply-To: <CAGXu5jL4ojFEWAkW0NDSERUFTQhPgwb6ToOcAsg_4QySex1GfA@mail.gmail.com>
On Sat, Sep 7, 2013 at 11:01 PM, Kees Cook <keescook@chromium.org> wrote:
> On Sat, Sep 7, 2013 at 9:54 PM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>> On Sun, Sep 08, 2013 at 02:37:03PM +1000, Herbert Xu wrote:
>>> On Sat, Sep 07, 2013 at 08:34:15PM -0700, Kees Cook wrote:
>>> >
>>> > However, I noticed on the "good" path (even without the above patch),
>>> > I sometimes see a double-kfree triggered by the modprobe process. I
>>> > can't, however, see how that's happening, since larval_destroy should
>>> > only be called when refcnt == 0.
>>>
>>> Do you still see this double free with this patch? Without the
>>> patch it is completely expected as killing the same lavral twice
>>> will cause memory corruption leading to all sorts of weirdness,
>>> even if you stop it from deleting the list entry twice.
>>
>> Actually I know what it is. sha512 registers two algorithms.
>> Therefore, it will create two larvals in sequence and then destroy
>> them in turn. So it's not a double free at all. If you put a
>> printk in crypto_larval_alloc that should confirm this.
>
> Ah! That would make sense; it just happens to re-allocate to the exact
> same location, yes. Whew, that's certainly what's happening. I can
> retest to confirm in my morning.
Confirmed: 2 allocs happen, and then 2 kfrees. :)
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: race condition in crypto larval handling
From: James Yonan @ 2013-09-09 1:41 UTC (permalink / raw)
Cc: linux-crypto
In-Reply-To: <20130908013210.GA30627@gondor.apana.org.au>
On 07/09/2013 19:32, Herbert Xu wrote:
> On Fri, Sep 06, 2013 at 04:20:50PM -0700, Kees Cook wrote:
>>
>> In the two-thread situation, the first thread gets a larval with
>> refcnt 2 via crypto_larval_add. (Why 2?) The next thread finds the
>> larval via crypto_larval_add's call to __crypto_alg_lookup() and sees
>> the ref bump to 3. While exiting crypto_alg_mod_lookup, each thread
>> decrements the ref count twice.
>>
>> It seems to me like either each call to crypto_larval_lookup() should
>> result in a refcount bumped by two, or crypto_alg_mod_lookup() should
>> decrement only once, and the initial refcount should be 1 not 2 from
>> crypto_larval_add. But it's not clear to me which is sensible here.
>>
>> What's the right solution here?
>
> First of all thanks a lot for tracking this problem down! It's
> been bothering me for months but I was unable to find a good
> reproducer.
>
> So now that you've identified the problem, the solution is easy.
> crypto_larval_lookup should only ever return a larval if it created
> one. Any larval created earlier must be waited on first before we
> return.
>
> So does this patch fix the crash for you?
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 320ea4d..a2b39c5 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -34,6 +34,8 @@ EXPORT_SYMBOL_GPL(crypto_alg_sem);
> BLOCKING_NOTIFIER_HEAD(crypto_chain);
> EXPORT_SYMBOL_GPL(crypto_chain);
>
> +static struct crypto_alg *crypto_larval_wait(struct crypto_alg *alg);
> +
> struct crypto_alg *crypto_mod_get(struct crypto_alg *alg)
> {
> return try_module_get(alg->cra_module) ? crypto_alg_get(alg) : NULL;
> @@ -144,8 +146,11 @@ static struct crypto_alg *crypto_larval_add(const char *name, u32 type,
> }
> up_write(&crypto_alg_sem);
>
> - if (alg != &larval->alg)
> + if (alg != &larval->alg) {
> kfree(larval);
> + if (crypto_is_larval(alg))
> + alg = crypto_larval_wait(alg);
> + }
>
> return alg;
> }
I tried this patch, but I still see an apparent module lookup/load race
if code on several CPUs calls crypto_alloc_aead at the same time, and an
external module such as aes needs to be loaded.
Seeing this in the log: "request_module: runaway loop modprobe gcm(aes)"
Shouldn't module lookup/load be bracketed by some sort of lock to
prevent this?
James
^ permalink raw reply
* [PATCH] crypto: tegra: use kernel entropy instead of ad-hoc
From: Linus Walleij @ 2013-09-09 7:35 UTC (permalink / raw)
To: linux-crypto-u79uwXL29TY76Z2rM5mHXA, Herbert Xu
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij,
Stephen Warren, Varun Wadekar, Neil Horman,
linux-tegra-u79uwXL29TY76Z2rM5mHXA
The way I read the Tegra AES RNG is that it has a homebrew
algorithm for initializing the 128bit RNG using timespec and
the unique chip ID. This looks like reinventing the (square)
wheel, instead just grab 128bits from the kernel entropy pool
where the time and (after another patch) chip unique ID is
already mixed in.
Incidentally this also gets rid of a rather ugly
cross-dependence on the machine using an extern declaration.
Cc: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Cc: Varun Wadekar <vwadekar-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
Only compile-tested as I don't have this platform.
---
drivers/crypto/tegra-aes.c | 15 +++------------
1 file changed, 3 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/tegra-aes.c b/drivers/crypto/tegra-aes.c
index 2d58da9..7f42bfe 100644
--- a/drivers/crypto/tegra-aes.c
+++ b/drivers/crypto/tegra-aes.c
@@ -199,8 +199,6 @@ static void aes_workqueue_handler(struct work_struct *work);
static DECLARE_WORK(aes_work, aes_workqueue_handler);
static struct workqueue_struct *aes_wq;
-extern unsigned long long tegra_chip_uid(void);
-
static inline u32 aes_readl(struct tegra_aes_dev *dd, u32 offset)
{
return readl(dd->io_base + offset);
@@ -713,9 +711,8 @@ static int tegra_aes_rng_reset(struct crypto_rng *tfm, u8 *seed,
struct tegra_aes_dev *dd = aes_dev;
struct tegra_aes_ctx *ctx = &rng_ctx;
struct tegra_aes_slot *key_slot;
- struct timespec ts;
int ret = 0;
- u64 nsec, tmp[2];
+ u8 tmp[16]; /* 16 bytes = 128 bits of entropy */
u8 *dt;
if (!ctx || !dd) {
@@ -778,14 +775,8 @@ static int tegra_aes_rng_reset(struct crypto_rng *tfm, u8 *seed,
if (dd->ivlen >= (2 * DEFAULT_RNG_BLK_SZ + AES_KEYSIZE_128)) {
dt = dd->iv + DEFAULT_RNG_BLK_SZ + AES_KEYSIZE_128;
} else {
- getnstimeofday(&ts);
- nsec = timespec_to_ns(&ts);
- do_div(nsec, 1000);
- nsec ^= dd->ctr << 56;
- dd->ctr++;
- tmp[0] = nsec;
- tmp[1] = tegra_chip_uid();
- dt = (u8 *)tmp;
+ get_random_bytes(tmp, sizeof(tmp));
+ dt = tmp;
}
memcpy(dd->dt, dt, DEFAULT_RNG_BLK_SZ);
--
1.8.3.1
^ permalink raw reply related
* [PATCH][RFC] Tests on 200 different CPUs/Arches and OSes with CPU Jitter RNG (resent)
From: Stephan Mueller @ 2013-09-09 9:31 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: sandy harris, LKML, linux-crypto
Hi Ted,
(this is a reply to [3] and possibly an addition to your blog [4])
I prepared a new release of the CPU Jitter RNG available at [1]. The
core of the RNG remains unchanged. However, there are the following
changes:
- addition of a patch to integrate the RNG into /dev/random as explained
in appendix B.3 of [2], although the long-term goal of the RNG is rather
the integration into the kernel crypto API when considering the Linux
kernel as outlined in appendix B.1 of [2]
- ensure that the code is compiled without optimization based on the
reasons outlined in section 5.1 of [2]
- addition of chapter 5.1 to [2] explaining how the entropy is collected
- additional code to execute the CPU Jitter RNG on different OSes
(specifically AIX, MacOS and z/OS -- other Unixes are good without
additional changes)
Please note, I will present the RNG with a talk at the Linux Symposium
in Ottawa this year.
I performed additional testing with a Von-Neumann De-Skew operation as
defined in RFC 1750 section 5.2.2 showing that this De-Skew operation
does not make the statistics of the RNG better.
>On Tue, May 21, 2013 at 3:01 PM, Theodore Ts'o <tytso@mit.edu> wrote:
>> I continue to be suspicious about claims that userspace timing
>> measurements are measuring anything other than OS behaviour.
>
>Yes, but they do seem to contain some entropy. See links in the
>original post of this thread, the havege stuff and especially the
>McGuire et al paper.
With the initially shown implementation and documentation I did not
really show that sufficient entropy is gathered from the CPU execution
jitter -- i.e. that there is unpredictable jitter in the execution time.
With a new test I now closed that hole. The newly added test
measures the entropy gathered during execution jitter collection, i.e.
heart of the RNG in terms of how much statistical entropy it provides.
The description of the test is given in section 5.1 of [2]. To ensure
that the statistical entropy measurements are indeed showing the
information theoretical entropy, section 4.4 of [2] outlines that
patterns are not identified in the output of the RNG which would
diminish the information theoretical entropy compared to the statistical
entropy.
That test was then executed on about 200 different systems with the
results given in appendix F of [2]. The table stated there supported by
the many graphs demonstrates that the CPU Jitter random number generator
delivers high-quality entropy on:
- a large range of CPUs ranging from embedded systems of MIPS and ARM
CPUs, covering desktop systems with AMD and Intel x86 32 bit and 64 bit
CPUs up to server CPUs of Intel Itanium, Sparc, POWER and IBM System Z;
- a large range of operating systems: Linux (including Android),
OpenBSD, FreeBSD, NetBSD, AIX, OpenIndiana (OpenSolaris), AIX, z/OS;
- a range of different compilers: GCC, Clang and the z/OS C compiler.
The test results show an interesting yet common trend -- i.e. common for
the different CPU types: the newer the CPU is, the more CPU execution
time jitter is present.
[2] appendix F.37 contains entropy measurements on different operating
systems on the very same hardware, indicating that the jitter
measurements are present regardless of the OS.
With the test results, Ted's concerns should be covered.
[...]
>> For devices like Linux routers, what we desperately need is hardware
>> assist; [or] mix
>> in additional timing information either at kernel device driver
>> level,
>> or from systems such as HAVEGE.
The concern with HAVEGE is that it is very complex. The implementation
is far from being straight forward.
>>
>> What I'm against is relying only on solutions such as HAVEGE or
>> replacing /dev/random with something scheme that only relies on CPU
>> timing and ignores interrupt timing.
>
>My question is how to incorporate some of that into /dev/random.
>At one point, timing info was used along with other stuff. Some
>of that got deleted later, What is the current state? Should we
>add more?
Please see the suggestion for an integration with /dev/random given in
appendix B.3 of [2]. The source code for the integration is given in
patches/linux-3.9-random.patch which is described in patches/README. The
patch only utilizes the CPU Jitter RNG when the entropy in the entropy
pool falls below the low threshold, i.e. when no entropy from other
sources is present.
[1] http://www.chronox.de/jent/jitterentropy-20130818.tar.bz2
[2] http://www.chronox.de/jent/doc/CPU-Jitter-NPTRNG.pdf
[3] http://www.gossamer-threads.com/lists/linux/kernel/1716565#1716565
[4] https://plus.google.com/u/0/117091380454742934025/posts/SDcoemc9V3J
Ciao
Stephan
^ permalink raw reply
* [PATCH 1/7] crypto: caam - fix RNG state handle instantiation descriptor
From: Alex Porosanu @ 2013-09-09 15:56 UTC (permalink / raw)
To: linux-crypto; +Cc: herbert, horia.geanta, vakul
In-Reply-To: <1378742194-32194-1-git-send-email-alexandru.porosanu@freescale.com>
The way the DECO runs a descriptor through the direct (debug)
interface is different from the JRI interface: the DECO will
continue to try and execute the next commands, after the descriptor
buffer has ended. This leads to unpredictable results and possibly
to locking up of the DECO. This patch adds a halt command at the
end of the descriptor to ensure the DECO halts when it reaches
the end of the descriptor buffer.
Signed-off-by: Alex Porosanu <alexandru.porosanu@freescale.com>
---
drivers/crypto/caam/ctrl.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index b010d42..11c7f29 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -73,6 +73,8 @@ static void build_instantiation_desc(u32 *desc)
/* generate secure keys (non-test) */
append_operation(desc, OP_TYPE_CLASS1_ALG | OP_ALG_ALGSEL_RNG |
OP_ALG_RNG4_SK);
+
+ append_jump(desc, JUMP_CLASS_CLASS1 | JUMP_TYPE_HALT);
}
static int instantiate_rng(struct device *ctrldev)
@@ -83,7 +85,7 @@ static int instantiate_rng(struct device *ctrldev)
u32 *desc;
int i, ret = 0;
- desc = kmalloc(CAAM_CMD_SZ * 6, GFP_KERNEL | GFP_DMA);
+ desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL | GFP_DMA);
if (!desc) {
dev_err(ctrldev, "can't allocate RNG init descriptor memory\n");
return -ENOMEM;
--
1.7.7.6
^ permalink raw reply related
* [PATCH 0/7] crypto: caam - RNG4 patches and fixes
From: Alex Porosanu @ 2013-09-09 15:56 UTC (permalink / raw)
To: linux-crypto; +Cc: herbert, horia.geanta, vakul
This patch series attempts to fix some identified issues and add some new
functionalities regarding the RNG4 block in the CAAM driver:
o if the CAAM driver isn't properly instantiated (e.g. RNG4 initialization
fails), then there's an illegal memory access generated by the modules
depending on it; patch 1 in the patch-set fixes this;
o if the CAAM module is removed, the state handles are not uninstantiated;
patch 3 in the patch-set adds the necessary descriptor to uninstantiate
state handle 0;
o the RNG4 block in CAAM needs to be 'seeded' first before being used
for generating pseudo-random data. The 'seeding' is done by getting
entropy from the TRNG ring oscillator. The RTFRQMAX register controls
the maximum allowable number of samples that can be acquired during
an entropy sample. Depending on the clock at which the RNG4 block
(and for that matter the SEC block) runs, it's possible that a
hard-coded value for the maximum frequency is inadequate, i.e. more
samples than needed are taken. This leads to failures on devices
like BSC913x. Patch number 2 fixes this issue by using a kind of
a software loop to increase the maximum number of samples taken
until the state handle can be properly initialized; o there are two
state handles present in the RNG4 block and only one
is initialized; patch 5 in the patch-set fixes this issue, also
adding the necessary code for deinstantiation only the handles that were
instantiated by the driver.
Alex Porosanu (7):
crypto: caam - fix RNG state handle instantiation descriptor
crypto: caam - fix hash, alg and rng registration if CAAM driver not
initialized
crypto: caam - fix RNG4 instantiation
crypto: caam - split RNG4 instantiation function
crypto: caam - uninstantiate RNG state handle 0 if instantiated by
caam driver
crypto: caam - fix RNG4 AAI defines
crypto: caam - enable instantiation of all RNG4 state handles
drivers/crypto/caam/caamalg.c | 7 +
drivers/crypto/caam/caamhash.c | 7 +
drivers/crypto/caam/caamrng.c | 7 +
drivers/crypto/caam/ctrl.c | 400 +++++++++++++++++++++++++++++++---------
drivers/crypto/caam/desc.h | 17 +-
drivers/crypto/caam/intern.h | 6 +
drivers/crypto/caam/regs.h | 14 +-
7 files changed, 364 insertions(+), 94 deletions(-)
--
1.7.7.6
^ 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