Re: [PATCH] crypto: algapi - guard against uninitialized spawn list in crypto_remove_spawns

[Eric Biggers]

Hi Michal,

On Tue, Jun 25, 2019 at 09:16:24AM +0200, Michal Suchanek wrote:
> Reportedly on Linux 4.12 the LTP testsuite crashes at pcrypt_aead01 infrequently.
> 
> To get it reproduce more frequently I tried
> 
> n=0 ; while true ; do /opt/ltp/testcases/bin/pcrypt_aead01 >& /dev/null ; n=$(expr $n + 1) ; echo -ne $n\\r ; done
> 
> but this is quite stable. However, holding ^C in the terminal where the loop is running tends to trigger the crash.
> 
> The backtrace is:
> 
> [  100.615804] Unable to handle kernel paging request for data at address 0x00000000
> [  100.615876] Faulting instruction address: 0xc000000000520e7c
> [  100.615943] Oops: Kernel access of bad area, sig: 11 [#1]
> [  100.616001] SMP NR_CPUS=2048 
> [  100.616002] NUMA 
> [  100.616030] pSeries
> [  100.616054] Modules linked in: authenc pcrypt crypto_user kvm_pr kvm ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter devlink ip_tables x_tables af_packet rtc_generic vmx_crypto ibmveth(X) gf128mul btrfs xor raid6_pq sd_mod ibmvscsi(X) scsi_transport_srp crc32c_vpmsum sg dm_multipath dm_mod scsi_dh_rdac scsi_dh_emc scsi_dh_alua scsi_mod autofs4
> [  100.616478] Supported: Yes, External
> [  100.616509] CPU: 5 PID: 6270 Comm: pcrypt_aead01 Tainted: G                   4.12.14-150.22-default #1 SLE15
> [  100.616632] task: c000000595084d80 task.stack: c0000005be6dc000
> [  100.616708] NIP: c000000000520e7c LR: c000000000521e3c CTR: c000000000521de0
> [  100.616801] REGS: c0000005be6df620 TRAP: 0300   Tainted: G                    (4.12.14-150.22-default)
> [  100.616906] MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE>
> [  100.616912]   CR: 24002844  XER: 20040000
> [  100.617003] CFAR: c000000000008860 DAR: 0000000000000000 DSISR: 40000000 SOFTE: 1 
>                GPR00: c0000005a331f810 c0000005be6df8a0 c00000000119aa00 c0000005a331f800 
>                GPR04: c0000005be6df930 c0000005be6df8c0 c0000005be6df8d0 0000000000000000 
>                GPR08: 7269632929290000 c0000005a331f800 0000000000000000 0000000000000000 
>                GPR12: c000000000521de0 c000000007a33700 00000001271a0ee0 00007fffcb9e7bb8 
>                GPR16: 00000001271c2d80 00000001271c2d88 00007fffcb9e7a50 00007fffcb9e7a44 
>                GPR20: 00007fffcb9e7a98 00007fffcb9e7a60 0000000000000010 0000000000000010 
>                GPR24: 0000000000000000 0000000000000000 fffffffffffff000 c0000005be6dfaf0 
>                GPR28: c0000005b9929d00 0000000000000c93 c0000005be6df930 c0000005be6df8e0 
> [  100.617774] NIP [c000000000520e7c] crypto_remove_spawns+0x6c/0x2e0
> [  100.617816] LR [c000000000521e3c] crypto_unregister_instance+0x5c/0xa0
> [  100.617881] Call Trace:
> [  100.617903] [c0000005be6df8a0] [c0000005b9929d00] 0xc0000005b9929d00 (unreliable)
> [  100.617971] [c0000005be6df910] [0000000000000000]           (null)
> [  100.618021] [c0000005be6df960] [d0000000098d0894] crypto_del_alg+0xdc/0x110 [crypto_user]
> [  100.618119] [c0000005be6df990] [d0000000098d0b58] crypto_user_rcv_msg+0xe0/0x260 [crypto_user]
> [  100.618222] [c0000005be6dfa30] [c00000000086d678] netlink_rcv_skb+0x78/0x170
> [  100.618309] [c0000005be6dfaa0] [d0000000098d0064] crypto_netlink_rcv+0x4c/0x80 [crypto_user]
> [  100.618407] [c0000005be6dfad0] [c00000000086cb98] netlink_unicast+0x208/0x2f0
> [  100.618488] [c0000005be6dfb40] [c00000000086d170] netlink_sendmsg+0x380/0x440
> [  100.618582] [c0000005be6dfbd0] [c0000000007e9ba4] sock_sendmsg+0x64/0x90
> [  100.618650] [c0000005be6dfc00] [c0000000007eb94c] ___sys_sendmsg+0x2cc/0x330
> [  100.618710] [c0000005be6dfd90] [c0000000007ed02c] __sys_sendmsg+0x5c/0xc0
> [  100.618766] [c0000005be6dfe30] [c00000000000b188] system_call+0x3c/0x130
> [  100.618822] Instruction dump:
> [  100.618839] e9430010 83a90020 38a10020 fbe10040 fbe10048 f8c10030 f8c10038 f8a10020 
> [  100.618902] f8a10028 38030010 7fa05040 7d475378 <e90a0000> 419e0064 60000000 60000000 
> [  100.618980] ---[ end trace 60475621348ca387 ]---
> 
> The code looks like this:
> 
>    0xc000000000520e10 <+0>:     c8 00 4c 3c     addis   r2,r12,200
>    0xc000000000520e14 <+4>:     f0 9b 42 38     addi    r2,r2,-25616
>    0xc000000000520e18 <+8>:     a6 02 08 7c     mflr    r0
>    0xc000000000520e1c <+12>:    00 00 00 60     nop
>    0xc000000000520e20 <+16>:    79 2b ab 7c     mr.     r11,r5
>    0xc000000000520e24 <+20>:    f0 ff c1 fb     std     r30,-16(r1)
>    0xc000000000520e28 <+24>:    e8 ff a1 fb     std     r29,-24(r1)
>    0xc000000000520e2c <+28>:    f8 ff e1 fb     std     r31,-8(r1)
>    0xc000000000520e30 <+32>:    91 ff 21 f8     stdu    r1,-112(r1)
>    0xc000000000520e34 <+36>:    78 1b 69 7c     mr      r9,r3
>    0xc000000000520e38 <+40>:    78 23 9e 7c     mr      r30,r4
>    0xc000000000520e3c <+44>:    08 00 82 41     beq     0xc000000000520e44 <crypto_remove_spawns+52>
>    0xc000000000520e40 <+48>:    78 5b 69 7d     mr      r9,r11
>    0xc000000000520e44 <+52>:    40 00 e1 3b     addi    r31,r1,64
>    0xc000000000520e48 <+56>:    30 00 c1 38     addi    r6,r1,48
>  # 0xc000000000520e4c <+60>:    10 00 43 e9     ld      r10,16(r3)
>    0xc000000000520e50 <+64>:    20 00 a9 83     lwz     r29,32(r9)
>    0xc000000000520e54 <+68>:    20 00 a1 38     addi    r5,r1,32
>    0xc000000000520e58 <+72>:    40 00 e1 fb     std     r31,64(r1)
>    0xc000000000520e5c <+76>:    48 00 e1 fb     std     r31,72(r1)
>    0xc000000000520e60 <+80>:    30 00 c1 f8     std     r6,48(r1)
>    0xc000000000520e64 <+84>:    38 00 c1 f8     std     r6,56(r1)
>    0xc000000000520e68 <+88>:    20 00 a1 f8     std     r5,32(r1)
>    0xc000000000520e6c <+92>:    28 00 a1 f8     std     r5,40(r1)
>    0xc000000000520e70 <+96>:    10 00 03 38     addi    r0,r3,16
>  & 0xc000000000520e74 <+100>:   40 50 a0 7f     cmpld   cr7,r0,r10
>    0xc000000000520e78 <+104>:   78 53 47 7d     mr      r7,r10
>  * 0xc000000000520e7c <+108>:   00 00 0a e9     ld      r8,0(r10)
>    0xc000000000520e80 <+112>:   64 00 9e 41     beq     cr7,0xc000000000520ee4 <crypto_remove_spawns+212>
> 
>  #) This looks like alg->cra_users.next is loaded to r10
>  &) This looks like r10 is compared with &alg->cra_users calculated on the line
>     above to terminate the loop
>  *) This looks like *alg->cra_users.next loaded into r8 which causes the null
>     pointer dereference
> 
> So the fixup needs to be applied to the first dereference of
> alg->cra_users.next as well to prevent crash.
> 
> Fixes: 9a00674213a3 ("crypto: algapi - fix NULL dereference in crypto_remove_spawns()")
> 
> Reported-by: chetjain@xxxxxxxxxx
> Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx>
> ---
> I cannot really test if this fix is effective because the crash is some
> heisenbug that heavily depends on timing. When the bug is not triggered it does
> not really mean anything. It is also qestionable if we should be getting these
> algs with uninitialized spawns.
> 
>  crypto/algapi.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/crypto/algapi.c b/crypto/algapi.c
> index 313a7682cef1..82125b82ffba 100644
> --- a/crypto/algapi.c
> +++ b/crypto/algapi.c
> @@ -151,6 +151,18 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>  	LIST_HEAD(top);
>  
>  	spawns = &alg->cra_users;
> +
> +	/*
> +	 * We may encounter an unregistered instance here, since an instance's
> +	 * spawns are set up prior to the instance being registered.
> +	 * An unregistered instance will have NULL ->cra_users.next, since
> +	 * ->cra_users isn't properly initialized until registration.  But an
> +	 * unregistered instance cannot have any users, so treat it the same as
> +	 * ->cra_users being empty.
> +	 */
> +	if (spawns->next == NULL)
> +		return;
> +
>  	list_for_each_entry_safe(spawn, n, spawns, list) {
>  		if ((spawn->alg->cra_flags ^ new_type) & spawn->mask)
>  			continue;
> @@ -177,15 +189,7 @@ void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
>  			spawn->alg = NULL;
>  			spawns = &inst->alg.cra_users;
>  
> -			/*
> -			 * We may encounter an unregistered instance here, since
> -			 * an instance's spawns are set up prior to the instance
> -			 * being registered.  An unregistered instance will have
> -			 * NULL ->cra_users.next, since ->cra_users isn't
> -			 * properly initialized until registration.  But an
> -			 * unregistered instance cannot have any users, so treat
> -			 * it the same as ->cra_users being empty.
> -			 */
> +			/* Guard against unregistered instance */
>  			if (spawns->next == NULL)
>  				break;
>  		}
> -- 
> 2.21.0
> 

The stack trace shows that crypto_remove_spawns() is being called from
crypto_unregister_instance().  Therefore, the instance should already be
registered and have initialized cra_users.  Now, I don't claim to understand the
spawn lists stuff that well, so I could have missed something; but if there *is*
a bug, I'd like to see a proper explanation.

Did you check whether this is actually reproducible on mainline, and not just
the SUSE v4.12 based kernel?

- Eric