Change pci_intr_handle_t into a struct and replace duplicated code that

implements mapping of MSI and MSI-X interrupts with new generic functions.
Fixes a use-after-free in sone PCI device drivers that call pci_intr_string(9)
after pci_intr_establish(9).

ok deraadt@

4 files changed, 102 insertions, 183 deletions

diff --git a/sys/arch/arm64/dev/acpipci.c b/sys/arch/arm64/dev/acpipci.c
index 22cc3200f5c..e5b05e7e1d0 100644
--- a/sys/arch/arm64/dev/acpipci.c
+++ b/sys/arch/arm64/dev/acpipci.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: acpipci.c,v 1.9 2019/05/31 08:02:04 kettenis Exp $	*/
+/*	$OpenBSD: acpipci.c,v 1.10 2019/06/02 18:40:58 kettenis Exp $	*/
 /*
  * Copyright (c) 2018 Mark Kettenis
  *
@@ -107,9 +107,6 @@ pcireg_t acpipci_conf_read(void *, pcitag_t, int);
 void	acpipci_conf_write(void *, pcitag_t, int, pcireg_t);
 
 int	acpipci_intr_map(struct pci_attach_args *, pci_intr_handle_t *);
-int	acpipci_intr_map_msi(struct pci_attach_args *, pci_intr_handle_t *);
-int	acpipci_intr_map_msix(struct pci_attach_args *, int,
-	    pci_intr_handle_t *);
 const char *acpipci_intr_string(void *, pci_intr_handle_t);
 void	*acpipci_intr_establish(void *, pci_intr_handle_t, int,
 	    int (*)(void *), void *, char *);
@@ -183,8 +180,8 @@ acpipci_attach(struct device *parent, struct device *self, void *aux)
 
 	sc->sc_pc->pc_intr_v = sc;
 	sc->sc_pc->pc_intr_map = acpipci_intr_map;
-	sc->sc_pc->pc_intr_map_msi = acpipci_intr_map_msi;
-	sc->sc_pc->pc_intr_map_msix = acpipci_intr_map_msix;
+	sc->sc_pc->pc_intr_map_msi = _pci_intr_map_msi;
+	sc->sc_pc->pc_intr_map_msix = _pci_intr_map_msix;
 	sc->sc_pc->pc_intr_string = acpipci_intr_string;
 	sc->sc_pc->pc_intr_establish = acpipci_intr_establish;
 	sc->sc_pc->pc_intr_disestablish = acpipci_intr_disestablish;
@@ -334,36 +331,20 @@ acpipci_conf_write(void *v, pcitag_t tag, int reg, pcireg_t data)
 	bus_space_write_4(am->am_iot, am->am_ioh, tag | reg, data);
 }
 
-#define PCI_INTX		0
-#define PCI_MSI			1
-#define PCI_MSIX		2
-
-struct acpipci_intr_handle {
-	pci_chipset_tag_t	ih_pc;
-	pcitag_t		ih_tag;
-	int			ih_intrpin;
-	int			ih_type;
-};
-
 int
 acpipci_intr_swizzle(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 {
-	struct acpipci_intr_handle *ih;
 	int dev, swizpin;
 
-	if (pa->pa_bridgetag == NULL)
+	if (pa->pa_bridgeih == NULL)
 		return -1;
 
 	pci_decompose_tag(pa->pa_pc, pa->pa_tag, NULL, &dev, NULL);
 	swizpin = PPB_INTERRUPT_SWIZZLE(pa->pa_rawintrpin, dev);
-	if ((void *)pa->pa_bridgeih[swizpin - 1] == NULL)
+	if (pa->pa_bridgeih[swizpin - 1].ih_type == PCI_NONE)
 		return -1;
 
-	ih = malloc(sizeof(struct acpipci_intr_handle), M_DEVBUF, M_WAITOK);
-	memcpy(ih, (void *)pa->pa_bridgeih[swizpin - 1],
-	    sizeof(struct acpipci_intr_handle));
-	*ihp = (pci_intr_handle_t)ih;
-
+	*ihp = pa->pa_bridgeih[swizpin - 1];
 	return 0;
 }
 
@@ -374,7 +355,6 @@ acpipci_intr_map(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 	struct aml_node *node = sc->sc_node;
 	struct aml_value res;
 	uint64_t addr, pin, source, index;
-	struct acpipci_intr_handle *ih;
 	int i;
 
 	/*
@@ -417,13 +397,10 @@ acpipci_intr_map(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 		    pin != pa->pa_intrpin - 1 || source != 0)
 			continue;
 		
-		ih = malloc(sizeof(struct acpipci_intr_handle),
-		    M_DEVBUF, M_WAITOK);
-		ih->ih_pc = pa->pa_pc;
-		ih->ih_tag = pa->pa_tag;
-		ih->ih_intrpin = index;
-		ih->ih_type = PCI_INTX;
-		*ihp = (pci_intr_handle_t)ih;
+		ihp->ih_pc = pa->pa_pc;
+		ihp->ih_tag = pa->pa_tag;
+		ihp->ih_intrpin = index;
+		ihp->ih_type = PCI_INTX;
 
 		return 0;
 	}
@@ -431,75 +408,27 @@ acpipci_intr_map(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 	return -1;
 }
 
-int
-acpipci_intr_map_msi(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
-{
-	pci_chipset_tag_t pc = pa->pa_pc;
-	pcitag_t tag = pa->pa_tag;
-	struct acpipci_intr_handle *ih;
-
-	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-	    pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
-		return -1;
-
-	ih = malloc(sizeof(struct acpipci_intr_handle), M_DEVBUF, M_WAITOK);
-	ih->ih_pc = pa->pa_pc;
-	ih->ih_tag = pa->pa_tag;
-	ih->ih_type = PCI_MSI;
-	*ihp = (pci_intr_handle_t)ih;
-
-	return 0;
-}
-
-int
-acpipci_intr_map_msix(struct pci_attach_args *pa, int vec,
-    pci_intr_handle_t *ihp)
-{
-	pci_chipset_tag_t pc = pa->pa_pc;
-	pcitag_t tag = pa->pa_tag;
-	struct acpipci_intr_handle *ih;
-	pcireg_t reg;
-
-	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-	    pci_get_capability(pc, tag, PCI_CAP_MSIX, NULL, &reg) == 0)
-		return -1;
-
-	if (vec > PCI_MSIX_MC_TBLSZ(reg))
-		return -1;
-
-	ih = malloc(sizeof(struct acpipci_intr_handle), M_DEVBUF, M_WAITOK);
-	ih->ih_pc = pa->pa_pc;
-	ih->ih_tag = pa->pa_tag;
-	ih->ih_intrpin = vec;
-	ih->ih_type = PCI_MSIX;
-	*ihp = (pci_intr_handle_t)ih;
-
-	return 0;
-}
-
 const char *
-acpipci_intr_string(void *v, pci_intr_handle_t ihp)
+acpipci_intr_string(void *v, pci_intr_handle_t ih)
 {
-	struct acpipci_intr_handle *ih = (struct acpipci_intr_handle *)ihp;
 	static char irqstr[32];
 
-	switch (ih->ih_type) {
+	switch (ih.ih_type) {
 	case PCI_MSI:
 		return "msi";
 	case PCI_MSIX:
 		return "msix";
 	}
 
-	snprintf(irqstr, sizeof(irqstr), "irq %d", ih->ih_intrpin);
+	snprintf(irqstr, sizeof(irqstr), "irq %d", ih.ih_intrpin);
 	return irqstr;
 }
 
 void *
-acpipci_intr_establish(void *v, pci_intr_handle_t ihp, int level,
+acpipci_intr_establish(void *v, pci_intr_handle_t ih, int level,
     int (*func)(void *), void *arg, char *name)
 {
 	struct acpipci_softc *sc = v;
-	struct acpipci_intr_handle *ih = (struct acpipci_intr_handle *)ihp;
 	struct interrupt_controller *ic;
 	void *cookie;
 
@@ -511,11 +440,13 @@ acpipci_intr_establish(void *v, pci_intr_handle_t ihp, int level,
 	if (ic == NULL)
 		return NULL;
 
-	if (ih->ih_type != PCI_INTX) {
+	KASSERT(ih.ih_type != PCI_NONE);
+
+	if (ih.ih_type != PCI_INTX) {
 		uint64_t addr, data;
 
 		/* Map Requester ID through IORT to get sideband data. */
-		data = acpipci_iort_map_msi(ih->ih_pc, ih->ih_tag);
+		data = acpipci_iort_map_msi(ih.ih_pc, ih.ih_tag);
 		cookie = ic->ic_establish_msi(ic->ic_cookie, &addr,
 		    &data, level, func, arg, name);
 		if (cookie == NULL)
@@ -523,17 +454,16 @@ acpipci_intr_establish(void *v, pci_intr_handle_t ihp, int level,
 
 		/* TODO: translate address to the PCI device's view */
 
-		if (ih->ih_type == PCI_MSIX) {
-			pci_msix_enable(ih->ih_pc, ih->ih_tag,
-			    &sc->sc_bus_memt, ih->ih_intrpin, addr, data);
+		if (ih.ih_type == PCI_MSIX) {
+			pci_msix_enable(ih.ih_pc, ih.ih_tag,
+			    &sc->sc_bus_memt, ih.ih_intrpin, addr, data);
 		} else
-			pci_msi_enable(ih->ih_pc, ih->ih_tag, addr, data);
+			pci_msi_enable(ih.ih_pc, ih.ih_tag, addr, data);
 	} else {
-		cookie = acpi_intr_establish(ih->ih_intrpin, 0, level,
+		cookie = acpi_intr_establish(ih.ih_intrpin, 0, level,
 		    func, arg, name);
 	}
 
-	free(ih, M_DEVBUF, sizeof(struct acpipci_intr_handle));
 	return cookie;
 }
 
diff --git a/sys/arch/arm64/dev/pci_machdep.c b/sys/arch/arm64/dev/pci_machdep.c
index 476de2e10f2..2bdea330300 100644
--- a/sys/arch/arm64/dev/pci_machdep.c
+++ b/sys/arch/arm64/dev/pci_machdep.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pci_machdep.c,v 1.1 2019/05/31 08:02:04 kettenis Exp $	*/
+/*	$OpenBSD: pci_machdep.c,v 1.2 2019/06/02 18:40:58 kettenis Exp $	*/
 
 /*
  * Copyright (c) 2019 Mark Kettenis <kettenis@openbsd.org>
@@ -83,3 +83,44 @@ pci_msix_enable(pci_chipset_tag_t pc, pcitag_t tag, bus_space_tag_t memt,
 
 	pci_conf_write(pc, tag, off, reg | PCI_MSIX_MC_MSIXE);
 }
+
+int
+_pci_intr_map_msi(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
+{
+	pci_chipset_tag_t pc = pa->pa_pc;
+	pcitag_t tag = pa->pa_tag;
+
+	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
+	    pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
+		return -1;
+
+	ihp->ih_pc = pa->pa_pc;
+	ihp->ih_tag = pa->pa_tag;
+	ihp->ih_type = PCI_MSI;
+
+	return 0;
+}
+
+int
+_pci_intr_map_msix(struct pci_attach_args *pa, int vec,
+    pci_intr_handle_t *ihp)
+{
+	pci_chipset_tag_t pc = pa->pa_pc;
+	pcitag_t tag = pa->pa_tag;
+	pcireg_t reg;
+
+	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
+	    pci_get_capability(pc, tag, PCI_CAP_MSIX, NULL, &reg) == 0)
+		return -1;
+
+	if (vec > PCI_MSIX_MC_TBLSZ(reg))
+		return -1;
+
+	ihp->ih_pc = pa->pa_pc;
+	ihp->ih_tag = pa->pa_tag;
+	ihp->ih_intrpin = vec;
+	ihp->ih_type = PCI_MSIX;
+
+	return 0;
+}
+
diff --git a/sys/arch/arm64/dev/pciecam.c b/sys/arch/arm64/dev/pciecam.c
index f3e050c1e07..2a9fde7415d 100644
--- a/sys/arch/arm64/dev/pciecam.c
+++ b/sys/arch/arm64/dev/pciecam.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: pciecam.c,v 1.8 2019/05/31 10:36:32 kettenis Exp $ */
+/* $OpenBSD: pciecam.c,v 1.9 2019/06/02 18:40:58 kettenis Exp $ */
 /*
  * Copyright (c) 2013,2017 Patrick Wildt <patrick@blueri.se>
  *
@@ -101,8 +101,6 @@ int pciecam_conf_size(void *, pcitag_t);
 pcireg_t pciecam_conf_read(void *, pcitag_t, int);
 void pciecam_conf_write(void *, pcitag_t, int, pcireg_t);
 int pciecam_intr_map(struct pci_attach_args *, pci_intr_handle_t *);
-int pciecam_intr_map_msi(struct pci_attach_args *, pci_intr_handle_t *);
-int pciecam_intr_map_msix(struct pci_attach_args *, int, pci_intr_handle_t *);
 const char *pciecam_intr_string(void *, pci_intr_handle_t);
 void *pciecam_intr_establish(void *, pci_intr_handle_t, int, int (*func)(void *), void *, char *);
 void pciecam_intr_disestablish(void *, void *);
@@ -230,8 +228,8 @@ pciecam_attach(struct device *parent, struct device *self, void *aux)
 
 	sc->sc_pc.pc_intr_v = sc;
 	sc->sc_pc.pc_intr_map = pciecam_intr_map;
-	sc->sc_pc.pc_intr_map_msi = pciecam_intr_map_msi;
-	sc->sc_pc.pc_intr_map_msix = pciecam_intr_map_msix;
+	sc->sc_pc.pc_intr_map_msi = _pci_intr_map_msi;
+	sc->sc_pc.pc_intr_map_msix = _pci_intr_map_msix;
 	sc->sc_pc.pc_intr_string = pciecam_intr_string;
 	sc->sc_pc.pc_intr_establish = pciecam_intr_establish;
 	sc->sc_pc.pc_intr_disestablish = pciecam_intr_disestablish;
@@ -318,85 +316,21 @@ pciecam_conf_write(void *v, pcitag_t tag, int reg, pcireg_t data)
 	HWRITE4(sc, PCIE_ADDR_OFFSET(bus, dev, fn, reg & ~0x3), data);
 }
 
-#define PCI_INTX		0
-#define PCI_MSI			1
-#define PCI_MSIX		2
-
-struct pciecam_intr_handle {
-	pci_chipset_tag_t	ih_pc;
-	pcitag_t		ih_tag;
-	int			ih_intrpin;
-	int			ih_type;
-};
-
-int
-pciecam_intr_map(struct pci_attach_args *pa,
-    pci_intr_handle_t *ihp)
-{
-	struct pciecam_intr_handle *ih;
-
-	ih = malloc(sizeof(struct pciecam_intr_handle), M_DEVBUF, M_WAITOK);
-	ih->ih_pc = pa->pa_pc;
-	ih->ih_tag = pa->pa_intrtag;
-	ih->ih_intrpin = pa->pa_intrpin;
-	ih->ih_type = PCI_INTX;
-	*ihp = (pci_intr_handle_t)ih;
-	return 0;
-}
-
-int
-pciecam_intr_map_msi(struct pci_attach_args *pa,
-    pci_intr_handle_t *ihp)
-{
-	pci_chipset_tag_t pc = pa->pa_pc;
-	pcitag_t tag = pa->pa_tag;
-	struct pciecam_intr_handle *ih;
-
-	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-	    pci_get_capability(pc, tag, PCI_CAP_MSI, NULL, NULL) == 0)
-		return 1;
-
-	ih = malloc(sizeof(struct pciecam_intr_handle), M_DEVBUF, M_WAITOK);
-	ih->ih_pc = pa->pa_pc;
-	ih->ih_tag = pa->pa_tag;
-	ih->ih_type = PCI_MSI;
-	*ihp = (pci_intr_handle_t)ih;
-
-	return 0;
-}
-
 int
-pciecam_intr_map_msix(struct pci_attach_args *pa,
-    int vec, pci_intr_handle_t *ihp)
+pciecam_intr_map(struct pci_attach_args *pa, pci_intr_handle_t *ihp)
 {
-	pci_chipset_tag_t pc = pa->pa_pc;
-	pcitag_t tag = pa->pa_tag;
-	struct pciecam_intr_handle *ih;
-	pcireg_t reg;
-
-	if ((pa->pa_flags & PCI_FLAGS_MSI_ENABLED) == 0 ||
-	    pci_get_capability(pc, tag, PCI_CAP_MSIX, NULL, &reg) == 0)
-		return -1;
-
-	if (vec > PCI_MSIX_MC_TBLSZ(reg))
-		return -1;
-
-	ih = malloc(sizeof(struct pciecam_intr_handle), M_DEVBUF, M_WAITOK);
-	ih->ih_pc = pa->pa_pc;
-	ih->ih_tag = pa->pa_tag;
-	ih->ih_intrpin = vec;
-	ih->ih_type = PCI_MSIX;
-	*ihp = (pci_intr_handle_t)ih;
+	ihp->ih_pc = pa->pa_pc;
+	ihp->ih_tag = pa->pa_intrtag;
+	ihp->ih_intrpin = pa->pa_intrpin;
+	ihp->ih_type = PCI_INTX;
 
 	return 0;
 }
 
 const char *
-pciecam_intr_string(void *sc, pci_intr_handle_t ihp)
+pciecam_intr_string(void *sc, pci_intr_handle_t ih)
 {
-	struct pciecam_intr_handle *ih = (struct pciecam_intr_handle *)ihp;
-
-	switch (ih->ih_type) {
+	switch (ih.ih_type) {
 	case PCI_MSI:
 		return "msi";
 	case PCI_MSIX:
@@ -407,18 +341,19 @@ pciecam_intr_string(void *sc, pci_intr_handle_t ihp)
 }
 
 void *
-pciecam_intr_establish(void *self, pci_intr_handle_t ihp, int level,
+pciecam_intr_establish(void *self, pci_intr_handle_t ih, int level,
     int (*func)(void *), void *arg, char *name)
 {
 	struct pciecam_softc *sc = (struct pciecam_softc *)self;
-	struct pciecam_intr_handle *ih = (struct pciecam_intr_handle *)ihp;
 	void *cookie;
 
-	if (ih->ih_type != PCI_INTX) {
+	KASSERT(ih.ih_type != PCI_NONE);
+
+	if (ih.ih_type != PCI_INTX) {
 		uint64_t addr, data;
 
 		/* Assume hardware passes Requester ID as sideband data. */
-		data = pci_requester_id(ih->ih_pc, ih->ih_tag);
+		data = pci_requester_id(ih.ih_pc, ih.ih_tag);
 		cookie = arm_intr_establish_fdt_msi(sc->sc_node, &addr,
 		    &data, level, func, arg, (void *)name);
 		if (cookie == NULL)
@@ -426,26 +361,25 @@ pciecam_intr_establish(void *self, pci_intr_handle_t ihp, int level,
 
 		/* TODO: translate address to the PCI device's view */
 
-		if (ih->ih_type == PCI_MSIX) {
-			pci_msix_enable(ih->ih_pc, ih->ih_tag,
-			    &sc->sc_bus, ih->ih_intrpin, addr, data);
+		if (ih.ih_type == PCI_MSIX) {
+			pci_msix_enable(ih.ih_pc, ih.ih_tag,
+			    &sc->sc_bus, ih.ih_intrpin, addr, data);
 		} else
-			pci_msi_enable(ih->ih_pc, ih->ih_tag, addr, data);
+			pci_msi_enable(ih.ih_pc, ih.ih_tag, addr, data);
 	} else {
 		int bus, dev, fn;
 		uint32_t reg[4];
 
-		pciecam_decompose_tag(sc, ih->ih_tag, &bus, &dev, &fn);
+		pciecam_decompose_tag(sc, ih.ih_tag, &bus, &dev, &fn);
 
 		reg[0] = bus << 16 | dev << 11 | fn << 8;
 		reg[1] = reg[2] = 0;
-		reg[3] = ih->ih_intrpin;
+		reg[3] = ih.ih_intrpin;
 
 		cookie = arm_intr_establish_fdt_imap(sc->sc_node, reg,
 		    sizeof(reg), level, func, arg, name);
 	}
 
-	free(ih, M_DEVBUF, sizeof(struct pciecam_intr_handle));
 	return cookie;
 }
 
diff --git a/sys/arch/arm64/include/pci_machdep.h b/sys/arch/arm64/include/pci_machdep.h
index 884e91889d6..5507cb98446 100644
--- a/sys/arch/arm64/include/pci_machdep.h
+++ b/sys/arch/arm64/include/pci_machdep.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pci_machdep.h,v 1.4 2019/05/31 08:02:04 kettenis Exp $ */
+/*	$OpenBSD: pci_machdep.h,v 1.5 2019/06/02 18:40:58 kettenis Exp $ */
 
 /*
  * Copyright (c) 2003-2004 Opsycon AB  (www.opsycon.se / www.opsycon.com)
@@ -28,7 +28,19 @@
 
 typedef struct arm64_pci_chipset *pci_chipset_tag_t;
 typedef u_long pcitag_t;
-typedef u_long pci_intr_handle_t;
+
+/* Supported interrupt types. */
+#define PCI_NONE		0
+#define PCI_INTX		1
+#define PCI_MSI			2
+#define PCI_MSIX		3
+
+typedef struct {
+	pci_chipset_tag_t	ih_pc;
+	pcitag_t		ih_tag;
+	int			ih_intrpin;
+	int			ih_type;
+} pci_intr_handle_t;
 
 struct pci_attach_args;
 
@@ -103,3 +115,5 @@ pci_chipset_tag_t pci_lookup_segment(int);
 void	pci_msi_enable(pci_chipset_tag_t, pcitag_t, bus_addr_t, uint32_t);
 void	pci_msix_enable(pci_chipset_tag_t, pcitag_t, bus_space_tag_t,
 	    int, bus_addr_t, uint32_t);
+int	_pci_intr_map_msi(struct pci_attach_args *, pci_intr_handle_t *);
+int	_pci_intr_map_msix(struct pci_attach_args *, int, pci_intr_handle_t *);