Linux-EROFS Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] erofs: correct the remaining shrink objects
@ 2020-02-26  2:30 Gao Xiang
  2020-02-26  2:30 ` [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding Gao Xiang
  2020-02-26  2:30 ` [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be Gao Xiang
  0 siblings, 2 replies; 8+ messages in thread
From: Gao Xiang @ 2020-02-26  2:30 UTC (permalink / raw)
  To: Chao Yu, linux-erofs; +Cc: Miao Xie, LKML

The remaining count should not included successful
shrink attempts.

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/erofs/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/erofs/utils.c b/fs/erofs/utils.c
index fddc5059c930..df42ea552a44 100644
--- a/fs/erofs/utils.c
+++ b/fs/erofs/utils.c
@@ -286,7 +286,7 @@ static unsigned long erofs_shrink_scan(struct shrinker *shrink,
 		spin_unlock(&erofs_sb_list_lock);
 		sbi->shrinker_run_no = run_no;
 
-		freed += erofs_shrink_workstation(sbi, nr);
+		freed += erofs_shrink_workstation(sbi, nr - freed);
 
 		spin_lock(&erofs_sb_list_lock);
 		/* Get the next list element before we move this one */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding
  2020-02-26  2:30 [PATCH 1/3] erofs: correct the remaining shrink objects Gao Xiang
@ 2020-02-26  2:30 ` Gao Xiang
  2020-03-03  9:49   ` Chao Yu
  2020-02-26  2:30 ` [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be Gao Xiang
  1 sibling, 1 reply; 8+ messages in thread
From: Gao Xiang @ 2020-02-26  2:30 UTC (permalink / raw)
  To: Chao Yu, linux-erofs; +Cc: Miao Xie, LKML, Lasse Collin

As Lasse pointed out, "EROFS uses LZ4_decompress_safe_partial
for both partial and full blocks. Thus when it is decoding a
full block, it doesn't know if the LZ4 decoder actually decoded
all the input. The real uncompressed size could be bigger than
the value stored in the file system metadata.

Using LZ4_decompress_safe instead of _safe_partial when
decompressing a full block would help to detect errors."

So it's reasonable to use _safe in case of corrupted images and
it might have some speed gain as well although I didn't observe
much difference.

Cc: Lasse Collin <lasse.collin@tukaani.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/erofs/decompressor.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index 5779a15c2cd6..c77cec4327fa 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -157,9 +157,14 @@ static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out)
 		}
 	}
 
-	ret = LZ4_decompress_safe_partial(src + inputmargin, out,
-					  inlen, rq->outputsize,
-					  rq->outputsize);
+	if (rq->partial_decoding)
+		ret = LZ4_decompress_safe_partial(src + inputmargin, out,
+						  inlen, rq->outputsize,
+						  rq->outputsize);
+	else
+		ret = LZ4_decompress_safe(src + inputmargin, out,
+					  inlen, rq->outputsize);
+
 	if (ret < 0) {
 		erofs_err(rq->sb, "failed to decompress, in[%u, %u] out[%u]",
 			  inlen, inputmargin, rq->outputsize);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be
  2020-02-26  2:30 [PATCH 1/3] erofs: correct the remaining shrink objects Gao Xiang
  2020-02-26  2:30 ` [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding Gao Xiang
@ 2020-02-26  2:30 ` Gao Xiang
  2020-02-26  2:34   ` Eric Biggers
  2020-03-03  9:51   ` Chao Yu
  1 sibling, 2 replies; 8+ messages in thread
From: Gao Xiang @ 2020-02-26  2:30 UTC (permalink / raw)
  To: Chao Yu, linux-erofs; +Cc: Miao Xie, LKML, Lasse Collin

As Lasse pointed out, "Looking at fs/erofs/decompress.c,
the return value from LZ4_decompress_safe_partial is only
checked for negative value to catch errors. ... So if
I understood it correctly, if there is bad data whose
uncompressed size is much less than it should be, it can
leave part of the output buffer untouched and expose the
previous data as the file content. "

Let's fix it now.

Cc: Lasse Collin <lasse.collin@tukaani.org>
Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/erofs/decompressor.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/erofs/decompressor.c b/fs/erofs/decompressor.c
index c77cec4327fa..be8d9adef236 100644
--- a/fs/erofs/decompressor.c
+++ b/fs/erofs/decompressor.c
@@ -165,14 +165,18 @@ static int z_erofs_lz4_decompress(struct z_erofs_decompress_req *rq, u8 *out)
 		ret = LZ4_decompress_safe(src + inputmargin, out,
 					  inlen, rq->outputsize);
 
-	if (ret < 0) {
-		erofs_err(rq->sb, "failed to decompress, in[%u, %u] out[%u]",
-			  inlen, inputmargin, rq->outputsize);
+	if (ret != rq->outputsize) {
+		erofs_err(rq->sb, "failed to decompress %d in[%u, %u] out[%u]",
+			  ret, inlen, inputmargin, rq->outputsize);
+
 		WARN_ON(1);
 		print_hex_dump(KERN_DEBUG, "[ in]: ", DUMP_PREFIX_OFFSET,
 			       16, 1, src + inputmargin, inlen, true);
 		print_hex_dump(KERN_DEBUG, "[out]: ", DUMP_PREFIX_OFFSET,
 			       16, 1, out, rq->outputsize, true);
+
+		if (ret >= 0)
+			memset(out + ret, 0, rq->outputsize - ret);
 		ret = -EIO;
 	}
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be
  2020-02-26  2:30 ` [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be Gao Xiang
@ 2020-02-26  2:34   ` Eric Biggers
  2020-02-26  2:40     ` Gao Xiang
  2020-03-03  9:51   ` Chao Yu
  1 sibling, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-02-26  2:34 UTC (permalink / raw)
  To: Gao Xiang; +Cc: Miao Xie, linux-erofs, LKML, Lasse Collin

On Wed, Feb 26, 2020 at 10:30:11AM +0800, Gao Xiang wrote:
> As Lasse pointed out, "Looking at fs/erofs/decompress.c,
> the return value from LZ4_decompress_safe_partial is only
> checked for negative value to catch errors. ... So if
> I understood it correctly, if there is bad data whose
> uncompressed size is much less than it should be, it can
> leave part of the output buffer untouched and expose the
> previous data as the file content. "
> 
> Let's fix it now.
> 
> Cc: Lasse Collin <lasse.collin@tukaani.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Shouldn't fixes like this have a Fixes tag and Cc stable?

- Eric

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be
  2020-02-26  2:34   ` Eric Biggers
@ 2020-02-26  2:40     ` Gao Xiang
  2020-02-26  2:44       ` Gao Xiang
  0 siblings, 1 reply; 8+ messages in thread
From: Gao Xiang @ 2020-02-26  2:40 UTC (permalink / raw)
  To: Eric Biggers; +Cc: Miao Xie, linux-erofs, LKML, Lasse Collin

Hi Eric,

On Tue, Feb 25, 2020 at 06:34:58PM -0800, Eric Biggers wrote:
> On Wed, Feb 26, 2020 at 10:30:11AM +0800, Gao Xiang wrote:
> > As Lasse pointed out, "Looking at fs/erofs/decompress.c,
> > the return value from LZ4_decompress_safe_partial is only
> > checked for negative value to catch errors. ... So if
> > I understood it correctly, if there is bad data whose
> > uncompressed size is much less than it should be, it can
> > leave part of the output buffer untouched and expose the
> > previous data as the file content. "
> > 
> > Let's fix it now.
> > 
> > Cc: Lasse Collin <lasse.collin@tukaani.org>
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> 
> Shouldn't fixes like this have a Fixes tag and Cc stable?
> 
> - Eric

Thanks for pointing out. *thumb up*

I reminded Fixes and Cc tags when I sent out. Yet
I'm not quite sure if these have some other potential
concernes which could cause unexpected behavior for
normal images (It seems impossible but not quite sure.)

I'd like to leave these two commits for corrupted images
to mainline and our products for a while and manually
backport to stable kernels and send them to stable
mailing list later. I keep these fixes in mind all
the time.

Thanks,
Gao Xiang


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be
  2020-02-26  2:40     ` Gao Xiang
@ 2020-02-26  2:44       ` Gao Xiang
  0 siblings, 0 replies; 8+ messages in thread
From: Gao Xiang @ 2020-02-26  2:44 UTC (permalink / raw)
  To: Eric Biggers; +Cc: Miao Xie, linux-erofs, LKML, Lasse Collin

On Wed, Feb 26, 2020 at 10:40:47AM +0800, Gao Xiang wrote:
> Hi Eric,
> 
> On Tue, Feb 25, 2020 at 06:34:58PM -0800, Eric Biggers wrote:
> > On Wed, Feb 26, 2020 at 10:30:11AM +0800, Gao Xiang wrote:
> > > As Lasse pointed out, "Looking at fs/erofs/decompress.c,
> > > the return value from LZ4_decompress_safe_partial is only
> > > checked for negative value to catch errors. ... So if
> > > I understood it correctly, if there is bad data whose
> > > uncompressed size is much less than it should be, it can
> > > leave part of the output buffer untouched and expose the
> > > previous data as the file content. "
> > > 
> > > Let's fix it now.
> > > 
> > > Cc: Lasse Collin <lasse.collin@tukaani.org>
> > > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > 
> > Shouldn't fixes like this have a Fixes tag and Cc stable?
> > 
> > - Eric
> 
> Thanks for pointing out. *thumb up*
> 
> I reminded Fixes and Cc tags when I sent out. Yet
> I'm not quite sure if these have some other potential
> concernes which could cause unexpected behavior for
> normal images (It seems impossible but not quite sure.)
> 
> I'd like to leave these two commits for corrupted images
> to mainline and our products for a while and manually
> backport to stable kernels and send them to stable
> mailing list later. I keep these fixes in mind all
> the time.

... Maybe I should add "Fixes:" tag in the commit message
anyway. Will resend them later.

Thanks,
Gao Xiang

> 
> Thanks,
> Gao Xiang
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding
  2020-02-26  2:30 ` [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding Gao Xiang
@ 2020-03-03  9:49   ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2020-03-03  9:49 UTC (permalink / raw)
  To: Gao Xiang, linux-erofs; +Cc: Lasse Collin, Miao Xie, LKML

On 2020/2/26 10:30, Gao Xiang wrote:
> As Lasse pointed out, "EROFS uses LZ4_decompress_safe_partial
> for both partial and full blocks. Thus when it is decoding a
> full block, it doesn't know if the LZ4 decoder actually decoded
> all the input. The real uncompressed size could be bigger than
> the value stored in the file system metadata.
> 
> Using LZ4_decompress_safe instead of _safe_partial when
> decompressing a full block would help to detect errors."
> 
> So it's reasonable to use _safe in case of corrupted images and
> it might have some speed gain as well although I didn't observe
> much difference.
> 
> Cc: Lasse Collin <lasse.collin@tukaani.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be
  2020-02-26  2:30 ` [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be Gao Xiang
  2020-02-26  2:34   ` Eric Biggers
@ 2020-03-03  9:51   ` Chao Yu
  1 sibling, 0 replies; 8+ messages in thread
From: Chao Yu @ 2020-03-03  9:51 UTC (permalink / raw)
  To: Gao Xiang, linux-erofs; +Cc: Lasse Collin, Miao Xie, LKML

On 2020/2/26 10:30, Gao Xiang wrote:
> As Lasse pointed out, "Looking at fs/erofs/decompress.c,
> the return value from LZ4_decompress_safe_partial is only
> checked for negative value to catch errors. ... So if
> I understood it correctly, if there is bad data whose
> uncompressed size is much less than it should be, it can
> leave part of the output buffer untouched and expose the
> previous data as the file content. "
> 
> Let's fix it now.
> 
> Cc: Lasse Collin <lasse.collin@tukaani.org>
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-26  2:30 [PATCH 1/3] erofs: correct the remaining shrink objects Gao Xiang
2020-02-26  2:30 ` [PATCH 2/3] erofs: use LZ4_decompress_safe() for full decoding Gao Xiang
2020-03-03  9:49   ` Chao Yu
2020-02-26  2:30 ` [PATCH 3/3] erofs: handle corrupted images whose decompressed size less than it'd be Gao Xiang
2020-02-26  2:34   ` Eric Biggers
2020-02-26  2:40     ` Gao Xiang
2020-02-26  2:44       ` Gao Xiang
2020-03-03  9:51   ` Chao Yu

Linux-EROFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-erofs/0 linux-erofs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-erofs linux-erofs/ https://lore.kernel.org/linux-erofs \
		linux-erofs@lists.ozlabs.org linux-erofs@ozlabs.org
	public-inbox-index linux-erofs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linux-erofs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git