Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
> Signed-off-by: Luis Henriques
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
> Signed-off-by: Luis Henriques
Cc Andrew
Acked-by: Sergey Senozhatsky
-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);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
Signed-off-by: Luis Henriques
---
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);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
Signed-off-by: Luis Henriques
---
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);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
> Signed-off-by: Luis Henriques
Cc Andrew
Acked-by: Sergey Senozhatsky
-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);
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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: sta...@vger.kernel.org
> Signed-off-by: Luis Henriques
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] zram: fix possible use after free in zcomp_create()
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
