Nokia N900: refcount_t underflow, use after free

Commit Message

Suman Anna March 9, 2018, 11:06 p.m. UTC

On 03/09/2018 04:18 PM, Pavel Machek wrote:
> On Fri 2018-03-09 16:13:36, Suman Anna wrote:
>> On 03/09/2018 06:08 AM, Robin Murphy wrote:
>>> On 08/03/18 18:50, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> * Pavel Machek <[e-mail]> [180308 14:31]:
>>>>>>> Hi!
>>>>>>>
>>>>>>> I'm getting this warning... Has anyone seen/debugged that before?
>>>>>>> Unfortunately the backtrace does not seem to be too useful :-(.
>>>>>>
>>>>>> Adding Suman to Cc, as it points to arm_iommu_release_mapping().
>>>>>
>>>>> Hmm, we need to find out if the failure paths in isp_probe() are
>>>>> mismatched, or if this is coming from some mismatch between the OMAP
>>>>> IOMMU driver and the DMA plumbing. AFAIK, the cleanup paths in this
>>>>
>>>> Well, camera only started to work on N900 pretty recently. Let me add
>>>> some debug printks...
>>>>
>>>> Camera does not work in 4.16.0-rc4-next-20180308-dirty.
>>>>
>>>> I see this. It looks like problem in isp error paths, indeed:
>>>
>>> Well, there certainly seems to be an obvious bug wherein
>>> isp_detach_iommu() just releases the mapping directly without calling
>>> arm_iommu_detach_device() to balance the equivalent attach. That can't
>>> be helping.
>>
>> Indeed, I have been able to reproduce the same warning using a
>> standalone test module, and the missing arm_iommu_detach_device() is
>> causing the warning after probe (during failure path) or during
>> remove.
> 
> Ok do you have an idea how to fix the isp error paths? Untested patch
> would be fine... But it seems that you know what needs to be fixed and
> I don't.
> 

OK, see if the following fixes the issue for you, only build tested.

8< ---------------------
From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
From: Suman Anna <[e-mail]>
Date: Fri, 9 Mar 2018 16:39:59 -0600
Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping

The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
ARM DMA backend. The current code creates a dma_iommu_mapping and
attaches this to the ISP device, but never detaches the mapping in
either the probe failure paths or the driver remove path resulting
in an unbalanced mapping refcount and a memory leak. Fix this properly.

Reported-by: Pavel Machek <[e-mail]>
Signed-off-by: Suman Anna <[e-mail]>
---
 drivers/media/platform/omap3isp/isp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

 }
@@ -1971,13 +1972,15 @@ static int isp_attach_iommu(struct isp_device *isp)
        ret = arm_iommu_attach_device(isp->dev, mapping);
        if (ret < 0) {
                dev_err(isp->dev, "failed to attach device to VA
mapping\n");
-               goto error;
+               goto error_attach;
        }

        return 0;

+error_attach:
+       arm_iommu_release_mapping(isp->mapping);
+       isp->mapping = NULL;
 error:
-       isp_detach_iommu(isp);
        return ret;
 }

Comments

Pavel Machek March 10, 2018, 11:26 a.m. UTC | #1

Hi!

> >>> Well, there certainly seems to be an obvious bug wherein
> >>> isp_detach_iommu() just releases the mapping directly without calling
> >>> arm_iommu_detach_device() to balance the equivalent attach. That can't
> >>> be helping.
> >>
> >> Indeed, I have been able to reproduce the same warning using a
> >> standalone test module, and the missing arm_iommu_detach_device() is
> >> causing the warning after probe (during failure path) or during
> >> remove.
> > 
> > Ok do you have an idea how to fix the isp error paths? Untested patch
> > would be fine... But it seems that you know what needs to be fixed and
> > I don't.
> > 
> 
> OK, see if the following fixes the issue for you, only build tested.

Word-wrapped, so I applied by hand. And yes, the oops at boot is
gone. Thanks!

(Camera still does not work in -next... kills system. Oh well. Lets
debug that some other day.)

> 8< ---------------------
> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
> From: Suman Anna <[e-mail]>
> Date: Fri, 9 Mar 2018 16:39:59 -0600
> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
> 
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek <[e-mail]>
> Signed-off-by: Suman Anna <[e-mail]>

Tested-by: Pavel Machek <[e-mail]>
									Pavel

Suman Anna March 12, 2018, 4:10 p.m. UTC | #2

On 03/10/2018 05:26 AM, Pavel Machek wrote:
> Hi!
> 
>>>>> Well, there certainly seems to be an obvious bug wherein
>>>>> isp_detach_iommu() just releases the mapping directly without calling
>>>>> arm_iommu_detach_device() to balance the equivalent attach. That can't
>>>>> be helping.
>>>>
>>>> Indeed, I have been able to reproduce the same warning using a
>>>> standalone test module, and the missing arm_iommu_detach_device() is
>>>> causing the warning after probe (during failure path) or during
>>>> remove.
>>>
>>> Ok do you have an idea how to fix the isp error paths? Untested patch
>>> would be fine... But it seems that you know what needs to be fixed and
>>> I don't.
>>>
>>
>> OK, see if the following fixes the issue for you, only build tested.
> 
> Word-wrapped, so I applied by hand. And yes, the oops at boot is
> gone. Thanks!

Sorry about that, have to check my mail settings. Anyway will post the
patch again, glad that it fixed your issue.

regards
Suman

> 
> (Camera still does not work in -next... kills system. Oh well. Lets
> debug that some other day.)
> 
>> 8< ---------------------
>> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
>> From: Suman Anna <[e-mail]>
>> Date: Fri, 9 Mar 2018 16:39:59 -0600
>> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
>>
>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>> attaches this to the ISP device, but never detaches the mapping in
>> either the probe failure paths or the driver remove path resulting
>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>
>> Reported-by: Pavel Machek <[e-mail]>
>> Signed-off-by: Suman Anna <[e-mail]>
> 
> Tested-by: Pavel Machek <[e-mail]>
> 									Pavel
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pavel Machek May 24, 2018, 11:05 a.m. UTC | #3

Hi!

> >>
> >> OK, see if the following fixes the issue for you, only build tested.
> > 
> > Word-wrapped, so I applied by hand. And yes, the oops at boot is
> > gone. Thanks!
> 
> Sorry about that, have to check my mail settings. Anyway will post the
> patch again, glad that it fixed your issue.

Any news here? AFAICT the bug creeped into v4.17-rcX in the
meantime...

								Pavel

> regards
> Suman
> 
> > 
> > (Camera still does not work in -next... kills system. Oh well. Lets
> > debug that some other day.)
> > 
> >> 8< ---------------------
> >> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
> >> From: Suman Anna <[e-mail]>
> >> Date: Fri, 9 Mar 2018 16:39:59 -0600
> >> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
> >>
> >> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> >> ARM DMA backend. The current code creates a dma_iommu_mapping and
> >> attaches this to the ISP device, but never detaches the mapping in
> >> either the probe failure paths or the driver remove path resulting
> >> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> >>
> >> Reported-by: Pavel Machek <[e-mail]>
> >> Signed-off-by: Suman Anna <[e-mail]>
> > 
> > Tested-by: Pavel Machek <[e-mail]>
> > 									Pavel
> >

Suman Anna May 25, 2018, 2:37 a.m. UTC | #4

On 05/24/2018 06:05 AM, Pavel Machek wrote:
> Hi!
> 
>>>>
>>>> OK, see if the following fixes the issue for you, only build tested.
>>>
>>> Word-wrapped, so I applied by hand. And yes, the oops at boot is
>>> gone. Thanks!
>>
>> Sorry about that, have to check my mail settings. Anyway will post the
>> patch again, glad that it fixed your issue.
> 
> Any news here? AFAICT the bug creeped into v4.17-rcX in the
> meantime...
>

The patch has been on linux-next for sometime now.

regards
Suman

> 								Pavel
> 
>> regards
>> Suman
>>
>>>
>>> (Camera still does not work in -next... kills system. Oh well. Lets
>>> debug that some other day.)
>>>
>>>> 8< ---------------------
>>>> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
>>>> From: Suman Anna <[e-mail]>
>>>> Date: Fri, 9 Mar 2018 16:39:59 -0600
>>>> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
>>>>
>>>> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
>>>> ARM DMA backend. The current code creates a dma_iommu_mapping and
>>>> attaches this to the ISP device, but never detaches the mapping in
>>>> either the probe failure paths or the driver remove path resulting
>>>> in an unbalanced mapping refcount and a memory leak. Fix this properly.
>>>>
>>>> Reported-by: Pavel Machek <[e-mail]>
>>>> Signed-off-by: Suman Anna <[e-mail]>
>>>
>>> Tested-by: Pavel Machek <[e-mail]>
>>> 									Pavel
>>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 8eb000e3d8fd..c7d667bfc2af 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1945,6 +1945,7 @@  static int isp_initialize_modules(struct
isp_device *isp)

 static void isp_detach_iommu(struct isp_device *isp)
 {
+       arm_iommu_detach_device(isp->dev);
        arm_iommu_release_mapping(isp->mapping);
        isp->mapping = NULL;