diff mbox series

[2/2] media: v4l2-core: explicitly clear ioctl input data

Message ID 20210318134334.2933141-2-arnd@kernel.org
State Superseded
Headers show
Series [1/2] media: v4l2-core: ignore native time32 ioctls on 64-bit | expand

Commit Message

Arnd Bergmann March 18, 2021, 1:43 p.m. UTC
From: Arnd Bergmann <arnd@arndb.de>


As seen from a recent syzbot bug report, mistakes in the compat ioctl
implementation can lead to uninitialized kernel stack data getting used
as input for driver ioctl handlers.

The reported bug is now fixed, but it's possible that other related
bugs are still present or get added in the future. As the drivers need
to check user input already, the possible impact is fairly low, but it
might still cause an information leak.

To be on the safe side, always clear the entire ioctl buffer before
calling the conversion handler functions that are meant to initialize
them.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
 drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++------------
 1 file changed, 29 insertions(+), 22 deletions(-)

-- 
2.29.2

Comments

Laurent Pinchart March 18, 2021, 3:07 p.m. UTC | #1
Hi Arnd,

Thank you for the patch.

On Thu, Mar 18, 2021 at 02:43:19PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>

> 

> As seen from a recent syzbot bug report, mistakes in the compat ioctl

> implementation can lead to uninitialized kernel stack data getting used

> as input for driver ioctl handlers.

> 

> The reported bug is now fixed, but it's possible that other related

> bugs are still present or get added in the future. As the drivers need

> to check user input already, the possible impact is fairly low, but it

> might still cause an information leak.

> 

> To be on the safe side, always clear the entire ioctl buffer before

> calling the conversion handler functions that are meant to initialize

> them.

> 

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>


Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>


> ---

>  drivers/media/v4l2-core/v4l2-ioctl.c | 51 ++++++++++++++++------------

>  1 file changed, 29 insertions(+), 22 deletions(-)

> 

> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c

> index 2b1bb68dc27f..6cec92d0972c 100644

> --- a/drivers/media/v4l2-core/v4l2-ioctl.c

> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c

> @@ -3164,12 +3164,23 @@ static int video_get_user(void __user *arg, void *parg,

>  

>  	if (cmd == real_cmd) {

>  		if (copy_from_user(parg, (void __user *)arg, n))

> -			err = -EFAULT;

> -	} else if (in_compat_syscall()) {

> -		err = v4l2_compat_get_user(arg, parg, cmd);

> -	} else {

> -		switch (cmd) {

> +			return -EFAULT;

> +

> +		/* zero out anything we don't copy from userspace */

> +		if (n < _IOC_SIZE(real_cmd))

> +			memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);

> +

> +		return 0;

> +	}

> +

> +	/* zero out whole buffer first to deal with missing emulation */

> +	memset(parg, 0, _IOC_SIZE(real_cmd));

> +

> +	if (in_compat_syscall())

> +		return v4l2_compat_get_user(arg, parg, cmd);

> +

>  #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)

> +	switch (cmd) {

>  		case VIDIOC_QUERYBUF_TIME32:

>  		case VIDIOC_QBUF_TIME32:

>  		case VIDIOC_DQBUF_TIME32:

> @@ -3182,28 +3193,24 @@ static int video_get_user(void __user *arg, void *parg,

>  

>  			*vb = (struct v4l2_buffer) {

>  				.index		= vb32.index,

> -					.type		= vb32.type,

> -					.bytesused	= vb32.bytesused,

> -					.flags		= vb32.flags,

> -					.field		= vb32.field,

> -					.timestamp.tv_sec	= vb32.timestamp.tv_sec,

> -					.timestamp.tv_usec	= vb32.timestamp.tv_usec,

> -					.timecode	= vb32.timecode,

> -					.sequence	= vb32.sequence,

> -					.memory		= vb32.memory,

> -					.m.userptr	= vb32.m.userptr,

> -					.length		= vb32.length,

> -					.request_fd	= vb32.request_fd,

> +				.type		= vb32.type,

> +				.bytesused	= vb32.bytesused,

> +				.flags		= vb32.flags,

> +				.field		= vb32.field,

> +				.timestamp.tv_sec	= vb32.timestamp.tv_sec,

> +				.timestamp.tv_usec	= vb32.timestamp.tv_usec,

> +				.timecode	= vb32.timecode,

> +				.sequence	= vb32.sequence,

> +				.memory		= vb32.memory,

> +				.m.userptr	= vb32.m.userptr,

> +				.length		= vb32.length,

> +				.request_fd	= vb32.request_fd,

>  			};

>  			break;

>  		}

> -#endif

> -		}

>  	}

> +#endif

>  

> -	/* zero out anything we don't copy from userspace */

> -	if (!err && n < _IOC_SIZE(real_cmd))

> -		memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);

>  	return err;

>  }

>  


-- 
Regards,

Laurent Pinchart
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 2b1bb68dc27f..6cec92d0972c 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -3164,12 +3164,23 @@  static int video_get_user(void __user *arg, void *parg,
 
 	if (cmd == real_cmd) {
 		if (copy_from_user(parg, (void __user *)arg, n))
-			err = -EFAULT;
-	} else if (in_compat_syscall()) {
-		err = v4l2_compat_get_user(arg, parg, cmd);
-	} else {
-		switch (cmd) {
+			return -EFAULT;
+
+		/* zero out anything we don't copy from userspace */
+		if (n < _IOC_SIZE(real_cmd))
+			memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
+
+		return 0;
+	}
+
+	/* zero out whole buffer first to deal with missing emulation */
+	memset(parg, 0, _IOC_SIZE(real_cmd));
+
+	if (in_compat_syscall())
+		return v4l2_compat_get_user(arg, parg, cmd);
+
 #if !defined(CONFIG_64BIT) && defined(CONFIG_COMPAT_32BIT_TIME)
+	switch (cmd) {
 		case VIDIOC_QUERYBUF_TIME32:
 		case VIDIOC_QBUF_TIME32:
 		case VIDIOC_DQBUF_TIME32:
@@ -3182,28 +3193,24 @@  static int video_get_user(void __user *arg, void *parg,
 
 			*vb = (struct v4l2_buffer) {
 				.index		= vb32.index,
-					.type		= vb32.type,
-					.bytesused	= vb32.bytesused,
-					.flags		= vb32.flags,
-					.field		= vb32.field,
-					.timestamp.tv_sec	= vb32.timestamp.tv_sec,
-					.timestamp.tv_usec	= vb32.timestamp.tv_usec,
-					.timecode	= vb32.timecode,
-					.sequence	= vb32.sequence,
-					.memory		= vb32.memory,
-					.m.userptr	= vb32.m.userptr,
-					.length		= vb32.length,
-					.request_fd	= vb32.request_fd,
+				.type		= vb32.type,
+				.bytesused	= vb32.bytesused,
+				.flags		= vb32.flags,
+				.field		= vb32.field,
+				.timestamp.tv_sec	= vb32.timestamp.tv_sec,
+				.timestamp.tv_usec	= vb32.timestamp.tv_usec,
+				.timecode	= vb32.timecode,
+				.sequence	= vb32.sequence,
+				.memory		= vb32.memory,
+				.m.userptr	= vb32.m.userptr,
+				.length		= vb32.length,
+				.request_fd	= vb32.request_fd,
 			};
 			break;
 		}
-#endif
-		}
 	}
+#endif
 
-	/* zero out anything we don't copy from userspace */
-	if (!err && n < _IOC_SIZE(real_cmd))
-		memset((u8 *)parg + n, 0, _IOC_SIZE(real_cmd) - n);
 	return err;
 }