[media] zoran: do not use kmalloc for memory mapped to userspace

Commit Message

Michal Hocko Jan. 21, 2016, 8:29 a.m. UTC

From: Michal Hocko <[e-mail]>

mmapping kmalloced memory is dangerous for two reasons. First the memory
is not guaranteed to be page aligned and secondly and more imporatantly
the allocates size _has_ to be in page units otherwise we grant an
access to an unrelated memory. This is has security implications of
course.

zoran_mmap calls remap_pfn_range on fbuffer_phys which is allocated by
kmalloc which alone is worrying. Yet the buffer size (buffer_size) is
calculated by mmap_mode_raw, map_mode_jpg resp. zoran_v4l2_calc_bufsize
which caps the resulting size to jpg_bufsize which is 512B so we expose
a full slab page with an unrelated content via mmap.

Fix the issue by using __get_free_pages/free_pages instead of
kmalloc/kfree and also use __GFP_ZERO to make sure the buffer is zeroed
out before it is exported to prevent from information leak.

Signed-off-by: Michal Hocko <[e-mail]>
---
Hi,
I am sending this offlist for the review because this has security
implications and I am not sure how you handle such issues. I do not own
the HW so I haven't tested this so please be careful and give it a deep
review. The issue has been pointed out by Sebastian Frias (CCed) who
was asking for a similar pattern used in an out of tree driver [1].

I do not have any active exploit for the issue nor I am sure whether
this can be exploited in the real life. Small sized slab caches (<=512)
are used quite heavily so there is a theoretical chance of having
something interesting in the same page though.

[1] http://lkml.kernel.org/r/[e-mail]

 drivers/media/pci/zoran/zoran_driver.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

Comments

Michal Hocko Jan. 21, 2016, 10:22 a.m. UTC | #1

On Thu 21-01-16 09:29:18, Michal Hocko wrote:
> Hi,
> I am sending this offlist for the review because this has security

Ups, forgot to update this. This went offlist before but Hans has noted
that it can go public because the number of users of this old HW is so
limitted that any potential security concerns are basically moot.

Patch

diff --git a/drivers/media/pci/zoran/zoran_driver.c b/drivers/media/pci/zoran/zoran_driver.c
index 80caa70c6360..e71af08c4972 100644
--- a/drivers/media/pci/zoran/zoran_driver.c
+++ b/drivers/media/pci/zoran/zoran_driver.c
@@ -227,8 +227,8 @@  static int v4l_fbuffer_alloc(struct zoran_fh *fh)
 				ZR_DEVNAME(zr), __func__, i);
 
 		//udelay(20);
-		mem = kmalloc(fh->buffers.buffer_size,
-			      GFP_KERNEL | __GFP_NOWARN);
+		mem = (unsigned char *)__get_free_pages(GFP_KERNEL | __GFP_ZERO | __GFP_NOWARN,
+				get_order(fh->buffers.buffer_size));
 		if (!mem) {
 			dprintk(1,
 				KERN_ERR
@@ -272,7 +272,8 @@  static void v4l_fbuffer_free(struct zoran_fh *fh)
 		for (off = 0; off < fh->buffers.buffer_size;
 		     off += PAGE_SIZE)
 			ClearPageReserved(virt_to_page(mem + off));
-		kfree(fh->buffers.buffer[i].v4l.fbuffer);
+		free_pages((unsigned long)fh->buffers.buffer[i].v4l.fbuffer,
+				get_order(fh->buffers.buffer_size));
 		fh->buffers.buffer[i].v4l.fbuffer = NULL;
 	}
 
@@ -335,7 +336,8 @@  static int jpg_fbuffer_alloc(struct zoran_fh *fh)
 		fh->buffers.buffer[i].jpg.frag_tab_bus = virt_to_bus(mem);
 
 		if (fh->buffers.need_contiguous) {
-			mem = kmalloc(fh->buffers.buffer_size, GFP_KERNEL);
+			mem = (void *)__get_free_pages(GFP_KERNEL|__GFP_ZERO,
+					get_order(fh->buffers.buffer_size));
 			if (mem == NULL) {
 				dprintk(1,
 					KERN_ERR
@@ -407,7 +409,8 @@  static void jpg_fbuffer_free(struct zoran_fh *fh)
 				mem = bus_to_virt(le32_to_cpu(frag_tab));
 				for (off = 0; off < fh->buffers.buffer_size; off += PAGE_SIZE)
 					ClearPageReserved(virt_to_page(mem + off));
-				kfree(mem);
+				free_pages((unsigned long)mem,
+						get_order(fh->buffers.buffer_size));
 				buffer->jpg.frag_tab[0] = 0;
 				buffer->jpg.frag_tab[1] = 0;
 			}