From: Jack Burton <jack@saosce.com.au>
Subject: [patch 2 of 3] iscsid(8): avoid double free & race during cleanup
To: tech@openbsd.org
Date: Tue, 31 Dec 2024 18:33:08 +1030

Avoid double free in two places.  Both occur only during cleanup after
receiving SIGTERM (which explains why they've gone unnoticed for so
long: I only noticed them while testing my next patch, but I've
confirmed that they do occur even with a pristine current tree).  To
trigger the first bug, either: (a) at least one connection needs to
have failed before receiving SIGTERM; or (b) at least one connection in
full-feature phase needs to be to a target *other than* the netbsd
iscsi target from ports (no idea why that one seems to be immune).

pdu_readbuf_free() had a fairly obvious double free.  It's called
(at least) twice during session shutdown.  The fix was also obvious.

Fixing that one unmasked a latent race condition during shutdown.
Depending on whether session reestablishment attempts result in any
new connections entering the login phase before ISCSI_EXIT_WAIT
expires, EITHER conn_free() would get called multiple times on the
same connection OR there'd be an outright access after free.

To avoid adding extra states to the fsm, simply check exit_rounds
(which gets set in shutdown_cb) before freeing connections in
sess_do_free(): if it's set then we're about to exit anyway, so it
doesn't matter that some connections may get reallocated without
freeing the old ones.

With this patch applied, iscsid(8) no longer crashes during cleanup
after receiving SIGTERM, regardless what targets it's connected to or
whether those connections are running or failed at the time.

This patch is independent of the other two in the set.

cvs server: Diffing .
Index: iscsid.h
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/iscsid.h,v
diff -u -p -r1.19 iscsid.h
--- iscsid.h	21 May 2024 05:00:48 -0000	1.19
+++ iscsid.h	29 Dec 2024 08:31:03 -0000
@@ -312,6 +312,7 @@ struct vscsi_stats {
 	u_int32_t	cnt_detach;
 };
 
+extern int exit_rounds;
 extern const struct session_params iscsi_sess_defaults;
 extern const struct connection_params iscsi_conn_defaults;
 extern struct session_params initiator_sess_defaults;
Index: pdu.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/pdu.c,v
diff -u -p -r1.13 pdu.c
--- pdu.c	12 Apr 2021 10:03:33 -0000	1.13
+++ pdu.c	29 Dec 2024 08:31:04 -0000
@@ -456,4 +456,5 @@ void
 pdu_readbuf_free(struct pdu_readbuf *rb)
 {
 	free(rb->buf);
+	rb->buf = NULL;
 }
Index: session.c
===================================================================
RCS file: /cvs/src/usr.sbin/iscsid/session.c,v
diff -u -p -r1.9 session.c
--- session.c	8 Mar 2023 04:43:13 -0000	1.9
+++ session.c	29 Dec 2024 08:31:04 -0000
@@ -410,7 +410,7 @@ sess_do_free(struct session *s, struct s
 {
 	struct connection *c;
 
-	while ((c = TAILQ_FIRST(&s->connections)) != NULL)
+	while (exit_rounds == 0 && (c = TAILQ_FIRST(&s->connections)) != NULL)
 		conn_free(c);
 
 	return SESS_FREE;