* [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata
@ 2026-11-02 13:45 shao.mingyin
2026-11-02 13:45 ` [PATCH 1/5] ubifs: reject inodes with an out-of-range size shao.mingyin
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:45 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
This series tightens the checks ubifs performs on the metadata it reads
from flash. All five patches are about size and accounting fields that
are taken from the image and later used without a range check; an image
whose CRCs and node tags are intact can therefore drive the code into
state it never expects.
Patch 1 rejects inodes whose size does not fit the signed loff_t the size
is stored in. A 64-bit value with the top bit set becomes negative and
passes the max_inode_sz check, and the read paths then do size arithmetic
with it, for example (i_size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT
in do_readpage() and (i_size - 1) >> PAGE_SHIFT in populate_page().
Patch 2 rejects xattr inodes whose i_size does not match data_len. ubifs
keeps the two equal for attribute inodes, and ubifs_xattr_get() and
remove_xattr() assert on the invariant; with assert=read-only or
assert=panic a plain getxattr() or removexattr() turns the file system
read-only or panics the kernel.
Patch 3 checks the lower bound of the master node's highest_inum. Today
only the upper bound is validated, so a value below UBIFS_FIRST_INO is
accepted and ubifs_new_inode() allocates numbers from the range reserved
for special inodes. With highest_inum == 0 the first new inode gets
number 1, which is UBIFS_ROOT_INO and already in use, and the index ends
up with two inodes sharing a number.
Patch 4 validates inode and data node sizes during replay. replay_bud()
trusts them, and ubifs_recover_size() may then extend an inode to a bogus
size. The normal read paths already reject such nodes, and truncation
nodes are checked in replay_bud() as well; do the same here. The data
node end offset is also computed in 64 bits so that a large block number
cannot wrap.
Patch 5 refuses xattr accounting that cannot be decremented. On an image
whose counters are already zero or smaller than the entry being removed,
the unsigned counters wrap and ubifs_jnl_delete_xattr() writes the bogus
values back to flash; the next mount then rejects the inode because
xattr_names + xattr_cnt exceeds XATTR_LIST_MAX, so a single removexattr()
makes the image unmountable.
To keep the severity claims honest: mounting such an image needs
CAP_SYS_ADMIN in the mounting namespace (or access to the flash), so none
of these is a non-root attack. Patch 2 only turns into read-only or panic
with the assert= option; the default action is to report. Patch 5 is the
one with a persistent effect. Patches 1-3 are mount-time validation,
patch 4 is in replay, patch 5 is runtime accounting.
The patches are independent, have no inter-patch dependencies and can be
applied in any order or as a subset. All five carry Fixes: and
Cc: stable.
Testing: each patch applies to next-20261030, passes checkpatch.pl
--strict with 0 errors/0 warnings/0 checks, and fs/ubifs/ builds with
CONFIG_UBIFS_FS_AUTHENTICATION, CONFIG_UBIFS_FS_XATTR and
CONFIG_UBIFS_FS_SECURITY enabled and W=1 without new warnings. I did not
mount crafted images: this machine has no mkfs.ubifs/ubi tools and no
mtdram or nandsim setup. I can put together an image for any of the five
if a runtime check would help the review.
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] ubifs: reject inodes with an out-of-range size
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
@ 2026-11-02 13:45 ` shao.mingyin
2026-11-02 13:49 ` [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value shao.mingyin
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:45 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
validate_inode() compares inode->i_size with c->max_inode_sz only. The
size is read from the on-flash inode node as a 64-bit value and stored
in the signed loff_t, so a value with the top bit set becomes negative
and passes the check.
The read paths then do size arithmetic with that negative value, for
example (i_size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT in
do_readpage() and (i_size - 1) >> PAGE_SHIFT in populate_page().
Reject such inodes like the other invalid on-flash fields.
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 38972786817e..602f0cd9713f 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -71,9 +71,9 @@ static int validate_inode(struct ubifs_info *c, const struct inode *inode)
int err;
const struct ubifs_inode *ui = ubifs_inode(inode);
- if (inode->i_size > c->max_inode_sz) {
- ubifs_err(c, "inode is too large (%lld)",
- (long long)inode->i_size);
+ if (inode->i_size < 0 || inode->i_size > c->max_inode_sz) {
+ ubifs_err(c, "inode size %lld is out of range (max. is %lld)",
+ (long long)inode->i_size, c->max_inode_sz);
return 1;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
2026-11-02 13:45 ` [PATCH 1/5] ubifs: reject inodes with an out-of-range size shao.mingyin
@ 2026-11-02 13:49 ` shao.mingyin
2026-11-03 2:06 ` Zhihao Cheng
2026-11-02 13:49 ` [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number shao.mingyin
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:49 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
An extended attribute inode stores the value in the inode node and
UBIFS keeps inode->i_size equal to ubifs_inode->data_len (see
create_xattr() and change_xattr()). validate_inode() does not check
this for inodes that carry UBIFS_XATTR_FL, so an image with a mismatch
is accepted. ubifs_xattr_get() and remove_xattr() then trip their
ubifs_assert() on the invariant; with the assert=read-only or
assert=panic mount option a plain getxattr() or removexattr() turns the
file system read-only or panics the kernel.
Reject such inodes at iget time.
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
index 38972786817e..9f608c10b39e 100644
--- a/fs/ubifs/super.c
+++ b/fs/ubifs/super.c
@@ -91,6 +91,18 @@ static int validate_inode(struct ubifs_info *c, const struct inode *inode)
if (ui->xattr && !S_ISREG(inode->i_mode))
return 5;
+ /*
+ * Extended attribute inodes keep the value in the inode node and
+ * maintain i_size == data_len everywhere (see create_xattr() and
+ * change_xattr()).
+ */
+ if (ui->xattr && inode->i_size != ui->data_len) {
+ ubifs_err(c, "xattr inode %llu has size %lld, but %d bytes of data",
+ inode->i_ino, (long long)inode->i_size,
+ ui->data_len);
+ return 6;
+ }
+
if (!ubifs_compr_present(c, ui->compr_type)) {
ubifs_warn(c, "inode %llu uses '%s' compression, but it was not compiled in",
inode->i_ino, ubifs_compr_name(c, ui->compr_type));
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
2026-11-02 13:45 ` [PATCH 1/5] ubifs: reject inodes with an out-of-range size shao.mingyin
2026-11-02 13:49 ` [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value shao.mingyin
@ 2026-11-02 13:49 ` shao.mingyin
2026-11-03 2:06 ` Zhihao Cheng
2026-11-02 13:49 ` [PATCH 4/5] ubifs: validate inode and data node sizes during replay shao.mingyin
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:49 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
validate_master() only rejects highest_inum values above
INUM_WATERMARK. A master node whose value is below UBIFS_FIRST_INO is
accepted, and ubifs_new_inode() then allocates inode numbers starting
from that value, i.e. from the range reserved for special inodes. With
highest_inum = 0 the very first new inode gets number 1, which is
UBIFS_ROOT_INO and is already used by the root inode, so two inodes end
up with the same number and the index is corrupted.
Check the lower bound as well.
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c
index a148760fa49e..8a1ba81c10bb 100644
--- a/fs/ubifs/master.c
+++ b/fs/ubifs/master.c
@@ -191,6 +191,16 @@ static int validate_master(const struct ubifs_info *c)
goto out;
}
+ /*
+ * Inode numbers below UBIFS_FIRST_INO are reserved. If the master
+ * node claims a smaller value, newly created inodes would reuse the
+ * root inode number or another reserved one.
+ */
+ if (c->highest_inum < UBIFS_FIRST_INO) {
+ err = 24;
+ goto out;
+ }
+
if (c->lhead_lnum < UBIFS_LOG_LNUM ||
c->lhead_lnum >= UBIFS_LOG_LNUM + c->log_lebs ||
c->lhead_offs < 0 || c->lhead_offs >= c->leb_size ||
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 4/5] ubifs: validate inode and data node sizes during replay
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
` (2 preceding siblings ...)
2026-11-02 13:49 ` [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number shao.mingyin
@ 2026-11-02 13:49 ` shao.mingyin
2026-11-02 13:49 ` [PATCH 5/5] ubifs: xattr: guard against xattr accounting underflow shao.mingyin
2026-11-03 2:06 ` [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata Zhihao Cheng
5 siblings, 0 replies; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:49 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
replay_bud() trusts the size fields of inode and data nodes from the
journal. An image that is corrupt but has a matching CRC can therefore
carry an inode node with a negative or too large size, or a data node
with a size outside 1..UBIFS_BLOCK_SIZE. During recovery
ubifs_recover_size_accum() uses these values and ubifs_recover_size()
may extend the inode to the bogus size.
The normal read paths already reject such nodes (read_block(),
populate_page()) and truncation nodes are validated in replay_bud() as
well. Do the same for inode and data nodes. While at it, compute the
data node end offset with 64-bit arithmetic so that a large block
number cannot wrap.
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/fs/ubifs/replay.c b/fs/ubifs/replay.c
index 263045e05cf1..14cd5598ca0b 100644
--- a/fs/ubifs/replay.c
+++ b/fs/ubifs/replay.c
@@ -741,6 +741,12 @@ static int replay_bud(struct ubifs_info *c, struct bud_entry *b)
struct ubifs_ino_node *ino = snod->node;
loff_t new_size = le64_to_cpu(ino->size);
+ /* Validate inode node */
+ if (new_size < 0 || new_size > c->max_inode_sz) {
+ ubifs_err(c, "bad inode node");
+ goto out_dump;
+ }
+
if (le32_to_cpu(ino->nlink) == 0)
deletion = 1;
err = insert_node(c, lnum, snod->offs, snod->len, hash,
@@ -751,9 +757,21 @@ static int replay_bud(struct ubifs_info *c, struct bud_entry *b)
case UBIFS_DATA_NODE:
{
struct ubifs_data_node *dn = snod->node;
- loff_t new_size = le32_to_cpu(dn->size) +
- key_block(c, &snod->key) *
- UBIFS_BLOCK_SIZE;
+ u32 dsize = le32_to_cpu(dn->size);
+ loff_t new_size;
+
+ /* Validate data node */
+ if (!dsize || dsize > UBIFS_BLOCK_SIZE) {
+ ubifs_err(c, "bad data node");
+ goto out_dump;
+ }
+
+ new_size = (loff_t)key_block(c, &snod->key) *
+ UBIFS_BLOCK_SIZE + dsize;
+ if (new_size > c->max_inode_sz) {
+ ubifs_err(c, "bad data node");
+ goto out_dump;
+ }
err = insert_node(c, lnum, snod->offs, snod->len, hash,
&snod->key, snod->sqnum, deletion,
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 5/5] ubifs: xattr: guard against xattr accounting underflow
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
` (3 preceding siblings ...)
2026-11-02 13:49 ` [PATCH 4/5] ubifs: validate inode and data node sizes during replay shao.mingyin
@ 2026-11-02 13:49 ` shao.mingyin
2026-11-03 2:06 ` [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata Zhihao Cheng
5 siblings, 0 replies; 14+ messages in thread
From: shao.mingyin @ 2026-11-02 13:49 UTC (permalink / raw)
To: richard, linux-mtd; +Cc: chengzhihao1, linux-kernel, stable
remove_xattr() decrements the host inode's xattr counters, which are
stored on flash. An image can contain an xattr entry for an inode
whose counters are already zero or smaller than the entry. The
unsigned counters then wrap, and ubifs_jnl_delete_xattr() writes the
bogus values back to flash. On the next mount validate_inode() rejects
the inode because xattr_names + xattr_cnt exceeds XATTR_LIST_MAX, so a
single removexattr() turns a mountable image into one that cannot be
mounted.
Refuse the operation when the accounting cannot be decremented.
Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
Cc: stable@vger.kernel.org
Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
---
diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index 3b0e8a270f59..8f0447de94f0 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -456,6 +456,21 @@ static int remove_xattr(struct ubifs_info *c, struct inode *host,
ubifs_assert(c, ui->data_len == inode->i_size);
+ /*
+ * The per-inode xattr accounting is stored on flash and can disagree
+ * with the entries found in the TNC. Do not let a corrupted image
+ * underflow the counters, because the bogus values would then be
+ * written back to flash by ubifs_jnl_delete_xattr().
+ */
+ if (!host_ui->xattr_cnt ||
+ host_ui->xattr_names < fname_len(nm) ||
+ host_ui->xattr_size < CALC_DENT_SIZE(fname_len(nm)) +
+ CALC_XATTR_BYTES(ui->data_len)) {
+ ubifs_err(c, "xattr accounting mismatch for inode %llu",
+ host->i_ino);
+ return -EUCLEAN;
+ }
+
err = ubifs_budget_space(c, &req);
if (err)
return err;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number
2026-11-02 13:49 ` [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number shao.mingyin
@ 2026-11-03 2:06 ` Zhihao Cheng
2026-11-03 4:31 ` shao.mingyin
0 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2026-11-03 2:06 UTC (permalink / raw)
To: shao.mingyin, richard, linux-mtd; +Cc: linux-kernel, stable
在 2026/11/2 21:49, shao.mingyin@zte.com.cn 写道:
> validate_master() only rejects highest_inum values above
> INUM_WATERMARK. A master node whose value is below UBIFS_FIRST_INO is
> accepted, and ubifs_new_inode() then allocates inode numbers starting
> from that value, i.e. from the range reserved for special inodes. With
> highest_inum = 0 the very first new inode gets number 1, which is
> UBIFS_ROOT_INO and is already used by the root inode, so two inodes end
> up with the same number and the index is corrupted.
>
> Check the lower bound as well.
>
> Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> diff --git a/fs/ubifs/master.c b/fs/ubifs/master.c
> index a148760fa49e..8a1ba81c10bb 100644
> --- a/fs/ubifs/master.c
> +++ b/fs/ubifs/master.c
> @@ -191,6 +191,16 @@ static int validate_master(const struct ubifs_info *c)
> goto out;
> }
>
> + /*
> + * Inode numbers below UBIFS_FIRST_INO are reserved. If the master
> + * node claims a smaller value, newly created inodes would reuse the
> + * root inode number or another reserved one.
> + */
> + if (c->highest_inum < UBIFS_FIRST_INO) {
> + err = 24;
> + goto out;
> + }
> +
This check could be merged into previous check:
f (c->highest_inum >= INUM_WATERMARK)
> if (c->lhead_lnum < UBIFS_LOG_LNUM ||
> c->lhead_lnum >= UBIFS_LOG_LNUM + c->log_lebs ||
> c->lhead_offs < 0 || c->lhead_offs >= c->leb_size ||
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value
2026-11-02 13:49 ` [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value shao.mingyin
@ 2026-11-03 2:06 ` Zhihao Cheng
2026-11-03 4:31 ` shao.mingyin
0 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2026-11-03 2:06 UTC (permalink / raw)
To: shao.mingyin, richard, linux-mtd; +Cc: linux-kernel, stable
在 2026/11/2 21:49, shao.mingyin@zte.com.cn 写道:
> An extended attribute inode stores the value in the inode node and
> UBIFS keeps inode->i_size equal to ubifs_inode->data_len (see
> create_xattr() and change_xattr()). validate_inode() does not check
> this for inodes that carry UBIFS_XATTR_FL, so an image with a mismatch
> is accepted. ubifs_xattr_get() and remove_xattr() then trip their
> ubifs_assert() on the invariant; with the assert=read-only or
> assert=panic mount option a plain getxattr() or removexattr() turns the
> file system read-only or panics the kernel.
>
> Reject such inodes at iget time.
>
> Fixes: 1e51764a3c2a ("UBIFS: add new flash file system")
> Cc: stable@vger.kernel.org
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
> index 38972786817e..9f608c10b39e 100644
> --- a/fs/ubifs/super.c
> +++ b/fs/ubifs/super.c
> @@ -91,6 +91,18 @@ static int validate_inode(struct ubifs_info *c, const struct inode *inode)
> if (ui->xattr && !S_ISREG(inode->i_mode))
> return 5;
>
> + /*
> + * Extended attribute inodes keep the value in the inode node and
> + * maintain i_size == data_len everywhere (see create_xattr() and
> + * change_xattr()).
> + */
> + if (ui->xattr && inode->i_size != ui->data_len) {
> + ubifs_err(c, "xattr inode %llu has size %lld, but %d bytes of data",
> + inode->i_ino, (long long)inode->i_size,
> + ui->data_len);
> + return 6;
> + }
> +
Above two ui->xattr check could be merged like:
if (ui->xattr) {
if (!S_ISREG(inode->i_mode))
return 5;
if (inode->i_size != ui->data_len)
return 6;
}
> if (!ubifs_compr_present(c, ui->compr_type)) {
> ubifs_warn(c, "inode %llu uses '%s' compression, but it was not compiled in",
> inode->i_ino, ubifs_compr_name(c, ui->compr_type));
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
` (4 preceding siblings ...)
2026-11-02 13:49 ` [PATCH 5/5] ubifs: xattr: guard against xattr accounting underflow shao.mingyin
@ 2026-11-03 2:06 ` Zhihao Cheng
2026-11-03 4:30 ` shao.mingyin
5 siblings, 1 reply; 14+ messages in thread
From: Zhihao Cheng @ 2026-11-03 2:06 UTC (permalink / raw)
To: shao.mingyin, richard, linux-mtd; +Cc: linux-kernel, stable
在 2026/11/2 21:45, shao.mingyin@zte.com.cn 写道:
> This series tightens the checks ubifs performs on the metadata it reads
> from flash. All five patches are about size and accounting fields that
> are taken from the image and later used without a range check; an image
> whose CRCs and node tags are intact can therefore drive the code into
> state it never expects.
>
> Patch 1 rejects inodes whose size does not fit the signed loff_t the size
> is stored in. A 64-bit value with the top bit set becomes negative and
> passes the max_inode_sz check, and the read paths then do size arithmetic
> with it, for example (i_size + UBIFS_BLOCK_SIZE - 1) >> UBIFS_BLOCK_SHIFT
> in do_readpage() and (i_size - 1) >> PAGE_SHIFT in populate_page().
>
> Patch 2 rejects xattr inodes whose i_size does not match data_len. ubifs
> keeps the two equal for attribute inodes, and ubifs_xattr_get() and
> remove_xattr() assert on the invariant; with assert=read-only or
> assert=panic a plain getxattr() or removexattr() turns the file system
> read-only or panics the kernel.
>
> Patch 3 checks the lower bound of the master node's highest_inum. Today
> only the upper bound is validated, so a value below UBIFS_FIRST_INO is
> accepted and ubifs_new_inode() allocates numbers from the range reserved
> for special inodes. With highest_inum == 0 the first new inode gets
> number 1, which is UBIFS_ROOT_INO and already in use, and the index ends
> up with two inodes sharing a number.
>
> Patch 4 validates inode and data node sizes during replay. replay_bud()
> trusts them, and ubifs_recover_size() may then extend an inode to a bogus
> size. The normal read paths already reject such nodes, and truncation
> nodes are checked in replay_bud() as well; do the same here. The data
> node end offset is also computed in 64 bits so that a large block number
> cannot wrap.
>
> Patch 5 refuses xattr accounting that cannot be decremented. On an image
> whose counters are already zero or smaller than the entry being removed,
> the unsigned counters wrap and ubifs_jnl_delete_xattr() writes the bogus
> values back to flash; the next mount then rejects the inode because
> xattr_names + xattr_cnt exceeds XATTR_LIST_MAX, so a single removexattr()
> makes the image unmountable.
>
> To keep the severity claims honest: mounting such an image needs
> CAP_SYS_ADMIN in the mounting namespace (or access to the flash), so none
> of these is a non-root attack. Patch 2 only turns into read-only or panic
> with the assert= option; the default action is to report. Patch 5 is the
> one with a persistent effect. Patches 1-3 are mount-time validation,
> patch 4 is in replay, patch 5 is runtime accounting.
>
> The patches are independent, have no inter-patch dependencies and can be
> applied in any order or as a subset. All five carry Fixes: and
> Cc: stable.
>
> Testing: each patch applies to next-20261030, passes checkpatch.pl
> --strict with 0 errors/0 warnings/0 checks, and fs/ubifs/ builds with
> CONFIG_UBIFS_FS_AUTHENTICATION, CONFIG_UBIFS_FS_XATTR and
> CONFIG_UBIFS_FS_SECURITY enabled and W=1 without new warnings. I did not
> mount crafted images: this machine has no mkfs.ubifs/ubi tools and no
> mtdram or nandsim setup. I can put together an image for any of the five
> if a runtime check would help the review.
>
Hi,
From my perspective, I'm not really interested in the problem scenarios
triggered by malicious fs image, let Richard decide whether to merge it.
This series does not seem to introduce any issues, there are two small
nits in patch 2&3.
> Signed-off-by: Shao Mingyin <shao.mingyin@zte.com.cn>
> ---
> base-commit: 7649ba73f9e7e1e04d0b46213e39c79ae09dd55c
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata
2026-11-03 2:06 ` [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata Zhihao Cheng
@ 2026-11-03 4:30 ` shao.mingyin
2026-11-03 4:50 ` Zhihao Cheng
0 siblings, 1 reply; 14+ messages in thread
From: shao.mingyin @ 2026-11-03 4:30 UTC (permalink / raw)
To: chengzhihao1; +Cc: richard, linux-mtd, linux-kernel
> From my perspective, I'm not really interested in the problem scenarios
> triggered by malicious fs image, let Richard decide whether to merge it.
> This series does not seem to introduce any issues, there are two small
> nits in patch 2&3.
Thanks for the review. The threat model in the cover is the only one the
series claims: mounting such an image needs CAP_SYS_ADMIN in the mounting
namespace or access to the flash.
Two of the five are not strictly "malicious image only", in case that is
useful for Richard's call:
- 4/5 is replay: a node that was corrupted on flash or written wrong would
otherwise let ubifs_recover_size() extend an inode to a bogus size.
- 5/5 leaves the image unmountable after a single removexattr(): the
underflowed counters are written back to flash, so it survives remount.
The other three are mount-time validation of fields that validate_inode()
and validate_master() already range-check for other values, so they change
nothing for a valid image. The patches are independent and any subset
applies, so dropping some is fine -- I will leave the decision to Richard.
Both nits are taken: patch 2 folds the size check into the existing
ui->xattr block, patch 3 merges the lower bound into the highest_inum
check (that one then reports error 3 for both bounds).
For the series: I will repost only those two as [PATCH v2] in a new
thread, each noting that it supersedes 2/5 or 3/5 of the v1 series. 1/5,
4/5 and 5/5 are untouched, so I would rather not resend them -- say the
word if you would prefer the whole series as v2.
Thanks,
Shao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value
2026-11-03 2:06 ` Zhihao Cheng
@ 2026-11-03 4:31 ` shao.mingyin
0 siblings, 0 replies; 14+ messages in thread
From: shao.mingyin @ 2026-11-03 4:31 UTC (permalink / raw)
To: chengzhihao1; +Cc: richard, linux-mtd, linux-kernel
> Above two ui->xattr check could be merged like:
> if (ui->xattr) {
> if (!S_ISREG(inode->i_mode))
> return 5;
>
> if (inode->i_size != ui->data_len)
> return 6;
> }
Agreed, that is cleaner. v2 folds the size check into the existing
ui->xattr block, with the mode check first, and keeps error codes 5 and 6.
Thanks,
Shao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number
2026-11-03 2:06 ` Zhihao Cheng
@ 2026-11-03 4:31 ` shao.mingyin
2026-11-03 4:50 ` Zhihao Cheng
0 siblings, 1 reply; 14+ messages in thread
From: shao.mingyin @ 2026-11-03 4:31 UTC (permalink / raw)
To: chengzhihao1; +Cc: richard, linux-mtd, linux-kernel
> This check could be merged into previous check:
> f (c->highest_inum >= INUM_WATERMARK)
Agreed, the two bounds belong in one test:
if (c->highest_inum >= INUM_WATERMARK ||
c->highest_inum < UBIFS_FIRST_INO)
One side effect: a too small highest_inum then reports error 3 instead of
the separate code 24. If you would rather keep the two ranges apart in
the log, I can leave the lower bound as its own check.
Thanks,
Shao
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number
2026-11-03 4:31 ` shao.mingyin
@ 2026-11-03 4:50 ` Zhihao Cheng
0 siblings, 0 replies; 14+ messages in thread
From: Zhihao Cheng @ 2026-11-03 4:50 UTC (permalink / raw)
To: shao.mingyin; +Cc: richard, linux-mtd, linux-kernel
在 2026/11/3 12:31, shao.mingyin@zte.com.cn 写道:
>> This check could be merged into previous check:
>> f (c->highest_inum >= INUM_WATERMARK)
>
> Agreed, the two bounds belong in one test:
>
> if (c->highest_inum >= INUM_WATERMARK ||
> c->highest_inum < UBIFS_FIRST_INO)
>
This looks fine, there are many places return the same err code
correponding several reasons, the err code help user to identify which
field is incorrect, then ubifs_dump_node() will show all details.
> One side effect: a too small highest_inum then reports error 3 instead of
> the separate code 24. If you would rather keep the two ranges apart in
> the log, I can leave the lower bound as its own check.
>
> Thanks,
> Shao
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata
2026-11-03 4:30 ` shao.mingyin
@ 2026-11-03 4:50 ` Zhihao Cheng
0 siblings, 0 replies; 14+ messages in thread
From: Zhihao Cheng @ 2026-11-03 4:50 UTC (permalink / raw)
To: shao.mingyin; +Cc: richard, linux-mtd, linux-kernel
在 2026/11/3 12:30, shao.mingyin@zte.com.cn 写道:
>> From my perspective, I'm not really interested in the problem scenarios
>> triggered by malicious fs image, let Richard decide whether to merge it.
>> This series does not seem to introduce any issues, there are two small
>> nits in patch 2&3.
>
> Thanks for the review. The threat model in the cover is the only one the
> series claims: mounting such an image needs CAP_SYS_ADMIN in the mounting
> namespace or access to the flash.
>
> Two of the five are not strictly "malicious image only", in case that is
> useful for Richard's call:
>
> - 4/5 is replay: a node that was corrupted on flash or written wrong would
> otherwise let ubifs_recover_size() extend an inode to a bogus size.
> - 5/5 leaves the image unmountable after a single removexattr(): the
> underflowed counters are written back to flash, so it survives remount.
>
> The other three are mount-time validation of fields that validate_inode()
> and validate_master() already range-check for other values, so they change
> nothing for a valid image. The patches are independent and any subset
> applies, so dropping some is fine -- I will leave the decision to Richard.
>
> Both nits are taken: patch 2 folds the size check into the existing
> ui->xattr block, patch 3 merges the lower bound into the highest_inum
> check (that one then reports error 3 for both bounds).
>
> For the series: I will repost only those two as [PATCH v2] in a new
> thread, each noting that it supersedes 2/5 or 3/5 of the v1 series. 1/5,
> 4/5 and 5/5 are untouched, so I would rather not resend them -- say the
> word if you would prefer the whole series as v2.
Resend v2 series please.
>
> Thanks,
> Shao
> .
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-11-03 4:51 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-02 13:45 [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata shao.mingyin
2026-11-02 13:45 ` [PATCH 1/5] ubifs: reject inodes with an out-of-range size shao.mingyin
2026-11-02 13:49 ` [PATCH 2/5] ubifs: reject xattr inodes whose size does not match the value shao.mingyin
2026-11-03 2:06 ` Zhihao Cheng
2026-11-03 4:31 ` shao.mingyin
2026-11-02 13:49 ` [PATCH 3/5] ubifs: reject master nodes claiming a too small inode number shao.mingyin
2026-11-03 2:06 ` Zhihao Cheng
2026-11-03 4:31 ` shao.mingyin
2026-11-03 4:50 ` Zhihao Cheng
2026-11-02 13:49 ` [PATCH 4/5] ubifs: validate inode and data node sizes during replay shao.mingyin
2026-11-02 13:49 ` [PATCH 5/5] ubifs: xattr: guard against xattr accounting underflow shao.mingyin
2026-11-03 2:06 ` [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata Zhihao Cheng
2026-11-03 4:30 ` shao.mingyin
2026-11-03 4:50 ` Zhihao Cheng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®