Re: [PATCH] jffs2: prevent xattr node from overflowing the eraseblock

[Christian Brauner]

On Fri, Apr 12, 2024 at 06:53:54PM +0300, Ilya Denisyev wrote:
> Add a check to make sure that the requested xattr node size is no larger
> than the eraseblock minus the cleanmarker.
> 
> Unlike the usual inode nodes, the xattr nodes aren't split into parts
> and spread across multiple eraseblocks, which means that a xattr node
> must not occupy more than one eraseblock. If the requested xattr value is
> too large, the xattr node can spill onto the next eraseblock, overwriting
> the nodes and causing errors such as:
> 
> jffs2: argh. node added in wrong place at 0x0000b050(2)
> jffs2: nextblock 0x0000a000, expected at 0000b00c
> jffs2: error: (823) do_verify_xattr_datum: node CRC failed at 0x01e050, 
> read=0xfc892c93, calc=0x000000
> jffs2: notice: (823) jffs2_get_inode_nodes: Node header CRC failed 
> at 0x01e00c. {848f,2fc4,0fef511f,59a3d171}
> jffs2: Node at 0x0000000c with length 0x00001044 would run over the 
> end of the erase block
> jffs2: Perhaps the file system was created with the wrong erase size?
> jffs2: jffs2_scan_eraseblock(): Magic bitmask 0x1985 not found
> at 0x00000010: 0x1044 instead
> 
> This breaks the filesystem and can lead to KASAN crashes such as:
> 
> BUG: KASAN: slab-out-of-bounds in jffs2_sum_add_kvec+0x125e/0x15d0
> Read of size 4 at addr ffff88802c31e914 by task repro/830
> CPU: 0 PID: 830 Comm: repro Not tainted 6.9.0-rc3+ #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
> BIOS Arch Linux 1.16.3-1-1 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0xc6/0x120
>  print_report+0xc4/0x620
>  ? __virt_addr_valid+0x308/0x5b0
>  kasan_report+0xc1/0xf0
>  ? jffs2_sum_add_kvec+0x125e/0x15d0
>  ? jffs2_sum_add_kvec+0x125e/0x15d0
>  jffs2_sum_add_kvec+0x125e/0x15d0
>  jffs2_flash_direct_writev+0xa8/0xd0
>  jffs2_flash_writev+0x9c9/0xef0
>  ? __x64_sys_setxattr+0xc4/0x160
>  ? do_syscall_64+0x69/0x140
>  ? entry_SYSCALL_64_after_hwframe+0x76/0x7e
>  [...]
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Fixes: aa98d7cf59b5 ("[JFFS2][XATTR] XATTR support on JFFS2 (version. 5)")
> Signed-off-by: Ilya Denisyev <dev@xxxxxxxx>
> ---
>  fs/jffs2/xattr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/jffs2/xattr.c b/fs/jffs2/xattr.c
> index 00224f3a8d6e..9509b33f7675 100644
> --- a/fs/jffs2/xattr.c
> +++ b/fs/jffs2/xattr.c
> @@ -1110,6 +1110,9 @@ int do_jffs2_setxattr(struct inode *inode, int xprefix, const char *xname,
>  		return rc;
>  
>  	request = PAD(sizeof(struct jffs2_raw_xattr) + strlen(xname) + 1 + size);
> +	if (request > c->sector_size - c->cleanmarker_size)

Can this overflow? I.e. can c->sector_size be smaller than c->cleanmarker_size?