linux-kernel - Re: [PATCH] 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]

Date:   Tue, 10 Oct 2017 09:33:07 +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] udf: Fix 64-bit sign extension issues affecting blocks >
 0x7FFFFFFF

On Mon 09-10-17 10:04:52, 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.
> 
> * Unsigned values > 0x7FFFFFFF are output as negative numbers in some
>   driver printks, e.g.:
>   Partition (0 type 1511) starts at physical 460, block length -1779968542
>  
> Take care to use "%u" when printing unsigned values and to use unsigned
> types to store UDF block addresses.
> 
> Signed-off-by: Steven J. Magnani <steve@...idescorp.com>

Thanks for looking into this and for the patch! However the patch seems to
be mixing two changes into one which I'd prefer to be separate patches:

1) Changes so that physical block numbers are stored in uint32_t (and
accompanying format string changes). Also when doing this, could you please
create a dedicated type like

typedef uint32_t udf_pblk_t;

and use it instead of uint32_t? That way it would be cleaner what's going
on. Thanks!

2) Changes fixing signedness in various format strings for various types -
put these in a separare patch please.

> --- a/fs/udf/balloc.c	(revision 26779)
> +++ b/fs/udf/balloc.c	(working copy)
...
> @@ -151,7 +151,7 @@
>  		bh = bitmap->s_block_bitmap[bitmap_nr];
>  		for (i = 0; i < count; i++) {
>  			if (udf_set_bit(bit + i, bh->b_data)) {
> -				udf_debug("bit %ld already set\n", bit + i);
> +				udf_debug("bit %lu already set\n", bit + i);

This change looks wrong - bit and i are signed. However they are ints, not
longs, so that should indeed be fixed.

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