linux-kernel - Re: [PATCH] isofs: fix High Sierra dirent flag accesses

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: <71f6cef7-f392-e1ba-1e79-2b767d2cff15@gmail.com>
Date:   Tue, 23 Jun 2020 04:21:17 +0300
From:   Egor Chelak <egor.chelak@...il.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Al Viro <viro@...iv.linux.org.uk>, Arnd Bergmann <arnd@...db.de>,
        Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, Egor Chelak <egor.chelak@...il.com>
Subject: Re: [PATCH] isofs: fix High Sierra dirent flag accesses

On 6/23/2020 12:22 AM, Matthew Wilcox wrote:
> It's been about 22 years since I contributed the patch which added
> support for the Acorn extensions ;-)  But I'm pretty sure that it's not
> possible to have an Acorn CD-ROM that is also an HSF CD-ROM.  That is,
> all Acorn formatted CD-ROMs are ISO-9660 compatible.  So I think this
> chunk of the patch is not required.

I couldn't find any info on Acorn extensions online, so I wasn't sure if
they were mutually exclusive or not, and fixed it there too, just to be
safe. Still, even though it won't be needed in practice, I think it's
better to access the flags in the same way everywhere. Having the same
field accessed differently in different places raises the question "why
it's done differently here?". If we go that way, at the very least there
should be an explanatory comment saying HSF+Acorn is an impossible
combination, and perhaps some logic to prevent HSF discs from mounting
with -o map=acorn. Just leaving it be doesn't seem like a clean
solution.

On 6/23/2020 12:31 AM, Matthew Wilcox wrote:
> Also, ew.  Why on earth do we do 'de->flags[-sbi->s_high_sierra]'?
> I'm surprised we don't have any tools that warn about references outside
> an array.  I would do this as ...
> 
> static inline u8 de_flags(struct isofs_sb_info *sbi,
> 		struct iso_directory_record *de)
> {
> 	if (sbi->s_high_sierra)
> 		return de->date[6];
> 	return de->flags;
> }
I would do something like that, but for this patch I'm just trying to do
a simple bugfix. The isofs code definitely needs a clean up, and perhaps
I'll do it in a future patch. I haven't submitted a patch before, so I
want to start with something simple and uncontroversial, while I learn
the process. :-)