| Message ID | 20260902124044.3942821-1-henrymei@tencent.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: inftl: validate MediaHeader partition geometry before allocating tables | expand |
Hi Henry, On 02/09/2026 at 20:40:19 +08, henrymei <ljp1205831794@gmail.com> wrote: > From: Aohan Mei <henrymei@tencent.com> > > find_boot_record() trusts the on-flash INFTL MediaHeader partition > fields without validating their relationship to each other or to the > device geometry. > > The sanity check > > if ((ip->lastUnit - ip->firstUnit + 1) < ip->virtualUnits) > > is evaluated in unsigned 32-bit arithmetic. With lastUnit < firstUnit lastUnit < firstUnit? Did you even read that sentence? Sorry but, no. Thanks, Miquèl
Hi Miquèl, On 02/09/2026, Miquel Raynal wrote: > > is evaluated in unsigned 32-bit arithmetic. With lastUnit < firstUnit > > lastUnit < firstUnit? Did you even read that sentence? I should have made the arithmetic explicit in the first place. All three fields are __u32 (struct INFTLPartition), so with the crafted header used in testing (firstUnit=7000, lastUnit=3, virtualUnits=100): (3 - 7000 + 1) == 4294960300 /* u32 wrap */ 4294960300 < 100 == false /* sanity check passes */ Execution then continues with nb_boot_blocks=7000 as the loop bound against a kmalloc_array(lastUnit + 1 = 4, 2) = 8-byte PUtable, and the boot-block marking loop writes ~14 KB past the object. Reproduced on v7.2-rc4 with a RAM-backed fake DiskOnChip MTD device carrying that header: BUG: KASAN: slab-out-of-bounds in find_boot_record Write of size 2 ... 0 bytes to the right of allocated 8-byte region That said, the reachability is admittedly narrow: this is a mount-time path, so triggering requires root (device registration) or physical control of the flash contents; unprivileged users cannot reach it. The intent is only hardening of the MediaHeader parser, in the same spirit as the sanity checks already in find_boot_record(). One fair point about v1: the new check runs on every partition entry during the scan, while only the selected BDTL entry's fields are actually used for the allocations. If entries with lastUnit < firstUnit can legitimately appear in other slots on real media, I can respin to validate only the selected partition (and drop the boot-record-unit check if preferred). Happy to send a v2 along those lines if you think the hardening is worthwhile; otherwise I will drop it. Either way, thanks for the time. Thanks, Aohan
On 03/09/2026 at 17:43:04 +08, 林佳鹏 <ljp1205831794@gmail.com> wrote: > Hi Miquèl, > > On 02/09/2026, Miquel Raynal wrote: >> > is evaluated in unsigned 32-bit arithmetic. With lastUnit < firstUnit >> >> lastUnit < firstUnit? Did you even read that sentence? > > I should have made the arithmetic explicit in the first place. > All three fields are __u32 (struct INFTLPartition), so with the > crafted header used in testing (firstUnit=7000, lastUnit=3, > virtualUnits=100): > > (3 - 7000 + 1) == 4294960300 /* u32 wrap */ > 4294960300 < 100 == false /* sanity check passes */ > > Execution then continues with nb_boot_blocks=7000 as the loop > bound against a kmalloc_array(lastUnit + 1 = 4, 2) = 8-byte > PUtable, and the boot-block marking loop writes ~14 KB past > the object. Reproduced on v7.2-rc4 with a RAM-backed fake > DiskOnChip MTD device carrying that header: > > BUG: KASAN: slab-out-of-bounds in find_boot_record > Write of size 2 ... 0 bytes to the right of allocated 8-byte region > > That said, the reachability is admittedly narrow: this is a > mount-time path, so triggering requires root (device > registration) or physical control of the flash contents; > unprivileged users cannot reach it. The intent is only > hardening of the MediaHeader parser, in the same spirit as > the sanity checks already in find_boot_record(). > > One fair point about v1: the new check runs on every partition > entry during the scan, while only the selected BDTL entry's > fields are actually used for the allocations. If entries with > lastUnit < firstUnit can legitimately appear in other slots > on real media, I can respin to validate only the selected > partition (and drop the boot-record-unit check if preferred). > > Happy to send a v2 along those lines if you think the > hardening is worthwhile; otherwise I will drop it. Either > way, thanks for the time. Since this is a root-only exploit, I don't see the point in going further. These "hardening" steps just darken the code from my opinion. Thanks, Miquèl
diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c index 87e246a6f488..b0f01db95280 100644 --- a/drivers/mtd/inftlmount.c +++ b/drivers/mtd/inftlmount.c @@ -192,6 +192,23 @@ static int find_boot_record(struct INFTLrecord *inftl) ip->lastUnit, ip->flags, ip->spareUnits); + /* + * Reject inconsistent partition geometry before it is + * used: lastUnit < firstUnit would make the + * (lastUnit - firstUnit + 1) check below underflow, + * and lastUnit must stay within the device as it later + * bounds the PUtable/VUtable allocations. + */ + if (ip->lastUnit < ip->firstUnit || + ip->lastUnit >= inftl->nb_blocks) { + pr_warn("INFTL: Media Header " + "Partition %d sanity check failed:\n" + " firstUnit %d lastUnit %d " + "(nb_blocks %d)\n", + i, ip->firstUnit, ip->lastUnit, + inftl->nb_blocks); + return -1; + } if (ip->Reserved0 != ip->firstUnit) { struct erase_info *instr = &inftl->instr; @@ -233,6 +250,18 @@ static int find_boot_record(struct INFTLrecord *inftl) return -1; } + /* + * The boot record unit must lie within the described + * extent; it is later marked through PUtable[block]. + */ + if (block > ip->lastUnit) { + pr_warn("INFTL: Media Header " + "Partition %d sanity check failed:\n" + " boot record unit %d beyond " + "lastUnit %d\n", + i, block, ip->lastUnit); + return -1; + } inftl->nb_boot_blocks = ip->firstUnit; inftl->numvunits = ip->virtualUnits; if (inftl->numvunits > (inftl->nb_blocks -