Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734605 > unrolled thread

[PATCH] zram: fix null dereference of handle

Back to article view | Back to linux.kernel

Contents

  [PATCH] zram: fix null dereference of handle Minchan Kim <minchan@kernel.org> - 2017-09-19 04:40 +0200
    Re: [PATCH] zram: fix null dereference of handle Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-19 07:40 +0200
      Re: [PATCH] zram: fix null dereference of handle Minchan Kim <minchan@kernel.org> - 2017-09-19 09:00 +0200
        Re: [PATCH] zram: fix null dereference of handle Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-09-19 12:20 +0200

#1734605 — [PATCH] 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 <sergey.senozhatsky@gmail.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 drivers/block/zram/zram_drv.c | 34 ++++++++++------------------------
 1 file changed, 10 insertions(+), 24 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 91a72df41ab0..23172641fc01 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -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);
-- 
2.7.4

[toc] | [next] | [standalone]

#1734662

On (09/19/17 11:34), Minchan Kim wrote:
[..]
>  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;


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

> +		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);
> -- 
> 2.7.4
> 

[toc] | [prev] | [next] | [standalone]

#1734692

Hi Sergey,

On Tue, Sep 19, 2017 at 02:39:35PM +0900, Sergey Senozhatsky wrote:
> On (09/19/17 11:34), Minchan Kim wrote:
> [..]
> >  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;
> 
> 
> 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.

> 
> 
> 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?

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.

> 
> 
> 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!

[toc] | [prev] | [next] | [standalone]

#1734791

Hi Minchan,

On (09/19/17 15:59), Minchan Kim wrote:
[..]
> > 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?
> 
> We discussed it before that we shouldn't return error.
> Userspace can ask reading unallocated buffer freely.

ok, so this is intentional behaviour.

> 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.

> > 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web