drm/radeon: fix NULL pointer dereference in UMS mode in radeon_cs_parser_fini()

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 <[email protected]>

CC [email protected] 3.7

---
drivers/gpu/drm/radeon/radeon_cs.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/radeon/radeon_cs.c +1 -1

index 469661f..d1c282c 100644
@@ -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]);
 		}

Signature · 8 lines

1.7.9.5



_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Quoted 10 lines · Ilija Hadzic

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.

Quoted 1 line

-- 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

Quoted 11 lines · Shuah Khan

Quoted 10 lines · Ilija Hadzic

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.

Quoted 6 lines

Quoted 1 line

-- 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.

Quoted 19 lines · Herton Ronaldo Krzesinski

Quoted 11 lines · Shuah Khan

Quoted 10 lines · Ilija Hadzic

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.

Quoted 6 lines

Quoted 1 line

-- 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.

Quoted 9 lines

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