diff mbox series

[v2,01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

Message ID 20230630110643.209761-2-hdegoede@redhat.com
State Accepted
Commit 284be5693163343e1cf17c03917eecd1d6681bcf
Headers show
Series media: ipu-bridge: Shared with atomisp, rework VCM instantiation | expand

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

Andy Shevchenko June 30, 2023, 2:23 p.m. UTC | #1
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
Dan Scally July 4, 2023, 11:02 a.m. UTC | #2
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
Andy Shevchenko July 4, 2023, 2:28 p.m. UTC | #3
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.
Hans de Goede July 4, 2023, 2:50 p.m. UTC | #4
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
diff mbox series

Patch

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,