From patchwork Tue Dec 3 21:20:10 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ricardo Ribalda X-Patchwork-Id: 847502 Received: from mail-oo1-f54.google.com (mail-oo1-f54.google.com [209.85.161.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 822F920B1F3 for ; Tue, 3 Dec 2024 21:20:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.161.54 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733260826; cv=none; b=tsEPeXhwfbrEr1yF4RqNLfCudfA65jIfFba9MKQTLOEeIMuYWy5zTz3voDDWihMzOqN8CTYgqOQSJCWJ7+BNYJXbic6rv9WvKJFR5HYy1tcffNI87Ky/pp/QGWljMjRxa9whl0PH6P4zxmUBQEuWfE9qlMfxi0KP8dwLaozmG24= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1733260826; c=relaxed/simple; bh=C3v2J+2poN58Kyl3O5+kAnV8oVk30/d9asKCtbOCpXQ=; h=From:Date:Subject:MIME-Version:Content-Type:Message-Id:References: In-Reply-To:To:Cc; b=dxiQxm2wFyBZwATlDX+LwdQzaF1B6x0slPN3KA1rg/RnLJGn5YsXlKJPooduWrhZm24RhK/J66SBqULLUoq4iF+Y2l/qk/WzhIRjo6zbutr+h3z+h2ivRu7jPg0FeCop9svAMVvhW4/qOZkW2sc7phsg1UIK78hXtEukzPUB2II= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org; spf=pass smtp.mailfrom=chromium.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b=AR4aT02T; arc=none smtp.client-ip=209.85.161.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=chromium.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="AR4aT02T" Received: by mail-oo1-f54.google.com with SMTP id 006d021491bc7-5f22ea6d874so2247113eaf.1 for ; Tue, 03 Dec 2024 13:20:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1733260823; x=1733865623; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=8ac3SDhZgwboifbpkv9WFxBdAVzUo9z6wiYYzPt9fB0=; b=AR4aT02TJkCl7TZbBkxYTst8fo2qkgQjde1s2ADTzzrdR6VZnOj8Y8kS2C2EJgYq0X 52Z4yryiwUacuZHQ2t6LsBHj0OilhfNTUCDOvCBM3ks8V5Q9SfPdV5LRrT6dof8MmPMj CnEc2OAFM20yjR+L/1kUb0V24yMi9I4uzZYIQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1733260823; x=1733865623; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=8ac3SDhZgwboifbpkv9WFxBdAVzUo9z6wiYYzPt9fB0=; b=wkmCuNn2U1Ab/Can7hT7954vr/Y8QyV237bNNsjyh29dkDlu0DAC8xRPKHVXcaJbik 6A1NxqP2BIja4ne81RUY0M+pLuxZ8MwLpfoU9ctZ4WLPLpJqZftCpYTA9MeO0lDP/A1V tdtVKQiX5N+x37H1c/I8eCj9BqHkT1H5rakxU3e6Bx/0EqDFkK0wjEaADOZVgvBJ3kxg ST13v1gGEbBesOdAOKqlBe6WbSDTNdFgn0wYeIJLI5fddpSmA7LDHdSayWcbnPMaSiLn eQAbCBZqvDxgvKTj0Fp1dwSjgDQ+c1ricUg0Z9FbEuJxlkTzernTCYsnHLcVTNxPUlgW /jYw== X-Forwarded-Encrypted: i=1; AJvYcCUvwaCGdqWGRedQXxu48VeBF23a/d96TRESCgZPoVbGxv3ofweqqfiBM74prWPKMr+im6ZjIH8PXtTuzA==@vger.kernel.org X-Gm-Message-State: AOJu0Yy/GD1ndS0D+CMAjXdv1QLqSgcV493BGecV9klkgoyUFsHWemCP h8fexdPwGt68PtMFGB5R0gO/4xYq9/R0oIJYclYPDA5LgzDhQhwihDYIvDN2Pw== X-Gm-Gg: ASbGnctFG+kQKTRs5M6F9D4fmQjSj6jRiLokyDjRLKhveSLtqDeF8pRQ1rtPGOswt3d rJhEzWxqbxowJCNVXEGaQOXT9EfsOxOtGRSxDyySasQsA18Q+Zs7ctnojF9hz/UpUKkzsMzkCwM IkxqUUkRyUj/ZqdrLPKtIxCx4EODBp3fN5zQ8KbnFcKob+LT2ULh37HBlh5RgKntS7dtgk9b2oZ lbAHcKlYM/93a3kPg9D5TGsRj22MWF/CITyJ/v7Bh6hhLhR7buncVI05TEszuJe5juR00BUH15I lgv1CtfK9KxsWJhRkgJxj09G X-Google-Smtp-Source: AGHT+IHGHLmqOEE6bDN/rik+bHTtTVn/fqIntJnNo8WaDnRDY6V/anFIBM7Fu6XvZHT/7pziDCMZzQ== X-Received: by 2002:a05:6359:7903:b0:1ca:a437:3322 with SMTP id e5c5f4694b2df-1caeac0e68cmr447232455d.16.1733260823542; Tue, 03 Dec 2024 13:20:23 -0800 (PST) Received: from denia.c.googlers.com (5.236.236.35.bc.googleusercontent.com. [35.236.236.5]) by smtp.gmail.com with ESMTPSA id a1e0cc1a2514c-85b82a89d5csm2140364241.8.2024.12.03.13.20.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 03 Dec 2024 13:20:22 -0800 (PST) From: Ricardo Ribalda Date: Tue, 03 Dec 2024 21:20:10 +0000 Subject: [PATCH v6 3/5] media: uvcvideo: Remove dangling pointers Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Message-Id: <20241203-uvc-fix-async-v6-3-26c867231118@chromium.org> References: <20241203-uvc-fix-async-v6-0-26c867231118@chromium.org> In-Reply-To: <20241203-uvc-fix-async-v6-0-26c867231118@chromium.org> To: Laurent Pinchart , Hans de Goede , Mauro Carvalho Chehab , Guennadi Liakhovetski Cc: Hans Verkuil , Mauro Carvalho Chehab , linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Ricardo Ribalda , stable@vger.kernel.org X-Mailer: b4 0.13.0 When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future. If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use. Clean all the dangling pointers during release(). To avoid adding a performance penalty in the most common case (no async operation), a counter has been introduced with some logic to make sure that it is properly handled. Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Reviewed-by: Hans de Goede Signed-off-by: Ricardo Ribalda --- drivers/media/usb/uvc/uvc_ctrl.c | 58 ++++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 9 ++++++- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 42b0a0cdc51c..def502195528 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1579,6 +1579,40 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); } +static void uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl, + struct uvc_fh *new_handle) +{ + lockdep_assert_held(&handle->chain->ctrl_mutex); + + if (new_handle) { + if (ctrl->handle) + dev_warn_ratelimited(&handle->stream->dev->udev->dev, + "UVC non compliance: Setting an async control with a pending operation."); + + if (new_handle == ctrl->handle) + return; + + if (ctrl->handle) { + WARN_ON(!ctrl->handle->pending_async_ctrls); + if (ctrl->handle->pending_async_ctrls) + ctrl->handle->pending_async_ctrls--; + } + + ctrl->handle = new_handle; + handle->pending_async_ctrls++; + return; + } + + /* Cannot clear the handle for a control not owned by us.*/ + if (WARN_ON(ctrl->handle != handle)) + return; + + ctrl->handle = NULL; + if (WARN_ON(!handle->pending_async_ctrls)) + return; + handle->pending_async_ctrls--; +} + void uvc_ctrl_status_event(struct uvc_video_chain *chain, struct uvc_control *ctrl, const u8 *data) { @@ -1589,7 +1623,8 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex); handle = ctrl->handle; - ctrl->handle = NULL; + if (handle) + uvc_ctrl_set_handle(handle, ctrl, NULL); list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -1863,7 +1898,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, if (!rollback && handle && ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) - ctrl->handle = handle; + uvc_ctrl_set_handle(handle, ctrl, handle); } return 0; @@ -2772,6 +2807,25 @@ int uvc_ctrl_init_device(struct uvc_device *dev) return 0; } +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) +{ + struct uvc_entity *entity; + + guard(mutex)(&handle->chain->ctrl_mutex); + + if (!handle->pending_async_ctrls) + return; + + list_for_each_entry(entity, &handle->chain->dev->entities, list) + for (unsigned int i = 0; i < entity->ncontrols; ++i) { + if (entity->controls[i].handle != handle) + continue; + uvc_ctrl_set_handle(handle, &entity->controls[i], NULL); + } + + WARN_ON(handle->pending_async_ctrls); +} + /* * Cleanup device controls. */ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 97c5407f6603..b425306a3b8c 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) uvc_dbg(stream->dev, CALLS, "%s\n", __func__); + uvc_ctrl_cleanup_fh(handle); + /* Only free resources if this is a privileged handle. */ if (uvc_has_privileges(handle)) uvc_queue_release(&stream->queue); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..92ecdd188587 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,7 +337,11 @@ struct uvc_video_chain { struct uvc_entity *processing; /* Processing unit */ struct uvc_entity *selector; /* Selector unit */ - struct mutex ctrl_mutex; /* Protects ctrl.info */ + struct mutex ctrl_mutex; /* + * Protects ctrl.info, + * ctrl.handle and + * uvc_fh.pending_async_ctrls + */ struct v4l2_prio_state prio; /* V4L2 priority state */ u32 caps; /* V4L2 chain-wide caps */ @@ -612,6 +616,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state; + unsigned int pending_async_ctrls; }; struct uvc_driver { @@ -797,6 +802,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry); +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); + /* Utility functions */ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, u8 epaddr);