A freed pointer is passed as an argument to the function "devres_destroy()" in
"kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
after free.
It's notabug at this time, but the code is dangerous.
Signed-off-by: Maxin B. John <[email protected]>
Reviewed-by: Rolf Eike Beer <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index 1ef4ffc..bd8e788 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -87,8 +87,8 @@ void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id)
{
struct irq_devres match_data = { irq, dev_id };
- free_irq(irq, dev_id);
WARN_ON(devres_destroy(dev, devm_irq_release, devm_irq_match,
&match_data));
+ free_irq(irq, dev_id);
}
EXPORT_SYMBOL(devm_free_irq);
diff --git a/lib/devres.c b/lib/devres.c
index 6efddf5..7c0e953 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -79,9 +79,9 @@ EXPORT_SYMBOL(devm_ioremap_nocache);
*/
void devm_iounmap(struct device *dev, void __iomem *addr)
{
- iounmap(addr);
WARN_ON(devres_destroy(dev, devm_ioremap_release, devm_ioremap_match,
(void *)addr));
+ iounmap(addr);
}
EXPORT_SYMBOL(devm_iounmap);
Hello,
On Tue, Jun 14, 2011 at 10:07:32AM +0300, Maxin B John wrote:
> A freed pointer is passed as an argument to the function "devres_destroy()" in
> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
> after free.
>
> It's notabug at this time, but the code is dangerous.
The code is not dangerous. The pointer value is used strictly as key
and the code path is always properly serialized. The change is to
appease access-after-free detection logic, similar to the way we
change the code to better accomodate sparse or other code analysys
tools. Can you please update the patch description to reflect that?
Thank you.
--
tejun
Hi,
On Tue, Jun 14, 2011 at 8:15 AM, Tejun Heo <[email protected]> wrote:
> On Tue, Jun 14, 2011 at 10:07:32AM +0300, Maxin B John wrote:
>> A freed pointer is passed as an argument to the function "devres_destroy()" in
>> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
>> after free.
>>
>> It's notabug at this time, but the code is dangerous.
>
> The code is not dangerous. ?The pointer value is used strictly as key
> and the code path is always properly serialized. ?The change is to
> appease access-after-free detection logic, similar to the way we
> change the code to better accomodate sparse or other code analysys
> tools. ?Can you please update the patch description to reflect that?
You are right. I shouldn't have mentioned it as dangerous. I was trying to
make this patch description similar to the description present in the
previous patch:
http://www.spinics.net/lists/mm-commits/msg84313.html
This patch silences the Coverity Prevent's complains about this as
use-after-free bug. Please let me know if the updated patch description is OK.
"
A freed pointer is passed as an argument to the function "devres_destroy()" in
"kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
after free.
The change silences the static analysis tool (Coverity Prevent) complains
about this as use-after-free bug.
"
Best Regards,
Maxin
Hello, Maxin.
On Tue, Jun 14, 2011 at 03:09:34PM +0100, Maxin B John wrote:
> You are right. I shouldn't have mentioned it as dangerous. I was trying to
> make this patch description similar to the description present in the
> previous patch:
> http://www.spinics.net/lists/mm-commits/msg84313.html
>
> This patch silences the Coverity Prevent's complains about this as
> use-after-free bug. Please let me know if the updated patch description is OK.
>
> "
> A freed pointer is passed as an argument to the function "devres_destroy()" in
> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible use
> after free.
> The change silences the static analysis tool (Coverity Prevent) complains
> about this as use-after-free bug.
> "
Yeap, sounds mostly okay but there's no 'possible use after free'.
Maybe something like, "devres uses the pointer value as key after it's
freed, which is safe but triggers spurious use-after-free warnings on
some static analysis tools. Rearrange code to avoid such warnings".
Thanks.
--
tejun
Hi Tejun,
On Wed, Jun 15, 2011 at 9:13 AM, Tejun Heo <[email protected]> wrote:
>
> Yeap, sounds mostly okay but there's no 'possible use after free'.
> Maybe something like, "devres uses the pointer value as key after it's
> freed, which is safe but triggers spurious use-after-free warnings on
> some static analysis tools. ?Rearrange code to avoid such warnings".
>
Thanks a lot for this suggestion. I think, this describes the patch in
a much better way.
Warm Regards,
Maxin
> Hello, Maxin.
>
> On Tue, Jun 14, 2011 at 03:09:34PM +0100, Maxin B John wrote:
>> You are right. I shouldn't have mentioned it as dangerous. I was trying
>> to
>> make this patch description similar to the description present in the
>> previous patch:
>> http://www.spinics.net/lists/mm-commits/msg84313.html
>>
>> This patch silences the Coverity Prevent's complains about this as
>> use-after-free bug. Please let me know if the updated patch description
>> is OK.
>>
>> "
>> A freed pointer is passed as an argument to the function
>> "devres_destroy()" in
>> "kernel/irq/devres.c" and "lib/devres.c". This patch fixes the possible
>> use
>> after free.
>> The change silences the static analysis tool (Coverity Prevent)
>> complains
>> about this as use-after-free bug.
>> "
>
> Yeap, sounds mostly okay but there's no 'possible use after free'.
> Maybe something like, "devres uses the pointer value as key after it's
> freed, which is safe but triggers spurious use-after-free warnings on
> some static analysis tools. Rearrange code to avoid such warnings".
Sounds good. And please merge both patches together.
Eike
Hi Eike,
On Wed, Jun 15, 2011 at 11:00 AM, Rolf Eike Beer <[email protected]> wrote:
>
> Sounds good. And please merge both patches together.
>
I'll merge both patches into a single patch and repost it today itself.
Warm Regards,
Maxin
devres uses the pointer value as key after it's freed, which is safe but
triggers spurious use-after-free warnings on some static analysis tools.
Rearrange code to avoid such warnings.
Signed-off-by: Maxin B. John <[email protected]>
Reviewed-by: Rolf Eike Beer <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
diff --git a/kernel/irq/devres.c b/kernel/irq/devres.c
index 1ef4ffc..bd8e788 100644
--- a/kernel/irq/devres.c
+++ b/kernel/irq/devres.c
@@ -87,8 +87,8 @@ void devm_free_irq(struct device *dev, unsigned int irq, void *dev_id)
{
struct irq_devres match_data = { irq, dev_id };
- free_irq(irq, dev_id);
WARN_ON(devres_destroy(dev, devm_irq_release, devm_irq_match,
&match_data));
+ free_irq(irq, dev_id);
}
EXPORT_SYMBOL(devm_free_irq);
diff --git a/lib/devres.c b/lib/devres.c
index 6efddf5..7c0e953 100644
--- a/lib/devres.c
+++ b/lib/devres.c
@@ -79,9 +79,9 @@ EXPORT_SYMBOL(devm_ioremap_nocache);
*/
void devm_iounmap(struct device *dev, void __iomem *addr)
{
- iounmap(addr);
WARN_ON(devres_destroy(dev, devm_ioremap_release, devm_ioremap_match,
(void *)addr));
+ iounmap(addr);
}
EXPORT_SYMBOL(devm_iounmap);
diff --git a/mm/dmapool.c b/mm/dmapool.c
index 03bf3bb..fbb58e3 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -500,7 +500,7 @@ void dmam_pool_destroy(struct dma_pool *pool)
{
struct device *dev = pool->dev;
- dma_pool_destroy(pool);
WARN_ON(devres_destroy(dev, dmam_pool_release, dmam_pool_match, pool));
+ dma_pool_destroy(pool);
}
EXPORT_SYMBOL(dmam_pool_destroy);