On Tuesday 21 June 2005 20:57, Vladimir Dergachev wrote: > Now that the driver paints usable pictures without lockups on many cards, > including AGP versions of X800 and Mobility M10, it would make sense to > ready it for inclusion into main DRI codebase. > > I do not think that elusive lockups of Radeon 9800 cards, or issues with > PowerPC will require any drastic changes. > > As we discussed earlier, the major reason against inclusion into > mainstream DRI CVS is that the driver is not secure in its current state. > > Below, I will attempt to list current known issues - please reply with > your additions. > > * r300_emit_unchecked_state - it is not as unchecked as it has been > initially, however a few poorly checked registers remain:
Those poorly checked registers should be moved out of unchecked_state and
into its own function. Adding these checks into unchecked_state will just
add overhead to what should be a fast path.
The idea would be to add something like r300_emit_special_state which
doesn't use register addresses but has subcommands like the state setting
for radeon/r200.
> from r300_cmdbuf.c:
>
> ADD_RANGE(R300_RB3D_COLOROFFSET0, 1); /* Dangerous */
> ADD_RANGE(R300_RB3D_COLORPITCH0, 1); /* Dangerous */
> /* .. snip ... */
> ADD_RANGE(R300_RB3D_DEPTHOFFSET, 2); /* Dangerous */
>
> In principle an attacker can set these to point to AGP or system
> RAM and then cause a paint operation to overwrite particular
> memory range.
>
> Ideally we should check that these point inside the framebuffer,
> i.e. are within range specified by MC_FB_LOCATION register.
Right. Actually, to be on the safe side, we'd have to set min/max clipping
rects at the same time as setting those buffer offsets. This is currently
not a problem, but it will become one when (if? ;)) we implement
framebuffer_object etc.
> /* Texture offset is dangerous and needs more checking */
> ADD_RANGE(R300_TX_OFFSET_0, 16);
>
> I don't think texture offsets are ever written to, however if
they
> point in the wrong place they can be used to read memory
directly.
Setting those texture offsets wrong can actually lock up the machine as I
found out when I temporarily put MC_FB_LOCATION into its natural position
(i.e. where it's put on older radeons).
> ideally we would check these to be either with MC_FB_LOCATION
> or MC_AGP_LOCATION ranges. Problem is what do we do on PCI
cards ?
> use AIC controller settings ?
Unfortunately, I don't know enough about PCI to comment, and what's AIC
anyway? I've seen some register names with AIC in it, but they don't really
seem to be used.
> * r300_emit_raw - we do not have code that checks any of bufferred 3d
> packets, in particular VBUF_2, IMMD_2, INDX_2 and INDX_BUFFER.
>
> I think that none of these can be exploited except to cause a lockup
-
> please correct me if I am wrong
>
> * r300_emit_raw - RADEON_3D_LOAD_VBPNTR - this sets offsets and so
> like texture offset registers could be exploited to read protected
> memory locations.
>
> Again, we need to check the offsets against something reasonable.
Note that by putting the offset at the end of allowed memory and setting the
number of vertices very high, you could read memory that you shouldn't have
access to.
But the more important thing is: What's up with r300_emit_raw anyway? It was
originally supposed to do what the name suggests: Emit raw data into the
ring buffer, purely as a hack for experimentation.
People have bent it in extreme ways, so it has clearly gone beyond that, and
that's a Bad Thing. For one thing, r300_emit_raw doesn't get cliprects
right. If you have more than four cliprects, you need to emit rendering
commands multiple times.
Seriously, all the stuff that uses emit_raw should just be migrated to use
r300_emit_packet3, which will clean this up a lot.
> * anything I forgot ?
Talking about security, have a look at radeon_state.c. Where on earth does
*this* come from:
/* 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)) {
drm_free(kbuf, orig_bufsz, DRM_MEM_DRIVER);
return DRM_ERR(EFAULT);
}
cmdbuf.buf = kbuf;
}
This just shouts insanity. It calls kmalloc every single time you emit a
command buffer!
The security issue mentioned in the comment is real, but there's a really
simple rule of thumb that eliminates it completely:
Never copy the same memory from user space twice.
The problem is that if you copy from userspace once to verify the commands
and then copy it again to push it to the card, you have a nice race
condition (which would be difficult, but not impossible, to exploit on
preempt or SMP systems) where userspace changes the commands retroactively
after the kernel does its verification.
r300_emit_raw (which should just die anyway) is actually unsafe in this
sense (although that bug is currently masked by the huge copy operation).
There's a point of using the single big copy is a performance improvement,
but I don't buy it (even if you get rid of the beast called kmalloc). It
should be possible to do the access checks once for the entire buffer, and
then just use the faster _unchecked functions to read the data. This saves
all the function calls (because it ends up inline) and it saves an
unnecessary copy operation.
cu,
Nicolai
pgphrj3bMffMi.pgp
Description: PGP signature
