linux-kernel - Re: [PATCH v3] usb: gadget: uvc: Fix crash when encoding data for usb request

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Yl8frWT5VYRdt5zA@pendragon.ideasonboard.com>
Date:   Tue, 19 Apr 2022 23:46:37 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Dan Vacura <w36195@...orola.com>
Cc:     linux-usb@...r.kernel.org, stable@...r.kernel.org,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Bhupesh Sharma <bhupesh.sharma@...com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] usb: gadget: uvc: Fix crash when encoding data for
 usb request

Hi Dan,

Thank you for the patch.

On Thu, Mar 31, 2022 at 01:40:23PM -0500, Dan Vacura wrote:
> During the uvcg_video_pump() process, if an error occurs and
> uvcg_queue_cancel() is called, the buffer queue will be cleared out, but
> the current marker (queue->buf_used) of the active buffer (no longer
> active) is not reset. On the next iteration of uvcg_video_pump() the
> stale buf_used count will be used and the logic of min((unsigned
> int)len, buf->bytesused - queue->buf_used) may incorrectly calculate a
> nbytes size, causing an invalid memory access.
> 
> [80802.185460][  T315] configfs-gadget gadget: uvc: VS request completed
> with status -18.
> [80802.185519][  T315] configfs-gadget gadget: uvc: VS request completed
> with status -18.
> ...
> uvcg_queue_cancel() is called and the queue is cleared out, but the
> marker queue->buf_used is not reset.
> ...
> [80802.262328][ T8682] Unable to handle kernel paging request at virtual
> address ffffffc03af9f000
> ...
> ...
> [80802.263138][ T8682] Call trace:
> [80802.263146][ T8682]  __memcpy+0x12c/0x180
> [80802.263155][ T8682]  uvcg_video_pump+0xcc/0x1e0
> [80802.263165][ T8682]  process_one_work+0x2cc/0x568
> [80802.263173][ T8682]  worker_thread+0x28c/0x518
> [80802.263181][ T8682]  kthread+0x160/0x170
> [80802.263188][ T8682]  ret_from_fork+0x10/0x18
> [80802.263198][ T8682] Code: a8c12829 a88130cb a8c130
> 
> Fixes: d692522577c0 ("usb: gadget/uvc: Port UVC webcam gadget to use videobuf2 framework")
> Cc: <stable@...r.kernel.org>
> Signed-off-by: Dan Vacura <w36195@...orola.com>

This indeed fixes an issue, so I think we can merge the patch, but I
also believe we need further improvements on top (of course if you would
like to improve the implementation in a v4, I won't complain :-))

As replied in v2 (sorry for the late reply), it seems that this error
can occur under normal conditions. This means we shouldn't cancel the
queue, at least when the error is intermitent (if all URBs fail that's
another story).

> ---
> Changes in v3:
> - Cc stable
> Changes in v2:
> - Add Fixes tag
> 
>  drivers/usb/gadget/function/uvc_queue.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_queue.c b/drivers/usb/gadget/function/uvc_queue.c
> index d852ac9e47e7..2cda982f3765 100644
> --- a/drivers/usb/gadget/function/uvc_queue.c
> +++ b/drivers/usb/gadget/function/uvc_queue.c
> @@ -264,6 +264,8 @@ void uvcg_queue_cancel(struct uvc_video_queue *queue, int disconnect)
>  		buf->state = UVC_BUF_STATE_ERROR;
>  		vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_ERROR);
>  	}

A blank line would be nice here, and I would also like a comment to
state that further improvements are required:

	/*
	 * When the queue is cancelled due to an error (either when queuing a
	 * USB request, or in the request completion handler), the we empty the
	 * irqqueue but userspace may queue futher buffers. We need to reset
	 * buf_used to 0 or uvcg_video_pump() will use an incorrect stale value.
	 *
	 * TODO: It seems that a -EXDEV error can occur in the request
	 * completion handler under normal circumstances. Don't cancel the queue
	 * in that case but recover gracefully (likely with rate-limiting, to
	 * still cancel the queue if errors occur too often).
	 */

We likely need to differentiate between -EXDEV and other errors in
uvc_video_complete(), as I'd like to be conservative and cancel the
queue for unknown errors. We also need to improve the queue cancellation
implementation so that userspace gets an error when queuing further
buffers.

> +	queue->buf_used = 0;
> +
>  	/* This must be protected by the irqlock spinlock to avoid race
>  	 * conditions between uvc_queue_buffer and the disconnection event that
>  	 * could result in an interruptible wait in uvc_dequeue_buffer. Do not

-- 
Regards,

Laurent Pinchart