Eric Anholt discovered a race condition in the radeon drm driver that might allow local priv escalation (read it would take a fair bit of work and maybe the radeon docs...., but possible), I've put the patch along with an ioctl bounds check fix into my tree at bk://drm.bkbits.net/drm-linus I think this should go in before 2.6.11, I'm not sure if warning the distros is necessary, but I've cc'ed vendor-sec... 2.4 might need a fix but I've no idea about the DRM in 2.4....
---- Woops, the first bit the ioctl fix is only recent since core, the second bit radeon one (the bad one..) goes back to day one but may have been made worse by bitkeeper v1.25 of radeon_state.c, but I think it goes the whole way back... diff -Nru a/drivers/char/drm/drm_drv.c b/drivers/char/drm/drm_drv.c --- a/drivers/char/drm/drm_drv.c 2005-02-09 10:01:56 +11:00 +++ b/drivers/char/drm/drm_drv.c 2005-02-09 10:01:56 +11:00 @@ -516,7 +516,7 @@ if (nr < DRIVER_IOCTL_COUNT) ioctl = &drm_ioctls[nr]; - else if ((nr >= DRM_COMMAND_BASE) || (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) + else if ((nr >= DRM_COMMAND_BASE) && (nr < DRM_COMMAND_BASE + dev->driver->num_ioctls)) ioctl = &dev->driver->ioctls[nr - DRM_COMMAND_BASE]; else goto err_i1; diff -Nru a/drivers/char/drm/drm_os_linux.h b/drivers/char/drm/drm_os_linux.h --- a/drivers/char/drm/drm_os_linux.h 2005-02-09 10:01:56 +11:00 +++ b/drivers/char/drm/drm_os_linux.h 2005-02-09 10:01:56 +11:00 @@ -96,9 +96,6 @@ __copy_to_user(arg1, arg2, arg3) #define DRM_GET_USER_UNCHECKED(val, uaddr) \ __get_user(val, uaddr) -#define DRM_PUT_USER_UNCHECKED(uaddr, val) \ - __put_user(val, uaddr) - #define DRM_GET_PRIV_WITH_RETURN(_priv, _filp) _priv = _filp->private_data diff -Nru a/drivers/char/drm/radeon_drv.h b/drivers/char/drm/radeon_drv.h --- a/drivers/char/drm/radeon_drv.h 2005-02-09 10:01:56 +11:00 +++ b/drivers/char/drm/radeon_drv.h 2005-02-09 10:01:56 +11:00 @@ -1027,25 +1027,27 @@ } while (0) -#define OUT_RING_USER_TABLE( tab, sz ) do { \ +#define OUT_RING_TABLE( tab, sz ) do { \ int _size = (sz); \ - int __user *_tab = (tab); \ + int *_tab = (int *)(tab); \ \ if (write + _size > mask) { \ - int i = (mask+1) - write; \ - if (DRM_COPY_FROM_USER_UNCHECKED( (int *)(ring+write), \ - _tab, i*4 )) \ - return DRM_ERR(EFAULT); \ + int _i = (mask+1) - write; \ + _size -= _i; \ + while (_i > 0 ) { \ + *(int *)(ring + write) = *_tab++; \ + write++; \ + _i--; \ + } \ write = 0; \ - _size -= i; \ - _tab += i; \ + _tab += _i; \ } \ \ - if (_size && DRM_COPY_FROM_USER_UNCHECKED( (int *)(ring+write), \ - _tab, _size*4 )) \ - return DRM_ERR(EFAULT); \ - \ - write += _size; \ + while (_size > 0) { \ + *(ring + write) = *_tab++; \ + write++; \ + _size--; \ + } \ write &= mask; \ } while (0) diff -Nru a/drivers/char/drm/radeon_state.c b/drivers/char/drm/radeon_state.c --- a/drivers/char/drm/radeon_state.c 2005-02-09 10:01:56 +11:00 +++ b/drivers/char/drm/radeon_state.c 2005-02-09 10:01:56 +11:00 @@ -93,21 +93,6 @@ return 0; } -static __inline__ int radeon_check_and_fixup_offset_user( drm_radeon_private_t *dev_priv, - drm_file_t *filp_priv, - u32 __user *offset ) { - u32 off; - - DRM_GET_USER_UNCHECKED( off, offset ); - - if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, &off ) ) - return DRM_ERR( EINVAL ); - - DRM_PUT_USER_UNCHECKED( offset, off ); - - return 0; -} - static __inline__ int radeon_check_and_fixup_packets( drm_radeon_private_t *dev_priv, drm_file_t *filp_priv, int id, @@ -115,18 +100,18 @@ switch ( id ) { case RADEON_EMIT_PP_MISC: - if ( radeon_check_and_fixup_offset_user( dev_priv, filp_priv, - &data[( RADEON_RB3D_DEPTHOFFSET - - RADEON_PP_MISC ) / 4] ) ) { + if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, + &data[( RADEON_RB3D_DEPTHOFFSET + - RADEON_PP_MISC ) / 4] ) ) { DRM_ERROR( "Invalid depth buffer offset\n" ); return DRM_ERR( EINVAL ); } break; case RADEON_EMIT_PP_CNTL: - if ( radeon_check_and_fixup_offset_user( dev_priv, filp_priv, - &data[( RADEON_RB3D_COLOROFFSET - - RADEON_PP_CNTL ) / 4] ) ) { + if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, + &data[( RADEON_RB3D_COLOROFFSET + - RADEON_PP_CNTL ) / 4] ) ) { DRM_ERROR( "Invalid colour buffer offset\n" ); return DRM_ERR( EINVAL ); } @@ -138,8 +123,8 @@ case R200_EMIT_PP_TXOFFSET_3: case R200_EMIT_PP_TXOFFSET_4: case R200_EMIT_PP_TXOFFSET_5: - if ( radeon_check_and_fixup_offset_user( dev_priv, filp_priv, - &data[0] ) ) { + if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, + &data[0] ) ) { DRM_ERROR( "Invalid R200 texture offset\n" ); return DRM_ERR( EINVAL ); } @@ -148,9 +133,9 @@ case RADEON_EMIT_PP_TXFILTER_0: case RADEON_EMIT_PP_TXFILTER_1: case RADEON_EMIT_PP_TXFILTER_2: - if ( radeon_check_and_fixup_offset_user( dev_priv, filp_priv, - &data[( RADEON_PP_TXOFFSET_0 - - RADEON_PP_TXFILTER_0 ) / 4] ) ) { + if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, + &data[( RADEON_PP_TXOFFSET_0 + - RADEON_PP_TXFILTER_0 ) / 4] ) ) { DRM_ERROR( "Invalid R100 texture offset\n" ); return DRM_ERR( EINVAL ); } @@ -164,9 +149,8 @@ case R200_EMIT_PP_CUBIC_OFFSETS_5: { int i; for ( i = 0; i < 5; i++ ) { - if ( radeon_check_and_fixup_offset_user( dev_priv, - filp_priv, - &data[i] ) ) { + if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, + &data[i] ) ) { DRM_ERROR( "Invalid R200 cubic texture offset\n" ); return DRM_ERR( EINVAL ); } @@ -250,17 +234,11 @@ drm_file_t *filp_priv, drm_radeon_cmd_buffer_t *cmdbuf, unsigned int *cmdsz ) { - u32 tmp[4]; - u32 __user *cmd = (u32 __user *)cmdbuf->buf; - - if ( DRM_COPY_FROM_USER_UNCHECKED( tmp, cmd, sizeof( tmp ) ) ) { - DRM_ERROR( "Failed to copy data from user space\n" ); - return DRM_ERR( EFAULT ); - } + u32 *cmd = (u32 *) cmdbuf->buf; - *cmdsz = 2 + ( ( tmp[0] & RADEON_CP_PACKET_COUNT_MASK ) >> 16 ); + *cmdsz = 2 + ( ( cmd[0] & RADEON_CP_PACKET_COUNT_MASK ) >> 16 ); - if ( ( tmp[0] & 0xc0000000 ) != RADEON_CP_PACKET3 ) { + if ( ( cmd[0] & 0xc0000000 ) != RADEON_CP_PACKET3 ) { DRM_ERROR( "Not a type 3 packet\n" ); return DRM_ERR( EINVAL ); } @@ -271,32 +249,27 @@ } /* Check client state and fix it up if necessary */ - if ( tmp[0] & 0x8000 ) { /* MSB of opcode: next DWORD GUI_CNTL */ + if ( cmd[0] & 0x8000 ) { /* MSB of opcode: next DWORD GUI_CNTL */ u32 offset; - if ( tmp[1] & ( RADEON_GMC_SRC_PITCH_OFFSET_CNTL + if ( cmd[1] & ( RADEON_GMC_SRC_PITCH_OFFSET_CNTL | RADEON_GMC_DST_PITCH_OFFSET_CNTL ) ) { - offset = tmp[2] << 10; + offset = cmd[2] << 10; if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, &offset ) ) { DRM_ERROR( "Invalid first packet offset\n" ); return DRM_ERR( EINVAL ); } - tmp[2] = ( tmp[2] & 0xffc00000 ) | offset >> 10; + cmd[2] = ( cmd[2] & 0xffc00000 ) | offset >> 10; } - if ( ( tmp[1] & RADEON_GMC_SRC_PITCH_OFFSET_CNTL ) && - ( tmp[1] & RADEON_GMC_DST_PITCH_OFFSET_CNTL ) ) { - offset = tmp[3] << 10; + if ( ( cmd[1] & RADEON_GMC_SRC_PITCH_OFFSET_CNTL ) && + ( cmd[1] & RADEON_GMC_DST_PITCH_OFFSET_CNTL ) ) { + offset = cmd[3] << 10; if ( radeon_check_and_fixup_offset( dev_priv, filp_priv, &offset ) ) { DRM_ERROR( "Invalid second packet offset\n" ); return DRM_ERR( EINVAL ); } - tmp[3] = ( tmp[3] & 0xffc00000 ) | offset >> 10; - } - - if ( DRM_COPY_TO_USER_UNCHECKED( cmd, tmp, sizeof( tmp ) ) ) { - DRM_ERROR( "Failed to copy data to user space\n" ); - return DRM_ERR( EFAULT ); + cmd[3] = ( cmd[3] & 0xffc00000 ) | offset >> 10; } } @@ -2473,7 +2446,7 @@ { int id = (int)header.packet.packet_id; int sz, reg; - int __user *data = (int __user *)cmdbuf->buf; + int *data = (int *)cmdbuf->buf; RING_LOCALS; if (id >= RADEON_MAX_STATE_PACKETS) @@ -2494,7 +2467,7 @@ BEGIN_RING(sz+1); OUT_RING( CP_PACKET0( reg, (sz-1) ) ); - OUT_RING_USER_TABLE( data, sz ); + OUT_RING_TABLE( data, sz ); ADVANCE_RING(); cmdbuf->buf += sz * sizeof(int); @@ -2508,7 +2481,6 @@ drm_radeon_cmd_buffer_t *cmdbuf ) { int sz = header.scalars.count; - int __user *data = (int __user *)cmdbuf->buf; int start = header.scalars.offset; int stride = header.scalars.stride; RING_LOCALS; @@ -2517,7 +2489,7 @@ OUT_RING( CP_PACKET0( RADEON_SE_TCL_SCALAR_INDX_REG, 0 ) ); OUT_RING( start | (stride << RADEON_SCAL_INDX_DWORD_STRIDE_SHIFT)); OUT_RING( CP_PACKET0_TABLE( RADEON_SE_TCL_SCALAR_DATA_REG, sz-1 ) ); - OUT_RING_USER_TABLE( data, sz ); + OUT_RING_TABLE( cmdbuf->buf, sz ); ADVANCE_RING(); cmdbuf->buf += sz * sizeof(int); cmdbuf->bufsz -= sz * sizeof(int); @@ -2532,7 +2504,6 @@ drm_radeon_cmd_buffer_t *cmdbuf ) { int sz = header.scalars.count; - int __user *data = (int __user *)cmdbuf->buf; int start = ((unsigned int)header.scalars.offset) + 0x100; int stride = header.scalars.stride; RING_LOCALS; @@ -2541,7 +2512,7 @@ OUT_RING( CP_PACKET0( RADEON_SE_TCL_SCALAR_INDX_REG, 0 ) ); OUT_RING( start | (stride << RADEON_SCAL_INDX_DWORD_STRIDE_SHIFT)); OUT_RING( CP_PACKET0_TABLE( RADEON_SE_TCL_SCALAR_DATA_REG, sz-1 ) ); - OUT_RING_USER_TABLE( data, sz ); + OUT_RING_TABLE( cmdbuf->buf, sz ); ADVANCE_RING(); cmdbuf->buf += sz * sizeof(int); cmdbuf->bufsz -= sz * sizeof(int); @@ -2554,7 +2525,6 @@ drm_radeon_cmd_buffer_t *cmdbuf ) { int sz = header.vectors.count; - int __user *data = (int __user *)cmdbuf->buf; int start = header.vectors.offset; int stride = header.vectors.stride; RING_LOCALS; @@ -2563,7 +2533,7 @@ OUT_RING( CP_PACKET0( RADEON_SE_TCL_VECTOR_INDX_REG, 0 ) ); OUT_RING( start | (stride << RADEON_VEC_INDX_OCTWORD_STRIDE_SHIFT)); OUT_RING( CP_PACKET0_TABLE( RADEON_SE_TCL_VECTOR_DATA_REG, (sz-1) ) ); - OUT_RING_USER_TABLE( data, sz ); + OUT_RING_TABLE( cmdbuf->buf, sz ); ADVANCE_RING(); cmdbuf->buf += sz * sizeof(int); @@ -2578,7 +2548,6 @@ { drm_radeon_private_t *dev_priv = dev->dev_private; unsigned int cmdsz; - int __user *cmd = (int __user *)cmdbuf->buf; int ret; RING_LOCALS; @@ -2591,7 +2560,7 @@ } BEGIN_RING( cmdsz ); - OUT_RING_USER_TABLE( cmd, cmdsz ); + OUT_RING_TABLE( cmdbuf->buf, cmdsz ); ADVANCE_RING(); cmdbuf->buf += cmdsz * 4; @@ -2608,7 +2577,6 @@ drm_radeon_private_t *dev_priv = dev->dev_private; drm_clip_rect_t box; unsigned int cmdsz; - int __user *cmd = (int __user *)cmdbuf->buf; int ret; drm_clip_rect_t __user *boxes = cmdbuf->boxes; int i = 0; @@ -2627,7 +2595,7 @@ do { if ( i < cmdbuf->nbox ) { - if (DRM_COPY_FROM_USER_UNCHECKED( &box, &boxes[i], sizeof(box) )) + if (DRM_COPY_FROM_USER( &box, &boxes[i], sizeof(box) )) return DRM_ERR(EFAULT); /* FIXME The second and subsequent times round * this loop, send a WAIT_UNTIL_3D_IDLE before @@ -2650,7 +2618,7 @@ } BEGIN_RING( cmdsz ); - OUT_RING_USER_TABLE( cmd, cmdsz ); + OUT_RING_TABLE( cmdbuf->buf, cmdsz ); ADVANCE_RING(); } while ( ++i < cmdbuf->nbox ); @@ -2703,7 +2671,8 @@ int idx; drm_radeon_cmd_buffer_t cmdbuf; drm_radeon_cmd_header_t header; - int orig_nbox; + int orig_nbox, orig_bufsz; + char *kbuf=NULL; LOCK_TEST_WITH_RETURN( dev, filp ); @@ -2720,24 +2689,29 @@ RING_SPACE_TEST_WITH_RETURN( dev_priv ); VB_AGE_TEST_WITH_RETURN( dev_priv ); + if (cmdbuf.bufsz > 64*1024 || cmdbuf.bufsz<0) { + return DRM_ERR(EINVAL); + } - if (DRM_VERIFYAREA_READ( cmdbuf.buf, cmdbuf.bufsz )) - return DRM_ERR(EFAULT); - - if (cmdbuf.nbox && - DRM_VERIFYAREA_READ(cmdbuf.boxes, - cmdbuf.nbox * sizeof(drm_clip_rect_t))) - return DRM_ERR(EFAULT); + /* Allocate an in-kernel area and copy in the cmdbuf. Do this to avoid + * races between checking values and using those values in other code, + * and simply to avoid a lot of function calls to copy in data. + */ + orig_bufsz = cmdbuf.bufsz; + if (orig_bufsz != 0) { + kbuf = drm_alloc(cmdbuf.bufsz, DRM_MEM_DRIVER); + if (kbuf == NULL) + return DRM_ERR(ENOMEM); + if (DRM_COPY_FROM_USER(kbuf, cmdbuf.buf, cmdbuf.bufsz)) + return DRM_ERR(EFAULT); + cmdbuf.buf = kbuf; + } orig_nbox = cmdbuf.nbox; while ( cmdbuf.bufsz >= sizeof(header) ) { - - if (DRM_GET_USER_UNCHECKED( header.i, (int __user *)cmdbuf.buf )) { - DRM_ERROR("__get_user %p\n", cmdbuf.buf); - return DRM_ERR(EFAULT); - } + header.i = *(int *)cmdbuf.buf; cmdbuf.buf += sizeof(header); cmdbuf.bufsz -= sizeof(header); @@ -2746,7 +2720,7 @@ DRM_DEBUG("RADEON_CMD_PACKET\n"); if (radeon_emit_packets( dev_priv, filp_priv, header, &cmdbuf )) { DRM_ERROR("radeon_emit_packets failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2754,7 +2728,7 @@ DRM_DEBUG("RADEON_CMD_SCALARS\n"); if (radeon_emit_scalars( dev_priv, header, &cmdbuf )) { DRM_ERROR("radeon_emit_scalars failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2762,7 +2736,7 @@ DRM_DEBUG("RADEON_CMD_VECTORS\n"); if (radeon_emit_vectors( dev_priv, header, &cmdbuf )) { DRM_ERROR("radeon_emit_vectors failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2772,14 +2746,14 @@ if ( idx < 0 || idx >= dma->buf_count ) { DRM_ERROR( "buffer index %d (of %d max)\n", idx, dma->buf_count - 1 ); - return DRM_ERR(EINVAL); + goto err; } buf = dma->buflist[idx]; if ( buf->filp != filp || buf->pending ) { DRM_ERROR( "bad buffer %p %p %d\n", buf->filp, filp, buf->pending); - return DRM_ERR(EINVAL); + goto err; } radeon_cp_discard_buffer( dev, buf ); @@ -2789,7 +2763,7 @@ DRM_DEBUG("RADEON_CMD_PACKET3\n"); if (radeon_emit_packet3( dev, filp_priv, &cmdbuf )) { DRM_ERROR("radeon_emit_packet3 failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2797,7 +2771,7 @@ DRM_DEBUG("RADEON_CMD_PACKET3_CLIP\n"); if (radeon_emit_packet3_cliprect( dev, filp_priv, &cmdbuf, orig_nbox )) { DRM_ERROR("radeon_emit_packet3_clip failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2805,7 +2779,7 @@ DRM_DEBUG("RADEON_CMD_SCALARS2\n"); if (radeon_emit_scalars2( dev_priv, header, &cmdbuf )) { DRM_ERROR("radeon_emit_scalars2 failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; @@ -2813,21 +2787,28 @@ DRM_DEBUG("RADEON_CMD_WAIT\n"); if (radeon_emit_wait( dev, header.wait.flags )) { DRM_ERROR("radeon_emit_wait failed\n"); - return DRM_ERR(EINVAL); + goto err; } break; default: DRM_ERROR("bad cmd_type %d at %p\n", header.header.cmd_type, cmdbuf.buf - sizeof(header)); - return DRM_ERR(EINVAL); + goto err; } } + if (orig_bufsz != 0) + drm_free(kbuf, orig_bufsz, DRM_MEM_DRIVER); DRM_DEBUG("DONE\n"); COMMIT_RING(); return 0; + +err: + if (orig_bufsz != 0) + drm_free(kbuf, orig_bufsz, DRM_MEM_DRIVER); + return DRM_ERR(EINVAL); }
Frmo Ubuntu's latest: Eric Anholt discovered a race condition in the Radeon DRI driver. In some cases this allowed a local user with DRI privileges on a Radeon card to execute arbitrary code with root privileges.
Mass-Ccing kern-sec@gentoo.org to make sure Kernel Security guys know about all of these...
gentoo-dev-sources is unaffected
Created attachment 56375 [details, diff] 2.6 Patch
This is CAN-2005-0767: Race condition in the Radeon DRI driver for Linux kernel 2.6.8.1 allows local users with DRI privileges to execute arbitrary code as root
All fixed, closing bug.
