Re: PBKDF2 support in the linux kernel

[Theodore Y. Ts'o]

On Sat, May 26, 2018 at 03:36:37PM +0200, Stephan Mueller wrote:
> - security related code should be vetted (which arguably is the case when the 
> discussed PBKDF is part of the kernel)
> > 
> > If he/she were to add their own userland code then he would surely be
> > criticized for rolling his own implementation.

If one were to copy and paste cryptographic algorithms, that's surely
not "rolling your own implementation" --- you're using someone else's
implemntation.  The argument for why code reuse by copy and paste is
not a good thing is that you won't pick up bug fixes that might show
up later.  But for a cryptographic algorithm (e.g., sHA256, CHACHA20,
etc.) with test vectors, in general there aren't cases where there are
bug fixes that come later.  An argument which makes sense for, say, an
implementation of TLS, is not really applicable here.

Given that there are other ways to address using well-vetted and
reviewed code, I would argue that "run code at with least privleges"
is clearly the far more important consideration here.

- Ted

Re: KASAN: use-after-free Read in crypto_destroy_tfm

[Dmitry Vyukov]

On Sat, May 26, 2018 at 7:40 PM, syzbot
 wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit:0644f186fc9d Merge tag 'for_linus' of git://git.kernel.org..
> git tree:   upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=102bc25780
> kernel config:  https://syzkaller.appspot.com/x/.config?x=61c12b53c2a25ec4
> dashboard link: https://syzkaller.appspot.com/bug?extid=352126a5be7ccb25754e
> compiler:   gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+352126a5be7ccb257...@syzkaller.appspotmail.com

Eric, do you remember if we had any recent fixes for this?

> ==
> BUG: KASAN: use-after-free in crypto_destroy_tfm+0x2a3/0x300
> crypto/api.c:573
> Read of size 8 at addr 8801d9023238 by task syz-executor6/10078
>
> CPU: 1 PID: 10078 Comm: syz-executor6 Not tainted 4.17.0-rc2+ #19
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Call Trace:
>  __dump_stack lib/dump_stack.c:77 [inline]
>  dump_stack+0x1b9/0x294 lib/dump_stack.c:113
>  print_address_description+0x6c/0x20b mm/kasan/report.c:256
>  kasan_report_error mm/kasan/report.c:354 [inline]
>  kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
>  crypto_destroy_tfm+0x2a3/0x300 crypto/api.c:573
>  crypto_free_rng include/crypto/rng.h:122 [inline]
>  rng_release+0x18/0x20 crypto/algif_rng.c:124
>  alg_do_release crypto/af_alg.c:119 [inline]
>  alg_sock_destruct+0x92/0xe0 crypto/af_alg.c:362
>  __sk_destruct+0xff/0xa40 net/core/sock.c:1566
>  sk_destruct+0x78/0x90 net/core/sock.c:1601
>  __sk_free+0x22e/0x340 net/core/sock.c:1612
>  sk_free+0x42/0x50 net/core/sock.c:1623
>  sock_put include/net/sock.h:1664 [inline]
>  af_alg_release+0x6e/0x90 crypto/af_alg.c:126
>  sock_release+0x96/0x1b0 net/socket.c:594
>  sock_close+0x16/0x20 net/socket.c:1149
>  __fput+0x34d/0x890 fs/file_table.c:209
>  fput+0x15/0x20 fs/file_table.c:243
>  task_work_run+0x1e4/0x290 kernel/task_work.c:113
>  exit_task_work include/linux/task_work.h:22 [inline]
>  do_exit+0x1aee/0x2730 kernel/exit.c:865
>  do_group_exit+0x16f/0x430 kernel/exit.c:968
>  get_signal+0x886/0x1960 kernel/signal.c:2469
>  do_signal+0x98/0x2040 arch/x86/kernel/signal.c:810
>  exit_to_usermode_loop+0x28a/0x310 arch/x86/entry/common.c:162
>  prepare_exit_to_usermode arch/x86/entry/common.c:196 [inline]
>  syscall_return_slowpath arch/x86/entry/common.c:265 [inline]
>  do_syscall_64+0x6ac/0x800 arch/x86/entry/common.c:290
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x455979
> RSP: 002b:7f4f4bbc2c68 EFLAGS: 0246 ORIG_RAX: 0036
> RAX:  RBX: 7f4f4bbc36d4 RCX: 00455979
> RDX: 0001 RSI: 0117 RDI: 0014
> RBP: 0072bf50 R08:  R09: 
> R10: 204f7000 R11: 0246 R12: 
> R13: 0519 R14: 006faaf8 R15: 0001
>
> Allocated by task 4484:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>  set_track mm/kasan/kasan.c:460 [inline]
>  kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
>  __do_kmalloc_node mm/slab.c:3682 [inline]
>  __kmalloc_node_track_caller+0x47/0x70 mm/slab.c:3696
>  __kmalloc_reserve.isra.38+0x3a/0xe0 net/core/skbuff.c:137
>  __alloc_skb+0x14d/0x780 net/core/skbuff.c:205
>  alloc_skb include/linux/skbuff.h:987 [inline]
>  netlink_alloc_large_skb net/netlink/af_netlink.c:1182 [inline]
>  netlink_sendmsg+0xb01/0xfa0 net/netlink/af_netlink.c:1876
>  sock_sendmsg_nosec net/socket.c:629 [inline]
>  sock_sendmsg+0xd5/0x120 net/socket.c:639
>  ___sys_sendmsg+0x805/0x940 net/socket.c:2117
>  __sys_sendmsg+0x115/0x270 net/socket.c:2155
>  __do_sys_sendmsg net/socket.c:2164 [inline]
>  __se_sys_sendmsg net/socket.c:2162 [inline]
>  __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
>  do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
>  entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Freed by task 4484:
>  save_stack+0x43/0xd0 mm/kasan/kasan.c:448
>  set_track mm/kasan/kasan.c:460 [inline]
>  __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
>  kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
>  __cache_free mm/slab.c:3498 [inline]
>  kfree+0xd9/0x260 mm/slab.c:3813
>  skb_free_head+0x99/0xc0 net/core/skbuff.c:550
>  skb_release_data+0x690/0x860 net/core/skbuff.c:570
>  skb_release_all+0x4a/0x60 net/core/skbuff.c:627
>  __kfree_skb net/core/skbuff.c:641 [inline]
>  consume_skb+0x18b/0x550 net/core/skbuff.c:701
>  netlink_unicast_kernel net/netlink/af_netlink.c:1311 [inline]
>  netlink_unicast+0x593/0x740 net/netlink/af_netlink.c:1336
>  netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901
>  

KASAN: use-after-free Read in crypto_destroy_tfm

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit:0644f186fc9d Merge tag 'for_linus' of git://git.kernel.org..
git tree:   upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=102bc25780
kernel config:  https://syzkaller.appspot.com/x/.config?x=61c12b53c2a25ec4
dashboard link: https://syzkaller.appspot.com/bug?extid=352126a5be7ccb25754e
compiler:   gcc (GCC) 8.0.1 20180413 (experimental)

Unfortunately, I don't have any reproducer for this crash yet.

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+352126a5be7ccb257...@syzkaller.appspotmail.com

==
BUG: KASAN: use-after-free in crypto_destroy_tfm+0x2a3/0x300  
crypto/api.c:573

Read of size 8 at addr 8801d9023238 by task syz-executor6/10078

CPU: 1 PID: 10078 Comm: syz-executor6 Not tainted 4.17.0-rc2+ #19
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS  
Google 01/01/2011

Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x1b9/0x294 lib/dump_stack.c:113
 print_address_description+0x6c/0x20b mm/kasan/report.c:256
 kasan_report_error mm/kasan/report.c:354 [inline]
 kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
 __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
 crypto_destroy_tfm+0x2a3/0x300 crypto/api.c:573
 crypto_free_rng include/crypto/rng.h:122 [inline]
 rng_release+0x18/0x20 crypto/algif_rng.c:124
 alg_do_release crypto/af_alg.c:119 [inline]
 alg_sock_destruct+0x92/0xe0 crypto/af_alg.c:362
 __sk_destruct+0xff/0xa40 net/core/sock.c:1566
 sk_destruct+0x78/0x90 net/core/sock.c:1601
 __sk_free+0x22e/0x340 net/core/sock.c:1612
 sk_free+0x42/0x50 net/core/sock.c:1623
 sock_put include/net/sock.h:1664 [inline]
 af_alg_release+0x6e/0x90 crypto/af_alg.c:126
 sock_release+0x96/0x1b0 net/socket.c:594
 sock_close+0x16/0x20 net/socket.c:1149
 __fput+0x34d/0x890 fs/file_table.c:209
 fput+0x15/0x20 fs/file_table.c:243
 task_work_run+0x1e4/0x290 kernel/task_work.c:113
 exit_task_work include/linux/task_work.h:22 [inline]
 do_exit+0x1aee/0x2730 kernel/exit.c:865
 do_group_exit+0x16f/0x430 kernel/exit.c:968
 get_signal+0x886/0x1960 kernel/signal.c:2469
 do_signal+0x98/0x2040 arch/x86/kernel/signal.c:810
 exit_to_usermode_loop+0x28a/0x310 arch/x86/entry/common.c:162
 prepare_exit_to_usermode arch/x86/entry/common.c:196 [inline]
 syscall_return_slowpath arch/x86/entry/common.c:265 [inline]
 do_syscall_64+0x6ac/0x800 arch/x86/entry/common.c:290
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x455979
RSP: 002b:7f4f4bbc2c68 EFLAGS: 0246 ORIG_RAX: 0036
RAX:  RBX: 7f4f4bbc36d4 RCX: 00455979
RDX: 0001 RSI: 0117 RDI: 0014
RBP: 0072bf50 R08:  R09: 
R10: 204f7000 R11: 0246 R12: 
R13: 0519 R14: 006faaf8 R15: 0001

Allocated by task 4484:
 save_stack+0x43/0xd0 mm/kasan/kasan.c:448
 set_track mm/kasan/kasan.c:460 [inline]
 kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
 __do_kmalloc_node mm/slab.c:3682 [inline]
 __kmalloc_node_track_caller+0x47/0x70 mm/slab.c:3696
 __kmalloc_reserve.isra.38+0x3a/0xe0 net/core/skbuff.c:137
 __alloc_skb+0x14d/0x780 net/core/skbuff.c:205
 alloc_skb include/linux/skbuff.h:987 [inline]
 netlink_alloc_large_skb net/netlink/af_netlink.c:1182 [inline]
 netlink_sendmsg+0xb01/0xfa0 net/netlink/af_netlink.c:1876
 sock_sendmsg_nosec net/socket.c:629 [inline]
 sock_sendmsg+0xd5/0x120 net/socket.c:639
 ___sys_sendmsg+0x805/0x940 net/socket.c:2117
 __sys_sendmsg+0x115/0x270 net/socket.c:2155
 __do_sys_sendmsg net/socket.c:2164 [inline]
 __se_sys_sendmsg net/socket.c:2162 [inline]
 __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
 do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
 entry_SYSCALL_64_after_hwframe+0x49/0xbe

Freed by task 4484:
 save_stack+0x43/0xd0 mm/kasan/kasan.c:448
 set_track mm/kasan/kasan.c:460 [inline]
 __kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
 kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
 __cache_free mm/slab.c:3498 [inline]
 kfree+0xd9/0x260 mm/slab.c:3813
 skb_free_head+0x99/0xc0 net/core/skbuff.c:550
 skb_release_data+0x690/0x860 net/core/skbuff.c:570
 skb_release_all+0x4a/0x60 net/core/skbuff.c:627
 __kfree_skb net/core/skbuff.c:641 [inline]
 consume_skb+0x18b/0x550 net/core/skbuff.c:701
 netlink_unicast_kernel net/netlink/af_netlink.c:1311 [inline]
 netlink_unicast+0x593/0x740 net/netlink/af_netlink.c:1336
 netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901
 sock_sendmsg_nosec net/socket.c:629 [inline]
 sock_sendmsg+0xd5/0x120 net/socket.c:639
 ___sys_sendmsg+0x805/0x940 net/socket.c:2117
 __sys_sendmsg+0x115/0x270 net/socket.c:2155
 __do_sys_sendmsg net/socket.c:2164 [inline]
 __se_sys_sendmsg net/socket.c:2162 [inline]
 __x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
 do_syscall_64+0x1b1/0x800 

Re: [PATCH] crypto: x86/aegis256 - Fix wrong key buffer size

[Herbert Xu]

On Sun, May 20, 2018 at 10:57:23AM +0200, Ondrej Mosnáček wrote:
> From: Ondrej Mosnacek 
> 
> AEGIS-256 key is two blocks, not one.
> 
> Fixes: 1d373d4e8e15 ("crypto: x86 - Add optimized AEGIS implementations")
> Reported-by: Eric Biggers 
> Signed-off-by: Ondrej Mosnacek 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 0/6] crypto: crc32 cleanups and unkeyed tests

[Herbert Xu]

On Sat, May 19, 2018 at 10:07:36PM -0700, Eric Biggers wrote:
> This series fixes up alignment for crc32-generic and crc32c-generic,
> removes test vectors for bfin_crc that are no longer needed, and adds
> unkeyed test vectors for crc32 and an extra unkeyed test vector for
> crc32c.  Adding the unkeyed test vectors also required a testmgr change
> to allow a single hash algorithm to have both unkeyed and keyed tests,
> without relying on having it work by accident.
> 
> The new test vectors pass with the generic and x86 CRC implementations.
> I haven't tested others yet; if any happen to be broken, they'll need to
> be fixed.
> 
> Eric Biggers (6):
>   crypto: crc32-generic - use unaligned access macros when needed
>   crypto: crc32c-generic - remove cra_alignmask
>   crypto: crc32-generic - remove __crc32_le()
>   crypto: testmgr - remove bfin_crc "hmac(crc32)" test vectors
>   crypto: testmgr - fix testing OPTIONAL_KEY hash algorithms
>   crypto: testmgr - add more unkeyed crc32 and crc32c test vectors
> 
>  crypto/crc32_generic.c  |  15 ++
>  crypto/crc32c_generic.c |   8 ++--
>  crypto/tcrypt.c |   4 --
>  crypto/testmgr.c|  56 +-
>  crypto/testmgr.h| 102 ++--
>  5 files changed, 66 insertions(+), 119 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: chtls - fix a missing-check bug

[Herbert Xu]

On Fri, May 18, 2018 at 02:55:35PM -0500, Wenwen Wang wrote:
> In do_chtls_setsockopt(), the tls crypto info is first copied from the
> poiner 'optval' in userspace and saved to 'tmp_crypto_info'. Then the
> 'version' of the crypto info is checked. If the version is not as expected,
> i.e., TLS_1_2_VERSION, error code -ENOTSUPP is returned to indicate that
> the provided crypto info is not supported yet. Then, the 'cipher_type'
> field of the 'tmp_crypto_info' is also checked to see if it is
> TLS_CIPHER_AES_GCM_128. If it is, the whole struct of
> tls12_crypto_info_aes_gcm_128 is copied from the pointer 'optval' and then
> the function chtls_setkey() is invoked to set the key.
> 
> Given that the 'optval' pointer resides in userspace, a malicious userspace
> process can race to change the data pointed by 'optval' between the two
> copies. For example, a user can provide a crypto info with TLS_1_2_VERSION
> and TLS_CIPHER_AES_GCM_128. After the first copy, the user can modify the
> 'version' and the 'cipher_type' fields to any versions and/or cipher types
> that are not allowed. This way, the user can bypass the checks, inject
> bad data to the kernel, cause chtls_setkey() to set a wrong key or other
> issues.
> 
> This patch reuses the data copied in the first try so as to ensure these
> checks will not be bypassed.
> 
> Signed-off-by: Wenwen Wang 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: inside-secure - do not use memset on MMIO

[Herbert Xu]

On Thu, May 17, 2018 at 03:22:14PM +0200, Antoine Tenart wrote:
> This patch fixes the Inside Secure driver which uses a memtset() call to
> set an MMIO area from the cryptographic engine to 0. This is wrong as
> memset() isn't guaranteed to work on MMIO for many reasons. This led to
> kernel paging request panics in certain cases. Use memset_io() instead.
> 
> Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto 
> engine driver")
> Reported-by: Ofer Heifetz 
> Signed-off-by: Antoine Tenart 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2 00/10] crypto: inside-secure - AEAD support

[Herbert Xu]

On Mon, May 14, 2018 at 03:10:54PM +0200, Antoine Tenart wrote:
> This series brings AEAD algorithms to the Inside Secure SafeXcel driver.
> The first 7 commits rework the driver to allow the future AEAD addition,
> and then 3 commits add AEAD functions and 3 algorithms.
> 
> This is based on top of v4.17-rc5.
> 
> Thanks!
> Antoine
> 
> Since v1:
>   - Reworked the driver to remove VLA's and added a custom on-stack
> request definition in the driver, to be used in the invalidation
> process. (Patch 1/10 was replaced, patch 8/10 was reworked).
>   - Rebased on top of v4.17-rc5 (was -rc3).
> 
> Antoine Tenart (10):
>   crypto: inside-secure - remove VLAs
>   crypto: inside-secure - rework cipher functions for future AEAD
> support
>   crypto: inside-secure - rework the alg type settings in the context
>   crypto: inside-secure - make the context control size dynamic
>   crypto: inside-secure - make the key and context size computation
> dynamic
>   crypto: inside-secure - fix the hash then encrypt/decrypt types
>   crypto: inside-secure - improve error reporting
>   crypto: inside-secure - authenc(hmac(sha256),cbc(aes)) support
>   crypto: inside-secure - authenc(hmac(sha224),cbc(aes)) support
>   crypto: inside-secure - authenc(hmac(sha1),cbc(aes)) support
> 
>  drivers/crypto/Kconfig|   1 +
>  drivers/crypto/inside-secure/safexcel.c   |  32 +
>  drivers/crypto/inside-secure/safexcel.h   |  44 +-
>  .../crypto/inside-secure/safexcel_cipher.c| 642 ++
>  drivers/crypto/inside-secure/safexcel_hash.c  |  23 +-
>  5 files changed, 600 insertions(+), 142 deletions(-)

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: chtls: generic handling of data and hdr

[Herbert Xu]

On Mon, May 14, 2018 at 04:41:38PM +0530, Atul Gupta wrote:
> removed redundant check and made TLS PDU and header recv
> handling common as received from HW.
> Ensure that only tls header is read in cpl_rx_tls_cmp
> read-ahead and skb is freed when entire data is processed.
> 
> Signed-off-by: Atul Gupta 
> Signed-off-by: Harsh Jain 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: PBKDF2 support in the linux kernel

[Stephan Mueller]

Am Samstag, 26. Mai 2018, 14:17:11 CEST schrieb Jeffrey Walton:

Hi Jeffrey,

> On Thu, May 24, 2018 at 5:11 AM, Stephan Mueller  
wrote:
> > Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
> > 
> > Hi Rafael,
> > 
> >> So the problem is that Yu would like to use this for hibernation
> >> encryption
> >> done entirely in the kernel.
> > 
> > But why do you need to perform PBKDF in kernel space?
> 
> I may be mis-parsing things, but using audited kernel code is a matter
> of governance and good security engineering. I don't believe it is not
> a matter of laziness.

I agree that using a well-vetted crypto implementation is the best thing(TM) 
:-)

But there are two competing problems we face in our discussion:

- code should only execute with elevated privileges like kernel space if 
really needed (i.e. PBKDF should not be in the kernel)

- security related code should be vetted (which arguably is the case when the 
discussed PBKDF is part of the kernel)
> 
> If he/she were to add their own userland code then he would surely be
> criticized for rolling his own implementation.

For crypto implementations, the correctness can always be verified using 
public test vectors. For PBKDF, the RFC has them.

The question remains about whether the code is (as much as possible) free of 
security bugs. Here, using well-vetted and reviewed code would make most 
sense.

The problem is that the two conflicting problems do not seem to allow to make 
a straight-forward decision. Some folks think the one problem is valued higher 
than the other. In general, I personally fall into the camp that likes to not 
have code executing with elevated privileges unless absolutely required. I 
understand that others have the opposite view. And I do not know how we can 
reach an agreement.

All I can say is: If you do not want to have a large scale crypto lib like 
libgcrypt in your code tree (which naturally has code that you do not need and 
may cause problems), you may simply use a PBKDF implementation in user space 
and use AF_ALG to access the crypto primitives. This would mean that the code 
size you need in user space could be fairly limited.

Unfortunately I cannot speak about other people's AF_ALG PBKDF implementation. 
The design of libkcapi is such that you can pull individual C files (i.e. only 
those code parts you really need) out of the code tree and add them to your 
project code path (see the bottom of libkcapi's README for details). This way, 
you can minimize the compiled code which would benefit also the overall 
security as the attack surface is kept small. And as long as you simply pull 
these C files, you do not maintain your private code, but you can benefit from 
any updates to libkcapi in the future.

Ciao
Stephan

Re: PBKDF2 support in the linux kernel

[Jeffrey Walton]

On Thu, May 24, 2018 at 5:11 AM, Stephan Mueller  wrote:
> Am Donnerstag, 24. Mai 2018, 10:33:07 CEST schrieb Rafael J. Wysocki:
>
> Hi Rafael,
>
>> So the problem is that Yu would like to use this for hibernation encryption
>> done entirely in the kernel.
>
> But why do you need to perform PBKDF in kernel space?

I may be mis-parsing things, but using audited kernel code is a matter
of governance and good security engineering. I don't believe it is not
a matter of laziness.

If he/she were to add their own userland code then he would surely be
criticized for rolling his own implementation.

Jeff

Re: WARNING: kernel stack regs has bad 'bp' value (3)

[Eric Biggers]

On Sat, May 12, 2018 at 10:43:08AM +0200, Dmitry Vyukov wrote:
> On Fri, Feb 2, 2018 at 11:18 PM, Eric Biggers  wrote:
> > On Fri, Feb 02, 2018 at 02:57:32PM +0100, Dmitry Vyukov wrote:
> >> On Fri, Feb 2, 2018 at 2:48 PM, syzbot
> >>  wrote:
> >> > Hello,
> >> >
> >> > syzbot hit the following crash on upstream commit
> >> > 7109a04eae81c41ed529da9f3c48c3655ccea741 (Thu Feb 1 17:37:30 2018 +)
> >> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/ide
> >> >
> >> > So far this crash happened 4 times on net-next, upstream.
> >> > C reproducer is attached.
> >> > syzkaller reproducer is attached.
> >> > Raw console output is attached.
> >> > compiler: gcc (GCC) 7.1.1 20170620
> >> > .config is attached.
> >>
> >>
> >> From suspicious frames I see salsa20_asm_crypt there, so +crypto 
> >> maintainers.
> >>
> >
> > Looks like the x86 implementations of Salsa20 (both i586 and x86_64) need 
> > to be
> > updated to not use %ebp/%rbp.
> 
> Ard,
> 
> This was bisected as introduced by:
> 
> commit 83dee2ce1ae791c3dc0c9d4d3a8d42cb109613f6
> Author: Ard Biesheuvel 
> Date:   Fri Jan 19 12:04:34 2018 +
> 
> crypto: sha3-generic - rewrite KECCAK transform to help the
> compiler optimize
> 
> https://gist.githubusercontent.com/dvyukov/47f93f5a0679170dddf93bc019b42f6d/raw/65beac8ddd30003bbd4e9729236dc8572094abf7/gistfile1.txt

Note that syzbot's original C reproducer (from Feb 1) for this actually
triggered the warning through salsa20-asm, which I've just proposed to "fix" by
https://patchwork.kernel.org/patch/10428863/.  sha3-generic is apparently
another instance of the same bug, where the %rbp register is used for data.

Eric

[PATCH 1/2] crypto: x86/salsa20 - remove x86 salsa20 implementations

[Eric Biggers]

From: Eric Biggers 

The x86 assembly implementations of Salsa20 use the frame base pointer
register (%ebp or %rbp), which breaks frame pointer convention and
breaks stack traces when unwinding from an interrupt in the crypto code.
Recent (v4.10+) kernels will warn about this, e.g.

WARNING: kernel stack regs at a8291e69 in syzkaller047086:4677 has bad 
'bp' value 1077994c
[...]

But after looking into it, I believe there's very little reason to still
retain the x86 Salsa20 code.  First, these are *not* vectorized
(SSE2/SSSE3/AVX2) implementations, which would be needed to get anywhere
close to the best Salsa20 performance on any remotely modern x86
processor; they're just regular x86 assembly.  Second, it's still
unclear that anyone is actually using the kernel's Salsa20 at all,
especially given that now ChaCha20 is supported too, and with much more
efficient SSSE3 and AVX2 implementations.  Finally, in benchmarks I did
on both Intel and AMD processors with both gcc 8.1.0 and gcc 4.9.4, the
x86_64 salsa20-asm is actually slightly *slower* than salsa20-generic
(~3% slower on Skylake, ~10% slower on Zen), while the i686 salsa20-asm
is only slightly faster than salsa20-generic (~15% faster on Skylake,
~20% faster on Zen).  The gcc version made little difference.

So, the x86_64 salsa20-asm is pretty clearly useless.  That leaves just
the i686 salsa20-asm, which based on my tests provides a 15-20% speed
boost.  But that's without updating the code to not use %ebp.  And given
the maintenance cost, the small speed difference vs. salsa20-generic,
the fact that few people still use i686 kernels, the doubt that anyone
is even using the kernel's Salsa20 at all, and the fact that a SSE2
implementation would almost certainly be much faster on any remotely
modern x86 processor yet no one has cared enough to add one yet, I don't
think it's worthwhile to keep.

Thus, just remove both the x86_64 and i686 salsa20-asm implementations.

Reported-by: syzbot+ffa3a158337bbc01f...@syzkaller.appspotmail.com
Signed-off-by: Eric Biggers 
---
 arch/x86/crypto/Makefile|   4 -
 arch/x86/crypto/salsa20-i586-asm_32.S   | 938 
 arch/x86/crypto/salsa20-x86_64-asm_64.S | 805 
 arch/x86/crypto/salsa20_glue.c  |  91 ---
 crypto/Kconfig  |  28 -
 5 files changed, 1866 deletions(-)
 delete mode 100644 arch/x86/crypto/salsa20-i586-asm_32.S
 delete mode 100644 arch/x86/crypto/salsa20-x86_64-asm_64.S
 delete mode 100644 arch/x86/crypto/salsa20_glue.c

diff --git a/arch/x86/crypto/Makefile b/arch/x86/crypto/Makefile
index 3813e7cdaada..2e07a0e66314 100644
--- a/arch/x86/crypto/Makefile
+++ b/arch/x86/crypto/Makefile
@@ -15,7 +15,6 @@ obj-$(CONFIG_CRYPTO_GLUE_HELPER_X86) += glue_helper.o
 
 obj-$(CONFIG_CRYPTO_AES_586) += aes-i586.o
 obj-$(CONFIG_CRYPTO_TWOFISH_586) += twofish-i586.o
-obj-$(CONFIG_CRYPTO_SALSA20_586) += salsa20-i586.o
 obj-$(CONFIG_CRYPTO_SERPENT_SSE2_586) += serpent-sse2-i586.o
 
 obj-$(CONFIG_CRYPTO_AES_X86_64) += aes-x86_64.o
@@ -24,7 +23,6 @@ obj-$(CONFIG_CRYPTO_CAMELLIA_X86_64) += camellia-x86_64.o
 obj-$(CONFIG_CRYPTO_BLOWFISH_X86_64) += blowfish-x86_64.o
 obj-$(CONFIG_CRYPTO_TWOFISH_X86_64) += twofish-x86_64.o
 obj-$(CONFIG_CRYPTO_TWOFISH_X86_64_3WAY) += twofish-x86_64-3way.o
-obj-$(CONFIG_CRYPTO_SALSA20_X86_64) += salsa20-x86_64.o
 obj-$(CONFIG_CRYPTO_CHACHA20_X86_64) += chacha20-x86_64.o
 obj-$(CONFIG_CRYPTO_SERPENT_SSE2_X86_64) += serpent-sse2-x86_64.o
 obj-$(CONFIG_CRYPTO_AES_NI_INTEL) += aesni-intel.o
@@ -68,7 +66,6 @@ endif
 
 aes-i586-y := aes-i586-asm_32.o aes_glue.o
 twofish-i586-y := twofish-i586-asm_32.o twofish_glue.o
-salsa20-i586-y := salsa20-i586-asm_32.o salsa20_glue.o
 serpent-sse2-i586-y := serpent-sse2-i586-asm_32.o serpent_sse2_glue.o
 
 aes-x86_64-y := aes-x86_64-asm_64.o aes_glue.o
@@ -77,7 +74,6 @@ camellia-x86_64-y := camellia-x86_64-asm_64.o camellia_glue.o
 blowfish-x86_64-y := blowfish-x86_64-asm_64.o blowfish_glue.o
 twofish-x86_64-y := twofish-x86_64-asm_64.o twofish_glue.o
 twofish-x86_64-3way-y := twofish-x86_64-asm_64-3way.o twofish_glue_3way.o
-salsa20-x86_64-y := salsa20-x86_64-asm_64.o salsa20_glue.o
 chacha20-x86_64-y := chacha20-ssse3-x86_64.o chacha20_glue.o
 serpent-sse2-x86_64-y := serpent-sse2-x86_64-asm_64.o serpent_sse2_glue.o
 
diff --git a/arch/x86/crypto/salsa20-i586-asm_32.S 
b/arch/x86/crypto/salsa20-i586-asm_32.S
deleted file mode 100644
index 6014b7b9e52a..
--- a/arch/x86/crypto/salsa20-i586-asm_32.S
+++ /dev/null
@@ -1,938 +0,0 @@
-# Derived from:
-#  salsa20_pm.s version 20051229
-#  D. J. Bernstein
-#  Public domain.
-
-#include 
-
-.text
-
-# enter salsa20_encrypt_bytes
-ENTRY(salsa20_encrypt_bytes)
-   mov %esp,%eax
-   and $31,%eax
-   add $256,%eax
-   sub %eax,%esp
-   # eax_stack = eax
-   movl%eax,80(%esp)
-   # ebx_stack = ebx
-   movl

[PATCH 2/2] crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"

[Eric Biggers]

From: Eric Biggers 

This reverts commit eb772f37ae8163a89e28a435f6a18742ae06653b, as now the
x86 Salsa20 implementation has been removed and the generic helpers are
no longer needed outside of salsa20_generic.c.

We could keep this just in case someone else wants to add a new
optimized Salsa20 implementation.  But given that we have ChaCha20 now
too, I think it's unlikely.  And this can always be reverted back.

Signed-off-by: Eric Biggers 
---
 crypto/salsa20_generic.c | 20 +---
 include/crypto/salsa20.h | 27 ---
 2 files changed, 13 insertions(+), 34 deletions(-)
 delete mode 100644 include/crypto/salsa20.h

diff --git a/crypto/salsa20_generic.c b/crypto/salsa20_generic.c
index 5074006a56c3..8c77bc78a09f 100644
--- a/crypto/salsa20_generic.c
+++ b/crypto/salsa20_generic.c
@@ -21,9 +21,17 @@
 
 #include 
 #include 
-#include 
 #include 
 
+#define SALSA20_IV_SIZE8
+#define SALSA20_MIN_KEY_SIZE  16
+#define SALSA20_MAX_KEY_SIZE  32
+#define SALSA20_BLOCK_SIZE64
+
+struct salsa20_ctx {
+   u32 initial_state[16];
+};
+
 static void salsa20_block(u32 *state, __le32 *stream)
 {
u32 x[16];
@@ -93,16 +101,15 @@ static void salsa20_docrypt(u32 *state, u8 *dst, const u8 
*src,
}
 }
 
-void crypto_salsa20_init(u32 *state, const struct salsa20_ctx *ctx,
+static void salsa20_init(u32 *state, const struct salsa20_ctx *ctx,
 const u8 *iv)
 {
memcpy(state, ctx->initial_state, sizeof(ctx->initial_state));
state[6] = get_unaligned_le32(iv + 0);
state[7] = get_unaligned_le32(iv + 4);
 }
-EXPORT_SYMBOL_GPL(crypto_salsa20_init);
 
-int crypto_salsa20_setkey(struct crypto_skcipher *tfm, const u8 *key,
+static int salsa20_setkey(struct crypto_skcipher *tfm, const u8 *key,
  unsigned int keysize)
 {
static const char sigma[16] = "expand 32-byte k";
@@ -143,7 +150,6 @@ int crypto_salsa20_setkey(struct crypto_skcipher *tfm, 
const u8 *key,
 
return 0;
 }
-EXPORT_SYMBOL_GPL(crypto_salsa20_setkey);
 
 static int salsa20_crypt(struct skcipher_request *req)
 {
@@ -155,7 +161,7 @@ static int salsa20_crypt(struct skcipher_request *req)
 
err = skcipher_walk_virt(, req, true);
 
-   crypto_salsa20_init(state, ctx, walk.iv);
+   salsa20_init(state, ctx, walk.iv);
 
while (walk.nbytes > 0) {
unsigned int nbytes = walk.nbytes;
@@ -183,7 +189,7 @@ static struct skcipher_alg alg = {
.max_keysize= SALSA20_MAX_KEY_SIZE,
.ivsize = SALSA20_IV_SIZE,
.chunksize  = SALSA20_BLOCK_SIZE,
-   .setkey = crypto_salsa20_setkey,
+   .setkey = salsa20_setkey,
.encrypt= salsa20_crypt,
.decrypt= salsa20_crypt,
 };
diff --git a/include/crypto/salsa20.h b/include/crypto/salsa20.h
deleted file mode 100644
index 19ed48aefc86..
--- a/include/crypto/salsa20.h
+++ /dev/null
@@ -1,27 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * Common values for the Salsa20 algorithm
- */
-
-#ifndef _CRYPTO_SALSA20_H
-#define _CRYPTO_SALSA20_H
-
-#include 
-
-#define SALSA20_IV_SIZE8
-#define SALSA20_MIN_KEY_SIZE   16
-#define SALSA20_MAX_KEY_SIZE   32
-#define SALSA20_BLOCK_SIZE 64
-
-struct crypto_skcipher;
-
-struct salsa20_ctx {
-   u32 initial_state[16];
-};
-
-void crypto_salsa20_init(u32 *state, const struct salsa20_ctx *ctx,
-const u8 *iv);
-int crypto_salsa20_setkey(struct crypto_skcipher *tfm, const u8 *key,
- unsigned int keysize);
-
-#endif /* _CRYPTO_SALSA20_H */
-- 
2.17.0

[PATCH 0/2] crypto: remove x86 salsa20 implementations

[Eric Biggers]

Hello,

The x86 asm implementations of Salsa20 have been missed so far in the
fixes to stop abusing %ebp/%rbp in asm code to get correct stack traces.
This has been causing the unwinder warnings reported by syzkaller to
continue.

This series "fixes" it by just removing the offending salsa20-asm
implementations, which as far as I can tell are basically useless these
days; the x86_64 asm version in particular isn't actually any faster
than the C version anymore.  (And possibly no one even uses these
anyway.)  See the patch for the full explanation.

Eric Biggers (2):
  crypto: x86/salsa20 - remove x86 salsa20 implementations
  crypto: salsa20 - Revert "crypto: salsa20 - export generic helpers"

 arch/x86/crypto/Makefile|   4 -
 arch/x86/crypto/salsa20-i586-asm_32.S   | 938 
 arch/x86/crypto/salsa20-x86_64-asm_64.S | 805 
 arch/x86/crypto/salsa20_glue.c  |  91 ---
 crypto/Kconfig  |  28 -
 crypto/salsa20_generic.c|  20 +-
 include/crypto/salsa20.h|  27 -
 7 files changed, 13 insertions(+), 1900 deletions(-)
 delete mode 100644 arch/x86/crypto/salsa20-i586-asm_32.S
 delete mode 100644 arch/x86/crypto/salsa20-x86_64-asm_64.S
 delete mode 100644 arch/x86/crypto/salsa20_glue.c
 delete mode 100644 include/crypto/salsa20.h

-- 
2.17.0