linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
@ 2017-11-07 22:15 Eric Biggers
  2017-11-08 12:50 ` David Howells
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2017-11-07 22:15 UTC (permalink / raw)
  To: linux-crypto, Herbert Xu
  Cc: Tudor-Dan Ambarus, Mat Martineau, Salvatore Benedetto, keyrings,
	linux-kernel, Eric Biggers, stable

From: Eric Biggers <ebiggers@google.com>

On a non-preemptible kernel, if KEYCTL_DH_COMPUTE is called with the
largest permitted inputs (16384 bits), the kernel spends 10+ seconds
doing modular exponentiation in mpi_powm() without rescheduling.  If all
threads do it, it locks up the system.  Moreover, it can cause
rcu_sched-stall warnings.

Notwithstanding the insanity of doing this calculation in kernel mode
rather than in userspace, fix it by calling cond_resched() as each bit
from the exponent is processed.  It's still noninterruptible, but at
least it's preemptible now.

Do the cond_resched() once per bit rather than once per MPI limb because
each limb might still easily take 100+ milliseconds on slow CPUs.

Cc: <stable@vger.kernel.org> # v4.12+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 lib/mpi/mpi-pow.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/mpi/mpi-pow.c b/lib/mpi/mpi-pow.c
index e24388a863a7..468fb7cd1221 100644
--- a/lib/mpi/mpi-pow.c
+++ b/lib/mpi/mpi-pow.c
@@ -26,6 +26,7 @@
  *	 however I decided to publish this code under the plain GPL.
  */
 
+#include <linux/sched.h>
 #include <linux/string.h>
 #include "mpi-internal.h"
 #include "longlong.h"
@@ -256,6 +257,7 @@ int mpi_powm(MPI res, MPI base, MPI exp, MPI mod)
 				}
 				e <<= 1;
 				c--;
+				cond_resched();
 			}
 
 			i--;
-- 
2.15.0.403.gc27cc4dac6-goog

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
  2017-11-07 22:15 [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop Eric Biggers
@ 2017-11-08 12:50 ` David Howells
  2017-11-08 19:18   ` Eric Biggers
  2017-11-08 20:17   ` David Howells
  0 siblings, 2 replies; 6+ messages in thread
From: David Howells @ 2017-11-08 12:50 UTC (permalink / raw)
  To: Eric Biggers
  Cc: dhowells, linux-crypto, Herbert Xu, Tudor-Dan Ambarus,
	Mat Martineau, Salvatore Benedetto, keyrings, linux-kernel,
	Eric Biggers, stable

Eric Biggers <ebiggers3@gmail.com> wrote:

> On a non-preemptible kernel, if KEYCTL_DH_COMPUTE is called with the
> largest permitted inputs (16384 bits), the kernel spends 10+ seconds
> doing modular exponentiation in mpi_powm() without rescheduling.  If all
> threads do it, it locks up the system.  Moreover, it can cause
> rcu_sched-stall warnings.

Do you want this to go in immediately, or I should I push it to James for the
next merge window?

David

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
  2017-11-08 12:50 ` David Howells
@ 2017-11-08 19:18   ` Eric Biggers
  2017-11-08 20:17   ` David Howells
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Biggers @ 2017-11-08 19:18 UTC (permalink / raw)
  To: David Howells
  Cc: linux-crypto, Herbert Xu, Tudor-Dan Ambarus, Mat Martineau,
	Salvatore Benedetto, keyrings, linux-kernel, Eric Biggers,
	stable

Hi David,

On Wed, Nov 08, 2017 at 12:50:17PM +0000, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > On a non-preemptible kernel, if KEYCTL_DH_COMPUTE is called with the
> > largest permitted inputs (16384 bits), the kernel spends 10+ seconds
> > doing modular exponentiation in mpi_powm() without rescheduling.  If all
> > threads do it, it locks up the system.  Moreover, it can cause
> > rcu_sched-stall warnings.
> 
> Do you want this to go in immediately, or I should I push it to James for the
> next merge window?
> 
> David

This probably should be grouped with my series "crypto: dh - input validation
fixes", as this is also a fix for Diffie-Hellman.  I was actually expecting
Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto
API (crypto/dh.c); none of the patches actually touch security/keys/.  If you'd
like to maintain the Diffie-Hellman implementation(s) you should get yourself
added to MAINTAINERS for the relevant files.

As for the criticality of the fixes, the code is new in v4.12, so I doubt many
people have CONFIG_KEY_DH_OPERATIONS=y enabled yet.  syzbot doesn't actually
have it enabled either (we should turn it on); I just happened to find these
bugs by running syzkaller myself.  So it may not be particularly critical to
take these fixes immediately.  However, at least the fix for the double free bug
("crypto: dh - Fix double free of ctx->p") shouldn't wait for too long...

Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
  2017-11-08 12:50 ` David Howells
  2017-11-08 19:18   ` Eric Biggers
@ 2017-11-08 20:17   ` David Howells
  2017-11-08 20:28     ` Eric Biggers
  1 sibling, 1 reply; 6+ messages in thread
From: David Howells @ 2017-11-08 20:17 UTC (permalink / raw)
  To: Eric Biggers
  Cc: dhowells, linux-crypto, Herbert Xu, Tudor-Dan Ambarus,
	Mat Martineau, Salvatore Benedetto, keyrings, linux-kernel,
	Eric Biggers, stable

Eric Biggers <ebiggers3@gmail.com> wrote:

> This probably should be grouped with my series "crypto: dh - input validation
> fixes", as this is also a fix for Diffie-Hellman.  I was actually expecting
> Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto
> API (crypto/dh.c); none of the patches actually touch security/keys/.

Fine by me.

> If you'd like to maintain the Diffie-Hellman implementation(s) you should
> get yourself added to MAINTAINERS for the relevant files.

It touches the MPI lib, though, so it also potentially affects RSA and stuff,
which is why I introduced it to the kernel originally.

David

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
  2017-11-08 20:17   ` David Howells
@ 2017-11-08 20:28     ` Eric Biggers
  2017-11-08 20:49       ` Mat Martineau
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Biggers @ 2017-11-08 20:28 UTC (permalink / raw)
  To: David Howells
  Cc: linux-crypto, Herbert Xu, Tudor-Dan Ambarus, Mat Martineau,
	Salvatore Benedetto, keyrings, linux-kernel, Eric Biggers,
	stable

On Wed, Nov 08, 2017 at 08:17:12PM +0000, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > This probably should be grouped with my series "crypto: dh - input validation
> > fixes", as this is also a fix for Diffie-Hellman.  I was actually expecting
> > Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto
> > API (crypto/dh.c); none of the patches actually touch security/keys/.
> 
> Fine by me.
> 
> > If you'd like to maintain the Diffie-Hellman implementation(s) you should
> > get yourself added to MAINTAINERS for the relevant files.
> 
> It touches the MPI lib, though, so it also potentially affects RSA and stuff,
> which is why I introduced it to the kernel originally.
> 

True, mpi_powm() might also be reachable by adding a key of type "asymmetric",
in which case this fix would need to go to older kernels than v4.12.  If I have
a chance I'll see if I can find a reproducer.

Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop
  2017-11-08 20:28     ` Eric Biggers
@ 2017-11-08 20:49       ` Mat Martineau
  0 siblings, 0 replies; 6+ messages in thread
From: Mat Martineau @ 2017-11-08 20:49 UTC (permalink / raw)
  To: Eric Biggers
  Cc: David Howells, linux-crypto, Herbert Xu, Tudor-Dan Ambarus,
	Salvatore Benedetto, keyrings, linux-kernel, Eric Biggers,
	stable

On Wed, 8 Nov 2017, Eric Biggers wrote:

> On Wed, Nov 08, 2017 at 08:17:12PM +0000, David Howells wrote:
>> Eric Biggers <ebiggers3@gmail.com> wrote:
>>
>>> This probably should be grouped with my series "crypto: dh - input validation
>>> fixes", as this is also a fix for Diffie-Hellman.  I was actually expecting
>>> Herbert Xu to take these patches, as Diffie-Hellman is now part of the crypto
>>> API (crypto/dh.c); none of the patches actually touch security/keys/.
>>
>> Fine by me.
>>
>>> If you'd like to maintain the Diffie-Hellman implementation(s) you should
>>> get yourself added to MAINTAINERS for the relevant files.
>>
>> It touches the MPI lib, though, so it also potentially affects RSA and stuff,
>> which is why I introduced it to the kernel originally.
>>
>
> True, mpi_powm() might also be reachable by adding a key of type "asymmetric",
> in which case this fix would need to go to older kernels than v4.12.  If I have
> a chance I'll see if I can find a reproducer.

CONFIG_KEY_DH_OPERATIONS and use of mpi_powm() by KEYCTL_DH_COMPUTE goes 
back to v4.7, when the MPI library was called directly. KPP was not 
implemented yet.

--
Mat Martineau
Intel OTC

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-11-08 20:49 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-07 22:15 [PATCH v2] lib/mpi: call cond_resched() from mpi_powm() loop Eric Biggers
2017-11-08 12:50 ` David Howells
2017-11-08 19:18   ` Eric Biggers
2017-11-08 20:17   ` David Howells
2017-11-08 20:28     ` Eric Biggers
2017-11-08 20:49       ` Mat Martineau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).