[PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors

[PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors

[Phillip Lougher]

Fix potential use of uninitialised variable caused by recent decompressor
code optimisations.

In zlib_uncompress (zlib_wrapper.c) we have

	int zlib_err, zlib_init = 0;
	...
	do {
		...
			if (avail == 0) {
				offset = 0;
				put_bh(bh[k++]);
				continue;
			}
		...
		zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH);
		...
	} while (zlib_err == Z_OK);

If continue is executed (avail == 0) then the while condition will be
evaluated testing zlib_err, which is uninitialised first time around the
loop.

Fix this by getting rid of the 'if (avail == 0)' condition test, this
edge condition should not be being handled in the decompressor code, and
instead handle it generically in the caller code.

Similarly for xz_wrapper.c.

Incidentally, on most architectures (bar Mips and Parisc), no
uninitialised variable warning is generated by gcc, this is because
the while condition test on continue is optimised out and not performed
(when executing continue zlib_err has not been changed since entering the
loop, and logically if the while condition was true previously, then it's
still true).

Reported-by: Jesper Juhl <jj@chaosbits.net>
Signed-off-by: Phillip Lougher <phillip@lougher.demon.co.uk>
---
  fs/squashfs/block.c        |    8 ++++++++
  fs/squashfs/xz_wrapper.c   |    6 ------
  fs/squashfs/zlib_wrapper.c |    6 ------
  3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/squashfs/block.c b/fs/squashfs/block.c
index 2fb2882..8ab48bc 100644
--- a/fs/squashfs/block.c
+++ b/fs/squashfs/block.c
@@ -63,6 +63,14 @@ static struct buffer_head *get_block_length(struct super_block *sb,
  		*length = (unsigned char) bh->b_data[*offset] |
  			(unsigned char) bh->b_data[*offset + 1] << 8;
  		*offset += 2;
+
+		if (*offset == msblk->devblksize) {
+			put_bh(bh);
+			bh = sb_bread(sb, ++(*cur_index));
+			if (bh == NULL)
+				return NULL;
+			*offset = 0;
+		}
  	}

  	return bh;
diff --git a/fs/squashfs/xz_wrapper.c b/fs/squashfs/xz_wrapper.c
index 856756c..c4eb400 100644
--- a/fs/squashfs/xz_wrapper.c
+++ b/fs/squashfs/xz_wrapper.c
@@ -95,12 +95,6 @@ static int squashfs_xz_uncompress(struct squashfs_sb_info *msblk, void **buffer,
  			if (!buffer_uptodate(bh[k]))
  				goto release_mutex;

-			if (avail == 0) {
-				offset = 0;
-				put_bh(bh[k++]);
-				continue;
-			}
-
  			stream->buf.in = bh[k]->b_data + offset;
  			stream->buf.in_size = avail;
  			stream->buf.in_pos = 0;
diff --git a/fs/squashfs/zlib_wrapper.c b/fs/squashfs/zlib_wrapper.c
index 818a5e0..4661ae2 100644
--- a/fs/squashfs/zlib_wrapper.c
+++ b/fs/squashfs/zlib_wrapper.c
@@ -82,12 +82,6 @@ static int zlib_uncompress(struct squashfs_sb_info *msblk, void **buffer,
  			if (!buffer_uptodate(bh[k]))
  				goto release_mutex;

-			if (avail == 0) {
-				offset = 0;
-				put_bh(bh[k++]);
-				continue;
-			}
-
  			stream->next_in = bh[k]->b_data + offset;
  			stream->avail_in = avail;
  			offset = 0;
-- 
1.6.3.3

Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors

[Geert Uytterhoeven]

On Tue, Jan 25, 2011 at 02:33, Phillip Lougher
<phillip@lougher.demon.co.uk> wrote:
> Fix potential use of uninitialised variable caused by recent decompressor
> code optimisations.
>
> In zlib_uncompress (zlib_wrapper.c) we have
>
>        int zlib_err, zlib_init = 0;
>        ...
>        do {
>                ...
>                        if (avail == 0) {
>                                offset = 0;
>                                put_bh(bh[k++]);
>                                continue;
>                        }
>                ...
>                zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH);
>                ...
>        } while (zlib_err == Z_OK);
>
> If continue is executed (avail == 0) then the while condition will be
> evaluated testing zlib_err, which is uninitialised first time around the
> loop.
>
> Fix this by getting rid of the 'if (avail == 0)' condition test, this
> edge condition should not be being handled in the decompressor code, and
> instead handle it generically in the caller code.
>
> Similarly for xz_wrapper.c.
>
> Incidentally, on most architectures (bar Mips and Parisc), no
> uninitialised variable warning is generated by gcc, this is because
> the while condition test on continue is optimised out and not performed
> (when executing continue zlib_err has not been changed since entering the
> loop, and logically if the while condition was true previously, then it's
> still true).

As this is a "do { ... } while (...);" construct and not a "while
(...) { ... }" construct,
the condition is not checked before doing the first iteration. Furthermore the
"continue" may happen during the first iteration (this depends on parameters
passed to the function), so the compiler cannot make any assumptions about the
value of zlib_err, except that may be uninitialized.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors

[Phillip Lougher]

Geert Uytterhoeven wrote:
> On Tue, Jan 25, 2011 at 02:33, Phillip Lougher
> <phillip@lougher.demon.co.uk> wrote:
>> Incidentally, on most architectures (bar Mips and Parisc), no
>> uninitialised variable warning is generated by gcc, this is because
>> the while condition test on continue is optimised out and not performed
>> (when executing continue zlib_err has not been changed since entering the
>> loop, and logically if the while condition was true previously, then it's
>> still true).
> 
> As this is a "do { ... } while (...);" construct and not a "while
> (...) { ... }" construct,
> the condition is not checked before doing the first iteration. Furthermore the
> "continue" may happen during the first iteration (this depends on parameters
> passed to the function), so the compiler cannot make any assumptions about the
> value of zlib_err, except that may be uninitialized.
> 

No, I've checked the assembly language produced - the while condition test has
been optimised out in the case of continue.

This is the assembly language output for x86_64, the relevant
bits manually annotated by me,

zlib_uncompress:
.LFB1375:
	...
	call	mutex_lock_nested
	movl	$0, 32(%r12)
	movl	$0, 8(%r12)
	xorl	%edx, %edx
	xorl	%eax, %eax

# jump to top of loop

	jmp	.L12
	.p2align 4,,10
	.p2align 3

.L25:

# whole if (stream->avail_in == 0 && k < b) { block of code moved out of
# main loop, so continue can fall through to main loop

#  if (stream->avail_in == 0)

	testl	%edx, %edx
	jne	.L2

# wait_on_buffer(bh[k]);

	movq	32(%rsp), %rsi
	movq	48(%rsp), %rdx
	movslq	%r13d,%rcx
	movq	%rcx, 56(%rsp)
	movl	$.LC0, %edi
	movl	%eax, (%rsp)
	leaq	(%rsi,%rcx,8), %rcx
	movl	8(%rdx), %edx
	movl	$306, %esi
	movq	(%rcx), %r8
	movq	%rcx, 8(%rsp)
	movl	%edx, 68(%rsp)
	xorl	%edx, %edx
	movq	%r8, 16(%rsp)
	call	__might_sleep
	movq	16(%rsp), %r8
	movl	(%rsp), %eax
	movq	8(%rsp), %rcx
	movq	(%r8), %rdx
	andl	$4, %edx
	jne	.L24
.L3:

# if (!buffer_uptodate(bh[k]))
#	goto release_mutex;

	movq	(%rcx), %rdx
	movq	(%rdx), %rcx
	andl	$1, %ecx
	je	.L4

# int avail = min(length, msblk->devblksize - offset);

	movl	68(%rsp), %ecx
	subl	44(%rsp), %ecx
	cmpl	28(%rsp), %ecx
	cmovg	28(%rsp), %ecx

# length -= avail;

	subl	%ecx, 28(%rsp)

# if (avail == 0)

	testl	%ecx, %ecx
	jne	.L5

# put_bh(bh[k++]);

	addl	$1, %r13d
#APP
# 107 "/stripe/git-trees/linux-linus-bugfix-1/arch/x86/include/asm/atomic.h" 1
	.section .smp_locks,"a"
.balign 4
.long 671f - .
.previous
671:
	lock; decl 96(%rdx)
# 0 "" 2
#NO_APP

# offset = 0;

	movl	$0, 44(%rsp)
	.p2align 4,,10
	.p2align 3

# *continue* fails through to top of loop
# no while condition test


# top of loop

.L6:
	movl	8(%r12), %edx

# optimised  case - first iteration around loop enters here as
# edx doesn't need to be loaded (it already holds 0 from
# stream->avail_in = 0)

.L12:

# if (k < b)

	cmpl	%ebp, %r13d
	setl	%r15b
	jl	.L25
.L2:

# if (stream->avail_out == 0 && page < pages) {

	cmpl	152(%rsp), %r14d
	jge	.L7
	movl	32(%r12), %ecx
	testl	%ecx, %ecx
	jne	.L7
	movslq	%r14d,%rdx
	movl	$4096, 32(%r12)
	addl	$1, %r14d
	movq	(%rbx,%rdx,8), %rdx
	movq	%rdx, 24(%r12)
.L7:

# if (!zlib_init) {

	testl	%eax, %eax
	jne	.L8
	movl	$15, %esi
	movq	%r12, %rdi
	call	zlib_inflateInit2
	testl	%eax, %eax
	jne	.L26
.L8:

# zlib_err = zlib_inflate(stream, Z_SYNC_FLUSH);

	movl	$3, %esi
	movq	%r12, %rdi
	call	zlib_inflate

# if( stream->avail_in == 0 && k < b) {

	testb	%r15b, %r15b
	je	.L10
	movl	8(%r12), %edx
	testl	%edx, %edx
	jne	.L10
	movq	32(%rsp), %rcx
	movslq	%r13d,%rdx
	addl	$1, %r13d
	movq	(%rcx,%rdx,8), %rdx
#APP
# 107 "/stripe/git-trees/linux-linus-bugfix-1/arch/x86/include/asm/atomic.h" 1
	.section .smp_locks,"a"
.balign 4
.long 671f - .
.previous
671:
	lock; decl 96(%rdx)
# 0 "" 2
#NO_APP


.L10:

# } while (zlib_err == Z_OK);

	testl	%eax, %eax
	jne	.L11
	movb	$1, %al

# jump to top of loop

	jmp	.L6
	.p2align 4,,10
	.p2align 3

Phillip

Re: [PATCH] Squashfs: Fix use of uninitialised variable in zlib & xz decompressors

[Geert Uytterhoeven]

On Tue, Jan 25, 2011 at 19:57, Phillip Lougher
<phillip@lougher.demon.co.uk> wrote:
> Geert Uytterhoeven wrote:
>>
>> On Tue, Jan 25, 2011 at 02:33, Phillip Lougher
>> <phillip@lougher.demon.co.uk> wrote:
>>>
>>> Incidentally, on most architectures (bar Mips and Parisc), no
>>> uninitialised variable warning is generated by gcc, this is because
>>> the while condition test on continue is optimised out and not performed
>>> (when executing continue zlib_err has not been changed since entering the
>>> loop, and logically if the while condition was true previously, then it's
>>> still true).
>>
>> As this is a "do { ... } while (...);" construct and not a "while
>> (...) { ... }" construct,
>> the condition is not checked before doing the first iteration. Furthermore
>> the
>> "continue" may happen during the first iteration (this depends on
>> parameters
>> passed to the function), so the compiler cannot make any assumptions about
>> the
>> value of zlib_err, except that may be uninitialized.
>>
>
> No, I've checked the assembly language produced - the while condition test
> has
> been optimised out in the case of continue.
>
> This is the assembly language output for x86_64, the relevant
> bits manually annotated by me,

While the continue skips the test at the end of the loop, that doesn't
change the
fact that zlib_err was never initialized in the first place.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds