linux-kernel - Re: [WIP PATCH 1/4] udf: Do not access LVIDIU revision members when they are not filled

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]

Date:   Tue, 14 Jan 2020 13:01:38 +0100
From:   Jan Kara <jack@...e.cz>
To:     Pali Rohár <pali.rohar@...il.com>
Cc:     Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [WIP PATCH 1/4] udf: Do not access LVIDIU revision members when
 they are not filled

On Mon 13-01-20 19:37:28, Pali Rohár wrote:
> On Monday 13 January 2020 13:00:49 Jan Kara wrote:
> > On Sun 12-01-20 18:59:30, Pali Rohár wrote:
> > > minUDFReadRev, minUDFWriteRev and maxUDFWriteRev members were introduced in
> > > UDF 1.02. Previous UDF revisions used that area for implementation specific
> > > data. So in this case do not touch these members.
> > > 
> > > To check if LVIDIU contain revisions members, first read UDF revision from
> > > LVD. If revision is at least 1.02 LVIDIU should contain revision members.
> > > 
> > > This change should fix mounting UDF 1.01 images in R/W mode. Kernel would
> > > not touch, read overwrite implementation specific area of LVIDIU.
> > > 
> > > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> > 
> > Maybe we could store the fs revision in the superblock as well to avoid
> > passing the udf_rev parameter?
> 
> Unfortunately not. Function udf_verify_domain_identifier() is called
> also when parsing FSD. FSD is stored on partition map and e.g. Metadata
> partition map depends on UDF revision. So it is not a good idea to
> overwrite UDF revision from FSD. This is reason why I decided to use
> initial UDF revision number only from LVD.
> 
> But whole stuff around UDF revision is a mess. UDF revision is stored on
> these locations:
> 
> main LVD
> reserve LVD
> main IUVD
> reserve IUVD
> FSD
> 
> And optionally (when specific UDF feature is used) also on:
> 
> sparable partition map 1.50+
> virtual partition map 1.50+
> all sparing tables 1.50+
> VAT 1.50
> 
> Plus tuple minimal read, minimal write, maximal write UDF revision is
> stored on:
> 
> LVIDIU 1.02+
> VAT 2.00+
> 
> VAT in 2.00+ format overrides information stored on LVIDIU.

Thanks for the summary. This is indeed a mess in the standard so let's not
overcomplicate it. I agree with just taking the revision from 'main LVD'
and storing it in the superblock like you do in this patch. I'd just
slightly change your code so that extracting a revision from 'struct regid'
is a separate function and not "hidden" inside
udf_verify_domain_identifier(). There's no strong reason for combining
these two.

WRT parsing of minUDFReadRev and friends, I'd handle them similarly to
numDirs and numFiles. I'd initialize them to the version we've got from
LVD, then possibly override them in udf_load_logicalvolint(), and finally
possibly override them in udf_load_vat().

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