Bug#403871: radeon DRM incorrectly rejects valid offsets in some cases
Package: linux-2.6
Severity: normal
Tags: patch
Subject: radeon DRM incorrectly rejects valid offsets in some cases
Package: linux-2.6
Severity: normal
Tags: patch
[ CC to stable@kernel.org for consideration of putting the attached fixes into
stable kernel releases ]
Note: Even though this bug is not release critical per se, I strongly suggest
considering to handle it for the etch release. Justification for this is in the
description below.
The radeon DRM performs checks on hardware offsets supplied from userspace, to
prevent unauthorized GPU access to memory outside of the framebuffer and GART
areas. Unfortunately, the initial versions of these checks contained an integer
overflow which would cause them to reject valid offsets when the framebuffer
area is located at the very end of the GPU's 32 bit address space. As this is
location can not be altered directly by the user (it's mostly derived from PCI
resource assignments), this effectively prevents 3D acceleration randomly. The
only remedy I know of on affected systems is to rebuild a kernel which has this
fixed, or at least the DRM from upstream drm git.
My initial fix for this was merged into kernel 2.6.19. Unfortunately, when I
made that fix, I missed a second r300 specific implementation of the same
functionality, which was broken in the same way. I fixed that by unifying both
implementations, but this fix is not available in any kernel tree yet (although
Dave Airlie is pushing it for 2.6.20).
I'm including both fixes for convenience. Thanks for your consideration.
-- System Information:
Debian Release: 4.0
APT prefers unstable
APT policy: (500, 'unstable'), (500, 'stable'), (102, 'experimental')
Architecture: powerpc (ppc)
Shell: /bin/sh linked to /bin/bash
Kernel: Linux 2.6.18-3-powerpc
Locale: LANG=de_CH.UTF-8, LC_CTYPE=de_CH.UTF-8 (charmap=UTF-8)
-- System Information:
Debian Release: 4.0
APT prefers unstable
APT policy: (500, 'unstable'), (500, 'stable'), (102, 'experimental'), (101, 'dapper-security'), (101, 'dapper')
Architecture: powerpc (ppc)
Shell: /bin/sh linked to /bin/bash
Kernel: Linux 2.6.18-3-powerpc
Locale: LANG=de_CH.UTF-8, LC_CTYPE=de_CH.UTF-8 (charmap=UTF-8)
commit b99e332236ca5fcc11e8d7c89566bbf3bcf959ee
Author: Michel Dänzer <michel@tungstengraphics.com>
Date: Sat Aug 26 12:21:11 2006 +0200
Bug #7595: Avoid u32 overflows in radeon_check_and_fixup_offset().
The overflows could cause valid offsets to get rejected under some
circumstances, e.g. when the framebuffer resides at the very end of the card's
address space.
diff --git a/shared-core/radeon_state.c b/shared-core/radeon_state.c
index 58251dd..38b8714 100644
--- a/shared-core/radeon_state.c
+++ b/shared-core/radeon_state.c
@@ -42,7 +42,11 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
drm_file_t * filp_priv,
u32 * offset)
{
- u32 off = *offset;
+ u64 off = *offset;
+ u32 fb_start = dev_priv->fb_location;
+ u32 fb_end = fb_start + dev_priv->fb_size - 1;
+ u32 gart_start = dev_priv->gart_vm_start;
+ u32 gart_end = gart_start + dev_priv->gart_size - 1;
struct drm_radeon_driver_file_fields *radeon_priv;
/* Hrm ... the story of the offset ... So this function converts
@@ -62,10 +66,8 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
/* First, the best case, the offset already lands in either the
* framebuffer or the GART mapped space
*/
- if ((off >= dev_priv->fb_location &&
- off < (dev_priv->fb_location + dev_priv->fb_size)) ||
- (off >= dev_priv->gart_vm_start &&
- off < (dev_priv->gart_vm_start + dev_priv->gart_size)))
+ if ((off >= fb_start && off <= fb_end) ||
+ (off >= gart_start && off <= gart_end))
return 0;
/* Ok, that didn't happen... now check if we have a zero based
@@ -78,16 +80,13 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
}
/* Finally, assume we aimed at a GART offset if beyond the fb */
- if (off > (dev_priv->fb_location + dev_priv->fb_size))
- off = off - (dev_priv->fb_location + dev_priv->fb_size) +
- dev_priv->gart_vm_start;
+ if (off > fb_end)
+ off = off - fb_end - 1 + gart_start;
/* Now recheck and fail if out of bounds */
- if ((off >= dev_priv->fb_location &&
- off < (dev_priv->fb_location + dev_priv->fb_size)) ||
- (off >= dev_priv->gart_vm_start &&
- off < (dev_priv->gart_vm_start + dev_priv->gart_size))) {
- DRM_DEBUG("offset fixed up to 0x%x\n", off);
+ if ((off >= fb_start && off <= fb_end) ||
+ (off >= gart_start && off <= gart_end)) {
+ DRM_DEBUG("offset fixed up to 0x%x\n", (unsigned int)off);
*offset = off;
return 0;
}
commit aefc7a34431a8f1540b261e23d8b8d05d824b60a
Author: Michel Dänzer <michel@tungstengraphics.com>
Date: Thu Dec 14 19:31:56 2006 +0100
Unify radeon offset checking.
Replace r300_check_offset() with generic radeon_check_offset(), which doesn't
reject valid offsets when the framebuffer area is at the very end of the card's
32 bit address space. Make radeon_check_and_fixup_offset() use
radeon_check_offset() as well.
This fixes https://bugs.freedesktop.org/show_bug.cgi?id=7697 .
diff --git a/shared-core/r300_cmdbuf.c b/shared-core/r300_cmdbuf.c
index c65ffd5..0c04b5f 100644
--- a/shared-core/r300_cmdbuf.c
+++ b/shared-core/r300_cmdbuf.c
@@ -242,26 +242,6 @@ static __inline__ int r300_check_range(unsigned reg, int count)
return 0;
}
-/*
- * we expect offsets passed to the framebuffer to be either within video
- * memory or within AGP space
- */
-static __inline__ int r300_check_offset(drm_radeon_private_t *dev_priv,
- u32 offset)
-{
- /* we realy want to check against end of video aperture
- but this value is not being kept.
- This code is correct for now (does the same thing as the
- code that sets MC_FB_LOCATION) in radeon_cp.c */
- if (offset >= dev_priv->fb_location &&
- offset < (dev_priv->fb_location + dev_priv->fb_size))
- return 0;
- if (offset >= dev_priv->gart_vm_start &&
- offset < (dev_priv->gart_vm_start + dev_priv->gart_size))
- return 0;
- return 1;
-}
-
static __inline__ int r300_emit_carefully_checked_packet0(drm_radeon_private_t *
dev_priv,
drm_radeon_kcmd_buffer_t
@@ -290,7 +270,7 @@ static __inline__ int r300_emit_carefully_checked_packet0(drm_radeon_private_t *
case MARK_SAFE:
break;
case MARK_CHECK_OFFSET:
- if (r300_check_offset(dev_priv, (u32) values[i])) {
+ if (!radeon_check_offset(dev_priv, (u32) values[i])) {
DRM_ERROR
("Offset failed range check (reg=%04x sz=%d)\n",
reg, sz);
@@ -452,7 +432,7 @@ static __inline__ int r300_emit_3d_load_vbpntr(drm_radeon_private_t *dev_priv,
i = 1;
while ((k < narrays) && (i < (count + 1))) {
i++; /* skip attribute field */
- if (r300_check_offset(dev_priv, payload[i])) {
+ if (!radeon_check_offset(dev_priv, payload[i])) {
DRM_ERROR
("Offset failed range check (k=%d i=%d) while processing 3D_LOAD_VBPNTR packet.\n",
k, i);
@@ -463,7 +443,7 @@ static __inline__ int r300_emit_3d_load_vbpntr(drm_radeon_private_t *dev_priv,
if (k == narrays)
break;
/* have one more to process, they come in pairs */
- if (r300_check_offset(dev_priv, payload[i])) {
+ if (!radeon_check_offset(dev_priv, payload[i])) {
DRM_ERROR
("Offset failed range check (k=%d i=%d) while processing 3D_LOAD_VBPNTR packet.\n",
k, i);
@@ -508,7 +488,7 @@ static __inline__ int r300_emit_bitblt_multi(drm_radeon_private_t *dev_priv,
if (cmd[1] & (RADEON_GMC_SRC_PITCH_OFFSET_CNTL
| RADEON_GMC_DST_PITCH_OFFSET_CNTL)) {
offset = cmd[2] << 10;
- ret = r300_check_offset(dev_priv, offset);
+ ret = !radeon_check_offset(dev_priv, offset);
if (ret) {
DRM_ERROR("Invalid bitblt first offset is %08X\n", offset);
return DRM_ERR(EINVAL);
@@ -518,7 +498,7 @@ static __inline__ int r300_emit_bitblt_multi(drm_radeon_private_t *dev_priv,
if ((cmd[1] & RADEON_GMC_SRC_PITCH_OFFSET_CNTL) &&
(cmd[1] & RADEON_GMC_DST_PITCH_OFFSET_CNTL)) {
offset = cmd[3] << 10;
- ret = r300_check_offset(dev_priv, offset);
+ ret = !radeon_check_offset(dev_priv, offset);
if (ret) {
DRM_ERROR("Invalid bitblt second offset is %08X\n", offset);
return DRM_ERR(EINVAL);
@@ -551,7 +531,7 @@ static __inline__ int r300_emit_indx_buffer(drm_radeon_private_t *dev_priv,
DRM_ERROR("Invalid indx_buffer reg address %08X\n", cmd[1]);
return DRM_ERR(EINVAL);
}
- ret = r300_check_offset(dev_priv, cmd[2]);
+ ret = !radeon_check_offset(dev_priv, cmd[2]);
if (ret) {
DRM_ERROR("Invalid indx_buffer offset is %08X\n", cmd[2]);
return DRM_ERR(EINVAL);
diff --git a/shared-core/radeon_drv.h b/shared-core/radeon_drv.h
index 6ea2a17..5c426fe 100644
--- a/shared-core/radeon_drv.h
+++ b/shared-core/radeon_drv.h
@@ -306,6 +306,21 @@ extern int radeon_no_wb;
extern drm_ioctl_desc_t radeon_ioctls[];
extern int radeon_max_ioctl;
+/* Check whether the given hardware address is inside the framebuffer or the
+ * GART area.
+ */
+static __inline__ int radeon_check_offset(drm_radeon_private_t *dev_priv,
+ u64 off)
+{
+ u32 fb_start = dev_priv->fb_location;
+ u32 fb_end = fb_start + dev_priv->fb_size - 1;
+ u32 gart_start = dev_priv->gart_vm_start;
+ u32 gart_end = gart_start + dev_priv->gart_size - 1;
+
+ return ((off >= fb_start && off <= fb_end) ||
+ (off >= gart_start && off <= gart_end));
+}
+
/* radeon_cp.c */
extern int radeon_cp_init(DRM_IOCTL_ARGS);
extern int radeon_cp_start(DRM_IOCTL_ARGS);
diff --git a/shared-core/radeon_state.c b/shared-core/radeon_state.c
index bf5e3d2..40b7d6c 100644
--- a/shared-core/radeon_state.c
+++ b/shared-core/radeon_state.c
@@ -43,10 +43,7 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
u32 * offset)
{
u64 off = *offset;
- u32 fb_start = dev_priv->fb_location;
- u32 fb_end = fb_start + dev_priv->fb_size - 1;
- u32 gart_start = dev_priv->gart_vm_start;
- u32 gart_end = gart_start + dev_priv->gart_size - 1;
+ u32 fb_end = dev_priv->fb_location + dev_priv->fb_size - 1;
struct drm_radeon_driver_file_fields *radeon_priv;
/* Hrm ... the story of the offset ... So this function converts
@@ -66,8 +63,7 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
/* First, the best case, the offset already lands in either the
* framebuffer or the GART mapped space
*/
- if ((off >= fb_start && off <= fb_end) ||
- (off >= gart_start && off <= gart_end))
+ if (radeon_check_offset(dev_priv, off))
return 0;
/* Ok, that didn't happen... now check if we have a zero based
@@ -81,11 +77,10 @@ static __inline__ int radeon_check_and_fixup_offset(drm_radeon_private_t *
/* Finally, assume we aimed at a GART offset if beyond the fb */
if (off > fb_end)
- off = off - fb_end - 1 + gart_start;
+ off = off - fb_end - 1 + dev_priv->gart_vm_start;
/* Now recheck and fail if out of bounds */
- if ((off >= fb_start && off <= fb_end) ||
- (off >= gart_start && off <= gart_end)) {
+ if (radeon_check_offset(dev_priv, off)) {
DRM_DEBUG("offset fixed up to 0x%x\n", (unsigned int)off);
*offset = off;
return 0;
Reply to: