From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: ihidev ibuf malloc sequence fix
To: Marcus Glocker <marcus@nazgul.ch>
Cc: tech@openbsd.org
Date: Thu, 02 Jan 2025 23:14:20 +0100

On Thu, 02 Jan 2025 22:03:12 +0100,
Marcus Glocker <marcus@nazgul.ch> wrote:
> 
> 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!
>

I had tested it on two laptops:
 - Huawe Matebook X 2020 no change, everything works as before
 - Honor Magicbook 14 Art Snapdragon it boots!

FWIW OK kirill@

> 
> 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;
> 

-- 
wbr, Kirill