Both of these bugs look exploitable. The vt.c patch is
self-explanatory.
In gamma_dma.c, argument "d" to gamma_dma_priority() points to a
structure copied from userspace (see gamma_dma()). That means that
d->send_indices is a pointer under user control, so it shouldn't be
dereferenced. The patch just safely copies the contents to a kernel
buffer and uses that instead. Ditto for d->send_sizes.
Also, I notice the drm code uses it's own memory allocation wrappers. I
don't know all the details of the drm code, so I just used kmalloc.
You'll probably want to change those two calls after applying the
patch. Sorry for the inconvenience.
Thanks for looking at this, and let me know if you have any questions.
Best,
Rob
P.S. Both of these bugs were found using the source code verification
tool, CQual, developed by Jeff Foster, myself, and others, and available
from http://www.cs.umd.edu/~jfoster/cqual/.
--- drivers/char/vt.c.orig Thu Jan 8 10:53:01 2004
+++ drivers/char/vt.c Wed Jan 7 15:22:17 2004
@@ -288,7 +288,7 @@
case KDGKBSENT:
sz = sizeof(tmp.kb_string) - 1; /* sz should have been
a struct member */
- q = user_kdgkb->kb_string;
+ q = tmp.kb_string;
p = func_table[i];
if(p)
for ( ; *p && sz; p++, sz--)
--- drivers/char/drm/gamma_dma.c.orig Thu Jan 8 10:56:47 2004
+++ drivers/char/drm/gamma_dma.c Wed Jan 7 19:21:57 2004
@@ -352,6 +352,8 @@
drm_buf_t *buf;
drm_buf_t *last_buf = NULL;
drm_device_dma_t *dma = dev->dma;
+ int *drm_send_indices = NULL;
+ int *drm_send_sizes = NULL;
DECLARE_WAITQUEUE(entry, current);
/* Turn off interrupt handling */
@@ -371,11 +373,27 @@
++must_free;
}
+ drm_send_indices = kmalloc (d->send_count * sizeof(*drm_send_indices), GFP_KERNEL);
+ drm_send_sizes = kmalloc (d->send_count * sizeof(*drm_send_sizes), GFP_KERNEL);
+ if (! drm_send_indices || ! drm_send_sizes)
+ {
+ retcode = -ENOMEM;
+ goto cleanup;
+ }
+ if (copy_from_user(drm_send_indices, d->send_indices,
+ d->send_count * sizeof(*drm_send_indices)) ||
+ copy_from_user(drm_send_sizes, d->send_sizes,
+ d->send_count * sizeof(*drm_send_sizes)))
+ {
+ retcode = -EFAULT;
+ goto cleanup;
+ }
+
for (i = 0; i < d->send_count; i++) {
- idx = d->send_indices[i];
+ idx = drm_send_indices[i];
if (idx < 0 || idx >= dma->buf_count) {
DRM_ERROR("Index %d (of %d max)\n",
- d->send_indices[i], dma->buf_count - 1);
+ drm_send_indices[i], dma->buf_count - 1);
continue;
}
buf = dma->buflist[ idx ];
@@ -397,7 +415,7 @@
process closes the /dev/drm? handle, so
it can't also be doing DMA. */
buf->list = DRM_LIST_PRIO;
- buf->used = d->send_sizes[i];
+ buf->used = drm_send_sizes[i];
buf->context = d->context;
buf->while_locked = d->flags & _DRM_DMA_WHILE_LOCKED;
address = (unsigned long)buf->address;
@@ -408,14 +426,14 @@
if (buf->pending) {
DRM_ERROR("Sending pending buffer:"
" buffer %d, offset %d\n",
- d->send_indices[i], i);
+ drm_send_indices[i], i);
retcode = -EINVAL;
goto cleanup;
}
if (buf->waiting) {
DRM_ERROR("Sending waiting buffer:"
" buffer %d, offset %d\n",
- d->send_indices[i], i);
+ drm_send_indices[i], i);
retcode = -EINVAL;
goto cleanup;
}
@@ -464,6 +482,10 @@
cleanup:
+ if (drm_send_indices)
+ kfree(drm_send_indices);
+ if (drm_send_sizes)
+ kfree(drm_send_sizes);
if (last_buf) {
gamma_dma_ready(dev);
gamma_free_buffer(dev, last_buf);
@@ -487,7 +509,11 @@
drm_device_dma_t *dma = dev->dma;
if (d->flags & _DRM_DMA_BLOCK) {
- last_buf = dma->buflist[d->send_indices[d->send_count-1]];
+ int lastindex;
+ if (copy_from_user(&lastindex, &d->send_indices[d->send_count-1],
+ sizeof(lastindex)))
+ return -EFAULT;
+ last_buf = dma->buflist[lastindex];
add_wait_queue(&last_buf->dma_wait, &entry);
}
On Iau, 2004-01-08 at 20:08, Robert T. Johnson wrote:
> Both of these bugs look exploitable. The vt.c patch is
> self-explanatory.
>
> In gamma_dma.c, argument "d" to gamma_dma_priority() points to a
> structure copied from userspace (see gamma_dma()). That means that
> d->send_indices is a pointer under user control, so it shouldn't be
> dereferenced. The patch just safely copies the contents to a kernel
> buffer and uses that instead. Ditto for d->send_sizes.
Fortunately (in this case) Gamma hasn't worked since about 1999. The SiS
DRM driver in XFree 4.4 snapshot is also exploitable although the 4.3
one seems ok. If you feed the memory allocator random crap it oopses.
With 4.3.x (ie the code in 2.4.x) it doesn't oops but requires sis_fb.
With 4.3.99... it oopses if I dont have sisfb.
> Also, I notice the drm code uses it's own memory allocation wrappers. I
> don't know all the details of the drm code, so I just used kmalloc.
> You'll probably want to change those two calls after applying the
> patch. Sorry for the inconvenience.
It comes out as kmalloc, but its done so it will be portable to other
systems. So on *BSD it comes out appropriately too.
Incidentally the gamme code appears to have maths overflow problems in
the kmalloc paths too.
On Gwe, 2004-01-09 at 00:39, Eric Anholt wrote:
> Uhoh, the SiS is my fault. I'll take a look soon.
Thats ok - I found it because I screwed up textures on your sis6326 code
8)
On Thu, 2004-01-08 at 15:52, Alan Cox wrote:
> On Iau, 2004-01-08 at 20:08, Robert T. Johnson wrote:
> > Both of these bugs look exploitable. The vt.c patch is
> > self-explanatory.
> >
> > In gamma_dma.c, argument "d" to gamma_dma_priority() points to a
> > structure copied from userspace (see gamma_dma()). That means that
> > d->send_indices is a pointer under user control, so it shouldn't be
> > dereferenced. The patch just safely copies the contents to a kernel
> > buffer and uses that instead. Ditto for d->send_sizes.
>
> Fortunately (in this case) Gamma hasn't worked since about 1999. The SiS
> DRM driver in XFree 4.4 snapshot is also exploitable although the 4.3
> one seems ok. If you feed the memory allocator random crap it oopses.
> With 4.3.x (ie the code in 2.4.x) it doesn't oops but requires sis_fb.
> With 4.3.99... it oopses if I dont have sisfb.
Uhoh, the SiS is my fault. I'll take a look soon.
> > Also, I notice the drm code uses it's own memory allocation wrappers. I
> > don't know all the details of the drm code, so I just used kmalloc.
> > You'll probably want to change those two calls after applying the
> > patch. Sorry for the inconvenience.
>
> It comes out as kmalloc, but its done so it will be portable to other
> systems. So on *BSD it comes out appropriately too.
Actually, they were wrapped originally because there was a bunch of
debugging and statistics code for memory allocation. That's been axed
because nobody actually used it, so now they're just left as functions
wrapping the OS's malloc/free.
--
Eric Anholt [email protected]
http://people.freebsd.org/~anholt/ [email protected]
On Thu, 8 Jan 2004, Robert T. Johnson wrote:
> Both of these bugs look exploitable. The vt.c patch is
> self-explanatory.
>
> Thanks for looking at this, and let me know if you have any questions.
>
> Best,
> Rob
>
> P.S. Both of these bugs were found using the source code verification
> tool, CQual, developed by Jeff Foster, myself, and others, and available
> from http://www.cs.umd.edu/~jfoster/cqual/.
>
>
> --- drivers/char/vt.c.orig Thu Jan 8 10:53:01 2004
> +++ drivers/char/vt.c Wed Jan 7 15:22:17 2004
> @@ -288,7 +288,7 @@
> case KDGKBSENT:
> sz = sizeof(tmp.kb_string) - 1; /* sz should have been
> a struct member */
> - q = user_kdgkb->kb_string;
> + q = tmp.kb_string;
> p = func_table[i];
> if(p)
> for ( ; *p && sz; p++, sz--)
The "q" variable is only used as an argument to put_user() (the kernel is
not reading from that address), so I think it is not a problem.
I think your patch will break the ioctl.
Marcelo Tosatti wrote:
On Thu, 8 Jan 2004, Robert T. Johnson wrote:
Both of these bugs look exploitable. The vt.c patch is
self-explanatory.
Thanks for looking at this, and let me know if you have any questions.
Best,
Rob
P.S. Both of these bugs were found using the source code verification
tool, CQual, developed by Jeff Foster, myself, and others, and available
from http://www.cs.umd.edu/~jfoster/cqual/.
--- drivers/char/vt.c.orig Thu Jan 8 10:53:01 2004
+++ drivers/char/vt.c Wed Jan 7 15:22:17 2004
@@ -288,7 +288,7 @@
case KDGKBSENT:
sz = sizeof(tmp.kb_string) - 1; /* sz should have been
a struct member */
- q = user_kdgkb->kb_string;
+ q = tmp.kb_string;
p = func_table[i];
if(p)
for ( ; *p && sz; p++, sz--)
The "q" variable is only used as an argument to put_user() (the kernel is
not reading from that address), so I think it is not a problem.
I think your patch will break the ioctl.
Whoops. I thought user_kdgkb->kb_string was a pointer, not an array.
You're absolutely right. Please ignore this patch. I'm very sorry for
the mistake.
Best,
Rob