lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a79a7e9d-f6b5-00c8-65c8-f914cf0be0b2@gmx.com>
Date:   Tue, 23 May 2023 15:33:22 +0800
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     pengfuyuan <pengfuyuan@...inos.cn>, Chris Mason <clm@...com>
Cc:     Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] btrfs: Fix csum_tree_block to avoid tripping on
 -Werror=array-bounds



On 2023/5/23 15:09, pengfuyuan wrote:
>
> When compiling on a mips 64-bit machine we get these warnings:
>
>      In file included from ./arch/mips/include/asm/cacheflush.h:13,
> 	             from ./include/linux/cacheflush.h:5,
> 	             from ./include/linux/highmem.h:8,
> 		     from ./include/linux/bvec.h:10,
> 		     from ./include/linux/blk_types.h:10,
>                       from ./include/linux/blkdev.h:9,
> 	             from fs/btrfs/disk-io.c:7:
>      fs/btrfs/disk-io.c: In function ‘csum_tree_block’:
>      fs/btrfs/disk-io.c:100:34: error: array subscript 1 is above array bounds of ‘struct page *[1]’ [-Werror=array-bounds]
>        100 |   kaddr = page_address(buf->pages[i]);
>            |                        ~~~~~~~~~~^~~
>      ./include/linux/mm.h:2135:48: note: in definition of macro ‘page_address’
>       2135 | #define page_address(page) lowmem_page_address(page)
>            |                                                ^~~~
>      cc1: all warnings being treated as errors
>
> We can check if i overflows to solve the problem. However, this doesn't make
> much sense, since i == 1 and num_pages == 1 doesn't execute the body of the loop.
> In addition, i < num_pages can also ensure that buf->pages[i] will not cross
> the boundary. Unfortunately, this doesn't help with the problem observed here:
> gcc still complains.

So still false alerts, thus this bug should mostly be reported to GCC.

>
> To fix this, start the loop at index 0 instead of 1. Also, a conditional was
> added to skip the case where the index is 0, so that the loop iterations follow
> the desired logic, and it makes all versions of gcc happy.
>
> Signed-off-by: pengfuyuan <pengfuyuan@...inos.cn>
> ---
>   fs/btrfs/disk-io.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index fbf9006c6234..8b05d556d747 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -96,9 +96,13 @@ static void csum_tree_block(struct extent_buffer *buf, u8 *result)
>   	crypto_shash_update(shash, kaddr + BTRFS_CSUM_SIZE,
>   			    first_page_part - BTRFS_CSUM_SIZE);
>
> -	for (i = 1; i < num_pages; i++) {
> -		kaddr = page_address(buf->pages[i]);
> -		crypto_shash_update(shash, kaddr, PAGE_SIZE);
> +	for (i = 0; i < num_pages; i++) {
> +		struct page *p = buf->pages[i];
> +
> +		if (i != 0) {
> +			kaddr = page_address(p);
> +			crypto_shash_update(shash, kaddr, PAGE_SIZE);

Unfortunately this damages the readability.

If you really want to starts from page index 0, I don't think doing this
is the correct way.

Instead, you may take the chance to merge the first
crypto_shahs_update() call, so the overall procedure looks like this:

static void csum_tree_block()
{
	for (int i = 0; i < num_pages; i++) {
		int page_off = whatever_to_calculate_the_offset;
		int page_len = whatever_to_calculate_the_lengh;
		char *kaddr = page_address(buf->pages[i]) + page_off;

		crypto_shash_update(shash, kaddr, page_len);
	}
	memset();
	crypto_shash_final();
}

Although even with such change, I'm still not sure if it's any better or
worse, as most of the calculation can still be bulky.

I'll let David to give the final call.

Thanks,
Qu
> +		}
>   	}
>   	memset(result, 0, BTRFS_CSUM_SIZE);
>   	crypto_shash_final(shash, result);
>
>
> Content-type: Text/plain
>
> No virus found
> 		Checked by Hillstone Network AntiVirus

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ