Re: [PATCH] jffs2: validate summary offset in the XIP scan path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] jffs2: missing lower-bound check on sumlen in
  jffs2_scan_eraseblock()
- [Low] jffs2: missing Fixes trailer for XIP summary check in
  jffs2_scan_eraseblock()
--

commit c169a1630c9003fb886cb75a5950e0c3097a1251
Author: Hyunjung Ko <hj351016@gmail.com>

> jffs2: validate summary offset in the XIP scan path
>
> jffs2_scan_eraseblock() locates the summary node at the end of an
> eraseblock and derives both a pointer and a length from the on-flash
> summary marker:
>
>         sumptr = buf + je32_to_cpu(sm->offset);
>         sumlen = c->sector_size - je32_to_cpu(sm->offset);
>
> In the non-XIP branch this is followed by
>
>         if (sumlen > c->sector_size)
>                 goto full_scan;
>
> so a bogus sm->offset (for which sector_size - offset underflows to a
> large value) is rejected.  The XIP branch (taken when the whole device
> was successfully mtd_point()ed, e.g. an mtdram-backed MTD) is missing
> that check.
>
> On a crafted image an attacker controls the bytes of the summary marker
> at the end of the last eraseblock.  Setting sm->magic == JFFS2_SUM_MAGIC
> and sm->offset > sector_size makes sumlen wrap to ~4 GiB while sumptr
> still points inside the mapped flash.  jffs2_sum_scan_sumnode() then runs
>
>         crc = crc32(0, summary->sum,
>                     sumsize - sizeof(struct jffs2_raw_summary));
>
> with the attacker-controlled sumsize, reading far past the end of the
> mtd_point() mapping -- an out-of-bounds read (KASAN: vmalloc OOB /
> general protection fault) of controlled length.  The preceding header
> CRC checks are computed over data read from the attacker-controlled
> flash and can be satisfied, so they do not prevent the over-read.
>
> Mirror the non-XIP guard in the XIP branch.
>
> Reproduced under KASAN on 7.4-rc1 by mounting a crafted mtdram-backed
> image (the XIP mtd_point() branch): KASAN reports a vmalloc-out-of-bounds
> read in the summary-node crc32 over the mapped flash.
>
> Signed-off-by: Hyunjung Ko <hj351016@gmail.com>

[Severity: Low]
Should this include a Fixes: tag for the commit that introduced the XIP
summary scan path?

  Fixes: 9641b784ff82 ("[JFFS2] Optimise reading of eraseblock summary nodes")

> diff --git a/fs/jffs2/scan.c b/fs/jffs2/scan.c
> index 06e4947977242..e197d56e82dcc 100644
> --- a/fs/jffs2/scan.c
> +++ b/fs/jffs2/scan.c
> @@ -495,6 +495,10 @@ static int jffs2_scan_eraseblock (struct jffs2_sb_info *c, struct jffs2_eraseblo
>  			if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC) {
>  				sumptr = buf + je32_to_cpu(sm->offset);
>  				sumlen = c->sector_size - je32_to_cpu(sm->offset);
> +
> +				/* sm->offset may be garbage even if MAGIC matched */
> +				if (sumlen > c->sector_size)
> +					goto full_scan;
>  			}
>  		} else {
>  			/* If NAND flash, read a whole page of it. Else just the end */

[Severity: Medium]
Can a small sumlen (where 0 <= sumlen < sizeof(struct jffs2_raw_summary))
still read past the end of the buffer and underflow in
jffs2_sum_scan_sumnode()?

When je32_to_cpu(sm->offset) is within the last 31 bytes of the
eraseblock, sumlen is between 0 and 31 and passes the sumlen >
c->sector_size check.  jffs2_sum_scan_sumnode() then reads the 32-byte
struct jffs2_raw_summary from sumptr (reading past the mtd_point()
mapping on the last eraseblock, or past buf + buf_size in the non-XIP
branch), and on a non-final eraseblock where the header and node CRCs are
satisfied across the block boundary, sumsize - sizeof(struct
jffs2_raw_summary) underflows in crc32().

Should both branches (or jffs2_sum_scan_sumnode() itself) also reject
sumlen < sizeof(struct jffs2_raw_summary)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106095108.2405364-1-hj351016@gmail.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/