[SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

Joseph Salisbury-3
BugLink: https://bugs.launchpad.net/bugs/1790832

== SRU Justification ==
IBM is requesting this patch in all releases order to fix the sleep-in-atomic
bugs in AES-CBC and AES-XTS VMX implementations.

This patch is a clean cherry pick and has already been applied to
Cosmic.  It is also needed in Xenial, but there was a minor context
diff, so the Xenial SRU will be sent separatly.  

== Fix ==
0522236d4f9c ("crypto: vmx - Fix sleep-in-atomic bugs")

== Regression Potential ==
Low.  This patch has also been cc'd to upstream stable, so it has had
additional upstream review.

== Test Case ==
A test kernel was built with this patch and tested by the original bug reporter.
The bug reporter states the test kernel resolved the bug.


Ondrej Mosnacek (1):
  crypto: vmx - Fix sleep-in-atomic bugs

 drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
 drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
 2 files changed, 28 insertions(+), 23 deletions(-)

--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Bionic][PATCH 1/1] crypto: vmx - Fix sleep-in-atomic bugs

Joseph Salisbury-3
From: Ondrej Mosnacek <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1790832

This patch fixes sleep-in-atomic bugs in AES-CBC and AES-XTS VMX
implementations. The problem is that the blkcipher_* functions should
not be called in atomic context.

The bugs can be reproduced via the AF_ALG interface by trying to
encrypt/decrypt sufficiently large buffers (at least 64 KiB) using the
VMX implementations of 'cbc(aes)' or 'xts(aes)'. Such operations then
trigger BUG in crypto_yield():

[  891.863680] BUG: sleeping function called from invalid context at include/crypto/algapi.h:424
[  891.864622] in_atomic(): 1, irqs_disabled(): 0, pid: 12347, name: kcapi-enc
[  891.864739] 1 lock held by kcapi-enc/12347:
[  891.864811]  #0: 00000000f5d42c46 (sk_lock-AF_ALG){+.+.}, at: skcipher_recvmsg+0x50/0x530
[  891.865076] CPU: 5 PID: 12347 Comm: kcapi-enc Not tainted 4.19.0-0.rc0.git3.1.fc30.ppc64le #1
[  891.865251] Call Trace:
[  891.865340] [c0000003387578c0] [c000000000d67ea4] dump_stack+0xe8/0x164 (unreliable)
[  891.865511] [c000000338757910] [c000000000172a58] ___might_sleep+0x2f8/0x310
[  891.865679] [c000000338757990] [c0000000006bff74] blkcipher_walk_done+0x374/0x4a0
[  891.865825] [c0000003387579e0] [d000000007e73e70] p8_aes_cbc_encrypt+0x1c8/0x260 [vmx_crypto]
[  891.865993] [c000000338757ad0] [c0000000006c0ee0] skcipher_encrypt_blkcipher+0x60/0x80
[  891.866128] [c000000338757b10] [c0000000006ec504] skcipher_recvmsg+0x424/0x530
[  891.866283] [c000000338757bd0] [c000000000b00654] sock_recvmsg+0x74/0xa0
[  891.866403] [c000000338757c10] [c000000000b00f64] ___sys_recvmsg+0xf4/0x2f0
[  891.866515] [c000000338757d90] [c000000000b02bb8] __sys_recvmsg+0x68/0xe0
[  891.866631] [c000000338757e30] [c00000000000bbe4] system_call+0x5c/0x70

Fixes: 8c755ace357c ("crypto: vmx - Adding CBC routines for VMX module")
Fixes: c07f5d3da643 ("crypto: vmx - Adding support for XTS")
Cc: [hidden email]
Signed-off-by: Ondrej Mosnacek <[hidden email]>
Signed-off-by: Herbert Xu <[hidden email]>
(cherry picked from commit 0522236d4f9c5ab2e79889cb020d1acbe5da416e)
Signed-off-by: Joseph Salisbury <[hidden email]>
---
 drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
 drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
 2 files changed, 28 insertions(+), 23 deletions(-)

diff --git a/drivers/crypto/vmx/aes_cbc.c b/drivers/crypto/vmx/aes_cbc.c
index 5285ece..b718958 100644
--- a/drivers/crypto/vmx/aes_cbc.c
+++ b/drivers/crypto/vmx/aes_cbc.c
@@ -107,24 +107,23 @@ static int p8_aes_cbc_encrypt(struct blkcipher_desc *desc,
  ret = crypto_skcipher_encrypt(req);
  skcipher_request_zero(req);
  } else {
- preempt_disable();
- pagefault_disable();
- enable_kernel_vsx();
-
  blkcipher_walk_init(&walk, dst, src, nbytes);
  ret = blkcipher_walk_virt(desc, &walk);
  while ((nbytes = walk.nbytes)) {
+ preempt_disable();
+ pagefault_disable();
+ enable_kernel_vsx();
  aes_p8_cbc_encrypt(walk.src.virt.addr,
    walk.dst.virt.addr,
    nbytes & AES_BLOCK_MASK,
    &ctx->enc_key, walk.iv, 1);
+ disable_kernel_vsx();
+ pagefault_enable();
+ preempt_enable();
+
  nbytes &= AES_BLOCK_SIZE - 1;
  ret = blkcipher_walk_done(desc, &walk, nbytes);
  }
-
- disable_kernel_vsx();
- pagefault_enable();
- preempt_enable();
  }
 
  return ret;
@@ -147,24 +146,23 @@ static int p8_aes_cbc_decrypt(struct blkcipher_desc *desc,
  ret = crypto_skcipher_decrypt(req);
  skcipher_request_zero(req);
  } else {
- preempt_disable();
- pagefault_disable();
- enable_kernel_vsx();
-
  blkcipher_walk_init(&walk, dst, src, nbytes);
  ret = blkcipher_walk_virt(desc, &walk);
  while ((nbytes = walk.nbytes)) {
+ preempt_disable();
+ pagefault_disable();
+ enable_kernel_vsx();
  aes_p8_cbc_encrypt(walk.src.virt.addr,
    walk.dst.virt.addr,
    nbytes & AES_BLOCK_MASK,
    &ctx->dec_key, walk.iv, 0);
+ disable_kernel_vsx();
+ pagefault_enable();
+ preempt_enable();
+
  nbytes &= AES_BLOCK_SIZE - 1;
  ret = blkcipher_walk_done(desc, &walk, nbytes);
  }
-
- disable_kernel_vsx();
- pagefault_enable();
- preempt_enable();
  }
 
  return ret;
diff --git a/drivers/crypto/vmx/aes_xts.c b/drivers/crypto/vmx/aes_xts.c
index 8bd9aff..e9954a7 100644
--- a/drivers/crypto/vmx/aes_xts.c
+++ b/drivers/crypto/vmx/aes_xts.c
@@ -116,32 +116,39 @@ static int p8_aes_xts_crypt(struct blkcipher_desc *desc,
  ret = enc? crypto_skcipher_encrypt(req) : crypto_skcipher_decrypt(req);
  skcipher_request_zero(req);
  } else {
+ blkcipher_walk_init(&walk, dst, src, nbytes);
+
+ ret = blkcipher_walk_virt(desc, &walk);
+
  preempt_disable();
  pagefault_disable();
  enable_kernel_vsx();
 
- blkcipher_walk_init(&walk, dst, src, nbytes);
-
- ret = blkcipher_walk_virt(desc, &walk);
  iv = walk.iv;
  memset(tweak, 0, AES_BLOCK_SIZE);
  aes_p8_encrypt(iv, tweak, &ctx->tweak_key);
 
+ disable_kernel_vsx();
+ pagefault_enable();
+ preempt_enable();
+
  while ((nbytes = walk.nbytes)) {
+ preempt_disable();
+ pagefault_disable();
+ enable_kernel_vsx();
  if (enc)
  aes_p8_xts_encrypt(walk.src.virt.addr, walk.dst.virt.addr,
  nbytes & AES_BLOCK_MASK, &ctx->enc_key, NULL, tweak);
  else
  aes_p8_xts_decrypt(walk.src.virt.addr, walk.dst.virt.addr,
  nbytes & AES_BLOCK_MASK, &ctx->dec_key, NULL, tweak);
+ disable_kernel_vsx();
+ pagefault_enable();
+ preempt_enable();
 
  nbytes &= AES_BLOCK_SIZE - 1;
  ret = blkcipher_walk_done(desc, &walk, nbytes);
  }
-
- disable_kernel_vsx();
- pagefault_enable();
- preempt_enable();
  }
  return ret;
 }
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/cmnt: [SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

Khaled Elmously
In reply to this post by Joseph Salisbury-3
On 2018-10-12 14:22:14 , Joseph Salisbury wrote:

> BugLink: https://bugs.launchpad.net/bugs/1790832
>
> == SRU Justification ==
> IBM is requesting this patch in all releases order to fix the sleep-in-atomic
> bugs in AES-CBC and AES-XTS VMX implementations.
>
> This patch is a clean cherry pick and has already been applied to
> Cosmic.  It is also needed in Xenial, but there was a minor context
> diff, so the Xenial SRU will be sent separatly.  
>
> == Fix ==
> 0522236d4f9c ("crypto: vmx - Fix sleep-in-atomic bugs")
>
> == Regression Potential ==
> Low.  This patch has also been cc'd to upstream stable, so it has had
> additional upstream review.
>
> == Test Case ==
> A test kernel was built with this patch and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
>
> Ondrej Mosnacek (1):
>   crypto: vmx - Fix sleep-in-atomic bugs
>
>  drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
>  drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
>  2 files changed, 28 insertions(+), 23 deletions(-)
>

Acked-by: Khalid Elmously <[hidden email]>

I think the expectation for non-clean cherry-picks is to say "backported from XXXXXXXXX" followed by something like [jsalisbury: minor context adjustment] - at least that's what I've been told. I'm fine with it as-is though.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/cmnt: [SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

Kleber Souza
In reply to this post by Joseph Salisbury-3
On 10/12/18 20:22, Joseph Salisbury wrote:

> BugLink: https://bugs.launchpad.net/bugs/1790832
>
> == SRU Justification ==
> IBM is requesting this patch in all releases order to fix the sleep-in-atomic
> bugs in AES-CBC and AES-XTS VMX implementations.
>
> This patch is a clean cherry pick and has already been applied to
> Cosmic.  It is also needed in Xenial, but there was a minor context
> diff, so the Xenial SRU will be sent separatly.  
>
> == Fix ==
> 0522236d4f9c ("crypto: vmx - Fix sleep-in-atomic bugs")
>
> == Regression Potential ==
> Low.  This patch has also been cc'd to upstream stable, so it has had
> additional upstream review.
>
> == Test Case ==
> A test kernel was built with this patch and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
>
> Ondrej Mosnacek (1):
>   crypto: vmx - Fix sleep-in-atomic bugs
>
>  drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
>  drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
>  2 files changed, 28 insertions(+), 23 deletions(-)
>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>


Hi Joe,

If I understood correctly, only the Xenial version of the patch needed
some context adjustment, is that correct?

If that's the case this patch can be applied as is, otherwise the
provenance needs to be adjusted as well.

Thanks,
Kleber

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK/cmnt: [SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

Joseph Salisbury-3
In reply to this post by Khaled Elmously
On 10/14/2018 12:14 PM, Khaled Elmously wrote:

> On 2018-10-12 14:22:14 , Joseph Salisbury wrote:
>> BugLink: https://bugs.launchpad.net/bugs/1790832
>>
>> == SRU Justification ==
>> IBM is requesting this patch in all releases order to fix the sleep-in-atomic
>> bugs in AES-CBC and AES-XTS VMX implementations.
>>
>> This patch is a clean cherry pick and has already been applied to
>> Cosmic.  It is also needed in Xenial, but there was a minor context
>> diff, so the Xenial SRU will be sent separatly.  
>>
>> == Fix ==
>> 0522236d4f9c ("crypto: vmx - Fix sleep-in-atomic bugs")
>>
>> == Regression Potential ==
>> Low.  This patch has also been cc'd to upstream stable, so it has had
>> additional upstream review.
>>
>> == Test Case ==
>> A test kernel was built with this patch and tested by the original bug reporter.
>> The bug reporter states the test kernel resolved the bug.
>>
>>
>> Ondrej Mosnacek (1):
>>   crypto: vmx - Fix sleep-in-atomic bugs
>>
>>  drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
>>  drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
>>  2 files changed, 28 insertions(+), 23 deletions(-)
>>
> Acked-by: Khalid Elmously <[hidden email]>
>
> I think the expectation for non-clean cherry-picks is to say "backported from XXXXXXXXX" followed by something like [jsalisbury: minor context adjustment] - at least that's what I've been told. I'm fine with it as-is though.

This commit does cherry pick cleanly for both Xenail and Bionic. 
However, the 'git format-patch' output is slightly different for each
kernel after the cherry pick.  This is due to whatever the 'git cherry
pick' does behind the scenes to pick cleanly.  Because I didn't need to
make any changes or do any backporting, I kept the 'cherry picked from'
line for both Xenail and Bionic.  I just SRU'd them separately. 

Do we want to do something different in these cases?


Thanks,

Joe



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Bionic][PATCH 0/1] crypto: vmx - Fix sleep-in-atomic bugs

Khaled Elmously
In reply to this post by Joseph Salisbury-3
On 2018-10-12 14:22:14 , Joseph Salisbury wrote:

> BugLink: https://bugs.launchpad.net/bugs/1790832
>
> == SRU Justification ==
> IBM is requesting this patch in all releases order to fix the sleep-in-atomic
> bugs in AES-CBC and AES-XTS VMX implementations.
>
> This patch is a clean cherry pick and has already been applied to
> Cosmic.  It is also needed in Xenial, but there was a minor context
> diff, so the Xenial SRU will be sent separatly.  
>
> == Fix ==
> 0522236d4f9c ("crypto: vmx - Fix sleep-in-atomic bugs")
>
> == Regression Potential ==
> Low.  This patch has also been cc'd to upstream stable, so it has had
> additional upstream review.
>
> == Test Case ==
> A test kernel was built with this patch and tested by the original bug reporter.
> The bug reporter states the test kernel resolved the bug.
>
>
> Ondrej Mosnacek (1):
>   crypto: vmx - Fix sleep-in-atomic bugs
>
>  drivers/crypto/vmx/aes_cbc.c | 30 ++++++++++++++----------------
>  drivers/crypto/vmx/aes_xts.c | 21 ++++++++++++++-------
>  2 files changed, 28 insertions(+), 23 deletions(-)
>
> --
> 2.7.4
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team