* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox