* [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
@ 2013-01-16 23:43 Shuah Khan
2013-01-17 3:06 ` Ilija Hadzic
0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2013-01-16 23:43 UTC (permalink / raw)
To: airlied, deathsimple, jglisse, airlied, alexander.deucher
Cc: dri-devel, LKML, shuahkhan, Greg KH
Fix parser->rdev NULL pointer dereference in radeon_cs_parser_fini().
While back-porting drm/radeon: fix NULL pointer dereference in UMS mode
patch (commit-id: ff4bd0827764e10a428a9d39e6814c5478863f94) to 3,7.y, noticed
another instance of NULL pointer dereference in radeon_cs_parser_fini()
function.
Signed-off-by: Shuah Khan <shuah.khan@hp.com>
CC: stable@vger.kernel.org 3.7
---
drivers/gpu/drm/radeon/radeon_cs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
index 469661f..d1c282c 100644
--- a/drivers/gpu/drm/radeon/radeon_cs.c
+++ b/drivers/gpu/drm/radeon/radeon_cs.c
@@ -329,7 +329,7 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error)
kfree(parser->relocs_ptr);
for (i = 0; i < parser->nchunks; i++) {
kfree(parser->chunks[i].kdata);
- if ((parser->rdev->flags & RADEON_IS_AGP)) {
+ if (parser->rdev && (parser->rdev->flags & RADEON_IS_AGP)) {
kfree(parser->chunks[i].kpage[0]);
kfree(parser->chunks[i].kpage[1]);
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
2013-01-16 23:43 [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini() Shuah Khan
@ 2013-01-17 3:06 ` Ilija Hadzic
2013-01-17 17:09 ` Shuah Khan
0 siblings, 1 reply; 6+ messages in thread
From: Ilija Hadzic @ 2013-01-17 3:06 UTC (permalink / raw)
To: Shuah Khan
Cc: airlied, deathsimple, jglisse, airlied, Deucher, Alexander,
Greg KH, LKML, dri-devel, shuahkhan
Actually, the code path affected by your patch is not executed in UMS mode
at all. Notice that radeon_cs_parser_fini is only called from
radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
The equivalent of the fix you are trying to do is in
a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
the one used by legacy-CS ioctl), which you should go together
with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
-- Ilija
On Wed, 16 Jan 2013, Shuah Khan wrote:
> Fix parser->rdev NULL pointer dereference in radeon_cs_parser_fini().
> While back-porting drm/radeon: fix NULL pointer dereference in UMS mode
> patch (commit-id: ff4bd0827764e10a428a9d39e6814c5478863f94) to 3,7.y, noticed
> another instance of NULL pointer dereference in radeon_cs_parser_fini()
> function.
>
> Signed-off-by: Shuah Khan <shuah.khan@hp.com>
> CC: stable@vger.kernel.org 3.7
> ---
> drivers/gpu/drm/radeon/radeon_cs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c
> index 469661f..d1c282c 100644
> --- a/drivers/gpu/drm/radeon/radeon_cs.c
> +++ b/drivers/gpu/drm/radeon/radeon_cs.c
> @@ -329,7 +329,7 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error)
> kfree(parser->relocs_ptr);
> for (i = 0; i < parser->nchunks; i++) {
> kfree(parser->chunks[i].kdata);
> - if ((parser->rdev->flags & RADEON_IS_AGP)) {
> + if (parser->rdev && (parser->rdev->flags & RADEON_IS_AGP)) {
> kfree(parser->chunks[i].kpage[0]);
> kfree(parser->chunks[i].kpage[1]);
> }
> --
> 1.7.9.5
>
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
2013-01-17 3:06 ` Ilija Hadzic
@ 2013-01-17 17:09 ` Shuah Khan
2013-01-23 16:07 ` Herton Ronaldo Krzesinski
0 siblings, 1 reply; 6+ messages in thread
From: Shuah Khan @ 2013-01-17 17:09 UTC (permalink / raw)
To: Ilija Hadzic
Cc: airlied, deathsimple, jglisse, airlied, Deucher, Alexander,
Greg KH, LKML, dri-devel, shuahkhan
On Wed, 2013-01-16 at 21:06 -0600, Ilija Hadzic wrote:
> Actually, the code path affected by your patch is not executed in UMS mode
> at all. Notice that radeon_cs_parser_fini is only called from
> radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
>
> The equivalent of the fix you are trying to do is in
> a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
> the one used by legacy-CS ioctl), which you should go together
> with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
> fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
>
> -- Ilija
Thanks. I will take a look at a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e.
I sent back-ported ff4bd0827764e10a428a9d39e6814c5478863f94 patch to
stable and I will back-port and send
a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e as well.
-- Shuah
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
2013-01-17 17:09 ` Shuah Khan
@ 2013-01-23 16:07 ` Herton Ronaldo Krzesinski
2013-01-23 17:21 ` Ilija Hadzic
0 siblings, 1 reply; 6+ messages in thread
From: Herton Ronaldo Krzesinski @ 2013-01-23 16:07 UTC (permalink / raw)
To: Shuah Khan
Cc: Ilija Hadzic, airlied, deathsimple, jglisse, airlied, Deucher,
Alexander, Greg KH, LKML, dri-devel, shuahkhan
On Thu, Jan 17, 2013 at 10:09:42AM -0700, Shuah Khan wrote:
> On Wed, 2013-01-16 at 21:06 -0600, Ilija Hadzic wrote:
> > Actually, the code path affected by your patch is not executed in UMS mode
> > at all. Notice that radeon_cs_parser_fini is only called from
> > radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
> >
> > The equivalent of the fix you are trying to do is in
> > a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
> > the one used by legacy-CS ioctl), which you should go together
> > with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
> > fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
> >
> > -- Ilija
>
> Thanks. I will take a look at a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e.
> I sent back-ported ff4bd0827764e10a428a9d39e6814c5478863f94 patch to
> stable and I will back-port and send
> a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e as well.
While at it, also looks like commit
25d8999780f8c1f53928f4a24a09c01550423109 could also be added to stables.
But while looking at it, while the patch itself is correct, I noted that
there is a possibility of double free: if either of the
p->chunks[p->chunk_ib_idx].kpage[] items are non NULL, we will free it in
radeon_cs_parser_init and also when calling one of the fini functions
(radeon_cs_parser_fini or r600_cs_parser_fini). So either we need to set
kpage[n] to NULL after kfree, or just return the error.
>
> -- Shuah
--
[]'s
Herton
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
2013-01-23 16:07 ` Herton Ronaldo Krzesinski
@ 2013-01-23 17:21 ` Ilija Hadzic
2013-01-23 17:40 ` Herton Ronaldo Krzesinski
0 siblings, 1 reply; 6+ messages in thread
From: Ilija Hadzic @ 2013-01-23 17:21 UTC (permalink / raw)
To: Herton Ronaldo Krzesinski
Cc: Shuah Khan, Greg KH, jglisse, dri-devel, LKML, Deucher,
Alexander, shuahkhan, airlied
On Wed, Jan 23, 2013 at 11:07 AM, Herton Ronaldo Krzesinski
<herton.krzesinski@canonical.com> wrote:
> On Thu, Jan 17, 2013 at 10:09:42AM -0700, Shuah Khan wrote:
>> On Wed, 2013-01-16 at 21:06 -0600, Ilija Hadzic wrote:
>> > Actually, the code path affected by your patch is not executed in UMS mode
>> > at all. Notice that radeon_cs_parser_fini is only called from
>> > radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
>> >
>> > The equivalent of the fix you are trying to do is in
>> > a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
>> > the one used by legacy-CS ioctl), which you should go together
>> > with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
>> > fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
>> >
>> > -- Ilija
>>
>> Thanks. I will take a look at a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e.
>> I sent back-ported ff4bd0827764e10a428a9d39e6814c5478863f94 patch to
>> stable and I will back-port and send
>> a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e as well.
>
> While at it, also looks like commit
> 25d8999780f8c1f53928f4a24a09c01550423109 could also be added to stables.
> But while looking at it, while the patch itself is correct, I noted that
> there is a possibility of double free: if either of the
> p->chunks[p->chunk_ib_idx].kpage[] items are non NULL, we will free it in
> radeon_cs_parser_init and also when calling one of the fini functions
> (radeon_cs_parser_fini or r600_cs_parser_fini). So either we need to set
> kpage[n] to NULL after kfree, or just return the error.
>
Yes you are right. The error-handling path should force both kpage[]
pointers to NULL before returning -ENOMEM. That would fix the double
free.
Regarding, inclusion of 25d8999780f8c1f53928f4a24a09c01550423109 into
stable (and possible follow-up patch to fix potential double-kfree),
does this pass the "no fixes for theoretical problems in stable"
criterion ?
It is an obvious bug, but it happens so rarely that I am not surprised
that it has never happened: You need an (old) AGP card *and* you need
to run out of kmalloc memory.
-- Ilija
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()
2013-01-23 17:21 ` Ilija Hadzic
@ 2013-01-23 17:40 ` Herton Ronaldo Krzesinski
0 siblings, 0 replies; 6+ messages in thread
From: Herton Ronaldo Krzesinski @ 2013-01-23 17:40 UTC (permalink / raw)
To: Ilija Hadzic
Cc: Shuah Khan, Greg KH, jglisse, dri-devel, LKML, Deucher,
Alexander, shuahkhan, airlied
On Wed, Jan 23, 2013 at 12:21:29PM -0500, Ilija Hadzic wrote:
> On Wed, Jan 23, 2013 at 11:07 AM, Herton Ronaldo Krzesinski
> <herton.krzesinski@canonical.com> wrote:
> > On Thu, Jan 17, 2013 at 10:09:42AM -0700, Shuah Khan wrote:
> >> On Wed, 2013-01-16 at 21:06 -0600, Ilija Hadzic wrote:
> >> > Actually, the code path affected by your patch is not executed in UMS mode
> >> > at all. Notice that radeon_cs_parser_fini is only called from
> >> > radeon_cs_ioctl which is a KMS-only ioctl (see radeon_kms.c).
> >> >
> >> > The equivalent of the fix you are trying to do is in
> >> > a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e (function patched by that one is
> >> > the one used by legacy-CS ioctl), which you should go together
> >> > with ff4bd0827764e10a428a9d39e6814c5478863f94 if you are backporting UMS
> >> > fixes to 3.7. Both are needed to prevent kernel crashes in UMS mode.
> >> >
> >> > -- Ilija
> >>
> >> Thanks. I will take a look at a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e.
> >> I sent back-ported ff4bd0827764e10a428a9d39e6814c5478863f94 patch to
> >> stable and I will back-port and send
> >> a6b7e1a02b77ab8fe8775d20a88c53d8ba55482e as well.
> >
> > While at it, also looks like commit
> > 25d8999780f8c1f53928f4a24a09c01550423109 could also be added to stables.
> > But while looking at it, while the patch itself is correct, I noted that
> > there is a possibility of double free: if either of the
> > p->chunks[p->chunk_ib_idx].kpage[] items are non NULL, we will free it in
> > radeon_cs_parser_init and also when calling one of the fini functions
> > (radeon_cs_parser_fini or r600_cs_parser_fini). So either we need to set
> > kpage[n] to NULL after kfree, or just return the error.
> >
>
> Yes you are right. The error-handling path should force both kpage[]
> pointers to NULL before returning -ENOMEM. That would fix the double
> free.
>
> Regarding, inclusion of 25d8999780f8c1f53928f4a24a09c01550423109 into
> stable (and possible follow-up patch to fix potential double-kfree),
> does this pass the "no fixes for theoretical problems in stable"
> criterion ?
>
> It is an obvious bug, but it happens so rarely that I am not surprised
> that it has never happened: You need an (old) AGP card *and* you need
> to run out of kmalloc memory.
About the "theoretical problem" criteria, I think it applies more to
race conditions not detected in practice, things that are not obvious/
deterministic etc., but indeed commit 25d89997 looks not be worth to be
added on stables.
>
> -- Ilija
>
--
[]'s
Herton
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-23 17:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-01-16 23:43 [PATCH] drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini() Shuah Khan
2013-01-17 3:06 ` Ilija Hadzic
2013-01-17 17:09 ` Shuah Khan
2013-01-23 16:07 ` Herton Ronaldo Krzesinski
2013-01-23 17:21 ` Ilija Hadzic
2013-01-23 17:40 ` Herton Ronaldo Krzesinski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.