Message ID | 20230630110643.209761-2-hdegoede@redhat.com (mailing list archive) |
---|---|
State | Superseded |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1qFBzA-00ADVQ-4N; Fri, 30 Jun 2023 11:08:20 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230427AbjF3LIR (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Fri, 30 Jun 2023 07:08:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52506 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229508AbjF3LIB (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 30 Jun 2023 07:08:01 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 980A32D4A for <linux-media@vger.kernel.org>; Fri, 30 Jun 2023 04:06:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688123214; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=y+8vuyD5M67TFv6LoooeJoTqBLl6qlPDtYuFzoWTY50=; b=cUTOJ3o1XIMPOAqNxweogO6rCcnayIWOEbEU0OR2TU7abvRnSo0eW98foBArFZKNwiM2BP dw8fzOc6AQuTDIcWtD5whCKkB9e3JljiL4MBOJ3uEhvAC4vbltsFcnNfJOF3lEABvmsAax c5j3wgdy/+K10mctjIsm4xAo1mWp5H4= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-274-q-tl56TPMNGiE95ET7T4Zw-1; Fri, 30 Jun 2023 07:06:48 -0400 X-MC-Unique: q-tl56TPMNGiE95ET7T4Zw-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 83C9B29324B0; Fri, 30 Jun 2023 11:06:47 +0000 (UTC) Received: from shalem.redhat.com (unknown [10.39.193.184]) by smtp.corp.redhat.com (Postfix) with ESMTP id C4213492B02; Fri, 30 Jun 2023 11:06:45 +0000 (UTC) From: Hans de Goede <hdegoede@redhat.com> To: Sakari Ailus <sakari.ailus@linux.intel.com>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Daniel Scally <dan.scally@ideasonboard.com> Cc: Hans de Goede <hdegoede@redhat.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Andy Shevchenko <andy@kernel.org>, Kate Hsuan <hpa@redhat.com>, Hao Yao <hao.yao@intel.com>, Bingbu Cao <bingbu.cao@intel.com>, linux-media@vger.kernel.org, =?utf-8?q?Fabian_W=C3=BCthrich?= <me@fabwu.ch> Subject: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings Date: Fri, 30 Jun 2023 13:06:29 +0200 Message-ID: <20230630110643.209761-2-hdegoede@redhat.com> In-Reply-To: <20230630110643.209761-1-hdegoede@redhat.com> References: <20230630110643.209761-1-hdegoede@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -4.8 (----) X-LSpam-Report: No, score=-4.8 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
media: ipu-bridge: Shared with atomisp, rework VCM instantiation
|
|
Commit Message
Hans de Goede
June 30, 2023, 11:06 a.m. UTC
When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
sensor->adev is not set yet.
So if either of the dev_warn() calls about unknown values are hit this
will lead to a NULL pointer deref.
Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
on errors harder, to fix this.
Fixes: 485aa3df0dff ("media: ipu3-cio2: Parse sensor orientation and rotation")
Cc: Fabian Wüthrich <me@fabwu.ch>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/media/pci/intel/ipu-bridge.c | 5 +++++
1 file changed, 5 insertions(+)
Comments
On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > sensor->adev is not set yet. > > So if either of the dev_warn() calls about unknown values are hit this > will lead to a NULL pointer deref. > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > on errors harder, to fix this. TBH, I don't like this approach, it seems a bit dirty to me. First of all, why do we need pci_dev to be a parameter in this function? Second, why don't we consistently use the ACPI handle (with respective acpi_handle_*() macros to print messages)? So, my proposal here is to actually save the ACPI device handle in the sensor object and use it for the messaging. It makes it consistent and doesn't require to rewrite adev field which seems the dirty part to me. -- With Best Regards, Andy Shevchenko
On 30/06/2023 16:23, Andy Shevchenko wrote: > On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: >> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run >> sensor->adev is not set yet. >> >> So if either of the dev_warn() calls about unknown values are hit this >> will lead to a NULL pointer deref. >> >> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling >> on errors harder, to fix this. > TBH, I don't like this approach, it seems a bit dirty to me. > > First of all, why do we need pci_dev to be a parameter in this function? > Second, why don't we consistently use the ACPI handle (with respective > acpi_handle_*() macros to print messages)? > > So, my proposal here is to actually save the ACPI device handle in the > sensor object and use it for the messaging. It makes it consistent and > doesn't require to rewrite adev field which seems the dirty part to > me. It's a bit finicky but I don't think it's so bad; the refcounting is all fine, the later acpi_dev_get() is only to hold a reference once the next loop iteration frees the existing one and the rewrite should store the exact same pointer...we could just not store the result of the acpi_dev_get() call to avoid that weird rewrite perhaps? > > -- > With Best Regards, > Andy Shevchenko
On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote: > On 30/06/2023 16:23, Andy Shevchenko wrote: > > On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > > > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > > > sensor->adev is not set yet. > > > > > > So if either of the dev_warn() calls about unknown values are hit this > > > will lead to a NULL pointer deref. > > > > > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > > > on errors harder, to fix this. > > TBH, I don't like this approach, it seems a bit dirty to me. > > > > First of all, why do we need pci_dev to be a parameter in this function? > > Second, why don't we consistently use the ACPI handle (with respective > > acpi_handle_*() macros to print messages)? > > > > So, my proposal here is to actually save the ACPI device handle in the > > sensor object and use it for the messaging. It makes it consistent and > > doesn't require to rewrite adev field which seems the dirty part to > > me. > > It's a bit finicky but I don't think it's so bad; the refcounting is all > fine, the later acpi_dev_get() is only to hold a reference once the next > loop iteration frees the existing one and the rewrite should store the exact > same pointer...we could just not store the result of the acpi_dev_get() call > to avoid that weird rewrite perhaps? For short term solution in between the patches I might agree with you, but backporting. Backporting a bad code doesn't make it better even if it fixes nasty bug. And I proposed the solution. We may kill the handle same way as we are killing the awkwardness of this assignment later in the series.
Hi, On 7/4/23 16:28, Andy Shevchenko wrote: > On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote: >> On 30/06/2023 16:23, Andy Shevchenko wrote: >>> On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: >>>> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run >>>> sensor->adev is not set yet. >>>> >>>> So if either of the dev_warn() calls about unknown values are hit this >>>> will lead to a NULL pointer deref. >>>> >>>> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling >>>> on errors harder, to fix this. >>> TBH, I don't like this approach, it seems a bit dirty to me. >>> >>> First of all, why do we need pci_dev to be a parameter in this function? >>> Second, why don't we consistently use the ACPI handle (with respective >>> acpi_handle_*() macros to print messages)? >>> >>> So, my proposal here is to actually save the ACPI device handle in the >>> sensor object and use it for the messaging. It makes it consistent and >>> doesn't require to rewrite adev field which seems the dirty part to >>> me. >> >> It's a bit finicky but I don't think it's so bad; the refcounting is all >> fine, the later acpi_dev_get() is only to hold a reference once the next >> loop iteration frees the existing one and the rewrite should store the exact >> same pointer...we could just not store the result of the acpi_dev_get() call >> to avoid that weird rewrite perhaps? > > For short term solution in between the patches I might agree with you, but > backporting. Backporting a bad code doesn't make it better even if it fixes > nasty bug. And I proposed the solution. We may kill the handle same way as > we are killing the awkwardness of this assignment later in the series. Yeah, no sorry. As Dan pointed out this fix is fine and I don't feel like re-writing it just because you don't like it. I don't see any real technical arguments against this approach, just you not liking it. Regards, Hans
On Tue, Jul 4, 2023 at 5:50 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 7/4/23 16:28, Andy Shevchenko wrote: > > On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote: > >> On 30/06/2023 16:23, Andy Shevchenko wrote: > >>> On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote: > >>>> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run > >>>> sensor->adev is not set yet. > >>>> > >>>> So if either of the dev_warn() calls about unknown values are hit this > >>>> will lead to a NULL pointer deref. > >>>> > >>>> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling > >>>> on errors harder, to fix this. > >>> TBH, I don't like this approach, it seems a bit dirty to me. > >>> > >>> First of all, why do we need pci_dev to be a parameter in this function? > >>> Second, why don't we consistently use the ACPI handle (with respective > >>> acpi_handle_*() macros to print messages)? > >>> > >>> So, my proposal here is to actually save the ACPI device handle in the > >>> sensor object and use it for the messaging. It makes it consistent and > >>> doesn't require to rewrite adev field which seems the dirty part to > >>> me. > >> > >> It's a bit finicky but I don't think it's so bad; the refcounting is all > >> fine, the later acpi_dev_get() is only to hold a reference once the next > >> loop iteration frees the existing one and the rewrite should store the exact > >> same pointer...we could just not store the result of the acpi_dev_get() call > >> to avoid that weird rewrite perhaps? > > > > For short term solution in between the patches I might agree with you, but > > backporting. Backporting a bad code doesn't make it better even if it fixes > > nasty bug. And I proposed the solution. We may kill the handle same way as > > we are killing the awkwardness of this assignment later in the series. > > Yeah, no sorry. As Dan pointed out this fix is fine and I don't feel > like re-writing it just because you don't like it. > > I don't see any real technical arguments against this approach, just > you not liking it. OK.
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c index 62daa8c1f6b1..f0927f80184d 100644 --- a/drivers/media/pci/intel/ipu-bridge.c +++ b/drivers/media/pci/intel/ipu-bridge.c @@ -308,6 +308,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg, } sensor = &bridge->sensors[bridge->n_sensors]; + /* + * Borrow our adev ref to the sensor for now, on success + * acpi_dev_get(adev) is done further below. + */ + sensor->adev = adev; ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", &sensor->ssdb,