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] [day] [month] [year] [list]

Message-ID: <20171011082741.GM3667@quack2.suse.cz>
Date:   Wed, 11 Oct 2017 10:27:41 +0200
From:   Jan Kara <jack@...e.cz>
To:     Steve Magnani <steve.magnani@...idescorp.com>
Cc:     Jan Kara <jack@...e.cz>, 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 Tue 10-10-17 22:30:30, Steve Magnani wrote:
> 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).

Yeah, I'd be fine with that. It's better to start the conversion only in
some places than to not start it at all.

> >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.

Yeah, all the format string changes can be in one patch. Those are
generally of the type "fix up format string to match passed argument"
anyway.

> >>--- 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.

Ah, right. I got confused.

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