From 4c7886791264f03428d5424befb1b96f08fc90f4 Mon Sep 17 00:00:00 2001 From: Jerome Glisse Date: Fri, 20 Nov 2009 14:29:23 +0100 Subject: drm/radeon/kms: Rework radeon object handling The locking & protection of radeon object was somewhat messy. This patch completely rework it to now use ttm reserve as a protection for the radeon object structure member. It also shrink down the various radeon object structure by removing field which were redondant with the ttm information. Last it converts few simple functions to inline which should with performances. airlied: rebase on top of r600 and other changes. Signed-off-by: Jerome Glisse Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 5ab2cf96a264..65590a0f1d93 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -76,17 +76,17 @@ int radeon_cs_parser_relocs(struct radeon_cs_parser *p) } p->relocs_ptr[i] = &p->relocs[i]; p->relocs[i].robj = p->relocs[i].gobj->driver_private; - p->relocs[i].lobj.robj = p->relocs[i].robj; + p->relocs[i].lobj.bo = p->relocs[i].robj; p->relocs[i].lobj.rdomain = r->read_domains; p->relocs[i].lobj.wdomain = r->write_domain; p->relocs[i].handle = r->handle; p->relocs[i].flags = r->flags; INIT_LIST_HEAD(&p->relocs[i].lobj.list); - radeon_object_list_add_object(&p->relocs[i].lobj, - &p->validated); + radeon_bo_list_add_object(&p->relocs[i].lobj, + &p->validated); } } - return radeon_object_list_validate(&p->validated, p->ib->fence); + return radeon_bo_list_validate(&p->validated, p->ib->fence); } int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) @@ -190,9 +190,10 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error) unsigned i; if (error) { - radeon_object_list_unvalidate(&parser->validated); + radeon_bo_list_unvalidate(&parser->validated, + parser->ib->fence); } else { - radeon_object_list_clean(&parser->validated); + radeon_bo_list_unreserve(&parser->validated); } for (i = 0; i < parser->nrelocs; i++) { if (parser->relocs[i].gobj) { -- cgit v1.2.2 From c8c15ff1e90bfc4a2db1ba77a01b3b2783e723fc Mon Sep 17 00:00:00 2001 From: Jerome Glisse Date: Mon, 18 Jan 2010 13:01:36 +0100 Subject: drm/radeon: r6xx/r7xx possible security issue, system ram access This patch workaround a possible security issue which can allow user to abuse drm on r6xx/r7xx hw to access any system ram memory. This patch doesn't break userspace, it detect "valid" old use of CB_COLOR[0-7]_FRAG & CB_COLOR[0-7]_TILE registers and overwritte the address these registers are pointing to with the one of the last color buffer. This workaround will work for old mesa & xf86-video-ati and any old user which did use similar register programming pattern as those (we expect that there is no others user of those ioctl except possibly a malicious one). This patch add a warning if it detects such usage, warning encourage people to update their mesa & xf86-video-ati. New userspace will submit proper relocation. Fix for xf86-video-ati / mesa (this kernel patch is enough to prevent abuse, fix for userspace are to set proper cs stream and avoid kernel warning) : http://cgit.freedesktop.org/xorg/driver/xf86-video-ati/commit/?id=95d63e408cc88b6934bec84a0b1ef94dfe8bee7b http://cgit.freedesktop.org/mesa/mesa/commit/?id=46dc6fd3ed5ef96cda53641a97bc68c3bc104a9f Abusing this register to perform system ram memory is not easy, here is outline on how it could be achieve. First attacker must have access to the drm device and be able to submit command stream throught cs ioctl. Then attacker must build a proper command stream for r6xx/r7xx hw which will abuse the FRAG or TILE buffer to overwrite the GPU GART which is in VRAM. To achieve so attacker as to setup CB_COLOR[0-7]_FRAG or CB_COLOR[0-7]_TILE to point to the GPU GART, then it has to find a way to write predictable value into those buffer (with little cleverness i believe this can be done but this is an hard task). Once attacker have such program it can overwritte GPU GART to program GPU gart to point anywhere in system memory. It then can reusse same method as he used to reprogram GART to overwritte the system ram through the GART mapping. In the process the attacker has to be carefull to not overwritte any sensitive area of the GART table, like ring or IB gart entry as it will more then likely lead to GPU lockup. Bottom line is that i think it's very hard to use this flaw to get system ram access but in theory one can achieve so. Side note: I am not aware of anyone ever using the GPU as an attack vector, nevertheless we take great care in the opensource driver to try to detect and forbid malicious use of GPU. I don't think the closed source driver are as cautious as we are. Signed-off-by: Jerome Glisse Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 1 + 1 file changed, 1 insertion(+) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 65590a0f1d93..1496cb8658ef 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -231,6 +231,7 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) memset(&parser, 0, sizeof(struct radeon_cs_parser)); parser.filp = filp; parser.rdev = rdev; + parser.dev = rdev->dev; r = radeon_cs_parser_init(&parser, data); if (r) { DRM_ERROR("Failed to initialize parser !\n"); -- cgit v1.2.2 From 17aafccab4352b422aa01fa6ebf82daff693a5b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michel=20D=C3=A4nzer?= Date: Fri, 22 Jan 2010 09:20:00 +0100 Subject: drm/radeon/kms: Fix oops after radeon_cs_parser_init() failure. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If radeon_cs_parser_init() fails, radeon_cs_ioctl() calls radeon_cs_parser_fini() with the non-zero error value. The latter dereferenced parser->ib which hasn't been initialized yet -> boom. Add a test for parser->ib being non-NULL before dereferencing it. Signed-off-by: Michel Dänzer Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 1496cb8658ef..1190148cf5e6 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -189,7 +189,7 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error) { unsigned i; - if (error) { + if (error && parser->ib) { radeon_bo_list_unvalidate(&parser->validated, parser->ib->fence); } else { -- cgit v1.2.2 From bc9025bdc4e2b591734cca17697093845007b63d Mon Sep 17 00:00:00 2001 From: Luca Barbieri Date: Tue, 9 Feb 2010 05:49:12 +0000 Subject: Use drm_gem_object_[handle_]unreference_unlocked where possible Mostly obvious simplifications. The i915 pread/pwrite ioctls, intel_overlay_put_image and nouveau_gem_new were incorrectly using the locked versions without locking: this is also fixed in this patch. Signed-off-by: Luca Barbieri Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 1190148cf5e6..da59f5e78e09 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -196,11 +196,8 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error) radeon_bo_list_unreserve(&parser->validated); } for (i = 0; i < parser->nrelocs; i++) { - if (parser->relocs[i].gobj) { - mutex_lock(&parser->rdev->ddev->struct_mutex); - drm_gem_object_unreference(parser->relocs[i].gobj); - mutex_unlock(&parser->rdev->ddev->struct_mutex); - } + if (parser->relocs[i].gobj) + drm_gem_object_unreference_unlocked(parser->relocs[i].gobj); } kfree(parser->track); kfree(parser->relocs); -- cgit v1.2.2 From 94429bb6c8343722544e282d89dc4638672e49b4 Mon Sep 17 00:00:00 2001 From: Jerome Glisse Date: Mon, 15 Feb 2010 21:36:33 +0100 Subject: drm/radeon/kms: fix bo's fence association Previous code did associate fence to bo before the fence was emited and it also didn't lock protected access to ttm sync_obj member. Both of this flaw leads to possible race between different code path. This patch fix this by associating fence only once the fence is emitted and properly lock protect access to sync_obj member of ttm. Fix: https://bugs.freedesktop.org/show_bug.cgi?id=26438 and likely similar others bugs Signed-off-by: Jerome Glisse Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 1190148cf5e6..e9d085021c1f 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -86,7 +86,7 @@ int radeon_cs_parser_relocs(struct radeon_cs_parser *p) &p->validated); } } - return radeon_bo_list_validate(&p->validated, p->ib->fence); + return radeon_bo_list_validate(&p->validated); } int radeon_cs_parser_init(struct radeon_cs_parser *p, void *data) @@ -189,12 +189,10 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error) { unsigned i; - if (error && parser->ib) { - radeon_bo_list_unvalidate(&parser->validated, - parser->ib->fence); - } else { - radeon_bo_list_unreserve(&parser->validated); + if (!error && parser->ib) { + radeon_bo_list_fence(&parser->validated, parser->ib->fence); } + radeon_bo_list_unreserve(&parser->validated); for (i = 0; i < parser->nrelocs; i++) { if (parser->relocs[i].gobj) { mutex_lock(&parser->rdev->ddev->struct_mutex); -- cgit v1.2.2 From 97f23b3d85a4d734a8584dade3a34579931c8f8d Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Fri, 19 Mar 2010 10:33:44 +1000 Subject: drm/radeon/kms: don't print error on -ERESTARTSYS. We can get this if the user moves the mouse when we are waiting to move some stuff around in the validate. Don't fail. Cc: stable@kernel.org Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 70ba02ed7723..dd190f9315d3 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -243,7 +243,8 @@ int radeon_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp) } r = radeon_cs_parser_relocs(&parser); if (r) { - DRM_ERROR("Failed to parse relocation !\n"); + if (r != -ERESTARTSYS) + DRM_ERROR("Failed to parse relocation %d!\n", r); radeon_cs_parser_fini(&parser, r); mutex_unlock(&rdev->cs_mutex); return r; -- cgit v1.2.2 From fcbc451ba1948fba967198bd150ecbd10bbb7075 Mon Sep 17 00:00:00 2001 From: Pauli Nieminen Date: Fri, 19 Mar 2010 07:44:33 +0000 Subject: drm/radeon/kms: Fix NULL pointer dereference if memory allocation failed. When there is allocation failure in radeon_cs_parser_relocs parser->nrelocs is not cleaned. This causes NULL pointer defeference in radeon_cs_parser_fini when clean up code is trying to loop over the relocation array and free the objects. Fix adds a check for a possible NULL pointer in clean up code. Signed-off-by: Pauli Nieminen Cc: stable@kernel.org Signed-off-by: Dave Airlie --- drivers/gpu/drm/radeon/radeon_cs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'drivers/gpu/drm/radeon/radeon_cs.c') diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index dd190f9315d3..f9b0fe002c0a 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -193,9 +193,11 @@ static void radeon_cs_parser_fini(struct radeon_cs_parser *parser, int error) radeon_bo_list_fence(&parser->validated, parser->ib->fence); } radeon_bo_list_unreserve(&parser->validated); - for (i = 0; i < parser->nrelocs; i++) { - if (parser->relocs[i].gobj) - drm_gem_object_unreference_unlocked(parser->relocs[i].gobj); + if (parser->relocs != NULL) { + for (i = 0; i < parser->nrelocs; i++) { + if (parser->relocs[i].gobj) + drm_gem_object_unreference_unlocked(parser->relocs[i].gobj); + } } kfree(parser->track); kfree(parser->relocs); -- cgit v1.2.2