[PATCH] zram: fix null dereference of handle

[PATCH] zram: fix null dereference of handle

[Minchan Kim]

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

Re: [PATCH] zram: fix null dereference of handle

[Sergey Senozhatsky]

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
> 

Re: [PATCH] zram: fix null dereference of handle

[Minchan Kim]

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!

Re: [PATCH] zram: fix null dereference of handle

[Sergey Senozhatsky]

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

Re: [PATCH] zram: fix null dereference of handle

[Sergey Senozhatsky]

Minchan,

I just ran across it [because I had a bug to analize where this
part was involved]. I'd really prefer the kernel to BUG_ON immediately
instead of dying in agony.

can we, please, return BUG_ON() back?

there is no point in trying to save the kernel once it did that type
of violation.

---

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 7c38e850a8fc..685049a9048d 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1349,7 +1349,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
         * pools/users, we can't allow mapping in interrupt context
         * because it can corrupt another users mappings.
         */
-       WARN_ON_ONCE(in_interrupt());
+       BUG_ON(in_interrupt());
 
        /* From now on, migration cannot move the object */
        pin_tag(handle);

Re: [PATCH] zram: fix null dereference of handle

[Minchan Kim]

On Tue, Sep 19, 2017 at 07:21:25PM +0900, Sergey Senozhatsky wrote:
> Minchan,
> 
> I just ran across it [because I had a bug to analize where this
> part was involved]. I'd really prefer the kernel to BUG_ON immediately
> instead of dying in agony.
> 
> can we, please, return BUG_ON() back?
> 
> there is no point in trying to save the kernel once it did that type
> of violation.

I agree. If it happens, it would corrupt other user's buffer which ends
up leaking some private data from others so there is pointless to keep
system alive to debug it.

Do you mind sending a formal patch?
Thanks!

> 
> ---
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 7c38e850a8fc..685049a9048d 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -1349,7 +1349,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
>          * pools/users, we can't allow mapping in interrupt context
>          * because it can corrupt another users mappings.
>          */
> -       WARN_ON_ONCE(in_interrupt());
> +       BUG_ON(in_interrupt());
>  
>         /* From now on, migration cannot move the object */
>         pin_tag(handle);
> 

Re: [PATCH] zram: fix null dereference of handle

[Sergey Senozhatsky]

Hello,

On (09/20/17 14:51), Minchan Kim wrote:
> On Tue, Sep 19, 2017 at 07:21:25PM +0900, Sergey Senozhatsky wrote:
> > Minchan,
> > 
> > I just ran across it [because I had a bug to analize where this
> > part was involved]. I'd really prefer the kernel to BUG_ON immediately
> > instead of dying in agony.
> > 
> > can we, please, return BUG_ON() back?
> > 
> > there is no point in trying to save the kernel once it did that type
> > of violation.
> 
> I agree. If it happens, it would corrupt other user's buffer which ends
> up leaking some private data from others so there is pointless to keep
> system alive to debug it.

thanks!

yep, I guess we are also potentially looking at __zs_map_object()
preempting zsmalloc in normal context, which would overwrite
area->vm_addr from IRQ, map it, use it, unmap it in __zs_unmap_object().
and then resumed normal context will attempt to use unmapped area->vm_addr,
I suppose.


> Do you mind sending a formal patch?

sure, will do.

	-ss