[PATCH 0/3] udf: Better handling of corrupted dir and cleanup

[PATCH 0/3] udf: Better handling of corrupted dir and cleanup

[Jan Kara]

Hello,

the first patch fixes a bug in handling of corrupted directories (detects
such dirs properly). The other patches are just cleanups I've done while
reading the code.

If nobody objects, I'll merge the patch through my tree.

								Honza

[PATCH 1/3] udf: Detect incorrect directory size

[Jan Kara]

Detect when a directory entry is (possibly partially) beyond directory
size and return EIO in that case since it means the filesystem is
corrupted. Otherwise directory operations can further corrupt the
directory and possibly also oops the kernel.

CC: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
CC: stable@vger.kernel.org
Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/directory.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/udf/directory.c b/fs/udf/directory.c
index 0a98a2369738..3835f983cc99 100644
--- a/fs/udf/directory.c
+++ b/fs/udf/directory.c
@@ -152,6 +152,9 @@ struct fileIdentDesc *udf_fileident_read(struct inode *dir, loff_t *nf_pos,
 			       sizeof(struct fileIdentDesc));
 		}
 	}
+	/* Got last entry outside of dir size - fs is corrupted! */
+	if (*nf_pos > dir->i_size)
+		return NULL;
 	return fi;
 }
 
-- 
2.16.4

[PATCH 2/3] udf: Provide function for calculating dir entry length

[Jan Kara]

Provide function for calculating directory entry length and use to
reduce code duplication.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/directory.c |  5 +----
 fs/udf/namei.c     | 14 +++-----------
 fs/udf/udfdecl.h   |  6 ++++++
 3 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/fs/udf/directory.c b/fs/udf/directory.c
index 3835f983cc99..d9523013096f 100644
--- a/fs/udf/directory.c
+++ b/fs/udf/directory.c
@@ -141,10 +141,7 @@ struct fileIdentDesc *udf_fileident_read(struct inode *dir, loff_t *nf_pos,
 			       fibh->ebh->b_data,
 			       sizeof(struct fileIdentDesc) + fibh->soffset);
 
-			fi_len = (sizeof(struct fileIdentDesc) +
-				  cfi->lengthFileIdent +
-				  le16_to_cpu(cfi->lengthOfImpUse) + 3) & ~3;
-
+			fi_len = udf_dir_entry_len(cfi);
 			*nf_pos += fi_len - (fibh->eoffset - fibh->soffset);
 			fibh->eoffset = fibh->soffset + fi_len;
 		} else {
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index c586026508db..06f37ddd2997 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -351,8 +351,6 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
 	loff_t f_pos;
 	loff_t size = udf_ext0_offset(dir) + dir->i_size;
 	int nfidlen;
-	uint8_t lfi;
-	uint16_t liu;
 	udf_pblk_t block;
 	struct kernel_lb_addr eloc;
 	uint32_t elen = 0;
@@ -383,7 +381,7 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
 		namelen = 0;
 	}
 
-	nfidlen = (sizeof(struct fileIdentDesc) + namelen + 3) & ~3;
+	nfidlen = ALIGN(sizeof(struct fileIdentDesc) + namelen, UDF_NAME_PAD);
 
 	f_pos = udf_ext0_offset(dir);
 
@@ -424,12 +422,8 @@ static struct fileIdentDesc *udf_add_entry(struct inode *dir,
 			goto out_err;
 		}
 
-		liu = le16_to_cpu(cfi->lengthOfImpUse);
-		lfi = cfi->lengthFileIdent;
-
 		if ((cfi->fileCharacteristics & FID_FILE_CHAR_DELETED) != 0) {
-			if (((sizeof(struct fileIdentDesc) +
-					liu + lfi + 3) & ~3) == nfidlen) {
+			if (udf_dir_entry_len(cfi) == nfidlen) {
 				cfi->descTag.tagSerialNum = cpu_to_le16(1);
 				cfi->fileVersionNum = cpu_to_le16(1);
 				cfi->fileCharacteristics = 0;
@@ -1201,9 +1195,7 @@ static int udf_rename(struct inode *old_dir, struct dentry *old_dentry,
 
 	if (dir_fi) {
 		dir_fi->icb.extLocation = cpu_to_lelb(UDF_I(new_dir)->i_location);
-		udf_update_tag((char *)dir_fi,
-				(sizeof(struct fileIdentDesc) +
-				le16_to_cpu(dir_fi->lengthOfImpUse) + 3) & ~3);
+		udf_update_tag((char *)dir_fi, udf_dir_entry_len(dir_fi));
 		if (old_iinfo->i_alloc_type == ICBTAG_FLAG_AD_IN_ICB)
 			mark_inode_dirty(old_inode);
 		else
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index fc8d1b3384d2..844f1e134bf9 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -132,6 +132,12 @@ struct inode *udf_find_metadata_inode_efe(struct super_block *sb,
 extern int udf_write_fi(struct inode *inode, struct fileIdentDesc *,
 			struct fileIdentDesc *, struct udf_fileident_bh *,
 			uint8_t *, uint8_t *);
+static inline unsigned int udf_dir_entry_len(struct fileIdentDesc *cfi)
+{
+	return ALIGN(sizeof(struct fileIdentDesc) +
+		le16_to_cpu(cfi->lengthOfImpUse) + cfi->lengthFileIdent,
+		UDF_NAME_PAD);
+}
 
 /* file.c */
 extern long udf_ioctl(struct file *, unsigned int, unsigned long);
-- 
2.16.4

[PATCH 3/3] udf: Drop unused arguments of udf_delete_aext()

[Jan Kara]

udf_delete_aext() uses its last two arguments only as local variables.
Drop them.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/udf/balloc.c  | 5 ++---
 fs/udf/inode.c   | 8 ++++----
 fs/udf/udfdecl.h | 3 +--
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fs/udf/balloc.c b/fs/udf/balloc.c
index 1b961b1d9699..fcda0fc97b90 100644
--- a/fs/udf/balloc.c
+++ b/fs/udf/balloc.c
@@ -533,8 +533,7 @@ static int udf_table_prealloc_blocks(struct super_block *sb,
 			udf_write_aext(table, &epos, &eloc,
 					(etype << 30) | elen, 1);
 		} else
-			udf_delete_aext(table, epos, eloc,
-					(etype << 30) | elen);
+			udf_delete_aext(table, epos);
 	} else {
 		alloc_count = 0;
 	}
@@ -630,7 +629,7 @@ static udf_pblk_t udf_table_new_block(struct super_block *sb,
 	if (goal_elen)
 		udf_write_aext(table, &goal_epos, &goal_eloc, goal_elen, 1);
 	else
-		udf_delete_aext(table, goal_epos, goal_eloc, goal_elen);
+		udf_delete_aext(table, goal_epos);
 	brelse(goal_epos.bh);
 
 	udf_add_free_space(sb, partition, -1);
diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index c80765d62f7e..1f68dac7243e 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -1147,8 +1147,7 @@ static void udf_update_extents(struct inode *inode, struct kernel_long_ad *laarr
 
 	if (startnum > endnum) {
 		for (i = 0; i < (startnum - endnum); i++)
-			udf_delete_aext(inode, *epos, laarr[i].extLocation,
-					laarr[i].extLength);
+			udf_delete_aext(inode, *epos);
 	} else if (startnum < endnum) {
 		for (i = 0; i < (endnum - startnum); i++) {
 			udf_insert_aext(inode, *epos, laarr[i].extLocation,
@@ -2177,14 +2176,15 @@ static int8_t udf_insert_aext(struct inode *inode, struct extent_position epos,
 	return (nelen >> 30);
 }
 
-int8_t udf_delete_aext(struct inode *inode, struct extent_position epos,
-		       struct kernel_lb_addr eloc, uint32_t elen)
+int8_t udf_delete_aext(struct inode *inode, struct extent_position epos)
 {
 	struct extent_position oepos;
 	int adsize;
 	int8_t etype;
 	struct allocExtDesc *aed;
 	struct udf_inode_info *iinfo;
+	struct kernel_lb_addr eloc;
+	uint32_t elen;
 
 	if (epos.bh) {
 		get_bh(epos.bh);
diff --git a/fs/udf/udfdecl.h b/fs/udf/udfdecl.h
index 844f1e134bf9..a5b90297d95f 100644
--- a/fs/udf/udfdecl.h
+++ b/fs/udf/udfdecl.h
@@ -173,8 +173,7 @@ extern int udf_add_aext(struct inode *, struct extent_position *,
 			struct kernel_lb_addr *, uint32_t, int);
 extern void udf_write_aext(struct inode *, struct extent_position *,
 			   struct kernel_lb_addr *, uint32_t, int);
-extern int8_t udf_delete_aext(struct inode *, struct extent_position,
-			      struct kernel_lb_addr, uint32_t);
+extern int8_t udf_delete_aext(struct inode *, struct extent_position);
 extern int8_t udf_next_aext(struct inode *, struct extent_position *,
 			    struct kernel_lb_addr *, uint32_t *, int);
 extern int8_t udf_current_aext(struct inode *, struct extent_position *,
-- 
2.16.4

Re: [PATCH 1/3] udf: Detect incorrect directory size

[Anatoly Trosinenko]

Hello,

Thank you! I have tried this patch against v4.17 kernel. Considering
the original bug report
(https://www.spinics.net/lists/kernel/msg2820542.html), now it returns
Input/output errors:

/init: line 8: can't create
/mnt/1111111111111111111111111111111111111111111111111111111111111111111111111:
Input/output error
ln: /mnt/foo: Input/output error

... and does not page faults, as expected.

чт, 14 июн. 2018 г. в 18:28, Jan Kara <jack@suse.cz>:
>
> Detect when a directory entry is (possibly partially) beyond directory
> size and return EIO in that case since it means the filesystem is
> corrupted. Otherwise directory operations can further corrupt the
> directory and possibly also oops the kernel.
>
> CC: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> CC: stable@vger.kernel.org
> Reported-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/udf/directory.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/udf/directory.c b/fs/udf/directory.c
> index 0a98a2369738..3835f983cc99 100644
> --- a/fs/udf/directory.c
> +++ b/fs/udf/directory.c
> @@ -152,6 +152,9 @@ struct fileIdentDesc *udf_fileident_read(struct inode *dir, loff_t *nf_pos,
>                                sizeof(struct fileIdentDesc));
>                 }
>         }
> +       /* Got last entry outside of dir size - fs is corrupted! */
> +       if (*nf_pos > dir->i_size)
> +               return NULL;
>         return fi;
>  }
>
> --
> 2.16.4
>