| Message ID | 20260824-mtd_break_looper-v1-1-e46d7f02faab@kroah.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: parsers: afs: add size check to v2 partition | expand |
Hello Griffin, Thanks for the patch! On 24/08/2026 at 15:50:27 +02, Griffin Kroah-Hartman <griffin@kroah.com> wrote: > Add a size check to the loop in afs_parse_v2_partition(), avoiding > walking out of the imginfo[] array bounds if a malicious packet fakes a "packet" refers to network wording, whereas here, in the MTD world, we would rather talk about a malicious image. > large region count. > > Assisted-by: gkh_clanker_t1000 > Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com> I guess such a fix would require a Fixes tag, as well as Cc'ing the stable@vger.kernel.org alias. > --- > drivers/mtd/parsers/afs.c | 12 +++++++++--- > 1 file changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/parsers/afs.c b/drivers/mtd/parsers/afs.c > index 26116694c821..2c6f8768312c 100644 > --- a/drivers/mtd/parsers/afs.c > +++ b/drivers/mtd/parsers/afs.c > @@ -287,12 +287,18 @@ static int afs_parse_v2_partition(struct mtd_info *mtd, > block_start, block_end); > > for (i = 0; i < region_count; i++) { > - u32 region_load_addr = imginfo[pad + 3 + i*4]; > - u32 region_size = imginfo[pad + 4 + i*4]; > - u32 region_offset = imginfo[pad + 5 + i*4]; > + u32 region_load_addr; > + u32 region_size; > + u32 region_offset; > u32 region_start; > u32 region_end; > > + if (pad + 5 + i*4 >= ARRAY_SIZE(imginfo)) > + break; > + region_load_addr = imginfo[pad + 3 + i*4]; > + region_size = imginfo[pad + 4 + i*4]; > + region_offset = imginfo[pad + 5 + i*4]; > + Looking at the code, I see that just above the loop there is an actual check bailing out early in case the region count is overly big. In practice: - pad can only be 0, 1 or 2 - imginfo array size is 36 The check is: if (region_count > (ARRAY_SIZE(imginfo) - pad - 3) / 4) return -EINVAL; And the loop goes at most through: pad + 5 + (region_count - 1) * 4 So the only possibilities are: pad | max(region_count) | max(index) ----+-------------------+----------- 0 | 8 | 33 1 | 8 | 34 2 | 7 | 31 It seems like none of those situations could actually lead to a reachable/exploitable bug. So while I understand the wish for a more defensive hardening, the check you add seems redundant. I would therefore suggest to just keep the driver as-is. Thanks, Miquèl
On 8/25/26 3:42 PM, Miquel Raynal wrote: > Hello Griffin, > > Thanks for the patch! > > On 24/08/2026 at 15:50:27 +02, Griffin Kroah-Hartman <griffin@kroah.com> wrote: > >> Add a size check to the loop in afs_parse_v2_partition(), avoiding >> walking out of the imginfo[] array bounds if a malicious packet fakes a > "packet" refers to network wording, whereas here, in the MTD world, we > would rather talk about a malicious image. Good to know! thanks for clarifying. >> --- >> drivers/mtd/parsers/afs.c | 12 +++++++++--- >> 1 file changed, 9 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/parsers/afs.c b/drivers/mtd/parsers/afs.c >> index 26116694c821..2c6f8768312c 100644 >> --- a/drivers/mtd/parsers/afs.c >> +++ b/drivers/mtd/parsers/afs.c >> @@ -287,12 +287,18 @@ static int afs_parse_v2_partition(struct mtd_info *mtd, >> block_start, block_end); >> >> for (i = 0; i < region_count; i++) { >> - u32 region_load_addr = imginfo[pad + 3 + i*4]; >> - u32 region_size = imginfo[pad + 4 + i*4]; >> - u32 region_offset = imginfo[pad + 5 + i*4]; >> + u32 region_load_addr; >> + u32 region_size; >> + u32 region_offset; >> u32 region_start; >> u32 region_end; >> >> + if (pad + 5 + i*4 >= ARRAY_SIZE(imginfo)) >> + break; >> + region_load_addr = imginfo[pad + 3 + i*4]; >> + region_size = imginfo[pad + 4 + i*4]; >> + region_offset = imginfo[pad + 5 + i*4]; >> + > Looking at the code, I see that just above the loop there is an actual > check bailing out early in case the region count is overly big. In > practice: > - pad can only be 0, 1 or 2 > - imginfo array size is 36 > > The check is: > > if (region_count > (ARRAY_SIZE(imginfo) - pad - 3) / 4) > return -EINVAL; Sorry, I was not using linux-next, so I did not see that this was already checked. Thank you for the review! Griffin
diff --git a/drivers/mtd/parsers/afs.c b/drivers/mtd/parsers/afs.c index 26116694c821..2c6f8768312c 100644 --- a/drivers/mtd/parsers/afs.c +++ b/drivers/mtd/parsers/afs.c @@ -287,12 +287,18 @@ static int afs_parse_v2_partition(struct mtd_info *mtd, block_start, block_end); for (i = 0; i < region_count; i++) { - u32 region_load_addr = imginfo[pad + 3 + i*4]; - u32 region_size = imginfo[pad + 4 + i*4]; - u32 region_offset = imginfo[pad + 5 + i*4]; + u32 region_load_addr; + u32 region_size; + u32 region_offset; u32 region_start; u32 region_end; + if (pad + 5 + i*4 >= ARRAY_SIZE(imginfo)) + break; + region_load_addr = imginfo[pad + 3 + i*4]; + region_size = imginfo[pad + 4 + i*4]; + region_offset = imginfo[pad + 5 + i*4]; + pr_debug(" region %d: address: %08x, size: %08x, " "offset: %08x\n", i,
Add a size check to the loop in afs_parse_v2_partition(), avoiding walking out of the imginfo[] array bounds if a malicious packet fakes a large region count. Assisted-by: gkh_clanker_t1000 Signed-off-by: Griffin Kroah-Hartman <griffin@kroah.com> --- drivers/mtd/parsers/afs.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) --- base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f change-id: 20260824-mtd_break_looper-429d7cf5a7c0 Best regards, -- Griffin Kroah-Hartman <griffin@kroah.com>