From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: sys/uvideo: avoid using queue.h
To: Marcus Glocker <marcus@nazgul.ch>
Cc: "Kirill A. Korinsky" <kirill@korins.ky>, tech@openbsd.org
Date: Thu, 20 Mar 2025 13:07:03 +0100

On 19/03/25(Wed) 22:47, Marcus Glocker wrote:
> [...] 
> Index: sys/dev/video.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/video.c,v
> diff -u -p -u -p -r1.59 video.c
> --- sys/dev/video.c	16 Dec 2024 21:22:51 -0000	1.59
> +++ sys/dev/video.c	19 Mar 2025 21:19:42 -0000
> @@ -256,10 +256,15 @@ videoioctl(dev_t dev, u_long cmd, caddr_
>  
>  	KERNEL_ASSERT_LOCKED();
>  
> -	if (unit >= video_cd.cd_ndevs ||
> -	    (sc = video_cd.cd_devs[unit]) == NULL || sc->hw_if == NULL)
> +	sc = (struct video_softc *)device_lookup(&video_cd, unit);
> +	if (sc == NULL)
>  		return (ENXIO);
>  
> +	if (sc->hw_if == NULL) {
> +		error = ENXIO;
> +		goto done;
> +	}
> +
>  	DPRINTF(3, "video_ioctl(%zu, '%c', %zu)\n",
>  	    IOCPARM_LEN(cmd), (int) IOCGROUP(cmd), cmd & 0xff);
>  
> @@ -279,10 +284,10 @@ videoioctl(dev_t dev, u_long cmd, caddr_
>  		error = (ENOTTY);
>  	}
>  	if (error != ENOTTY)
> -		return (error);
> +		goto done;
>  
>  	if ((error = video_claim(sc, p->p_p)))
> -		return (error);
> +		goto done;
>  
>  	/*
>  	 * The following IOCTLs can only be called by the device owner.
> @@ -406,6 +411,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
>  		error = (ENOTTY);
>  	}
>  
> +done:
> +	device_unref(&sc->dev);
>  	return (error);
>  }

I believe this should be a different commit.  What's happening here is
that a thread might sleep inside videoioctl() and another thread might
free the memory at that moment.

The same applies to videoread().

videoclose() is also racy because video_stop() can sleep.