[PATCH 0/2] tty,vcs: vcs_seek fix + removing con_buf_mtx

[PATCH 0/2] tty,vcs: vcs_seek fix + removing con_buf_mtx

[Jiri Olsa]

hi,

the vcs lseek method might race with VC removal and cause panic.

Also it seems the vcs is the only one using con_buf and con_buf_mtx,
which does not seem necessary any longer.

attached patches:
1/2 - lseek/VC-release race fix
2/2 - removing con_buf and con_buf_mtx

wbr,
jirka
---
 drivers/tty/vt/vc_screen.c |  110 +++++++++++++++++++++++++++-----------------
 drivers/tty/vt/vt.c        |   12 -----
 include/linux/vt_kern.h    |    8 ---
 3 files changed, 67 insertions(+), 63 deletions(-)

[PATCH 1/2] tty,vcs: lseek/VC-release race fix

[Jiri Olsa]

hi,

there's a race between vcs's lseek handler and VC release.

The lseek handler does not hold console_lock and touches
VC's size info. If during this the VC got released, there's
an access violation.

Following program triggers the issue for me:

[SNIP]
#define _BSD_SOURCE
#include <stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <linux/vt.h>
#include <unistd.h>
#include <errno.h>

static int run_seek(void)
{
        while(1) {
                int fd;
                fd = open("./vcs30", O_RDWR);
                while(lseek(fd, 0, 0) != -1);
                close(fd);
        }
}

static int open_ioctl_tty(void)
{
        return open("/dev/tty1", O_RDWR);
}

static int do_ioctl(int fd, int req, int i)
{
        return ioctl(fd, req, i);
}

#define INIT(i) do_ioctl(ioctl_fd, VT_ACTIVATE, i)
#define SHUT(i) do_ioctl(ioctl_fd, VT_DISALLOCATE, i)

int main(int argc, char **argv)
{
        int ioctl_fd = open_ioctl_tty();

        if (ioctl < 0) {
                perror("open tty1 failed\n");
                return -1;
        }

        if ((-1 == mknod("vcs30", S_IFCHR|0666, makedev(7, 30))) &&
            (errno != EEXIST)) {
                printf("errno %d\n", errno);
                perror("failed to create vcs30");
                return -1;
        }

        do_ioctl(ioctl_fd, VT_LOCKSWITCH, 0);

        if (!fork())
                run_seek();

        while(1) {
                INIT(30);
                SHUT(30);
        }

        return 0;
}
[SNIP]

wbr,
jirka


Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 drivers/tty/vt/vc_screen.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index a672ed1..3c27c4b 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -159,7 +159,13 @@ static loff_t vcs_lseek(struct file *file, loff_t offset, int orig)
 	int size;
 
 	mutex_lock(&con_buf_mtx);
+	console_lock();
 	size = vcs_size(file->f_path.dentry->d_inode);
+	console_unlock();
+	if (size < 0) {
+		mutex_unlock(&con_buf_mtx);
+		return size;
+	}
 	switch (orig) {
 		default:
 			mutex_unlock(&con_buf_mtx);
@@ -237,6 +243,12 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		 * could sleep.
 		 */
 		size = vcs_size(inode);
+		if (size < 0) {
+			if (read)
+				break;
+			ret = size;
+			goto unlock_out;
+		}
 		if (pos >= size)
 			break;
 		if (count > size - pos)
@@ -436,6 +448,12 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 		 * Return data written up to now on failure.
 		 */
 		size = vcs_size(inode);
+		if (size < 0) {
+			if (written)
+				break;
+			ret = size;
+			goto unlock_out;
+		}
 		if (pos >= size)
 			break;
 		if (this_round > size - pos)
-- 
1.7.1

[PATCH 2/2] tty,vcs removing con_buf/conf_buf_mtx

[Jiri Olsa]

hi,

seems there's no longer need for using con_buf/conf_buf_mtx
as vcs_read/vcs_write buffer for user's data.

The do_con_write function, that was the other user of this,
is currently using its own kmalloc-ed buffer.

Not sure when this got changed, as I was able to find this code
in 2.6.9, but it's already gone as far as current git history
goes - 2.6.12-rc2.

AFAICS there's a behaviour change with the current change.
The lseek is not completely mutually exclusive with the
vcs_read/vcs_write - the file->f_pos might get updated
via lseek callback during the vcs_read/vcs_write processing.

I tried to find out if the prefered behaviour is to keep
this in sync within read/write/lseek functions, but I did
not find any pattern on different places.

I guess if user end up calling write/lseek from different
threads she should know what she's doing. If needed we
could use dedicated fd mutex/buffer.

wbr,
jirka


Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
 drivers/tty/vt/vc_screen.c |   98 +++++++++++++++++++++++--------------------
 drivers/tty/vt/vt.c        |   12 -----
 include/linux/vt_kern.h    |    8 ----
 3 files changed, 52 insertions(+), 66 deletions(-)

diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
index 3c27c4b..7b3bfbe 100644
--- a/drivers/tty/vt/vc_screen.c
+++ b/drivers/tty/vt/vc_screen.c
@@ -28,7 +28,6 @@
 #include <linux/interrupt.h>
 #include <linux/mm.h>
 #include <linux/init.h>
-#include <linux/mutex.h>
 #include <linux/vt_kern.h>
 #include <linux/selection.h>
 #include <linux/kbd_kern.h>
@@ -51,6 +50,8 @@
 #undef addr
 #define HEADER_SIZE	4
 
+#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
+
 struct vcs_poll_data {
 	struct notifier_block notifier;
 	unsigned int cons_num;
@@ -131,21 +132,45 @@ vcs_poll_data_get(struct file *file)
 	return poll;
 }
 
+/*
+ * Returns VC for inode.
+ * Must be called with console_lock.
+ */
+static struct vc_data*
+vcs_vc(struct inode *inode, int *viewed)
+{
+	unsigned int currcons = iminor(inode) & 127;
+
+	WARN_CONSOLE_UNLOCKED();
+
+	if (currcons == 0) {
+		currcons = fg_console;
+		if (viewed)
+			*viewed = 1;
+	} else {
+		currcons--;
+		if (viewed)
+			*viewed = 0;
+	}
+	return vc_cons[currcons].d;
+}
+
+/*
+ * Returns size for VC carried by inode.
+ * Must be called with console_lock.
+ */
 static int
 vcs_size(struct inode *inode)
 {
 	int size;
 	int minor = iminor(inode);
-	int currcons = minor & 127;
 	struct vc_data *vc;
 
-	if (currcons == 0)
-		currcons = fg_console;
-	else
-		currcons--;
-	if (!vc_cons_allocated(currcons))
+	WARN_CONSOLE_UNLOCKED();
+
+	vc = vcs_vc(inode, NULL);
+	if (!vc)
 		return -ENXIO;
-	vc = vc_cons[currcons].d;
 
 	size = vc->vc_rows * vc->vc_cols;
 
@@ -158,17 +183,13 @@ static loff_t vcs_lseek(struct file *file, loff_t offset, int orig)
 {
 	int size;
 
-	mutex_lock(&con_buf_mtx);
 	console_lock();
 	size = vcs_size(file->f_path.dentry->d_inode);
 	console_unlock();
-	if (size < 0) {
-		mutex_unlock(&con_buf_mtx);
+	if (size < 0)
 		return size;
-	}
 	switch (orig) {
 		default:
-			mutex_unlock(&con_buf_mtx);
 			return -EINVAL;
 		case 2:
 			offset += size;
@@ -179,11 +200,9 @@ static loff_t vcs_lseek(struct file *file, loff_t offset, int orig)
 			break;
 	}
 	if (offset < 0 || offset > size) {
-		mutex_unlock(&con_buf_mtx);
 		return -EINVAL;
 	}
 	file->f_pos = offset;
-	mutex_unlock(&con_buf_mtx);
 	return file->f_pos;
 }
 
@@ -196,12 +215,15 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	struct vc_data *vc;
 	struct vcs_poll_data *poll;
 	long pos;
-	long viewed, attr, read;
-	int col, maxcol;
+	long attr, read;
+	int col, maxcol, viewed;
 	unsigned short *org = NULL;
 	ssize_t ret;
+	char *con_buf;
 
-	mutex_lock(&con_buf_mtx);
+	con_buf = (char *) __get_free_page(GFP_KERNEL);
+	if (!con_buf)
+		return -ENOMEM;
 
 	pos = *ppos;
 
@@ -211,18 +233,10 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	console_lock();
 
 	attr = (currcons & 128);
-	currcons = (currcons & 127);
-	if (currcons == 0) {
-		currcons = fg_console;
-		viewed = 1;
-	} else {
-		currcons--;
-		viewed = 0;
-	}
 	ret = -ENXIO;
-	if (!vc_cons_allocated(currcons))
+	vc = vcs_vc(inode, &viewed);
+	if (!vc)
 		goto unlock_out;
-	vc = vc_cons[currcons].d;
 
 	ret = -EINVAL;
 	if (pos < 0)
@@ -367,7 +381,7 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		ret = read;
 unlock_out:
 	console_unlock();
-	mutex_unlock(&con_buf_mtx);
+	free_page((unsigned long) con_buf);
 	return ret;
 }
 
@@ -378,13 +392,16 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	unsigned int currcons = iminor(inode);
 	struct vc_data *vc;
 	long pos;
-	long viewed, attr, size, written;
+	long attr, size, written;
 	char *con_buf0;
-	int col, maxcol;
+	int col, maxcol, viewed;
 	u16 *org0 = NULL, *org = NULL;
 	size_t ret;
+	char *con_buf;
 
-	mutex_lock(&con_buf_mtx);
+	con_buf = (char *) __get_free_page(GFP_KERNEL);
+	if (!con_buf)
+		return -ENOMEM;
 
 	pos = *ppos;
 
@@ -394,19 +411,10 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	console_lock();
 
 	attr = (currcons & 128);
-	currcons = (currcons & 127);
-
-	if (currcons == 0) {
-		currcons = fg_console;
-		viewed = 1;
-	} else {
-		currcons--;
-		viewed = 0;
-	}
 	ret = -ENXIO;
-	if (!vc_cons_allocated(currcons))
+	vc = vcs_vc(inode, &viewed);
+	if (!vc)
 		goto unlock_out;
-	vc = vc_cons[currcons].d;
 
 	size = vcs_size(inode);
 	ret = -EINVAL;
@@ -561,9 +569,7 @@ vcs_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 
 unlock_out:
 	console_unlock();
-
-	mutex_unlock(&con_buf_mtx);
-
+	free_page((unsigned long) con_buf);
 	return ret;
 }
 
diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c
index 147ede3..0fd6df5 100644
--- a/drivers/tty/vt/vt.c
+++ b/drivers/tty/vt/vt.c
@@ -2068,18 +2068,6 @@ static void do_con_trol(struct tty_struct *tty, struct vc_data *vc, int c)
 	}
 }
 
-/* This is a temporary buffer used to prepare a tty console write
- * so that we can easily avoid touching user space while holding the
- * console spinlock.  It is allocated in con_init and is shared by
- * this code and the vc_screen read/write tty calls.
- *
- * We have to allocate this statically in the kernel data section
- * since console_init (and thus con_init) are called before any
- * kernel memory allocation is available.
- */
-char con_buf[CON_BUF_SIZE];
-DEFINE_MUTEX(con_buf_mtx);
-
 /* is_double_width() is based on the wcwidth() implementation by
  * Markus Kuhn -- 2007-05-26 (Unicode 5.0)
  * Latest version: http://www.cl.cam.ac.uk/~mgk25/ucs/wcwidth.c
diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h
index 6625cc1..4d05e14 100644
--- a/include/linux/vt_kern.h
+++ b/include/linux/vt_kern.h
@@ -142,14 +142,6 @@ static inline bool vt_force_oops_output(struct vc_data *vc)
 	return false;
 }
 
-/*
- * vc_screen.c shares this temporary buffer with the console write code so that
- * we can easily avoid touching user space while holding the console spinlock.
- */
-
-#define CON_BUF_SIZE (CONFIG_BASE_SMALL ? 256 : PAGE_SIZE)
-extern char con_buf[CON_BUF_SIZE];
-extern struct mutex con_buf_mtx;
 extern char vt_dont_switch;
 extern int default_utf8;
 extern int global_cursor_default;
-- 
1.7.1

Re: [PATCH 0/2] tty,vcs: vcs_seek fix + removing con_buf_mtx

[Jiri Olsa]

hi,
any feedback?

thanks,
jirka

On Mon, Feb 07, 2011 at 07:31:23PM +0100, Jiri Olsa wrote:
> hi,
> 
> the vcs lseek method might race with VC removal and cause panic.
> 
> Also it seems the vcs is the only one using con_buf and con_buf_mtx,
> which does not seem necessary any longer.
> 
> attached patches:
> 1/2 - lseek/VC-release race fix
> 2/2 - removing con_buf and con_buf_mtx
> 
> wbr,
> jirka
> ---
>  drivers/tty/vt/vc_screen.c |  110 +++++++++++++++++++++++++++-----------------
>  drivers/tty/vt/vt.c        |   12 -----
>  include/linux/vt_kern.h    |    8 ---
>  3 files changed, 67 insertions(+), 63 deletions(-)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/