linux-kernel - Re: [PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting blocks

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: <20171016141728.GF9762@quack2.suse.cz>
Date:   Mon, 16 Oct 2017 16:17:28 +0200
From:   Jan Kara <jack@...e.cz>
To:     Steve Magnani <steve.magnani@...idescorp.com>
Cc:     Jan Kara <jack@...e.com>, linux-kernel@...r.kernel.org,
        "Steven J . Magnani" <steve@...idescorp.com>
Subject: Re: [PATCH v2 1/3] udf: Fix 64-bit sign extension issues affecting
 blocks > 0x7FFFFFFF

On Thu 12-10-17 08:48:40, Steve Magnani wrote:
> Large (> 1 TiB) UDF filesystems appear subject to several problems when
> mounted on 64-bit systems:
> 
> * readdir() can fail on a directory containing File Identifiers residing
>   above 0x7FFFFFFF. This manifests as a 'ls' command failing with EIO.
> 
> * FIBMAP on a file block located above 0x7FFFFFFF can return a negative
>   value. The low 32 bits are correct, but applications that don't mask the
>   high 32 bits of the result can perform incorrectly.
> 
> Per suggestion by Jan Kara, introduce a udf_pblk_t type for representation
> of UDF block addresses. Ultimately, all driver functions that manipulate
> UDF block addresses should use this type; for now, deployment is limited
> to functions with actual or potential sign extension issues.
> 
> Changes to udf_readdir() and udf_block_map() address the issues noted
> above; other changes address potential similar issues uncovered during
> audit of the driver code.
> 
> Signed-off-by: Steven J. Magnani <steve@...idescorp.com>

Thanks for the patch. Two small comments below.

> ---
> diff -uprN a/fs/udf/balloc.c b/fs/udf/balloc.c
> --- a/fs/udf/balloc.c	2017-10-07 16:46:37.694130197 -0500
> +++ b/fs/udf/balloc.c	2017-10-11 13:08:47.969313878 -0500
> @@ -218,16 +218,17 @@ out:
>  	return alloc_count;
>  }
>  
> -static int udf_bitmap_new_block(struct super_block *sb,
> +static udf_pblk_t udf_bitmap_new_block(struct super_block *sb,
>  				struct udf_bitmap *bitmap, uint16_t partition,
>  				uint32_t goal, int *err)
>  {
>  	struct udf_sb_info *sbi = UDF_SB(sb);
> -	int newbit, bit = 0, block, block_group, group_start;
> +	int newbit, bit = 0;
> +	udf_pblk_t block, block_group, group_start;

Only 'block' should be udf_pblk_t here. group_start is just an offset of
bitmap start in a block and block_group is how many bitmap blocks in a
bitmap we need to skip - neither of these is a physical block number and
neither has a problem with int type...

> diff -uprN a/fs/udf/truncate.c b/fs/udf/truncate.c
> --- a/fs/udf/truncate.c	2017-10-07 12:06:18.749230803 -0500
> +++ b/fs/udf/truncate.c	2017-10-11 11:14:47.404965456 -0500
> @@ -31,9 +31,9 @@ static void extent_trunc(struct inode *i
>  			 uint32_t nelen)
>  {
>  	struct kernel_lb_addr neloc = {};
> -	int last_block = (elen + inode->i_sb->s_blocksize - 1) >>
> +	udf_pblk_t last_block = (elen + inode->i_sb->s_blocksize - 1) >>
>  		inode->i_sb->s_blocksize_bits;
> -	int first_block = (nelen + inode->i_sb->s_blocksize - 1) >>
> +	udf_pblk_t first_block = (nelen + inode->i_sb->s_blocksize - 1) >>
>  		inode->i_sb->s_blocksize_bits;
>  
>  	if (nelen) {

These two are actually logical file offsets, not physical block numbers.
And since extent length is u32 and we divide it by block size, they cannot
really overflow. So just leave this place as is.

If you agree with these changes, I'll update your patch and merge it to my
tree (along with the other two changes which look good to me).

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR