Skip to contentSkip to conversation

FAILED: patch "[PATCH] erofs: fix extended inode could cross boundary" failed to apply to 4.19-stable tree

PATCH [email protected] 2 participants 8 messages Aug 17, 2020 – Apr 25, 2021

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+
fs/erofs/inode.c +79 -42
index 577fc9df4471..139d0bed42f8 100644
@@ -8,31 +8,80 @@
 
 #include <trace/events/erofs.h>
 
-/* no locking */
-static int erofs_read_inode(struct inode *inode, void *data)
+/*
+ * if inode is successfully read, return its inode page (or sometimes
+ * the inode payload page if it's an extended inode) in order to fill
+ * inline data if possible.
+ */
+static struct page *erofs_read_inode(struct inode *inode,
+				     unsigned int *ofs)
 {
+	struct super_block *sb = inode->i_sb;
+	struct erofs_sb_info *sbi = EROFS_SB(sb);
 	struct erofs_inode *vi = EROFS_I(inode);
-	struct erofs_inode_compact *dic = data;
-	struct erofs_inode_extended *die;
+	const erofs_off_t inode_loc = iloc(sbi, vi->nid);
+
+	erofs_blk_t blkaddr, nblks = 0;
+	struct page *page;
+	struct erofs_inode_compact *dic;
+	struct erofs_inode_extended *die, *copied = NULL;
+	unsigned int ifmt;
+	int err;
 
-	const unsigned int ifmt = le16_to_cpu(dic->i_format);
-	struct erofs_sb_info *sbi = EROFS_SB(inode->i_sb);
-	erofs_blk_t nblks = 0;
+	blkaddr = erofs_blknr(inode_loc);
+	*ofs = erofs_blkoff(inode_loc);
 
-	vi->datalayout = erofs_inode_datalayout(ifmt);
+	erofs_dbg("%s, reading inode nid %llu at %u of blkaddr %u",
+		  __func__, vi->nid, *ofs, blkaddr);
+
+	page = erofs_get_meta_page(sb, blkaddr);
+	if (IS_ERR(page)) {
+		erofs_err(sb, "failed to get inode (nid: %llu) page, err %ld",
+			  vi->nid, PTR_ERR(page));
+		return page;
+	}
 
+	dic = page_address(page) + *ofs;
+	ifmt = le16_to_cpu(dic->i_format);
+
+	vi->datalayout = erofs_inode_datalayout(ifmt);
 	if (vi->datalayout >= EROFS_INODE_DATALAYOUT_MAX) {
 		erofs_err(inode->i_sb, "unsupported datalayout %u of nid %llu",
 			  vi->datalayout, vi->nid);
-		DBG_BUGON(1);
-		return -EOPNOTSUPP;
+		err = -EOPNOTSUPP;
+		goto err_out;
 	}
 
 	switch (erofs_inode_version(ifmt)) {
 	case EROFS_INODE_LAYOUT_EXTENDED:
-		die = data;
-
 		vi->inode_isize = sizeof(struct erofs_inode_extended);
+		/* check if the inode acrosses page boundary */
+		if (*ofs + vi->inode_isize <= PAGE_SIZE) {
+			*ofs += vi->inode_isize;
+			die = (struct erofs_inode_extended *)dic;
+		} else {
+			const unsigned int gotten = PAGE_SIZE - *ofs;
+
+			copied = kmalloc(vi->inode_isize, GFP_NOFS);
+			if (!copied) {
+				err = -ENOMEM;
+				goto err_out;
+			}
+			memcpy(copied, dic, gotten);
+			unlock_page(page);
+			put_page(page);
+
+			page = erofs_get_meta_page(sb, blkaddr + 1);
+			if (IS_ERR(page)) {
+				erofs_err(sb, "failed to get inode payload page (nid: %llu), err %ld",
+					  vi->nid, PTR_ERR(page));
+				kfree(copied);
+				return page;
+			}
+			*ofs = vi->inode_isize - gotten;
+			memcpy((u8 *)copied + gotten, page_address(page), *ofs);
+			die = copied;
+		}
 		vi->xattr_isize = erofs_xattr_ibody_size(die->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(die->i_mode);
@@ -69,9 +118,12 @@ static int erofs_read_inode(struct inode *inode, void *data)
 		/* total blocks for compressed files */
 		if (erofs_inode_is_data_compressed(vi->datalayout))
 			nblks = le32_to_cpu(die->i_u.compressed_blocks);
+
+		kfree(copied);
 		break;
 	case EROFS_INODE_LAYOUT_COMPACT:
 		vi->inode_isize = sizeof(struct erofs_inode_compact);
+		*ofs += vi->inode_isize;
 		vi->xattr_isize = erofs_xattr_ibody_size(dic->i_xattr_icount);
 
 		inode->i_mode = le16_to_cpu(dic->i_mode);
@@ -111,8 +163,8 @@ static int erofs_read_inode(struct inode *inode, void *data)
 		erofs_err(inode->i_sb,
 			  "unsupported on-disk inode version %u of nid %llu",
 			  erofs_inode_version(ifmt), vi->nid);
-		DBG_BUGON(1);
-		return -EOPNOTSUPP;
+		err = -EOPNOTSUPP;
+		goto err_out;
 	}
 
 	if (!nblks)
@@ -120,13 +172,18 @@ static int erofs_read_inode(struct inode *inode, void *data)
 		inode->i_blocks = roundup(inode->i_size, EROFS_BLKSIZ) >> 9;
 	else
 		inode->i_blocks = nblks << LOG_SECTORS_PER_BLOCK;
-	return 0;
+	return page;
 
 bogusimode:
 	erofs_err(inode->i_sb, "bogus i_mode (%o) @ nid %llu",
 		  inode->i_mode, vi->nid);
+	err = -EFSCORRUPTED;
+err_out:
 	DBG_BUGON(1);
-	return -EFSCORRUPTED;
+	kfree(copied);
+	unlock_page(page);
+	put_page(page);
+	return ERR_PTR(err);
 }
 
 static int erofs_fill_symlink(struct inode *inode, void *data,
@@ -146,7 +203,7 @@ static int erofs_fill_symlink(struct inode *inode, void *data,
 	if (!lnk)
 		return -ENOMEM;
 
-	m_pofs += vi->inode_isize + vi->xattr_isize;
+	m_pofs += vi->xattr_isize;
 	/* inline symlink data shouldn't cross page boundary as well */
 	if (m_pofs + inode->i_size > PAGE_SIZE) {
 		kfree(lnk);
@@ -167,37 +224,17 @@ static int erofs_fill_symlink(struct inode *inode, void *data,
 
 static int erofs_fill_inode(struct inode *inode, int isdir)
 {
-	struct super_block *sb = inode->i_sb;
 	struct erofs_inode *vi = EROFS_I(inode);
 	struct page *page;
-	void *data;
-	int err;
-	erofs_blk_t blkaddr;
 	unsigned int ofs;
-	erofs_off_t inode_loc;
+	int err = 0;
 
 	trace_erofs_fill_inode(inode, isdir);
-	inode_loc = iloc(EROFS_SB(sb), vi->nid);
-	blkaddr = erofs_blknr(inode_loc);
-	ofs = erofs_blkoff(inode_loc);
-
-	erofs_dbg("%s, reading inode nid %llu at %u of blkaddr %u",
-		  __func__, vi->nid, ofs, blkaddr);
 
-	page = erofs_get_meta_page(sb, blkaddr);
-
-	if (IS_ERR(page)) {
-		erofs_err(sb, "failed to get inode (nid: %llu) page, err %ld",
-			  vi->nid, PTR_ERR(page));
+	/* read inode base data from disk */
+	page = erofs_read_inode(inode, &ofs);
+	if (IS_ERR(page))
 		return PTR_ERR(page);
-	}
-
-	DBG_BUGON(!PageUptodate(page));
-	data = page_address(page);
-
-	err = erofs_read_inode(inode, data + ofs);
-	if (err)
-		goto out_unlock;
 
 	/* setup the new inode */
 	switch (inode->i_mode & S_IFMT) {
@@ -210,7 +247,7 @@ static int erofs_fill_inode(struct inode *inode, int isdir)
 		inode->i_fop = &erofs_dir_fops;
 		break;
 	case S_IFLNK:
-		err = erofs_fill_symlink(inode, data, ofs);
+		err = erofs_fill_symlink(inode, page_address(page), ofs);
 		if (err)
 			goto out_unlock;
 		inode_nohighmem(inode);
#1 lore raw
Quoted 29 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Quoted 7 lines

Let's fix now.

Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+

Yeah, due to code difference, will manually backport this later...

Thanks,
Gao Xiang

author maintainer
#2 lore raw
Quoted 32 lines · Gao Xiang
Quoted 31 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Quoted 7 lines
Quoted 5 lines
Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

thanks,

greg k-h

#3 lore raw

Hi Greg,

Quoted 39 lines · Greg KH
Quoted 38 lines · Gao Xiang
Quoted 36 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+
Quoted 3 lines
Quoted 1 line

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

Thanks for your reminder, since the codebase was cleaned up and 4.19 codebase is somewhat different from the current codebase.

Sorry for forgeting it, and I will try to pick it up and send it out soon.

Thanks,
Gao Xiang

Quoted 5 lines

thanks,

greg k-h

author maintainer
#4 lore raw
Quoted 42 lines · Gao Xiang

Hi Greg,

Quoted 39 lines · Greg KH
Quoted 38 lines · Gao Xiang
Quoted 36 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+
Quoted 8 lines
Quoted 3 lines
Quoted 1 line

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

Thanks for your reminder, since the codebase was cleaned up and 4.19 codebase is somewhat different from the current codebase.

Sorry for forgeting it, and I will try to pick it up and send it out soon.

No worries, just ran across this and wanted to make sure that I didn't drop it on my end somewhere.

thanks,

greg k-h

#5 lore raw
Quoted 47 lines · Greg KH
Quoted 46 lines · Gao Xiang

Hi Greg,

Quoted 42 lines · Greg KH
Quoted 32 lines · Gao Xiang
Quoted 31 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Quoted 7 lines
Quoted 5 lines
Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

Quoted 7 lines
Quoted 4 lines

Thanks for your reminder, since the codebase was cleaned up and 4.19 codebase is somewhat different from the current codebase.

Sorry for forgeting it, and I will try to pick it up and send it out soon.

No worries, just ran across this and wanted to make sure that I didn't drop it on my end somewhere.

Nope, that was my fault. :)

Due to 4.19 erofs staging version was quite an early version (1st upstreaming version), more non-trivial conflicts occur in this patch. But it needs to be fixed with careness if users would like to use 4.19 staging erofs and use extended inode. I'm addressing this now.

Yet, I've suggested all Android vendors / users use 5.4+ LTS fs/erofs versions, since in-place decompression has been supported since linux 5.3 which is great for performance. And the 5.4 erofs codebase is already shipped for many other SoC vendors with their in-market products.

Thanks,
Gao Xiang

Quoted 5 lines

thanks,

greg k-h

author maintainer
#6 lore raw
Quoted 63 lines · Gao Xiang
Quoted 47 lines · Greg KH
Quoted 46 lines · Gao Xiang

Hi Greg,

Quoted 42 lines · Greg KH
Quoted 32 lines · Gao Xiang
Quoted 31 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Quoted 7 lines
Quoted 5 lines
Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

Quoted 7 lines
Quoted 4 lines

Thanks for your reminder, since the codebase was cleaned up and 4.19 codebase is somewhat different from the current codebase.

Sorry for forgeting it, and I will try to pick it up and send it out soon.

No worries, just ran across this and wanted to make sure that I didn't drop it on my end somewhere.

Nope, that was my fault. :)

Due to 4.19 erofs staging version was quite an early version (1st upstreaming version), more non-trivial conflicts occur in this patch. But it needs to be fixed with careness if users would like to use 4.19 staging erofs and use extended inode. I'm addressing this now.

Quoted 4 lines

Yet, I've suggested all Android vendors / users use 5.4+ LTS fs/erofs versions, since in-place decompression has been supported since linux 5.3 which is great for performance. And the 5.4 erofs codebase is already shipped for many other SoC vendors with their in-market products.

I too would recommend that anyone using erofs use a newer version, but for those stuck on older kernels like 4.19, they don't seem to be able to want to do that.

Should we just mark the filesystem as "BROKEN" on the stable 4.19 tree to prevent anyone from using it there? That feels drastic, but it's your call what would work best here.

thanks,

greg k-h

#7 lore raw
Quoted 69 lines · Greg KH
Quoted 63 lines · Gao Xiang
Quoted 47 lines · Greg KH
Quoted 46 lines · Gao Xiang

Hi Greg,

Quoted 42 lines · Greg KH
Quoted 32 lines · Gao Xiang
Quoted 31 lines · On Mon, Aug 17, 2020 at 02:38:46PM +0200, [email protected] wrote:

The patch below does not apply to the 4.19-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 0dcd3c94e02438f4a571690e26f4ee997524102a Mon Sep 17 00:00:00 2001
From: Gao Xiang <[email protected]>
Date: Thu, 30 Jul 2020 01:58:01 +0800
Subject: [PATCH] erofs: fix extended inode could cross boundary

Each ondisk inode should be aligned with inode slot boundary (32-byte alignment) because of nid calculation formula, so all compact inodes (32 byte) cannot across page boundary. However, extended inode is now 64-byte form, which can across page boundary in principle if the location is specified on purpose, although it's hard to be generated by mkfs due to the allocation policy and rarely used by Android use case now mainly for > 4GiB files.

For now, only two fields i_ctime_nsec and `i_nlink' couldn't be read from disk properly and cause out-of-bound memory read with random value.

Let's fix now.

Quoted 7 lines
Quoted 5 lines
Fixes 431339ba9042 ("staging: erofs: add inode operations")
Cc <[email protected]> # 4.19+

Yeah, due to code difference, will manually backport this later...

What ever happened to this backport? Did I miss it somewhere?

Quoted 7 lines
Quoted 4 lines

Thanks for your reminder, since the codebase was cleaned up and 4.19 codebase is somewhat different from the current codebase.

Sorry for forgeting it, and I will try to pick it up and send it out soon.

No worries, just ran across this and wanted to make sure that I didn't drop it on my end somewhere.

Nope, that was my fault. :)

Due to 4.19 erofs staging version was quite an early version (1st upstreaming version), more non-trivial conflicts occur in this patch. But it needs to be fixed with careness if users would like to use 4.19 staging erofs and use extended inode. I'm addressing this now.

Quoted 4 lines

Yet, I've suggested all Android vendors / users use 5.4+ LTS fs/erofs versions, since in-place decompression has been supported since linux 5.3 which is great for performance. And the 5.4 erofs codebase is already shipped for many other SoC vendors with their in-market products.

Quoted 7 lines

I too would recommend that anyone using erofs use a newer version, but for those stuck on older kernels like 4.19, they don't seem to be able to want to do that.

Should we just mark the filesystem as "BROKEN" on the stable 4.19 tree to prevent anyone from using it there? That feels drastic, but it's your call what would work best here.

4.19 staging erofs version is also workable with old mkfs (but lack of some basic performance features compared with other actual in-market instances), but I'm also saying "yes", it should be better to use Linux 5.4/5.10 LTS or later codebase directly (or backporting such codebase to 4.19/4.14 manually rather than directly use 4.19 in-tree staging erofs.)

I agree marking 4.19 staging erofs "BROKEN" may be a better choice here and suggest them using 5.4/5.10 codebase instead if needed. But I'll still mark stable patches for 4.19 in case of users using it (Also I will still go on trying to backport this patch.)

Thanks,
Gao Xiang

Quoted 5 lines

thanks,

greg k-h