[PATCH 0/3] udf: Metadata partition fixes

[PATCH 0/3] udf: Metadata partition fixes

[Alden Tondettar]

This patchset (hopefully) fixes a problem with loading some
Windows-generated UDF 2.5 filesystems, and two other problems I noticed
along the way. This has only been tested on a single NetBSD-generated
image (works before, works now) and a few Windows-7-generated images
(triggered KASAN and failed to mount before, works fine now), but I don't
have any other UDF 2.5 images to test it on.

As I was getting ready to post this, I noticed on lkml that Laura Abbott has
just posted a similar fix (https://lkml.org/lkml/2016/5/18/489) for the
IS_ERR issue. However, I _think_ that fix is inadequate as iput() called
from udf_free_partition() will still dereference the error pointer in
s_mirror_fe on unmount.  If I'm wrong and that patch is fine, then let me
know if you want me to rework this to account for it.

Also, should MAINTAINERS list linux-fsdevel under UDF?

Alden Tondettar (3):
  udf: Don't BUG on missing metadata partition descriptor
  udf: Use IS_ERR when loading metadata mirror file entry
  udf: Use correct partition reference number for metadata

 fs/udf/partition.c | 13 +++++++++----
 fs/udf/super.c     | 22 ++++++++++++----------
 fs/udf/udf_sb.h    |  1 +
 3 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.1.4

[PATCH 0/3] udf: Metadata partition fixes

[Alden Tondettar]

This patchset (hopefully) fixes a problem with loading some
Windows-generated UDF 2.5 filesystems, and two other problems I noticed
along the way. This has only been tested on a single NetBSD-generated
image (works before, works now) and a few Windows-7-generated images
(triggered KASAN and failed to mount before, works fine now), but I don't
have any other UDF 2.5 images to test it on.

As I was getting ready to post this, I noticed on lkml that Laura Abbott has
just posted a similar fix for the IS_ERR issue
(https://lkml.org/lkml/2016/5/18/489). However, I _think_ that fix is
inadequate as iput() called from udf_free_partition() will still dereference
the error pointer in s_mirror_fe on unmount.  If I'm wrong and that patch is
fine, then let me know if you want me to rework this to account for it.

Also, should MAINTAINERS list linux-fsdevel under UDF?

Alden Tondettar (3):
  udf: Don't BUG on missing metadata partition descriptor
  udf: Use IS_ERR when loading metadata mirror file entry
  udf: Use correct partition reference number for metadata

 fs/udf/partition.c | 13 +++++++++----
 fs/udf/super.c     | 22 ++++++++++++----------
 fs/udf/udf_sb.h    |  1 +
 3 files changed, 22 insertions(+), 14 deletions(-)

-- 
2.1.4

[PATCH 1/3] udf: Don't BUG on missing metadata partition descriptor

[Alden Tondettar]

Currently, if a metadata partition map is missing its partition descriptor,
then udf_get_pblock_meta25() will BUG() out the first time it is called.
This is rather drastic for a corrupted filesystem, so just treat this case
as an invalid mapping instead.

Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>
---
 fs/udf/partition.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/udf/partition.c b/fs/udf/partition.c
index 5f861ed2..e4e9e70 100644
--- a/fs/udf/partition.c
+++ b/fs/udf/partition.c
@@ -317,8 +317,9 @@ uint32_t udf_get_pblock_meta25(struct super_block *sb, uint32_t block,
 	mdata = &map->s_type_specific.s_metadata;
 	inode = mdata->s_metadata_fe ? : mdata->s_mirror_fe;
 
-	/* We shouldn't mount such media... */
-	BUG_ON(!inode);
+	if (!inode)
+		return 0xFFFFFFFF;
+
 	retblk = udf_try_read_meta(inode, block, partition, offset);
 	if (retblk == 0xFFFFFFFF && mdata->s_metadata_fe) {
 		udf_warn(sb, "error reading from METADATA, trying to read from MIRROR\n");
-- 
2.1.4

Re: [PATCH 1/3] udf: Don't BUG on missing metadata partition descriptor

[Jan Kara]

On Wed 18-05-16 14:09:17, Alden Tondettar wrote:
> Currently, if a metadata partition map is missing its partition descriptor,
> then udf_get_pblock_meta25() will BUG() out the first time it is called.
> This is rather drastic for a corrupted filesystem, so just treat this case
> as an invalid mapping instead.
> 
> Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>

Thanks. I have added this patch to my tree.

								Honza

> ---
>  fs/udf/partition.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/udf/partition.c b/fs/udf/partition.c
> index 5f861ed2..e4e9e70 100644
> --- a/fs/udf/partition.c
> +++ b/fs/udf/partition.c
> @@ -317,8 +317,9 @@ uint32_t udf_get_pblock_meta25(struct super_block *sb, uint32_t block,
>  	mdata = &map->s_type_specific.s_metadata;
>  	inode = mdata->s_metadata_fe ? : mdata->s_mirror_fe;
>  
> -	/* We shouldn't mount such media... */
> -	BUG_ON(!inode);
> +	if (!inode)
> +		return 0xFFFFFFFF;
> +
>  	retblk = udf_try_read_meta(inode, block, partition, offset);
>  	if (retblk == 0xFFFFFFFF && mdata->s_metadata_fe) {
>  		udf_warn(sb, "error reading from METADATA, trying to read from MIRROR\n");
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] udf: Use IS_ERR when loading metadata mirror file entry

[Alden Tondettar]

Currently when udf_get_pblock_meta25() fails to map a block using the
primary metadata file, it will attempt to load the mirror file entry by
calling udf_find_metadata_inode_efe().  That function will return a ERR_PTR
if it fails, but the return value is only checked against NULL.  Test the
return value using IS_ERR() and change it to NULL if needed.

Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>
---
 fs/udf/partition.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/udf/partition.c b/fs/udf/partition.c
index e4e9e70..ca3cde3 100644
--- a/fs/udf/partition.c
+++ b/fs/udf/partition.c
@@ -326,6 +326,8 @@ uint32_t udf_get_pblock_meta25(struct super_block *sb, uint32_t block,
 		if (!(mdata->s_flags & MF_MIRROR_FE_LOADED)) {
 			mdata->s_mirror_fe = udf_find_metadata_inode_efe(sb,
 				mdata->s_mirror_file_loc, map->s_partition_num);
+			if (IS_ERR(mdata->s_mirror_fe))
+				mdata->s_mirror_fe = NULL;
 			mdata->s_flags |= MF_MIRROR_FE_LOADED;
 		}
 
-- 
2.1.4

Re: [PATCH 2/3] udf: Use IS_ERR when loading metadata mirror file entry

[Jan Kara]

On Wed 18-05-16 14:09:18, Alden Tondettar wrote:
> Currently when udf_get_pblock_meta25() fails to map a block using the
> primary metadata file, it will attempt to load the mirror file entry by
> calling udf_find_metadata_inode_efe().  That function will return a ERR_PTR
> if it fails, but the return value is only checked against NULL.  Test the
> return value using IS_ERR() and change it to NULL if needed.
> 
> Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>

Thanks. I have added this patch to my tree.

								Honza

> ---
>  fs/udf/partition.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/udf/partition.c b/fs/udf/partition.c
> index e4e9e70..ca3cde3 100644
> --- a/fs/udf/partition.c
> +++ b/fs/udf/partition.c
> @@ -326,6 +326,8 @@ uint32_t udf_get_pblock_meta25(struct super_block *sb, uint32_t block,
>  		if (!(mdata->s_flags & MF_MIRROR_FE_LOADED)) {
>  			mdata->s_mirror_fe = udf_find_metadata_inode_efe(sb,
>  				mdata->s_mirror_file_loc, map->s_partition_num);
> +			if (IS_ERR(mdata->s_mirror_fe))
> +				mdata->s_mirror_fe = NULL;
>  			mdata->s_flags |= MF_MIRROR_FE_LOADED;
>  		}
>  
> -- 
> 2.1.4
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/3] udf: Use correct partition reference number for metadata

[Alden Tondettar]

UDF/OSTA terminology is confusing. Partition Numbers (PNs) are arbitrary
16-bit values, one for each physical partition in the volume.  Partition
Reference Numbers (PRNs) are indices into the the Partition Map Table and
do not necessarily equal the PN of the mapped partition.

The current metadata code mistakenly uses the PN instead of the PRN when
mapping metadata blocks to physical/sparable blocks.  Windows-created
UDF 2.5 discs for some reason use large, arbitrary PNs, resulting in mount
failure and KASAN read warnings in udf_read_inode().

For example, a NetBSD UDF 2.5 partition might look like this:

PRN PN Type
--- -- ----
  0  0 Sparable
  1  0 Metadata

Since PRN == PN, we are fine.

But Windows could gives us:

PRN PN   Type
--- ---- ----
  0 8192 Sparable
  1 8192 Metadata

So udf_read_inode() will start out by checking the partition length in
sbi->s_partmaps[8192], which is obviously out of bounds.

Fix this by creating a new field (s_partition_ref) in struct udf_meta_data,
referencing whatever physical or sparable map has the same partition number
as the metadata partition.

Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>
---
 fs/udf/partition.c |  6 ++++--
 fs/udf/super.c     | 22 ++++++++++++----------
 fs/udf/udf_sb.h    |  1 +
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/fs/udf/partition.c b/fs/udf/partition.c
index ca3cde3..5960149 100644
--- a/fs/udf/partition.c
+++ b/fs/udf/partition.c
@@ -295,7 +295,8 @@ static uint32_t udf_try_read_meta(struct inode *inode, uint32_t block,
 		map = &UDF_SB(sb)->s_partmaps[partition];
 		/* map to sparable/physical partition desc */
 		phyblock = udf_get_pblock(sb, eloc.logicalBlockNum,
-			map->s_partition_num, ext_offset + offset);
+			map->s_type_specific.s_metadata.s_partition_ref,
+			ext_offset + offset);
 	}
 
 	brelse(epos.bh);
@@ -325,7 +326,8 @@ uint32_t udf_get_pblock_meta25(struct super_block *sb, uint32_t block,
 		udf_warn(sb, "error reading from METADATA, trying to read from MIRROR\n");
 		if (!(mdata->s_flags & MF_MIRROR_FE_LOADED)) {
 			mdata->s_mirror_fe = udf_find_metadata_inode_efe(sb,
-				mdata->s_mirror_file_loc, map->s_partition_num);
+				mdata->s_mirror_file_loc,
+				mdata->s_partition_ref);
 			if (IS_ERR(mdata->s_mirror_fe))
 				mdata->s_mirror_fe = NULL;
 			mdata->s_flags |= MF_MIRROR_FE_LOADED;
diff --git a/fs/udf/super.c b/fs/udf/super.c
index 5e2c8c8..26b6bf1 100644
--- a/fs/udf/super.c
+++ b/fs/udf/super.c
@@ -951,13 +951,13 @@ out2:
 }
 
 struct inode *udf_find_metadata_inode_efe(struct super_block *sb,
-					u32 meta_file_loc, u32 partition_num)
+					u32 meta_file_loc, u32 partition_ref)
 {
 	struct kernel_lb_addr addr;
 	struct inode *metadata_fe;
 
 	addr.logicalBlockNum = meta_file_loc;
-	addr.partitionReferenceNum = partition_num;
+	addr.partitionReferenceNum = partition_ref;
 
 	metadata_fe = udf_iget_special(sb, &addr);
 
@@ -974,7 +974,8 @@ struct inode *udf_find_metadata_inode_efe(struct super_block *sb,
 	return metadata_fe;
 }
 
-static int udf_load_metadata_files(struct super_block *sb, int partition)
+static int udf_load_metadata_files(struct super_block *sb, int partition,
+				   int type1_index)
 {
 	struct udf_sb_info *sbi = UDF_SB(sb);
 	struct udf_part_map *map;
@@ -984,20 +985,21 @@ static int udf_load_metadata_files(struct super_block *sb, int partition)
 
 	map = &sbi->s_partmaps[partition];
 	mdata = &map->s_type_specific.s_metadata;
+	mdata->s_partition_ref = type1_index;
 
 	/* metadata address */
 	udf_debug("Metadata file location: block = %d part = %d\n",
-		  mdata->s_meta_file_loc, map->s_partition_num);
+		  mdata->s_meta_file_loc, mdata->s_partition_ref);
 
 	fe = udf_find_metadata_inode_efe(sb, mdata->s_meta_file_loc,
-					 map->s_partition_num);
+					 mdata->s_partition_ref);
 	if (IS_ERR(fe)) {
 		/* mirror file entry */
 		udf_debug("Mirror metadata file location: block = %d part = %d\n",
-			  mdata->s_mirror_file_loc, map->s_partition_num);
+			  mdata->s_mirror_file_loc, mdata->s_partition_ref);
 
 		fe = udf_find_metadata_inode_efe(sb, mdata->s_mirror_file_loc,
-						 map->s_partition_num);
+						 mdata->s_partition_ref);
 
 		if (IS_ERR(fe)) {
 			udf_err(sb, "Both metadata and mirror metadata inode efe can not found\n");
@@ -1015,7 +1017,7 @@ static int udf_load_metadata_files(struct super_block *sb, int partition)
 	*/
 	if (mdata->s_bitmap_file_loc != 0xFFFFFFFF) {
 		addr.logicalBlockNum = mdata->s_bitmap_file_loc;
-		addr.partitionReferenceNum = map->s_partition_num;
+		addr.partitionReferenceNum = mdata->s_partition_ref;
 
 		udf_debug("Bitmap file location: block = %d part = %d\n",
 			  addr.logicalBlockNum, addr.partitionReferenceNum);
@@ -1283,7 +1285,7 @@ static int udf_load_partdesc(struct super_block *sb, sector_t block)
 	p = (struct partitionDesc *)bh->b_data;
 	partitionNumber = le16_to_cpu(p->partitionNumber);
 
-	/* First scan for TYPE1, SPARABLE and METADATA partitions */
+	/* First scan for TYPE1 and SPARABLE partitions */
 	for (i = 0; i < sbi->s_partitions; i++) {
 		map = &sbi->s_partmaps[i];
 		udf_debug("Searching map: (%d == %d)\n",
@@ -1333,7 +1335,7 @@ static int udf_load_partdesc(struct super_block *sb, sector_t block)
 		goto out_bh;
 
 	if (map->s_partition_type == UDF_METADATA_MAP25) {
-		ret = udf_load_metadata_files(sb, i);
+		ret = udf_load_metadata_files(sb, i, type1_idx);
 		if (ret < 0) {
 			udf_err(sb, "error loading MetaData partition map %d\n",
 				i);
diff --git a/fs/udf/udf_sb.h b/fs/udf/udf_sb.h
index 27b5335..51e6de0 100644
--- a/fs/udf/udf_sb.h
+++ b/fs/udf/udf_sb.h
@@ -61,6 +61,7 @@ struct udf_meta_data {
 	__u32	s_bitmap_file_loc;
 	__u32	s_alloc_unit_size;
 	__u16	s_align_unit_size;
+	__u16   s_partition_ref;
 	int	s_flags;
 	struct inode *s_metadata_fe;
 	struct inode *s_mirror_fe;
-- 
2.1.4

Re: [PATCH 3/3] udf: Use correct partition reference number for metadata

[Jan Kara]

On Wed 18-05-16 14:09:19, Alden Tondettar wrote:
> UDF/OSTA terminology is confusing. Partition Numbers (PNs) are arbitrary
> 16-bit values, one for each physical partition in the volume.  Partition
> Reference Numbers (PRNs) are indices into the the Partition Map Table and
> do not necessarily equal the PN of the mapped partition.
> 
> The current metadata code mistakenly uses the PN instead of the PRN when
> mapping metadata blocks to physical/sparable blocks.  Windows-created
> UDF 2.5 discs for some reason use large, arbitrary PNs, resulting in mount
> failure and KASAN read warnings in udf_read_inode().
> 
> For example, a NetBSD UDF 2.5 partition might look like this:
> 
> PRN PN Type
> --- -- ----
>   0  0 Sparable
>   1  0 Metadata
> 
> Since PRN == PN, we are fine.
> 
> But Windows could gives us:
> 
> PRN PN   Type
> --- ---- ----
>   0 8192 Sparable
>   1 8192 Metadata
> 
> So udf_read_inode() will start out by checking the partition length in
> sbi->s_partmaps[8192], which is obviously out of bounds.
> 
> Fix this by creating a new field (s_partition_ref) in struct udf_meta_data,
> referencing whatever physical or sparable map has the same partition number
> as the metadata partition.
> 
> Signed-off-by: Alden Tondettar <alden.tondettar@gmail.com>

Ah, I've missed this subtlety when reading the specification! Thanks for
fixing this. I've added this patch to my tree, I have just changed
s_partition_ref to s_phys_partition_ref and added a comment about it to
udf_sb.h..

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