tty: vt: remove multi-fetch, derive font.height from font.data (8ffb8209) · Commits · 戴 / test

In con_font_set(), when we need to guess font height (for compat reasons?), the current approach uses multiple userspace fetches, i.e., get_user(tmp, &charmap[32*i+h-1]), to derive the height. This has two drawbacks: 1. performance: accessing userspace memory is less efficient than directly de-reference the byte 2. security: a more critical problem is that the height derived might not match with the actual font.data. This is because a user thread might race condition to change the memory of op->data after the op->height guessing but before the second fetch: font.data = memdup_user(op->data, size). Leaving font.height = 32 while the actual height is 1 or vice-versa. This patch tries to resolve both issues by re-locating the height guessing part after the font.data is fetched in. In this way, the userspace data is fetched in one shot and we directly dereference the font.data in kernel space to probe for the height. Signed-off-by:

Meng Xu <mengxu.gatech@gmail.com> Signed-off-by:

Greg Kroah-Hartman <gregkh@linuxfoundation.org>

drivers/tty/vt/vt.c

+28 −20

Original line number	Diff line number	Diff line
		@@ -4119,37 +4119,45 @@ static int con_font_set(struct vc_data vc, struct console_font_op op)
		return -EINVAL;
		if (op->charcount > 512)
		return -EINVAL;
		if (op->width <= 0 \|\| op->width > 32 \|\| op->height > 32)
		return -EINVAL;
		size = (op->width+7)/8 * 32 * op->charcount;
		if (size > max_font_size)
		return -ENOSPC;

		font.data = memdup_user(op->data, size);
		if (IS_ERR(font.data))
		return PTR_ERR(font.data);

		if (!op->height) { /* Need to guess font height [compat] */
		int h, i;
		u8 __user *charmap = op->data;
		u8 tmp;
		u8 *charmap = font.data;

		/* If from KDFONTOP ioctl, don't allow things which can be done in userland,
		so that we can get rid of this soon */
		if (!(op->flags & KD_FONT_FLAG_OLD))
		/*
		* If from KDFONTOP ioctl, don't allow things which can be done
		* in userland,so that we can get rid of this soon
		*/
		if (!(op->flags & KD_FONT_FLAG_OLD)) {
		kfree(font.data);
		return -EINVAL;
		}

		for (h = 32; h > 0; h--)
		for (i = 0; i < op->charcount; i++) {
		if (get_user(tmp, &charmap[32*i+h-1]))
		return -EFAULT;
		if (tmp)
		for (i = 0; i < op->charcount; i++)
		if (charmap[32*i+h-1])
		goto nonzero;
		}

		kfree(font.data);
		return -EINVAL;

		nonzero:
		op->height = h;
		}
		if (op->width <= 0 \|\| op->width > 32 \|\| op->height > 32)
		return -EINVAL;
		size = (op->width+7)/8 * 32 * op->charcount;
		if (size > max_font_size)
		return -ENOSPC;

		font.charcount = op->charcount;
		font.height = op->height;
		font.width = op->width;
		font.data = memdup_user(op->data, size);
		if (IS_ERR(font.data))
		return PTR_ERR(font.data);
		font.height = op->height;

		console_lock();
		if (vc->vc_mode != KD_TEXT)
		rc = -EINVAL;

Admin message