[CFT] 2.6.0-test7: Fix use-after-free bug in ds.
Russell King
rmk+exim at arm.linux.org.uk
Sat Oct 11 23:36:44 BST 2003
The attached patch solves a use-after-free bug with ds:
Slab corruption: start=c1f80360, expend=c1f803df, problemat=c1f80374
Last user: [<c00c3c48>](kset_hotplug+0x20c/0x234)
Data: ********************74 03 F8 C1 74 03 F8 C1 ***************************************************************************************************A5
Next: 71 F0 2C .48 3C 0C C0 A5 C2 0F 17 24 D4 F6 C1 D4 02 F8 C1 24 D4 F6 C1 2C D2 21 C0 3B A3 FF FF
slab error in check_poison_obj(): cache `size-128': object was modified after freeing
[<c005e82c>] (kmem_cache_alloc+0x0/0x1f4) from [<c00c3ab4>] (kset_hotplug+0x78/0x234)
[<c00c3a3c>] (kset_hotplug+0x0/0x234) from [<c00c3fd8>] (kobject_del+0x68/0x80)
[<c00c3f70>] (kobject_del+0x0/0x80) from [<c00f7540>] (class_device_del+0x90/0xa8)
[<c00f74b0>] (class_device_del+0x0/0xa8) from [<c0151508>] (netdev_run_todo+0xe4/0x1f0)
[<c0151424>] (netdev_run_todo+0x0/0x1f0) from [<bf07221c>] (pcnet_detach+0x9c/0xbc [pcnet_cs])
[<bf072180>] (pcnet_detach+0x0/0xbc [pcnet_cs]) from [<bf06487c>] (unbind_request+0xcc/0x128 [ds])
[<bf0647b0>] (unbind_request+0x0/0x128 [ds]) from [<bf06522c>] (ds_ioctl+0x4ec/0x740 [ds])
[<bf064d40>] (ds_ioctl+0x0/0x740 [ds]) from [<c00842a4>] (sys_ioctl+0x224/0x24c)
[<c0084080>] (sys_ioctl+0x0/0x24c) from [<c00286e0>] (ret_fast_syscall+0x0/0x2c)
(the above was generated with a pcnet_cs card in the socket and the
driver loaded.)
Normally, cardmgr calls select() to wait for events on each PCMCIA
socket. This means that we will have added entries to each
pcmcia_bus_socket's "queue" wait queue.
If you perform the following commands in order:
# cardctl eject
# rmmod yenta_socket
# insmod drivers/pcmcia/yenta_socket.ko
# killall cardmgr
the rmmod ends up freeing the pcmcia_bus_socket while the wait
queue is still active. The killall cardmgr cases the the select()
to complete, and users to be removed from the "queue" - which ends
up writing to freed memory.
The following patch adds refcounting to pcmcia_bus_socket so we
won't free it until all users have gone. We also add "SOCKET_DEAD"
to mark the condition where the socket is no longer present in the
system.
Note that we don't wake up cardmgr when we remove sockets -
unfortunately cardmgr doesn't like receiving errors from read().
Really, cardmgr should treat EIO from read() as a fatal error
for that socket, and stop listening for events from it.
I've tested this patch locally, and it seems good. Please provide
feedback.
--- orig/drivers/pcmcia/ds.c Thu Oct 9 00:04:20 2003
+++ linux/drivers/pcmcia/ds.c Sat Oct 11 22:15:56 2003
@@ -51,6 +51,8 @@
#include <linux/list.h>
#include <linux/workqueue.h>
+#include <asm/atomic.h>
+
#include <pcmcia/version.h>
#include <pcmcia/cs_types.h>
#include <pcmcia/cs.h>
@@ -95,10 +97,12 @@
int event_head, event_tail;
event_t event[MAX_EVENTS];
struct user_info_t *next;
+ struct pcmcia_bus_socket *socket;
} user_info_t;
/* Socket state information */
struct pcmcia_bus_socket {
+ atomic_t refcount;
client_handle_t handle;
int state;
user_info_t *user;
@@ -113,6 +117,7 @@
#define SOCKET_PRESENT 0x01
#define SOCKET_BUSY 0x02
#define SOCKET_REMOVAL_PENDING 0x10
+#define SOCKET_DEAD 0x80
/*====================================================================*/
@@ -137,6 +142,24 @@
static struct pcmcia_driver * get_pcmcia_driver (dev_info_t *dev_info);
static struct pcmcia_bus_socket * get_socket_info_by_nr(unsigned int nr);
+static void pcmcia_put_bus_socket(struct pcmcia_bus_socket *s)
+{
+ if (atomic_dec_and_test(&s->refcount))
+ kfree(s);
+}
+
+static struct pcmcia_bus_socket *pcmcia_get_bus_socket(int nr)
+{
+ struct pcmcia_bus_socket *s;
+
+ s = get_socket_info_by_nr(nr);
+ if (s) {
+ WARN_ON(atomic_read(&s->refcount) == 0);
+ atomic_inc(&s->refcount);
+ }
+ return s;
+}
+
/**
* pcmcia_register_driver - register a PCMCIA driver with the bus core
*
@@ -501,7 +524,7 @@
DEBUG(0, "ds_open(socket %d)\n", i);
- s = get_socket_info_by_nr(i);
+ s = pcmcia_get_bus_socket(i);
if (!s)
return -ENODEV;
@@ -517,6 +540,7 @@
user->event_tail = user->event_head = 0;
user->next = s->user;
user->user_magic = USER_MAGIC;
+ user->socket = s;
s->user = user;
file->private_data = user;
@@ -535,14 +559,12 @@
DEBUG(0, "ds_release(socket %d)\n", i);
- s = get_socket_info_by_nr(i);
- if (!s)
- return 0;
-
user = file->private_data;
if (CHECK_USER(user))
goto out;
+ s = user->socket;
+
/* Unlink user data structure */
if ((file->f_flags & O_ACCMODE) != O_RDONLY)
s->state &= ~SOCKET_BUSY;
@@ -554,6 +576,7 @@
*link = user->next;
user->user_magic = 0;
kfree(user);
+ pcmcia_put_bus_socket(s);
out:
return 0;
} /* ds_release */
@@ -572,14 +595,14 @@
if (count < 4)
return -EINVAL;
- s = get_socket_info_by_nr(i);
- if (!s)
- return -ENODEV;
-
user = file->private_data;
if (CHECK_USER(user))
return -EIO;
+ s = user->socket;
+ if (s->state & SOCKET_DEAD)
+ return -EIO;
+
if (queue_empty(user)) {
interruptible_sleep_on(&s->queue);
if (signal_pending(current))
@@ -605,14 +628,14 @@
if ((file->f_flags & O_ACCMODE) == O_RDONLY)
return -EBADF;
- s = get_socket_info_by_nr(i);
- if (!s)
- return -ENODEV;
-
user = file->private_data;
if (CHECK_USER(user))
return -EIO;
+ s = user->socket;
+ if (s->state & SOCKET_DEAD)
+ return -EIO;
+
if (s->req_pending) {
s->req_pending--;
get_user(s->req_result, (int *)buf);
@@ -635,13 +658,14 @@
DEBUG(2, "ds_poll(socket %d)\n", i);
- s = get_socket_info_by_nr(i);
- if (!s)
- return POLLERR;
-
user = file->private_data;
if (CHECK_USER(user))
return POLLERR;
+ s = user->socket;
+ /*
+ * We don't check for a dead socket here since that
+ * will send cardmgr into an endless spin.
+ */
poll_wait(file, &s->queue, wait);
if (!queue_empty(user))
return POLLIN | POLLRDNORM;
@@ -658,12 +682,17 @@
u_int size;
int ret, err;
ds_ioctl_arg_t buf;
+ user_info_t *user;
DEBUG(2, "ds_ioctl(socket %d, %#x, %#lx)\n", i, cmd, arg);
- s = get_socket_info_by_nr(i);
- if (!s)
- return -ENODEV;
+ user = file->private_data;
+ if (CHECK_USER(user))
+ return -EIO;
+
+ s = user->socket;
+ if (s->state & SOCKET_DEAD)
+ return -EIO;
size = (cmd & IOCSIZE_MASK) >> IOCSIZE_SHIFT;
if (size > sizeof(ds_ioctl_arg_t)) return -EINVAL;
@@ -833,6 +862,7 @@
if(!s)
return -ENOMEM;
memset(s, 0, sizeof(struct pcmcia_bus_socket));
+ atomic_set(&s->refcount, 1);
/*
* Ugly. But we want to wait for the socket threads to have started up.
@@ -894,7 +924,8 @@
pcmcia_deregister_client(socket->pcmcia->handle);
- kfree(socket->pcmcia);
+ socket->pcmcia->state |= SOCKET_DEAD;
+ pcmcia_put_bus_socket(socket->pcmcia);
socket->pcmcia = NULL;
return;
--
Russell King (rmk at arm.linux.org.uk) http://www.arm.linux.org.uk/personal/
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 PCMCIA - http://pcmcia.arm.linux.org.uk/
2.6 Serial core
More information about the linux-pcmcia
mailing list