* [PATCH v2] zram: Fix use-after-free bug in disk write case
@ 2012-11-29 7:45 Nitin Gupta
2012-11-29 7:55 ` Minchan Kim
0 siblings, 1 reply; 4+ messages in thread
From: Nitin Gupta @ 2012-11-29 7:45 UTC (permalink / raw)
To: Greg KH
Cc: Jerome Marchand, Minchan Kim, Seth Jennings, Dan Carpenter,
Sam Hansen, Tomas M, Mihail Kasadjikov, Linux Driver Project,
linux-kernel
Changelog v2 vs v1:
- Changelog message now correctly explains the problem
Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
of incompressible pages") which caused a freed buffer to be used
in case a partial write (non PAGE_SIZED) request is received and
the data is found to be incompressible.
Fixes bug 50081:
https://bugzilla.kernel.org/show_bug.cgi?id=50081
Signed-off-by: Nitin Gupta <ngupta@vflare.org>
Reported-by: Mihail Kasadjikov <hamer.mk@gmail.com>
Reported-by: Tomas M <tomas@slax.org>
Reviewed-by: Minchan Kim <minchan@kernel.org>
---
drivers/staging/zram/zram_drv.c | 39 ++++++++++++++++++++++++---------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index fb4a7c9..f2a73bd 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -265,7 +265,7 @@ out_cleanup:
static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
int offset)
{
- int ret;
+ int ret = 0;
size_t clen;
unsigned long handle;
struct page *page;
@@ -286,10 +286,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
goto out;
}
ret = zram_decompress_page(zram, uncmem, index);
- if (ret) {
- kfree(uncmem);
+ if (ret)
goto out;
- }
}
/*
@@ -302,16 +300,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
user_mem = kmap_atomic(page);
- if (is_partial_io(bvec))
+ if (is_partial_io(bvec)) {
memcpy(uncmem + offset, user_mem + bvec->bv_offset,
bvec->bv_len);
- else
+ kunmap_atomic(user_mem);
+ user_mem = NULL;
+ } else {
uncmem = user_mem;
+ }
if (page_zero_filled(uncmem)) {
- kunmap_atomic(user_mem);
- if (is_partial_io(bvec))
- kfree(uncmem);
+ if (!is_partial_io(bvec))
+ kunmap_atomic(user_mem);
zram_stat_inc(&zram->stats.pages_zero);
zram_set_flag(zram, index, ZRAM_ZERO);
ret = 0;
@@ -321,9 +321,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
zram->compress_workmem);
- kunmap_atomic(user_mem);
- if (is_partial_io(bvec))
- kfree(uncmem);
+ if (!is_partial_io(bvec)) {
+ kunmap_atomic(user_mem);
+ user_mem = NULL;
+ uncmem = NULL;
+ }
if (unlikely(ret != LZO_E_OK)) {
pr_err("Compression failed! err=%d\n", ret);
@@ -332,8 +334,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
if (unlikely(clen > max_zpage_size)) {
zram_stat_inc(&zram->stats.bad_compress);
- src = uncmem;
clen = PAGE_SIZE;
+ src = NULL;
+ if (is_partial_io(bvec))
+ src = uncmem;
}
handle = zs_malloc(zram->mem_pool, clen);
@@ -345,7 +349,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
}
cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
+ if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
+ src = kmap_atomic(page);
memcpy(cmem, src, clen);
+ if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
+ kunmap_atomic(src);
zs_unmap_object(zram->mem_pool, handle);
@@ -358,9 +366,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
if (clen <= PAGE_SIZE / 2)
zram_stat_inc(&zram->stats.good_compress);
- return 0;
-
out:
+ if (is_partial_io(bvec))
+ kfree(uncmem);
+
if (ret)
zram_stat64_inc(zram, &zram->stats.failed_writes);
return ret;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] zram: Fix use-after-free bug in disk write case
2012-11-29 7:45 [PATCH v2] zram: Fix use-after-free bug in disk write case Nitin Gupta
@ 2012-11-29 7:55 ` Minchan Kim
2012-11-29 8:44 ` Nitin Gupta
0 siblings, 1 reply; 4+ messages in thread
From: Minchan Kim @ 2012-11-29 7:55 UTC (permalink / raw)
To: Nitin Gupta
Cc: Greg KH, Jerome Marchand, Seth Jennings, Dan Carpenter,
Sam Hansen, Tomas M, Mihail Kasadjikov, Linux Driver Project,
linux-kernel
Hi Nitin,
On Wed, Nov 28, 2012 at 11:45:06PM -0800, Nitin Gupta wrote:
> Changelog v2 vs v1:
> - Changelog message now correctly explains the problem
>
> Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
> of incompressible pages") which caused a freed buffer to be used
> in case a partial write (non PAGE_SIZED) request is received and
> the data is found to be incompressible.
>
> Fixes bug 50081:
> https://bugzilla.kernel.org/show_bug.cgi?id=50081
When I saw https://bugzilla.kernel.org/attachment.cgi?id=85571, it was
swap write usecase so parital write can not happen.
So this bug isn't related to freed buffer caused by partial write.
This bug is related to unmapped buffer access.
1) user_mem = kmap_atomic
2) uncmem = usermem
3) compress
4) kunmap_atomic(usermem) <-- So, uncmem is dangling.
5) src = uncmem; <-- So, src is dangling.
6) memcpy(cmem, src, clen) <-- HIT
>
> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
> Reported-by: Mihail Kasadjikov <hamer.mk@gmail.com>
> Reported-by: Tomas M <tomas@slax.org>
> Reviewed-by: Minchan Kim <minchan@kernel.org>
> ---
> drivers/staging/zram/zram_drv.c | 39 ++++++++++++++++++++++++---------------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index fb4a7c9..f2a73bd 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -265,7 +265,7 @@ out_cleanup:
> static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> int offset)
> {
> - int ret;
> + int ret = 0;
> size_t clen;
> unsigned long handle;
> struct page *page;
> @@ -286,10 +286,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> goto out;
> }
> ret = zram_decompress_page(zram, uncmem, index);
> - if (ret) {
> - kfree(uncmem);
> + if (ret)
> goto out;
> - }
> }
>
> /*
> @@ -302,16 +300,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>
> user_mem = kmap_atomic(page);
>
> - if (is_partial_io(bvec))
> + if (is_partial_io(bvec)) {
> memcpy(uncmem + offset, user_mem + bvec->bv_offset,
> bvec->bv_len);
> - else
> + kunmap_atomic(user_mem);
> + user_mem = NULL;
> + } else {
> uncmem = user_mem;
> + }
>
> if (page_zero_filled(uncmem)) {
> - kunmap_atomic(user_mem);
> - if (is_partial_io(bvec))
> - kfree(uncmem);
> + if (!is_partial_io(bvec))
> + kunmap_atomic(user_mem);
> zram_stat_inc(&zram->stats.pages_zero);
> zram_set_flag(zram, index, ZRAM_ZERO);
> ret = 0;
> @@ -321,9 +321,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
> zram->compress_workmem);
>
> - kunmap_atomic(user_mem);
> - if (is_partial_io(bvec))
> - kfree(uncmem);
> + if (!is_partial_io(bvec)) {
> + kunmap_atomic(user_mem);
> + user_mem = NULL;
> + uncmem = NULL;
> + }
>
> if (unlikely(ret != LZO_E_OK)) {
> pr_err("Compression failed! err=%d\n", ret);
> @@ -332,8 +334,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>
> if (unlikely(clen > max_zpage_size)) {
> zram_stat_inc(&zram->stats.bad_compress);
> - src = uncmem;
> clen = PAGE_SIZE;
> + src = NULL;
> + if (is_partial_io(bvec))
> + src = uncmem;
> }
>
> handle = zs_malloc(zram->mem_pool, clen);
> @@ -345,7 +349,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> }
> cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
>
> + if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
> + src = kmap_atomic(page);
> memcpy(cmem, src, clen);
> + if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
> + kunmap_atomic(src);
>
> zs_unmap_object(zram->mem_pool, handle);
>
> @@ -358,9 +366,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> if (clen <= PAGE_SIZE / 2)
> zram_stat_inc(&zram->stats.good_compress);
>
> - return 0;
> -
> out:
> + if (is_partial_io(bvec))
> + kfree(uncmem);
> +
> if (ret)
> zram_stat64_inc(zram, &zram->stats.failed_writes);
> return ret;
> --
> 1.7.10.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] zram: Fix use-after-free bug in disk write case
2012-11-29 7:55 ` Minchan Kim
@ 2012-11-29 8:44 ` Nitin Gupta
2012-11-29 8:47 ` Minchan Kim
0 siblings, 1 reply; 4+ messages in thread
From: Nitin Gupta @ 2012-11-29 8:44 UTC (permalink / raw)
To: Minchan Kim
Cc: Greg KH, Jerome Marchand, Seth Jennings, Dan Carpenter,
Sam Hansen, Tomas M, Mihail Kasadjikov, Linux Driver Project,
linux-kernel
On 11/28/2012 11:55 PM, Minchan Kim wrote:
> Hi Nitin,
>
> On Wed, Nov 28, 2012 at 11:45:06PM -0800, Nitin Gupta wrote:
>> Changelog v2 vs v1:
>> - Changelog message now correctly explains the problem
>>
>> Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
>> of incompressible pages") which caused a freed buffer to be used
>> in case a partial write (non PAGE_SIZED) request is received and
>> the data is found to be incompressible.
>>
>> Fixes bug 50081:
>> https://bugzilla.kernel.org/show_bug.cgi?id=50081
>
> When I saw https://bugzilla.kernel.org/attachment.cgi?id=85571, it was
> swap write usecase so parital write can not happen.
> So this bug isn't related to freed buffer caused by partial write.
>
> This bug is related to unmapped buffer access.
>
> 1) user_mem = kmap_atomic
> 2) uncmem = usermem
> 3) compress
> 4) kunmap_atomic(usermem) <-- So, uncmem is dangling.
> 5) src = uncmem; <-- So, src is dangling.
> 6) memcpy(cmem, src, clen) <-- HIT
>
This is what I had in mind, still missed it in description. Will repost
with updated description as below:
zram: fix invalid memory references during disk write
Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
of incompressible pages") which caused invalid memory references
during disk write. Invalid references could occur in two cases:
- Incoming data expands on compression: In this case, reference was
made to kunmap()'ed bio page.
- Partial (non PAGE_SIZE) write with incompressible data: In this
case, reference was made to a kfree()'ed buffer.
Please let me know if the description looks okay.
Thanks,
Nitin
>>
>> Signed-off-by: Nitin Gupta <ngupta@vflare.org>
>> Reported-by: Mihail Kasadjikov <hamer.mk@gmail.com>
>> Reported-by: Tomas M <tomas@slax.org>
>> Reviewed-by: Minchan Kim <minchan@kernel.org>
>> ---
>> drivers/staging/zram/zram_drv.c | 39 ++++++++++++++++++++++++---------------
>> 1 file changed, 24 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index fb4a7c9..f2a73bd 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -265,7 +265,7 @@ out_cleanup:
>> static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> int offset)
>> {
>> - int ret;
>> + int ret = 0;
>> size_t clen;
>> unsigned long handle;
>> struct page *page;
>> @@ -286,10 +286,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> goto out;
>> }
>> ret = zram_decompress_page(zram, uncmem, index);
>> - if (ret) {
>> - kfree(uncmem);
>> + if (ret)
>> goto out;
>> - }
>> }
>>
>> /*
>> @@ -302,16 +300,18 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>
>> user_mem = kmap_atomic(page);
>>
>> - if (is_partial_io(bvec))
>> + if (is_partial_io(bvec)) {
>> memcpy(uncmem + offset, user_mem + bvec->bv_offset,
>> bvec->bv_len);
>> - else
>> + kunmap_atomic(user_mem);
>> + user_mem = NULL;
>> + } else {
>> uncmem = user_mem;
>> + }
>>
>> if (page_zero_filled(uncmem)) {
>> - kunmap_atomic(user_mem);
>> - if (is_partial_io(bvec))
>> - kfree(uncmem);
>> + if (!is_partial_io(bvec))
>> + kunmap_atomic(user_mem);
>> zram_stat_inc(&zram->stats.pages_zero);
>> zram_set_flag(zram, index, ZRAM_ZERO);
>> ret = 0;
>> @@ -321,9 +321,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> ret = lzo1x_1_compress(uncmem, PAGE_SIZE, src, &clen,
>> zram->compress_workmem);
>>
>> - kunmap_atomic(user_mem);
>> - if (is_partial_io(bvec))
>> - kfree(uncmem);
>> + if (!is_partial_io(bvec)) {
>> + kunmap_atomic(user_mem);
>> + user_mem = NULL;
>> + uncmem = NULL;
>> + }
>>
>> if (unlikely(ret != LZO_E_OK)) {
>> pr_err("Compression failed! err=%d\n", ret);
>> @@ -332,8 +334,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>>
>> if (unlikely(clen > max_zpage_size)) {
>> zram_stat_inc(&zram->stats.bad_compress);
>> - src = uncmem;
>> clen = PAGE_SIZE;
>> + src = NULL;
>> + if (is_partial_io(bvec))
>> + src = uncmem;
>> }
>>
>> handle = zs_malloc(zram->mem_pool, clen);
>> @@ -345,7 +349,11 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> }
>> cmem = zs_map_object(zram->mem_pool, handle, ZS_MM_WO);
>>
>> + if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
>> + src = kmap_atomic(page);
>> memcpy(cmem, src, clen);
>> + if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
>> + kunmap_atomic(src);
>>
>> zs_unmap_object(zram->mem_pool, handle);
>>
>> @@ -358,9 +366,10 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>> if (clen <= PAGE_SIZE / 2)
>> zram_stat_inc(&zram->stats.good_compress);
>>
>> - return 0;
>> -
>> out:
>> + if (is_partial_io(bvec))
>> + kfree(uncmem);
>> +
>> if (ret)
>> zram_stat64_inc(zram, &zram->stats.failed_writes);
>> return ret;
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] zram: Fix use-after-free bug in disk write case
2012-11-29 8:44 ` Nitin Gupta
@ 2012-11-29 8:47 ` Minchan Kim
0 siblings, 0 replies; 4+ messages in thread
From: Minchan Kim @ 2012-11-29 8:47 UTC (permalink / raw)
To: Nitin Gupta
Cc: Greg KH, Jerome Marchand, Seth Jennings, Dan Carpenter,
Sam Hansen, Tomas M, Mihail Kasadjikov, Linux Driver Project,
linux-kernel
On Thu, Nov 29, 2012 at 12:44:14AM -0800, Nitin Gupta wrote:
> On 11/28/2012 11:55 PM, Minchan Kim wrote:
> >Hi Nitin,
> >
> >On Wed, Nov 28, 2012 at 11:45:06PM -0800, Nitin Gupta wrote:
> >>Changelog v2 vs v1:
> >> - Changelog message now correctly explains the problem
> >>
> >>Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
> >>of incompressible pages") which caused a freed buffer to be used
> >>in case a partial write (non PAGE_SIZED) request is received and
> >>the data is found to be incompressible.
> >>
> >>Fixes bug 50081:
> >>https://bugzilla.kernel.org/show_bug.cgi?id=50081
> >
> >When I saw https://bugzilla.kernel.org/attachment.cgi?id=85571, it was
> >swap write usecase so parital write can not happen.
> >So this bug isn't related to freed buffer caused by partial write.
> >
> >This bug is related to unmapped buffer access.
> >
> >1) user_mem = kmap_atomic
> >2) uncmem = usermem
> >3) compress
> >4) kunmap_atomic(usermem) <-- So, uncmem is dangling.
> >5) src = uncmem; <-- So, src is dangling.
> >6) memcpy(cmem, src, clen) <-- HIT
> >
>
> This is what I had in mind, still missed it in description. Will
> repost with updated description as below:
>
> zram: fix invalid memory references during disk write
>
> Fixes a bug introduced by commit c8f2f0db1 ("zram: Fix handling
> of incompressible pages") which caused invalid memory references
> during disk write. Invalid references could occur in two cases:
> - Incoming data expands on compression: In this case, reference was
> made to kunmap()'ed bio page.
> - Partial (non PAGE_SIZE) write with incompressible data: In this
> case, reference was made to a kfree()'ed buffer.
>
>
> Please let me know if the description looks okay.
Looks good to me.
Thanks!
--
Kind regards,
Minchan Kim
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-11-29 8:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-11-29 7:45 [PATCH v2] zram: Fix use-after-free bug in disk write case Nitin Gupta
2012-11-29 7:55 ` Minchan Kim
2012-11-29 8:44 ` Nitin Gupta
2012-11-29 8:47 ` Minchan Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox