kern/60281: crypto(4): bugs in reference counting and test

["campbell+netbsd%mumble.net@localhost via gnats" <gnats-admin%NetBSD.org@localhost>]

>Number:         60281
>Category:       kern
>Synopsis:       crypto(4): bugs in reference counting and test
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 19 05:50:00 +0000 2026
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The CryptoBSDev Foundabug, Inc.
>Environment:
>Description:

	Several issues found on first pass, but I suspect there's a lot
	more I didn't see on this quick skim.

	1. Makes no sense to mutex_exit _and then_ acquire a reference;
	   the object in question may have been freed already between
	   the mutex_exit and the atomic_inc_uint:

    944 	mutex_enter(&fcr->lock);
    945 	TAILQ_FOREACH_SAFE(cse, &fcr->csessions, next, cnext)
    946 		if (cse->ses == ses) {
    947 			mutex_exit(&fcr->lock);
    948 			atomic_inc_uint(&cse->refcnt);
    949 			return cse;

https://nxr.NetBSD.org/xref/src/sys/opencrypto/cryptodev.c?r=1.128#944

	2. For reference count release, atomic_dec must be surrounded
	   by release/acquire barriers, or else the object might be
	   destroyed by one thread while another thread is still using
	   it:

   1027 	if (atomic_dec_uint_nv(&cse->refcnt) > 0)
   1028 		return;
   1029 	mutex_destroy(&cse->lock);
   1030 	crypto_freesession(cse->sid);
   1031 	if (cse->key)
   1032 		free(cse->key, M_XDATA);
   1033 	if (cse->mackey)
   1034 		free(cse->mackey, M_XDATA);
   1035 	pool_put(&csepl, cse);

https://nxr.NetBSD.org/xref/src/sys/opencrypto/cryptodev.c?r=1.128#1027

	   This must instead be:

		membar_release();
		if (atomic_dec_uint_nv(&cse->refcnt) > 0)
			return;
		membar_acquire();

	   For details, see, e.g.:

	   https://mail-index.NetBSD.org/source-changes/2023/02/23/msg143446.html

	3. Lock order between cryptodev_mtx, struct csession::lock, and
	   struct fcrypt::lock is not documented.

	4. The new test is fundamentally racy and will randomly
	   spuriously fail depending on who wins the race:

		- The t_encrypt thread runs forever issuing
		  ioctl(CIOCCRYPT) operations:

     79 static void *
     80 t_encrypt(void *v)
     81 {
...
     87 	for (;;) {
...
     96 		res = ioctl(a->fd, CIOCCRYPT, &co);
...
    103 	}
    104 }

https://nxr.NetBSD.org/xref/src/tests/crypto/opencrypto/h_thread.c?r=1.1#79

		- The main thread starts two t_encrypt threads and then
		  tries to close the session a second later with
		  ioctl(CIOCFSESSION), and fails the test if the ioctl
		  fails:

    126 	pthread_create(&t, NULL, t_encrypt, &a);
    127 	b = a;
    128 	b.msg = '+';
    129 	pthread_create(&t, NULL, t_encrypt, &b);
    130 	sleep(1);
    131 	res = ioctl(a.fd, CIOCFSESSION, &cs.ses);
    132 	if (res == -1) {
    133 		warn("CIOCFSESSION");
    134 		return EXIT_FAILURE;
    135 	}

https://nxr.NetBSD.org/xref/src/tests/crypto/opencrypto/h_thread.c?r=1.1#126

		- ioctl(CIOCFSESSION) fails with EBUSY if another
		  thread is concurrently using the session:

    963 			if (atomic_load_relaxed(&cse->refcnt) != 1)
    964 				return EBUSY;

https://nxr.NetBSD.org/xref/src/sys/opencrypto/cryptodev.c?r=1.128#963

	   Result: random spurious failures like this:

	   https://www.NetBSD.org/~martin/sparc64-atf/1023_atf.html#crypto_opencrypto_t_opencrypto_thread

>How-To-Repeat:

	code inspection

>Fix:

	Yes, please!