Re: [PATCH 0/5] ubifs: harden validation of on-disk inode and node metadata

[Zhihao Cheng <chengzhihao1@huawei.com>]

在 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
> .
>