linux-kernel - Re: [WIP PATCH 2/4] udf: Fix reading numFiles and numDirs from UDF 2.00+ VAT discs

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]
Message-ID: <20200114113707.GG6466@quack2.suse.cz>
Date:   Tue, 14 Jan 2020 12:37:07 +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 2/4] udf: Fix reading numFiles and numDirs from UDF
 2.00+ VAT discs

On Tue 14-01-20 12:18:17, Jan Kara wrote:
> On Mon 13-01-20 19:11:38, Pali Rohár wrote:
> > On Monday 13 January 2020 12:58:22 Jan Kara wrote:
> > > On Sun 12-01-20 18:59:31, Pali Rohár wrote:
> > > > These two fields are stored in VAT and override previous values stored in
> > > > LVIDIU.
> > > > 
> > > > This change contains only implementation for UDF 2.00+. For UDF 1.50 there
> > > > is an optional structure "Logical Volume Extended Information" which is not
> > > > implemented in this change yet.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali.rohar@...il.com>
> > > 
> > > For this and the following patch, I'd rather have the 'additional data'
> > > like number of files, dirs, or revisions, stored in the superblock than
> > > having them hidden in the VAT partition structure. And places that parse
> > > corresponding on-disk structures would fill in the numbers into the
> > > superblock.
> > 
> > This is not simple. Kernel first reads and parses VAT and later parses
> > LVIDIU. VAT is optional UDF feature and in UDF 1.50 are even those data
> > optional.
> > 
> > Logic for determining minimal write UDF revision is currently in code
> > which parse LVIDIU. And this is the only place which needs access UDF
> > revisions stored in VAT and LVIDIU.
> > 
> > UDF revision from LVD is already stored into superblock.
> > 
> > And because VAT is parsed prior to parsing LVIDIU is is also not easy to
> > store number of files and directories into superblock. LVIDIU needs to
> > know if data from VAT were loaded to superblock or not and needs to know
> > if data from LVIDIU should be stored into superblock or not.
> > 
> > Any idea how to do it without complicating whole code?
> 
> Let's take the discussion about revision storage to the thread about the
> other patch. But for number of directories and files I was thinking like:
> 
> We could initialize values in the superblock to -1 (or whatever invalid
> value - define a constant for it). The first place that has valid values
> available (detected by the superblock having still invalid values) stores them
> in the superblock. We are guaranteed to parse VAT before LVIDIU because we
> need VAT to locate LVIDIU in the first place so this logic should be
> reliable.

Hum, now checking the code, I was wrong with "we are guaranteed to parse
VAT before LVIDIU". It is rather on contrary - we need to load LVID to be
able to locate VAT. So if we added processing of numDirs and numFiles from
LVID to udf_load_logicalvolint(), we can later just override the values when
parsing VAT.

								Honza

> 
> And later when using the values, we can also easily check whether we
> actually have sensible values available in the first place...
> 
> 								Honza
> 
> > > >  fs/udf/super.c  | 25 ++++++++++++++++++++++---
> > > >  fs/udf/udf_sb.h |  3 +++
> > > >  2 files changed, 25 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/udf/super.c b/fs/udf/super.c
> > > > index 8df6e9962..e8661bf01 100644
> > > > --- a/fs/udf/super.c
> > > > +++ b/fs/udf/super.c
> > > > @@ -1202,6 +1202,8 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > > >  		map->s_type_specific.s_virtual.s_start_offset = 0;
> > > >  		map->s_type_specific.s_virtual.s_num_entries =
> > > >  			(sbi->s_vat_inode->i_size - 36) >> 2;
> > > > +		/* TODO: Add support for reading Logical Volume Extended Information (UDF 1.50 Errata, DCN 5003, 3.3.4.5.1.3) */
> > > > +		map->s_type_specific.s_virtual.s_has_additional_data = false;
> > > >  	} else if (map->s_partition_type == UDF_VIRTUAL_MAP20) {
> > > >  		vati = UDF_I(sbi->s_vat_inode);
> > > >  		if (vati->i_alloc_type != ICBTAG_FLAG_AD_IN_ICB) {
> > > > @@ -1215,6 +1217,12 @@ static int udf_load_vat(struct super_block *sb, int p_index, int type1_index)
> > > >  							vati->i_ext.i_data;
> > > >  		}
> > > >  
> > > > +		map->s_type_specific.s_virtual.s_has_additional_data =
> > > > +			true;
> > > > +		map->s_type_specific.s_virtual.s_num_files =
> > > > +			le32_to_cpu(vat20->numFiles);
> > > > +		map->s_type_specific.s_virtual.s_num_dirs =
> > > > +			le32_to_cpu(vat20->numDirs);
> > > >  		map->s_type_specific.s_virtual.s_start_offset =
> > > >  			le16_to_cpu(vat20->lengthHeader);
> > > >  		map->s_type_specific.s_virtual.s_num_entries =
> > > > @@ -2417,9 +2425,20 @@ static int udf_statfs(struct dentry *dentry, struct kstatfs *buf)
> > > >  	buf->f_blocks = sbi->s_partmaps[sbi->s_partition].s_partition_len;
> > > >  	buf->f_bfree = udf_count_free(sb);
> > > >  	buf->f_bavail = buf->f_bfree;
> > > > -	buf->f_files = (lvidiu != NULL ? (le32_to_cpu(lvidiu->numFiles) +
> > > > -					  le32_to_cpu(lvidiu->numDirs)) : 0)
> > > > -			+ buf->f_bfree;
> > > > +
> > > > +	if ((sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP15 ||
> > > > +	     sbi->s_partmaps[sbi->s_partition].s_partition_type == UDF_VIRTUAL_MAP20) &&
> > > > +	     sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_has_additional_data)
> > > > +		buf->f_files = sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_files +
> > > > +			       sbi->s_partmaps[sbi->s_partition].s_type_specific.s_virtual.s_num_dirs +
> > > > +			       buf->f_bfree;
> > > > +	else if (lvidiu != NULL)
> > > > +		buf->f_files = le32_to_cpu(lvidiu->numFiles) +
> > > > +			       le32_to_cpu(lvidiu->numDirs) +
> > > > +			       buf->f_bfree;
> > > > +	else
> > > > +		buf->f_files = buf->f_bfree;
> > > > +
> > > >  	buf->f_ffree = buf->f_bfree;
> > > >  	buf->f_namelen = UDF_NAME_LEN;
> > > >  	buf->f_fsid.val[0] = (u32)id;
> > > > diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
> > > > index 6bd0d4430..c74abbc84 100644
> > > > --- a/fs/udf/udf_sb.h
> > > > +++ b/fs/udf/udf_sb.h
> > > > @@ -78,6 +78,9 @@ struct udf_sparing_data {
> > > >  struct udf_virtual_data {
> > > >  	__u32	s_num_entries;
> > > >  	__u16	s_start_offset;
> > > > +	bool	s_has_additional_data;
> > > > +	__u32	s_num_files;
> > > > +	__u32	s_num_dirs;
> > > >  };
> > > >  
> > > >  struct udf_bitmap {
> > > > -- 
> > > > 2.20.1
> > > > 
> > 
> > -- 
> > Pali Rohár
> > pali.rohar@...il.com
> 
> 
> -- 
> Jan Kara <jack@...e.com>
> SUSE Labs, CR
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR