acpi-video: Fix integer overflow and possible kernel stack trashing

PATCH New Michael Buesch 1 participant 1 message Jul 19, 2009 ACPI

Share & tools

Michael Buesch

author

2009-07-19 11:54 UTC #0 lore raw

PATCH v2 1/1 acpi-video: Fix integer overflow and possible kernel stack trashing

This patch fixes a possible kernel crash through stack trashing triggered by an integer overflow. If count passed from userspace is (size_t)-1lu, the range check will overflow and return false. So the copy_from_user() will end up attempting to copy 0xFFFFFFFF (or 0xFFFFFFFFFFFFFFFF) bytes to the kernel stack. Of course the copy will fail at some point, because we can't allocate a buffer that big. But it will copy as much as it can and then return with an -EFAULT.
This means the userspace process writing to this proc file controls
the kernel stack.

This is probably not useable for a privilege escalation, because the proc file s have permissions (S_IFREG | S_IRUGO | S_IWUSR). So only root will be able to crash the machine.

Signed-off-by Michael Buesch <[email protected]>

Cc [email protected]

---

version 2: Fix more functions with similar bugs.

This patch is completely untested, because I do not have a machine with acpi-video.

---
 drivers/acpi/video.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/acpi/video.c +4 -4

@@ -1185,21 +1185,21 @@ acpi_video_device_write_state(struct fil
 			      const char __user * buffer,
 			      size_t count, loff_t * data)
 {
 	int status;
 	struct seq_file *m = file->private_data;
 	struct acpi_video_device *dev = m->private;
 	char str[12] = { 0 };
 	u32 state = 0;
 
 
-	if (!dev || count + 1 > sizeof str)
+	if (!dev || count >= sizeof str)
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
 		return -EFAULT;
 
 	str[count] = 0;
 	state = simple_strtoul(str, NULL, 0);
 	state &= ((1ul << 31) | (1ul << 30) | (1ul << 0));
 
 	status = acpi_video_device_set_state(dev, state);

@@ -1242,21 +1242,21 @@ acpi_video_device_write_brightness(struc
 				   const char __user * buffer,
 				   size_t count, loff_t * data)
 {
 	struct seq_file *m = file->private_data;
 	struct acpi_video_device *dev = m->private;
 	char str[5] = { 0 };
 	unsigned int level = 0;
 	int i;
 
 
-	if (!dev || !dev->brightness || count + 1 > sizeof str)
+	if (!dev || !dev->brightness || count >= sizeof str)
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
 		return -EFAULT;
 
 	str[count] = 0;
 	level = simple_strtoul(str, NULL, 0);
 
 	if (level > 100)
 		return -EFAULT;

@@ -1524,21 +1524,21 @@ acpi_video_bus_write_POST(struct file *f
 			  const char __user * buffer,
 			  size_t count, loff_t * data)
 {
 	int status;
 	struct seq_file *m = file->private_data;
 	struct acpi_video_bus *video = m->private;
 	char str[12] = { 0 };
 	unsigned long long opt, options;
 
 
-	if (!video || count + 1 > sizeof str)
+	if (!video || count >= sizeof str)
 		return -EINVAL;
 
 	status = acpi_video_bus_POST_options(video, &options);
 	if (!ACPI_SUCCESS(status))
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
 		return -EFAULT;
 
 	str[count] = 0;

@@ -1564,21 +1564,21 @@ acpi_video_bus_write_DOS(struct file *fi
 			 const char __user * buffer,
 			 size_t count, loff_t * data)
 {
 	int status;
 	struct seq_file *m = file->private_data;
 	struct acpi_video_bus *video = m->private;
 	char str[12] = { 0 };
 	unsigned long opt;
 
 
-	if (!video || count + 1 > sizeof str)
+	if (!video || count >= sizeof str)
 		return -EINVAL;
 
 	if (copy_from_user(str, buffer, count))
 		return -EFAULT;
 
 	str[count] = 0;
 	opt = strtoul(str, NULL, 0);
 	if (opt > 7)
 		return -EFAULT;

Signature · 1 line

Greetings, Michael.