From: Hans-Jörg Höxer <hshoexer@genua.de>
Subject: Re: psp(4): Avoid sleep/wakeup race by raising SPL
To: <tech@openbsd.org>
Date: Wed, 6 Nov 2024 15:31:54 +0100

Hi,

got some more feedback, thanks!  Updated diff below.

Take care,
HJ.
----------------------------------------------------------------
commit a2d38daeedc1921e1d1a5befd13d3438f7d277c8
Author: Hans-Joerg Hoexer <hshoexer@genua.de>
Date:   Mon Nov 4 17:09:24 2024 +0100

    psp(4): Avoid sleep/wakeup race; make interrupt MP safe
    
    And therefore pspioctl() can release the kernel lock.

diff --git a/sys/dev/ic/psp.c b/sys/dev/ic/psp.c
index 72978ca5d4e..8814a13e7a0 100644
--- a/sys/dev/ic/psp.c
+++ b/sys/dev/ic/psp.c
@@ -20,12 +20,13 @@
 #include <sys/systm.h>
 #include <sys/device.h>
 #include <sys/malloc.h>
+#include <sys/mutex.h>
 #include <sys/pledge.h>
+#include <sys/proc.h>
 #include <sys/rwlock.h>
 
 #include <machine/bus.h>
 
-#include <sys/proc.h>
 #include <uvm/uvm.h>
 #include <crypto/xform.h>
 
@@ -56,6 +57,7 @@ struct psp_softc {
 	caddr_t			sc_tmr_kva;
 
 	struct rwlock		sc_lock;
+	struct mutex		psp_lock;
 
 	uint32_t		sc_flags;
 #define PSPF_INITIALIZED	0x1
@@ -90,8 +92,10 @@ psp_sev_intr(void *arg)
 	struct psp_softc *sc = (struct psp_softc *)csc->sc_psp;
 	uint32_t status;
 
+	mtx_enter(&sc->psp_lock);
 	status = bus_space_read_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_intsts);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_intsts, status);
+	mtx_leave(&sc->psp_lock);
 
 	if (!(status & PSP_CMDRESP_COMPLETE))
 		return (0);
@@ -137,6 +141,7 @@ psp_attach(struct device *parent, struct device *self, void *aux)
 		printf(" vers %d,", arg->version);
 
 	rw_init(&sc->sc_lock, "psp_lock");
+	mtx_init(&sc->psp_lock, IPL_BIO);
 
 	/* create and map SEV command buffer */
 	sc->sc_cmd_size = size = PAGE_SIZE;
@@ -209,7 +214,8 @@ ccp_wait(struct psp_softc *sc, uint32_t *status, int poll)
 		return (1);
 	}
 
-	if (tsleep_nsec(sc, PWAIT, "psp", SEC_TO_NSEC(2)) == EWOULDBLOCK)
+	if (msleep_nsec(sc, &sc->psp_lock, PWAIT, "psp", SEC_TO_NSEC(2))
+	    == EWOULDBLOCK)
 		return (1);
 
 	cmdword = bus_space_read_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_cmdresp);
@@ -223,6 +229,7 @@ static int
 ccp_docmd(struct psp_softc *sc, int cmd, uint64_t paddr)
 {
 	uint32_t	plo, phi, cmdword, status;
+	int		ret;
 
 	plo = ((paddr >> 0) & 0xffffffff);
 	phi = ((paddr >> 32) & 0xffffffff);
@@ -230,11 +237,14 @@ ccp_docmd(struct psp_softc *sc, int cmd, uint64_t paddr)
 	if (!cold)
 		cmdword |= PSP_CMDRESP_IOC;
 
+	mtx_enter(&sc->psp_lock);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_addrlo, plo);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_addrhi, phi);
 	bus_space_write_4(sc->sc_iot, sc->sc_ioh, sc->sc_reg_cmdresp, cmdword);
 
-	if (ccp_wait(sc, &status, cold))
+	ret = ccp_wait(sc, &status, cold);
+	mtx_leave(&sc->psp_lock);
+	if (ret)
 		return (1);
 
 	/* Did PSP sent a response code? */
@@ -783,6 +793,8 @@ pspioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 	if (sc == NULL)
 		return (ENXIO);
 
+	KERNEL_UNLOCK();
+
 	rw_enter_write(&sc->sc_lock);
 
 	switch (cmd) {
@@ -840,6 +852,8 @@ pspioctl(dev_t dev, u_long cmd, caddr_t data, int flag, struct proc *p)
 
 	rw_exit_write(&sc->sc_lock);
 
+	KERNEL_LOCK();
+
 	return (ret);
 }
 
diff --git a/sys/dev/pci/psp_pci.c b/sys/dev/pci/psp_pci.c
index e1356ceeb8a..09864796611 100644
--- a/sys/dev/pci/psp_pci.c
+++ b/sys/dev/pci/psp_pci.c
@@ -81,8 +81,8 @@ psp_pci_intr_map(struct ccp_softc *sc, struct pci_attach_args *pa)
 	}
 
 	intrstr = pci_intr_string(pa->pa_pc, ih);
-	sc->sc_irqh = pci_intr_establish(pa->pa_pc, ih, IPL_BIO, psp_sev_intr,
-	    sc, sc->sc_dev.dv_xname);
+	sc->sc_irqh = pci_intr_establish(pa->pa_pc, ih, IPL_BIO | IPL_MPSAFE,
+	    psp_sev_intr, sc, sc->sc_dev.dv_xname);
 	if (sc->sc_irqh != NULL)
 		printf(": %s", intrstr);
 }