[PATCH] zram: fix possible use after free in zcomp_create()

[PATCH] zram: fix possible use after free in zcomp_create()

[Luis Henriques]

zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
through comp->stream, which can potentially be pointing to memory that was
freed if these functions returned an error.

Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
Cc: stable@vger.kernel.org
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
 drivers/block/zram/zcomp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 965d1afb0eaa..f3db8158e172 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -212,6 +212,7 @@ static int zcomp_strm_multi_create(struct zcomp *comp, int max_strm)
 	zstrm = zcomp_strm_alloc(comp);
 	if (!zstrm) {
 		kfree(zs);
+		comp->stream = NULL;
 		return -ENOMEM;
 	}
 	list_add(&zstrm->list, &zs->idle_strm);
@@ -262,6 +263,7 @@ static int zcomp_strm_single_create(struct zcomp *comp)
 	zs->zstrm = zcomp_strm_alloc(comp);
 	if (!zs->zstrm) {
 		kfree(zs);
+		comp->stream = NULL;
 		return -ENOMEM;
 	}
 	return 0;

Re: [PATCH] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/07/15 11:33), Luis Henriques wrote:
> zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> through comp->stream, which can potentially be pointing to memory that was
> freed if these functions returned an error.
> 

good catch.

we probably better start checking the zcomp_strm_multi_create()/
zcomp_strm_single_create() status in zcomp_create(); that's much
more obvious and that's why we return it in the first place.

what do you think?

---

 drivers/block/zram/zcomp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 3456d5a..16bbc8c 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -360,6 +360,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
 {
 	struct zcomp *comp;
 	struct zcomp_backend *backend;
+	int ret;
 
 	backend = find_backend(compress);
 	if (!backend)
@@ -371,10 +372,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
 
 	comp->backend = backend;
 	if (max_strm > 1)
-		zcomp_strm_multi_create(comp, max_strm);
+		ret = zcomp_strm_multi_create(comp, max_strm);
 	else
-		zcomp_strm_single_create(comp);
-	if (!comp->stream) {
+		ret = zcomp_strm_single_create(comp);
+	if (ret != 0) {
 		kfree(comp);
 		return ERR_PTR(-ENOMEM);
 	}

Re: [PATCH] zram: fix possible use after free in zcomp_create()

[Luis Henriques]

On Mon, Sep 07, 2015 at 08:11:48PM +0900, Sergey Senozhatsky wrote:
> On (09/07/15 11:33), Luis Henriques wrote:
> > zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> > through comp->stream, which can potentially be pointing to memory that was
> > freed if these functions returned an error.
> > 
> 
> good catch.
> 
> we probably better start checking the zcomp_strm_multi_create()/
> zcomp_strm_single_create() status in zcomp_create(); that's much
> more obvious and that's why we return it in the first place.
> 
> what do you think?
>

Yep, that's probably a better solution.

Acked-by: Luis Henriques <luis.henriques@canonical.com>

Cheers,
--
Luís

> ---
> 
>  drivers/block/zram/zcomp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 3456d5a..16bbc8c 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -360,6 +360,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  {
>  	struct zcomp *comp;
>  	struct zcomp_backend *backend;
> +	int ret;
>  
>  	backend = find_backend(compress);
>  	if (!backend)
> @@ -371,10 +372,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  
>  	comp->backend = backend;
>  	if (max_strm > 1)
> -		zcomp_strm_multi_create(comp, max_strm);
> +		ret = zcomp_strm_multi_create(comp, max_strm);
>  	else
> -		zcomp_strm_single_create(comp);
> -	if (!comp->stream) {
> +		ret = zcomp_strm_single_create(comp);
> +	if (ret != 0) {
>  		kfree(comp);
>  		return ERR_PTR(-ENOMEM);
>  	}
> 

Re: [PATCH] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/07/15 13:53), Luis Henriques wrote:
> > On (09/07/15 11:33), Luis Henriques wrote:
> > > zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> > > through comp->stream, which can potentially be pointing to memory that was
> > > freed if these functions returned an error.
> > > 
> > 
> > good catch.
> > 
> > we probably better start checking the zcomp_strm_multi_create()/
> > zcomp_strm_single_create() status in zcomp_create(); that's much
> > more obvious and that's why we return it in the first place.
> > 
> > what do you think?
> >
> 
> Yep, that's probably a better solution.
> 
> Acked-by: Luis Henriques <luis.henriques@canonical.com>
> 

Oh, thanks. I don't mind if you will re-submit it; I just did minor
changes to our fix. Or I can handle it. I'm OK with either way. Btw,
did you hit that problem or you reviewed the code?

	-ss

> Cheers,
> --
> Luís
> 
> > ---
> > 
> >  drivers/block/zram/zcomp.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 3456d5a..16bbc8c 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -360,6 +360,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  {
> >  	struct zcomp *comp;
> >  	struct zcomp_backend *backend;
> > +	int ret;
> >  
> >  	backend = find_backend(compress);
> >  	if (!backend)
> > @@ -371,10 +372,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  
> >  	comp->backend = backend;
> >  	if (max_strm > 1)
> > -		zcomp_strm_multi_create(comp, max_strm);
> > +		ret = zcomp_strm_multi_create(comp, max_strm);
> >  	else
> > -		zcomp_strm_single_create(comp);
> > -	if (!comp->stream) {
> > +		ret = zcomp_strm_single_create(comp);
> > +	if (ret != 0) {
> >  		kfree(comp);
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> > 
> 

Re: [PATCH] zram: fix possible use after free in zcomp_create()

[Luis Henriques]

On Mon, Sep 07, 2015 at 10:33:32PM +0900, Sergey Senozhatsky wrote:
> On (09/07/15 13:53), Luis Henriques wrote:
> > > On (09/07/15 11:33), Luis Henriques wrote:
> > > > zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> > > > through comp->stream, which can potentially be pointing to memory that was
> > > > freed if these functions returned an error.
> > > > 
> > > 
> > > good catch.
> > > 
> > > we probably better start checking the zcomp_strm_multi_create()/
> > > zcomp_strm_single_create() status in zcomp_create(); that's much
> > > more obvious and that's why we return it in the first place.
> > > 
> > > what do you think?
> > >
> > 
> > Yep, that's probably a better solution.
> > 
> > Acked-by: Luis Henriques <luis.henriques@canonical.com>
> > 
> 
> Oh, thanks. I don't mind if you will re-submit it; I just did minor
> changes to our fix. Or I can handle it. I'm OK with either way.

Ok, I'll be sending v2 in a minute.

>Btw, did you hit that problem or you reviewed the code?
>

I've found this while looking at the code (I wouldn't really call it a
"code review"... :-))

Cheers,
--
Luís

[PATCH v2] zram: fix possible use after free in zcomp_create()

[Luis Henriques]

zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
through comp->stream, which can potentially be pointing to memory that was
freed if these functions returned an error.

Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
Cc: stable@vger.kernel.org
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---
Changes since v1:
 * Check zcomp_strm_{multi,siggle}_create() return code instead
   comp->stream (suggested by Sergey)

drivers/block/zram/zcomp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 965d1afb0eaa..8d2cdfd307db 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -336,6 +336,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
 {
 	struct zcomp *comp;
 	struct zcomp_backend *backend;
+	int ret;
 
 	backend = find_backend(compress);
 	if (!backend)
@@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
 
 	comp->backend = backend;
 	if (max_strm > 1)
-		zcomp_strm_multi_create(comp, max_strm);
+		ret = zcomp_strm_multi_create(comp, max_strm);
 	else
-		zcomp_strm_single_create(comp);
-	if (!comp->stream) {
+		ret = zcomp_strm_single_create(comp);
+	if (ret) {
 		kfree(comp);
 		return ERR_PTR(-ENOMEM);
 	}

Re: [PATCH v2] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/07/15 15:13), Luis Henriques wrote:
> zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> through comp->stream, which can potentially be pointing to memory that was
> freed if these functions returned an error.
> 
> Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
> Cc: stable@vger.kernel.org
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>

Cc Andrew


Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> ---
> Changes since v1:
>  * Check zcomp_strm_{multi,siggle}_create() return code instead
>    comp->stream (suggested by Sergey)
> 
> drivers/block/zram/zcomp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 965d1afb0eaa..8d2cdfd307db 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -336,6 +336,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  {
>  	struct zcomp *comp;
>  	struct zcomp_backend *backend;
> +	int ret;
>  
>  	backend = find_backend(compress);
>  	if (!backend)
> @@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  
>  	comp->backend = backend;
>  	if (max_strm > 1)
> -		zcomp_strm_multi_create(comp, max_strm);
> +		ret = zcomp_strm_multi_create(comp, max_strm);
>  	else
> -		zcomp_strm_single_create(comp);
> -	if (!comp->stream) {
> +		ret = zcomp_strm_single_create(comp);
> +	if (ret) {
>  		kfree(comp);
>  		return ERR_PTR(-ENOMEM);
>  	}
> 

Re: [PATCH v2] zram: fix possible use after free in zcomp_create()

[Minchan Kim]

Hello,

First of all, Thanks for catching a bug and review, Guys.
Below there are just some cleanup.
If you guys think it's better, please respin.

On Mon, Sep 07, 2015 at 03:13:10PM +0100, Luis Henriques wrote:
> zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> through comp->stream, which can potentially be pointing to memory that was
> freed if these functions returned an error.
> 
> Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
> Cc: stable@vger.kernel.org
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
> ---
> Changes since v1:
>  * Check zcomp_strm_{multi,siggle}_create() return code instead
>    comp->stream (suggested by Sergey)
> 
> drivers/block/zram/zcomp.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 965d1afb0eaa..8d2cdfd307db 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -336,6 +336,7 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  {
>  	struct zcomp *comp;
>  	struct zcomp_backend *backend;
> +	int ret;

For the clarification, I want to call it as 'error' instead of ret.

>  
>  	backend = find_backend(compress);
>  	if (!backend)
> @@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  
>  	comp->backend = backend;
>  	if (max_strm > 1)
> -		zcomp_strm_multi_create(comp, max_strm);
> +		ret = zcomp_strm_multi_create(comp, max_strm);
>  	else
> -		zcomp_strm_single_create(comp);
> -	if (!comp->stream) {
> +		ret = zcomp_strm_single_create(comp);
> +	if (ret) {
>  		kfree(comp);
>  		return ERR_PTR(-ENOMEM);
>  	}

And we could return ERR_PTR(error) rather than fixed -ENOMEM to propagate
other errors potentially could be happen in future(ex, crypto support).
Of course, we should change description of the function about error return.

Thanks.

Re: [PATCH v2] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/08/15 10:07), Minchan Kim wrote:
[..]
> > +	int ret;
> 
> For the clarification, I want to call it as 'error' instead of ret.
> 
> >  
> >  	backend = find_backend(compress);
> >  	if (!backend)
> > @@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  
> >  	comp->backend = backend;
> >  	if (max_strm > 1)
> > -		zcomp_strm_multi_create(comp, max_strm);
> > +		ret = zcomp_strm_multi_create(comp, max_strm);
> >  	else
> > -		zcomp_strm_single_create(comp);
> > -	if (!comp->stream) {
> > +		ret = zcomp_strm_single_create(comp);
> > +	if (ret) {
> >  		kfree(comp);
> >  		return ERR_PTR(-ENOMEM);
> >  	}
> 
> And we could return ERR_PTR(error) rather than fixed -ENOMEM to propagate

yep, I thought about that; looks to be minor so I didn't insist
on changing that. don't mind to change it to ERR_PTR(error).

> other errors potentially could be happen in future(ex, crypto support).
> Of course, we should change description of the function about error return.

on the other hand, this is zcomp_strm_FOO_create(), which is mostly
about memory allocation. but yeah, who knows. We can change this as
a part of crypto api rework; ERR_PTR(error) does not fix anything
per se, so may be we can avoid pushing this change into stable.

	-ss

Re: [PATCH v2] zram: fix possible use after free in zcomp_create()

[Minchan Kim]

On Tue, Sep 08, 2015 at 10:20:38AM +0900, Sergey Senozhatsky wrote:
> On (09/08/15 10:07), Minchan Kim wrote:
> [..]
> > > +	int ret;
> > 
> > For the clarification, I want to call it as 'error' instead of ret.
> > 
> > >  
> > >  	backend = find_backend(compress);
> > >  	if (!backend)
> > > @@ -347,10 +348,10 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> > >  
> > >  	comp->backend = backend;
> > >  	if (max_strm > 1)
> > > -		zcomp_strm_multi_create(comp, max_strm);
> > > +		ret = zcomp_strm_multi_create(comp, max_strm);
> > >  	else
> > > -		zcomp_strm_single_create(comp);
> > > -	if (!comp->stream) {
> > > +		ret = zcomp_strm_single_create(comp);
> > > +	if (ret) {
> > >  		kfree(comp);
> > >  		return ERR_PTR(-ENOMEM);
> > >  	}
> > 
> > And we could return ERR_PTR(error) rather than fixed -ENOMEM to propagate
> 
> yep, I thought about that; looks to be minor so I didn't insist
> on changing that. don't mind to change it to ERR_PTR(error).

Thanks.

> 
> > other errors potentially could be happen in future(ex, crypto support).
> > Of course, we should change description of the function about error return.
> 
> on the other hand, this is zcomp_strm_FOO_create(), which is mostly
> about memory allocation. but yeah, who knows. We can change this as
> a part of crypto api rework; ERR_PTR(error) does not fix anything
> per se, so may be we can avoid pushing this change into stable.

Strictly speaking, you're right. I asked two things bug fixing and
refactoring. But I thought it is really trivial enough to push
-stable.

With bonus, it's more readable(ie, error naming) and makes sense(
ie, not assume under layer function always fails with no memory).

So, hope to fix it altogether if anyone isn't strong against that.

Thank you.

[PATCH v3] zram: fix possible use after free in zcomp_create()

[Luis Henriques]

zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
through comp->stream, which can potentially be pointing to memory that was
freed if these functions returned an error.

While at it, replace a 'ERR_PTR(-ENOMEM)' by a more generic
'ERR_PTR(error)' as in the future zcomp_strm_{multi,siggle}_create() could
return other error codes.  Function documentation updated accordingly.

Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
Cc: stable@vger.kernel.org
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Minchan Kim <minchan@kernel.org>
Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
---

Changes since v2:
 * Renamed local variable 'ret' to 'error'
 * Usage of 'ERR_PTR(error)' to accommodate future error codes returned by
   zcomp_strm_{multi,siggle}_create
 (all suggested by Minchan)

Changes since v1:
 * Check zcomp_strm_{multi,siggle}_create() return code instead
   comp->stream (suggested by Sergey)

drivers/block/zram/zcomp.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
index 965d1afb0eaa..5cb13ca3a3ac 100644
--- a/drivers/block/zram/zcomp.c
+++ b/drivers/block/zram/zcomp.c
@@ -330,12 +330,14 @@ void zcomp_destroy(struct zcomp *comp)
  * allocate new zcomp and initialize it. return compressing
  * backend pointer or ERR_PTR if things went bad. ERR_PTR(-EINVAL)
  * if requested algorithm is not supported, ERR_PTR(-ENOMEM) in
- * case of allocation error.
+ * case of allocation error, or any other error potentially
+ * returned by functions zcomp_strm_{multi,single}_create.
  */
 struct zcomp *zcomp_create(const char *compress, int max_strm)
 {
 	struct zcomp *comp;
 	struct zcomp_backend *backend;
+	int error;
 
 	backend = find_backend(compress);
 	if (!backend)
@@ -347,12 +349,12 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
 
 	comp->backend = backend;
 	if (max_strm > 1)
-		zcomp_strm_multi_create(comp, max_strm);
+		error = zcomp_strm_multi_create(comp, max_strm);
 	else
-		zcomp_strm_single_create(comp);
-	if (!comp->stream) {
+		error = zcomp_strm_single_create(comp);
+	if (error) {
 		kfree(comp);
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(error);
 	}
 	return comp;
 }

Re: [PATCH v3] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/08/15 10:39), Luis Henriques wrote:
> zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> through comp->stream, which can potentially be pointing to memory that was
> freed if these functions returned an error.
> 
> While at it, replace a 'ERR_PTR(-ENOMEM)' by a more generic
> 'ERR_PTR(error)' as in the future zcomp_strm_{multi,siggle}_create() could
> return other error codes.  Function documentation updated accordingly.
> 
> Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
> Cc: stable@vger.kernel.org
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>


Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> ---
> 
> Changes since v2:
>  * Renamed local variable 'ret' to 'error'
>  * Usage of 'ERR_PTR(error)' to accommodate future error codes returned by
>    zcomp_strm_{multi,siggle}_create
>  (all suggested by Minchan)
> 
> Changes since v1:
>  * Check zcomp_strm_{multi,siggle}_create() return code instead
>    comp->stream (suggested by Sergey)
> 
> drivers/block/zram/zcomp.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> index 965d1afb0eaa..5cb13ca3a3ac 100644
> --- a/drivers/block/zram/zcomp.c
> +++ b/drivers/block/zram/zcomp.c
> @@ -330,12 +330,14 @@ void zcomp_destroy(struct zcomp *comp)
>   * allocate new zcomp and initialize it. return compressing
>   * backend pointer or ERR_PTR if things went bad. ERR_PTR(-EINVAL)
>   * if requested algorithm is not supported, ERR_PTR(-ENOMEM) in
> - * case of allocation error.
> + * case of allocation error, or any other error potentially
> + * returned by functions zcomp_strm_{multi,single}_create.
>   */
>  struct zcomp *zcomp_create(const char *compress, int max_strm)
>  {
>  	struct zcomp *comp;
>  	struct zcomp_backend *backend;
> +	int error;
>  
>  	backend = find_backend(compress);
>  	if (!backend)
> @@ -347,12 +349,12 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
>  
>  	comp->backend = backend;
>  	if (max_strm > 1)
> -		zcomp_strm_multi_create(comp, max_strm);
> +		error = zcomp_strm_multi_create(comp, max_strm);
>  	else
> -		zcomp_strm_single_create(comp);
> -	if (!comp->stream) {
> +		error = zcomp_strm_single_create(comp);
> +	if (error) {
>  		kfree(comp);
> -		return ERR_PTR(-ENOMEM);
> +		return ERR_PTR(error);
>  	}
>  	return comp;
>  }
> 

Re: [PATCH v3] zram: fix possible use after free in zcomp_create()

[Sergey Senozhatsky]

On (09/08/15 18:56), Sergey Senozhatsky wrote:
> On (09/08/15 10:39), Luis Henriques wrote:
> > zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> > through comp->stream, which can potentially be pointing to memory that was
> > freed if these functions returned an error.
> > 
> > While at it, replace a 'ERR_PTR(-ENOMEM)' by a more generic
> > 'ERR_PTR(error)' as in the future zcomp_strm_{multi,siggle}_create() could
> > return other error codes.  Function documentation updated accordingly.
> > 

Oh...forgot to Cc Andrew
Andrew, please disregard V2 of this patch.

V3
Acked-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss

> > ---
> > 
> > Changes since v2:
> >  * Renamed local variable 'ret' to 'error'
> >  * Usage of 'ERR_PTR(error)' to accommodate future error codes returned by
> >    zcomp_strm_{multi,siggle}_create
> >  (all suggested by Minchan)
> > 
> > Changes since v1:
> >  * Check zcomp_strm_{multi,siggle}_create() return code instead
> >    comp->stream (suggested by Sergey)
> > 
> > drivers/block/zram/zcomp.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zcomp.c b/drivers/block/zram/zcomp.c
> > index 965d1afb0eaa..5cb13ca3a3ac 100644
> > --- a/drivers/block/zram/zcomp.c
> > +++ b/drivers/block/zram/zcomp.c
> > @@ -330,12 +330,14 @@ void zcomp_destroy(struct zcomp *comp)
> >   * allocate new zcomp and initialize it. return compressing
> >   * backend pointer or ERR_PTR if things went bad. ERR_PTR(-EINVAL)
> >   * if requested algorithm is not supported, ERR_PTR(-ENOMEM) in
> > - * case of allocation error.
> > + * case of allocation error, or any other error potentially
> > + * returned by functions zcomp_strm_{multi,single}_create.
> >   */
> >  struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  {
> >  	struct zcomp *comp;
> >  	struct zcomp_backend *backend;
> > +	int error;
> >  
> >  	backend = find_backend(compress);
> >  	if (!backend)
> > @@ -347,12 +349,12 @@ struct zcomp *zcomp_create(const char *compress, int max_strm)
> >  
> >  	comp->backend = backend;
> >  	if (max_strm > 1)
> > -		zcomp_strm_multi_create(comp, max_strm);
> > +		error = zcomp_strm_multi_create(comp, max_strm);
> >  	else
> > -		zcomp_strm_single_create(comp);
> > -	if (!comp->stream) {
> > +		error = zcomp_strm_single_create(comp);
> > +	if (error) {
> >  		kfree(comp);
> > -		return ERR_PTR(-ENOMEM);
> > +		return ERR_PTR(error);
> >  	}
> >  	return comp;
> >  }
> > 
> 

Re: [PATCH v3] zram: fix possible use after free in zcomp_create()

[Minchan Kim]

On Tue, Sep 08, 2015 at 10:39:28AM +0100, Luis Henriques wrote:
> zcomp_create() verifies the success of zcomp_strm_{multi,siggle}_create()
> through comp->stream, which can potentially be pointing to memory that was
> freed if these functions returned an error.
> 
> While at it, replace a 'ERR_PTR(-ENOMEM)' by a more generic
> 'ERR_PTR(error)' as in the future zcomp_strm_{multi,siggle}_create() could
> return other error codes.  Function documentation updated accordingly.
> 
> Fixes: beca3ec71fe5 ("zram: add multi stream functionality")
> Cc: stable@vger.kernel.org
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Luis Henriques <luis.henriques@canonical.com>
Acked-by: Minchan Kim <minchan@kernel.org>

Thanks for the fix!