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 <[email protected]>
---
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)
+ return -ERANGE;
+
rc = jffs2_reserve_space(c, request, &length,
ALLOC_NORMAL, JFFS2_SUMMARY_XATTR_SIZE);
if (rc) {
--
2.44.0
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 <[email protected]>
> ---
> 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?
On 15.04.2024 17:28, Christian Brauner wrote:
> Can this overflow? I.e. can c->sector_size be smaller than c->cleanmarker_size?
I'm pretty sure that it can't. As far as I know, c->sector_size is at
least the MTD eraseblock size[1] or even bigger in the case of
DataFlash[2]. The cleanmarker is either the size of the smallest JFFS2
node[3] (currently 12 bytes), 0 bytes for certain flash types (like
NAND, where it's stored out of band[4]) or apparently at most the MTD
writesize (which, as far as I understand, is not larger than the
erasesize) for Intel Sibley[5].
Considering that any JFFS2 node must fit inside one block and that the
cleanmarker is the smallest node there is (if it's even a proper node),
it shouldn't overflow, otherwise JFFS2 won't work in the first place.
[1] https://elixir.bootlin.com/linux/v6.9-rc4/source/fs/jffs2/fs.c#L539
[2] https://elixir.bootlin.com/linux/v6.9-rc4/source/fs/jffs2/wbuf.c#L1253
[3] https://elixir.bootlin.com/linux/v6.9-rc4/source/fs/jffs2/fs.c#L557
[4] https://elixir.bootlin.com/linux/v6.9-rc4/source/fs/jffs2/wbuf.c#L1190
[5] https://elixir.bootlin.com/linux/v6.9-rc4/source/fs/jffs2/wbuf.c#L1296
---
Best regards,
Ilya
On Fri, 12 Apr 2024 18:53:54 +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:
>
> [...]
I have to say that the rick-roll of your domain doesn't inspire a lot of
confidence though...
Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc
[1/1] jffs2: prevent xattr node from overflowing the eraseblock
https://git.kernel.org/vfs/vfs/c/c6854e5a267c