From: Marcus Glocker <marcus@nazgul.ch>
Subject: ihidev ibuf malloc sequence fix
To: tech@openbsd.org
Date: Thu, 2 Jan 2025 22:03:12 +0100

kirill@ did discover a sequencing issue in ihidev, where we first
establish the interrupts, which require the sc->sc_ibuf buffer, and
afterwards we only allocate that buffer.  In most of the cases this
didn't cause an issue apparently, but if a device shots an interrupt
before we allocate the buffer, the ihidev interrupt handler will try
to write in to an un-allocated pointer.

The NetBSD driver, which initially was an import of our driver, did fix
this sequencing right at the import:

https://cvsweb.netbsd.org/bsdweb.cgi/~checkout~/src/sys/dev/i2c/ihidev.c?rev=1.1;content-type=text%2Fx-csrc

	sc->sc_ibuf = kmem_zalloc(sc->sc_isize, KM_NOSLEEP);
#if NACPICA > 0
	{
		char buf[100];

		sc->sc_ih = acpi_intr_establish(self, sc->sc_phandle, ihidev_intr, sc);
		if (sc->sc_ih == NULL)
			aprint_error_dev(self, "can't establish interrupt\n");
		aprint_normal_dev(self, "interrupting at %s\n",
		    acpi_intr_string(sc->sc_ih, buf, sizeof(buf)));
	}
#endif

The following patch re-shuffles the ordering so that we first allocate
the buffer, and then establish the interrupt handler.

It shouldn't change any functionality, nor the dmesg output.

Some initial testing was going well, but I would like to have a bit a
wider regression testing.  If you have an ihidev device, I would be
glad if you can give it a spin and report back.

Feedback, comments, and eventually OKs are welcome!


Index: sys/dev/i2c/ihidev.c
===================================================================
RCS file: /cvs/src/sys/dev/i2c/ihidev.c,v
diff -u -p -u -p -r1.33 ihidev.c
--- sys/dev/i2c/ihidev.c	18 Oct 2024 12:53:49 -0000	1.33
+++ sys/dev/i2c/ihidev.c	2 Jan 2025 13:41:20 -0000
@@ -118,32 +118,10 @@ ihidev_attach(struct device *parent, str
 		return;
 	}
 
-	if (ia->ia_intr) {
-		printf(" %s", iic_intr_string(sc->sc_tag, ia->ia_intr));
-
-		sc->sc_ih = iic_intr_establish(sc->sc_tag, ia->ia_intr,
-		    IPL_TTY, ihidev_intr, sc, sc->sc_dev.dv_xname);
-		if (sc->sc_ih == NULL)
-			printf(", can't establish interrupt");
-	}
-
-	if (ia->ia_poll || !sc->sc_ih) {
-		printf(" (polling)");
-		sc->sc_poll = 1;
-		sc->sc_fastpoll = 1;
-	}
-
-	printf(", vendor 0x%x product 0x%x, %s\n",
-	    letoh16(sc->hid_desc.wVendorID), letoh16(sc->hid_desc.wProductID),
-	    (char *)ia->ia_cookie);
-
 	sc->sc_nrepid = ihidev_maxrepid(sc->sc_report, sc->sc_reportlen);
 	if (sc->sc_nrepid < 0)
 		return;
 
-	printf("%s: %d report id%s\n", sc->sc_dev.dv_xname, sc->sc_nrepid,
-	    sc->sc_nrepid > 1 ? "s" : "");
-
 	sc->sc_nrepid++;
 	sc->sc_subdevs = mallocarray(sc->sc_nrepid, sizeof(struct ihidev *),
 	    M_DEVBUF, M_WAITOK | M_ZERO);
@@ -161,6 +139,29 @@ ihidev_attach(struct device *parent, str
 			    repid, repsz));
 	}
 	sc->sc_ibuf = malloc(sc->sc_isize, M_DEVBUF, M_WAITOK | M_ZERO);
+
+	if (ia->ia_intr) {
+		printf(" %s", iic_intr_string(sc->sc_tag, ia->ia_intr));
+
+		sc->sc_ih = iic_intr_establish(sc->sc_tag, ia->ia_intr,
+		    IPL_TTY, ihidev_intr, sc, sc->sc_dev.dv_xname);
+		if (sc->sc_ih == NULL)
+			printf("%s: can't establish interrupt\n",
+			    sc->sc_dev.dv_xname);
+	}
+
+	if (ia->ia_poll || !sc->sc_ih) {
+		printf(" (polling)");
+		sc->sc_poll = 1;
+		sc->sc_fastpoll = 1;
+	}
+
+	printf(", vendor 0x%x product 0x%x, %s\n",
+	    letoh16(sc->hid_desc.wVendorID), letoh16(sc->hid_desc.wProductID),
+	    (char *)ia->ia_cookie);
+
+	printf("%s: %d report id%s\n", sc->sc_dev.dv_xname, (sc->sc_nrepid - 1),
+	    (sc->sc_nrepid - 1) > 1 ? "s" : "");
 
 	iha.iaa = ia;
 	iha.parent = sc;