[PATCH 0/2] udf: Fix handling of device spec attrs

[PATCH 0/2] udf: Fix handling of device spec attrs

[Jan Kara]

Hello,

the two patches in this series fix validation of udf extended attribute
containing device information for block and character devices. I plan to merge
them through my tree.

								Honza

[PATCH 1/2] udf: validate the device specification EA before using it

[Jan Kara]

From: Yunpeng Tian <shionthanatos@gmail.com>

udf_get_extendedattr() bounds an extended attribute only against
struct genericFormat, which is 12 bytes.  A type-12 device
specification is struct deviceSpec, which is 24 bytes plus a flexible
impUse array. Thus a corrupted filesystem can contain extended attribute
that is accepted by udf_get_extendedattr() but udf_write_inode() or
udf_read_inode() may access beyond the end of provided buffer.

Provide a udf_device_spec_valid() check to validate correctness of
extended attribute that is a deviceSpec. While here, check the result of
udf_add_extendedattr() before dereferencing it as it can return NULL in
case of error.

[JK: Removed duplicated validity checks]

Reported-by: Yunpeng Tian <shionthanatos@gmail.com>
Reported-by: Gongming Wang <gmwgg05@gmail.com>
Reported-by: Mingda Zhang <npczmd@qq.com>
Reported-by: Qinrun Dai <jupmouse@gmail.com>
Cc: stable@vger.kernel.org
Signed-off-by: Yunpeng Tian <shionthanatos@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 43 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index e45e546a739a..baf9845ac06b 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1336,6 +1336,26 @@ int udf_setsize(struct inode *inode, loff_t newsize)
 	return err;
 }
 
+/*
+ * Verify validity of struct deviceSpec on disk. udf_get_extendedattr() has
+ * already verified the generic header and made sure attribute fits in the
+ * inode so we just have to make sure attribute space is large enough for
+ * deviceSpec struct and required impUse information.
+ */
+static bool udf_device_spec_valid(struct deviceSpec *dsea)
+{
+	u32 attr_length, imp_use_length;
+
+	attr_length = le32_to_cpu(dsea->attrLength);
+	imp_use_length = le32_to_cpu(dsea->impUseLength);
+	if (attr_length < sizeof(struct deviceSpec) ||
+	    imp_use_length < sizeof(struct regid) ||
+	    imp_use_length > attr_length - sizeof(struct deviceSpec))
+		return false;
+
+	return true;
+}
+
 /*
  * Maximum length of linked list formed by ICB hierarchy. The chosen number is
  * arbitrary - just that we hopefully don't limit any real use of rewritten
@@ -1654,13 +1674,12 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode)) {
 		struct deviceSpec *dsea =
 			(struct deviceSpec *)udf_get_extendedattr(inode, 12, 1);
-		if (dsea) {
-			init_special_inode(inode, inode->i_mode,
+
+		if (!dsea || !udf_device_spec_valid(dsea))
+			goto out;
+		init_special_inode(inode, inode->i_mode,
 				MKDEV(le32_to_cpu(dsea->majorDeviceIdent),
 				      le32_to_cpu(dsea->minorDeviceIdent)));
-			/* Developer ID ??? */
-		} else
-			goto out;
 	}
 	ret = 0;
 out:
@@ -1757,6 +1776,7 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 	struct udf_sb_info *sbi = UDF_SB(inode->i_sb);
 	unsigned char blocksize_bits = inode->i_sb->s_blocksize_bits;
 	struct udf_inode_info *iinfo = UDF_I(inode);
+	int err;
 
 	bh = sb_getblk(inode->i_sb,
 			udf_get_lb_pblock(inode->i_sb, &iinfo->i_location, 0));
@@ -1821,6 +1841,10 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 				udf_add_extendedattr(inode,
 						     sizeof(struct deviceSpec) +
 						     sizeof(struct regid), 12, 0x3);
+			if (!dsea) {
+				err = -ENOSPC;
+				goto out_unlock;
+			}
 			dsea->attrType = cpu_to_le32(12);
 			dsea->attrSubtype = 1;
 			dsea->attrLength = cpu_to_le32(
@@ -1828,6 +1852,10 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 						sizeof(struct regid));
 			dsea->impUseLength = cpu_to_le32(sizeof(struct regid));
 		}
+		if (!udf_device_spec_valid(dsea)) {
+			err = -EFSCORRUPTED;
+			goto out_unlock;
+		}
 		eid = (struct regid *)dsea->impUse;
 		memset(eid, 0, sizeof(*eid));
 		strcpy(eid->ident, UDF_ID_DEVELOPER);
@@ -1962,6 +1990,11 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 	set_inode_metadata_writeback(inode);
 
 	return 0;
+
+out_unlock:
+	unlock_buffer(bh);
+	brelse(bh);
+	return err;
 }
 
 struct inode *__udf_iget(struct super_block *sb, struct kernel_lb_addr *ino,
-- 
2.51.0

[PATCH 2/2] udf: Avoid confusion of missing and corrupted extended attributes

[Jan Kara]

udf_get_extendedattr() returns NULL either when extended attribute was
not found or when some corrupted attribute was found. Similarly
udf_add_extendedattr() returns NULL when extended attributes are
corrupted or when there's no space to insert the extended attribute.
Make both functions return proper error codes to discern these cases and
handle them in the callers.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/inode.c | 23 ++++++++++++++++-------
 fs/udf/misc.c  | 15 ++++++++-------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index baf9845ac06b..8babc2fbce9a 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1675,8 +1675,15 @@ static int udf_read_inode(struct inode *inode, bool hidden_inode)
 		struct deviceSpec *dsea =
 			(struct deviceSpec *)udf_get_extendedattr(inode, 12, 1);
 
-		if (!dsea || !udf_device_spec_valid(dsea))
+		if (IS_ERR(dsea)) {
+			ret = PTR_ERR(dsea);
 			goto out;
+		}
+		/* Device inodes must have a device spec attribute */
+		if (!dsea || !udf_device_spec_valid(dsea)) {
+			ret = -EFSCORRUPTED;
+			goto out;
+		}
 		init_special_inode(inode, inode->i_mode,
 				MKDEV(le32_to_cpu(dsea->majorDeviceIdent),
 				      le32_to_cpu(dsea->minorDeviceIdent)));
@@ -1836,13 +1843,19 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 		struct regid *eid;
 		struct deviceSpec *dsea =
 			(struct deviceSpec *)udf_get_extendedattr(inode, 12, 1);
+
+		/* Validity of extended attrs was checked on load */
+		if (WARN_ON_ONCE(IS_ERR(dsea))) {
+			err = PTR_ERR(dsea);
+			goto out_unlock;
+		}
 		if (!dsea) {
 			dsea = (struct deviceSpec *)
 				udf_add_extendedattr(inode,
 						     sizeof(struct deviceSpec) +
 						     sizeof(struct regid), 12, 0x3);
-			if (!dsea) {
-				err = -ENOSPC;
+			if (IS_ERR(dsea)) {
+				err = PTR_ERR(dsea);
 				goto out_unlock;
 			}
 			dsea->attrType = cpu_to_le32(12);
@@ -1852,10 +1865,6 @@ int udf_write_inode(struct inode *inode, struct writeback_control *wbc)
 						sizeof(struct regid));
 			dsea->impUseLength = cpu_to_le32(sizeof(struct regid));
 		}
-		if (!udf_device_spec_valid(dsea)) {
-			err = -EFSCORRUPTED;
-			goto out_unlock;
-		}
 		eid = (struct regid *)dsea->impUse;
 		memset(eid, 0, sizeof(*eid));
 		strcpy(eid->ident, UDF_ID_DEVELOPER);
diff --git a/fs/udf/misc.c b/fs/udf/misc.c
index 6928e378fbbd..a2084dfbfbd6 100644
--- a/fs/udf/misc.c
+++ b/fs/udf/misc.c
@@ -58,7 +58,7 @@ struct genericFormat *udf_add_extendedattr(struct inode *inode, uint32_t size,
 					cpu_to_le16(TAG_IDENT_EAHD) ||
 			    le32_to_cpu(eahd->descTag.tagLocation) !=
 					iinfo->i_location.logicalBlockNum)
-				return NULL;
+				return ERR_PTR(-EFSCORRUPTED);
 		} else {
 			struct udf_sb_info *sbi = UDF_SB(inode->i_sb);
 
@@ -122,7 +122,7 @@ struct genericFormat *udf_add_extendedattr(struct inode *inode, uint32_t size,
 		return (struct genericFormat *)&ea[offset];
 	}
 
-	return NULL;
+	return ERR_PTR(-ENOSPC);
 }
 
 struct genericFormat *udf_get_extendedattr(struct inode *inode, uint32_t type,
@@ -144,7 +144,7 @@ struct genericFormat *udf_get_extendedattr(struct inode *inode, uint32_t type,
 				cpu_to_le16(TAG_IDENT_EAHD) ||
 		    le32_to_cpu(eahd->descTag.tagLocation) !=
 				iinfo->i_location.logicalBlockNum)
-			return NULL;
+			return ERR_PTR(-EFSCORRUPTED);
 
 		if (type < 2048)
 			offset = sizeof(struct extendedAttrHeaderDesc);
@@ -153,16 +153,17 @@ struct genericFormat *udf_get_extendedattr(struct inode *inode, uint32_t type,
 		else
 			offset = le32_to_cpu(eahd->appAttrLocation);
 
-		while (offset + sizeof(*gaf) < iinfo->i_lenEAttr) {
+		while (offset <
+		       iinfo->i_lenEAttr - sizeof(struct genericFormat)) {
 			uint32_t attrLength;
 
 			gaf = (struct genericFormat *)&ea[offset];
 			attrLength = le32_to_cpu(gaf->attrLength);
 
 			/* Detect undersized elements and buffer overflows */
-			if ((attrLength < sizeof(*gaf)) ||
-			    (attrLength > (iinfo->i_lenEAttr - offset)))
-				break;
+			if (attrLength < sizeof(struct genericFormat) ||
+			    attrLength > iinfo->i_lenEAttr - offset)
+				return ERR_PTR(-EFSCORRUPTED);
 
 			if (le32_to_cpu(gaf->attrType) == type &&
 					gaf->attrSubtype == subtype)
-- 
2.51.0