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 22:30:30 -0500
From:   Steve Magnani <steve.magnani@...idescorp.com>
To:     Jan Kara <jack@...e.cz>
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

Jan -

On 10/10/2017 02:33 AM, Jan Kara wrote:
> On Mon 09-10-17 10:04:52, Steve Magnani wrote:
>
> ...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!
I agree with this in principle and in fact do something like it in my 
application code for just that reason. But, doing a complete job of this 
in the driver would increase the scope far beyond what is needed to fix 
the bugs I see and beyond what I am able to support. Would it be 
acceptable to limit usage of this type to a subset of the places it 
could ultimately be used? (Example: use it in udf_readdir(), which has a 
bug requiring a type change, but not necessarily in udf_read_tagged(), 
which doesn't).

> 2) Changes fixing signedness in various format strings for various types -
> put these in a separare patch please.
Sure - but would you be opposed to putting _all_ of the format string 
changes in that patch? There are some format string changes (i.e., in 
unicode.c) that obviously don't have anything to do with block numbers, 
but I think almost all of the rest do. It gets a little murky when block 
numbers or counts are used in calculations. The unifying idea behind all 
the format string changes is preventing sign extension from causing 
unsigned values to be printed as negative, so on that basis I think an 
argument can be made that they all "go" together.
>> --- 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.
'bit' and 'i' are ints in the function _below_ this change, but unsigned 
long within this function. So I think this is correct.

Regards,

------------------------------------------------------------------------
  Steven J. Magnani               "I claim this network for MARS!
  www.digidescorp.com              Earthling, return my space modulator!"

  #include <standard.disclaimer>