Skip to contentSkip to conversation

zram: fix null dereference of handle

PATCH Superseded Minchan Kim 2 participants 4 messages Sep 19, 2017

Subsystems (2)
Block layer zram

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Minchan Kim

author maintainer
#0 lore raw

PATCH 1/1 zram: fix null dereference of handle

For the testing, I found handle passed to zs_map_object in __zram_bvec_read is NULL so that kernel goes the oops by pin_object.

The reason is there is no routine to check the slot's freeing after getting the slot's lock. This patch fixes it.

Fixes 1f7319c74275 ("zram: partial IO refactoring")
Cc Sergey Senozhatsky <[email protected]>
Signed-off-by Minchan Kim <[email protected]>
---
 drivers/block/zram/zram_drv.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)
drivers/block/zram/zram_drv.c +10 -24
index 91a72df41ab0..23172641fc01 100644
@@ -758,27 +758,6 @@ static void zram_slot_unlock(struct zram *zram, u32 index)
 	bit_spin_unlock(ZRAM_ACCESS, &zram->table[index].value);
 }
 
-static bool zram_same_page_read(struct zram *zram, u32 index,
-				struct page *page,
-				unsigned int offset, unsigned int len)
-{
-	zram_slot_lock(zram, index);
-	if (unlikely(!zram_get_handle(zram, index) ||
-			zram_test_flag(zram, index, ZRAM_SAME))) {
-		void *mem;
-
-		zram_slot_unlock(zram, index);
-		mem = kmap_atomic(page);
-		zram_fill_page(mem + offset, len,
-					zram_get_element(zram, index));
-		kunmap_atomic(mem);
-		return true;
-	}
-	zram_slot_unlock(zram, index);
-
-	return false;
-}
-
 static void zram_meta_free(struct zram *zram, u64 disksize)
 {
 	size_t num_pages = disksize >> PAGE_SHIFT;

@@ -876,11 +855,18 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
 		zram_slot_unlock(zram, index);
 	}
 
-	if (zram_same_page_read(zram, index, page, 0, PAGE_SIZE))
-		return 0;
-
 	zram_slot_lock(zram, index);
 	handle = zram_get_handle(zram, index);
+	if (unlikely(!handle || zram_test_flag(zram, index, ZRAM_SAME))) {
+		void *mem;
+
+		mem = kmap_atomic(page);
+		zram_fill_page(mem, PAGE_SIZE, zram_get_element(zram, index));
+		kunmap_atomic(mem);
+		zram_slot_unlock(zram, index);
+		return 0;
+	}
+
 	size = zram_get_obj_size(zram, index);
 
 	src = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
Signature · 1 line
2.7.4
#1 lore raw

On (09/19/17 11:34), Minchan Kim wrote:
[..]

Quoted 4 lines

static void zram_meta_free(struct zram *zram, u64 disksize) {

drivers/block/zram/zram_drv.c +0 -0
 	size_t num_pages = disksize >> PAGE_SHIFT;

drivers/block/zram/zram_drv.c static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index, · line 861

Quoted 10 lines
drivers/block/zram/zram_drv.c +2 -3
@@ -876,11 +855,18 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
 		zram_slot_unlock(zram, index);
 	}
 
-	if (zram_same_page_read(zram, index, page, 0, PAGE_SIZE))
-		return 0;
-
 	zram_slot_lock(zram, index);
 	handle = zram_get_handle(zram, index);
+	if (unlikely(!handle || zram_test_flag(zram, index, ZRAM_SAME))) {
+		void *mem;

is this branch really unlikely()? ZRAM_SAME ratio really depends, on some setups it can be quite likely, I suspect.

another question, "!handle == value & ZRAM_SAME"? if so, then why not just check for `flags & ZRAM_SAME'? if not then:

- for `value & ZRAM_SAME' you fill the page with zram_get_element(zram, index) and return 0. ok.

- for !handle.... you also fill the page with zram_get_element(zram, index) and return 0. is this ok? shouldn't !handle return error in this case?

I really suspect that there are some paths that can lead to !handle entry, that will not be ZRAM_SAME. e.g. error return from compression path.

	-ss
Quoted 13 lines
drivers/block/zram/zram_drv.c +7 -0
+		mem = kmap_atomic(page);
+		zram_fill_page(mem, PAGE_SIZE, zram_get_element(zram, index));
+		kunmap_atomic(mem);
+		zram_slot_unlock(zram, index);
+		return 0;
+	}
+
 	size = zram_get_obj_size(zram, index);
 
 	src = zs_map_object(zram->mem_pool, handle, ZS_MM_RO);
Signature · 1 line
2.7.4
author maintainer
#2 lore raw

Hi Sergey,

Quoted 9 lines · Sergey Senozhatsky

On (09/19/17 11:34), Minchan Kim wrote:
[..]

Quoted 7 lines

static void zram_meta_free(struct zram *zram, u64 disksize) {

drivers/block/zram/zram_drv.c +0 -0
 	size_t num_pages = disksize >> PAGE_SHIFT;

@@ -876,11 +855,18 @@ static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index,
 		zram_slot_unlock(zram, index);
 	}
Quoted 11 lines

drivers/block/zram/zram_drv.c static int __zram_bvec_read(struct zram *zram, struct page *page, u32 index, · line 861

Quoted 7 lines
drivers/block/zram/zram_drv.c +2 -3
-	if (zram_same_page_read(zram, index, page, 0, PAGE_SIZE))
-		return 0;
-
 	zram_slot_lock(zram, index);
 	handle = zram_get_handle(zram, index);
+	if (unlikely(!handle || zram_test_flag(zram, index, ZRAM_SAME))) {
+		void *mem;

is this branch really unlikely()? ZRAM_SAME ratio really depends, on some setups it can be quite likely, I suspect.

Yub. Let's drop it.

Review

Quoted 10 lines

another question, "!handle == value & ZRAM_SAME"? if so, then why not just check for `flags & ZRAM_SAME'? if not then:

(quoted diff) +0 -2
-  for `value & ZRAM_SAME' you fill the page with zram_get_element(zram, index)
   and return 0. ok.
 
-  for !handle.... you also fill the page with zram_get_element(zram, index)
   and return 0. is this ok? shouldn't !handle return error in this case?

We discussed it before that we shouldn't return error. Userspace can ask reading unallocated buffer freely.

And in this case, it fills the buffer zero because handle and element is unified. However, if your concern is readability, I will make it more explict.

Quoted 5 lines

I really suspect that there are some paths that can lead to !handle entry, that will not be ZRAM_SAME. e.g. error return from compression path.

Could you be more specific?

Thanks for the review, Sergey!

#3 lore raw

Hi Minchan,

On (09/19/17 15:59), Minchan Kim wrote:
[..]

Quoted 11 lines

Review

Quoted 8 lines

another question, "!handle == value & ZRAM_SAME"? if so, then why not just check for `flags & ZRAM_SAME'? if not then:

(quoted diff) +0 -2
-  for `value & ZRAM_SAME' you fill the page with zram_get_element(zram, index)
   and return 0. ok.
 
-  for !handle.... you also fill the page with zram_get_element(zram, index)
   and return 0. is this ok? shouldn't !handle return error in this case?

We discussed it before that we shouldn't return error. Userspace can ask reading unallocated buffer freely.

ok, so this is intentional behaviour.

Quoted 2 lines

And in this case, it fills the buffer zero because handle and element is unified. However, if your concern is readability, I will make it more explict.

correct.
... but I thought that we would also return an error.

Quoted 5 lines
Quoted 3 lines

I really suspect that there are some paths that can lead to !handle entry, that will not be ZRAM_SAME. e.g. error return from compression path.

Could you be more specific?

I just meant that there are error paths in zram write, which will leave us both with !handle entries and !ZRAM_SAME. but it seems that this is the intentional behaviour.

	-ss