| Message ID | 20140915174821.GB14832@ld-irv-0074 |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, 2014-09-15 at 10:48 -0700, Brian Norris wrote: > ioctl(MEMGETREGIONINFO) has one input parameter (regionindex) and three > output parameters (info about the erase region). There are two problems > in mtdinfo/libmtd here: > > 1. mtdinfo.c doesn't initialize its region_info_user struct, instead > passing uninitialized data to mtd_regioninfo() > > 2. mtd_regioninfo() fails to utilize the 'regidx' parameter to fill out > the regionindex parameter properly, so the garbage from mtdinfo.c is > propagated to the ioctl() Thanks, I never had flashes with multiple regions so this code path was probably never tested.
On Mon, Sep 15, 2014 at 10:48:21AM -0700, Brian Norris wrote: > ioctl(MEMGETREGIONINFO) has one input parameter (regionindex) and three > output parameters (info about the erase region). There are two problems > in mtdinfo/libmtd here: > > 1. mtdinfo.c doesn't initialize its region_info_user struct, instead > passing uninitialized data to mtd_regioninfo() > > 2. mtd_regioninfo() fails to utilize the 'regidx' parameter to fill out > the regionindex parameter properly, so the garbage from mtdinfo.c is > propagated to the ioctl() > > This means that mtdinfo will continuously probe the same (possibly > out-of-range) erase region, instead of looping over the valid regions. > > Let's fix this in the mtd_regioninfo() helper, and at the same time, > let's zero out the mtdinfo.c buffer, as an additional precaution to keep > from using uninitialized data. > > Initial error report from Yang, when running "mtdinfo /dev/mtd0" on a > Cavium 6100 board: > > root@CN61XX:~# mtdinfo /dev/mtd0 > mtd0 > Name: phys_mapped_flash > Type: nor > Eraseblock size: 65536 bytes, 64.0 KiB > Amount of eraseblocks: 128 (8388608 bytes, 8.0 MiB) > Minimum input/output unit size: 1 byte > Sub-page size: 1 byte > Additional erase regions: 0 > Character device major/minor: 90:0 > Bad blocks are allowed: false > Device is writable: true > libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 0 > error 22 (Invalid argument) > Eraseblock region 0: info is unavailable > libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 1 > error 22 (Invalid argument) > Eraseblock region 1: info is unavailable > > Reported-by: Yang Wei <Wei.Yang@windriver.com> > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Pushed to mtd-utils.git. Brian
diff --git a/lib/libmtd.c b/lib/libmtd.c index aff4c8be01b5..a6665f08018e 100644 --- a/lib/libmtd.c +++ b/lib/libmtd.c @@ -901,6 +901,8 @@ int mtd_regioninfo(int fd, int regidx, struct region_info_user *reginfo) return -1; } + reginfo->regionindex = regidx; + ret = ioctl(fd, MEMGETREGIONINFO, reginfo); if (ret < 0) return sys_errmsg("%s ioctl failed for erase region %d", diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c index 5ac95aaabb8a..a86abd1542b4 100644 --- a/ubi-utils/mtdinfo.c +++ b/ubi-utils/mtdinfo.c @@ -253,6 +253,8 @@ static void print_region_info(const struct mtd_dev_info *mtd) if (!args.node || (!args.map && mtd->region_cnt == 0)) return; + memset(®info, 0, sizeof(reginfo)); + /* First open the device so we can query it */ fd = open(args.node, O_RDONLY | O_CLOEXEC); if (fd == -1) {
ioctl(MEMGETREGIONINFO) has one input parameter (regionindex) and three output parameters (info about the erase region). There are two problems in mtdinfo/libmtd here: 1. mtdinfo.c doesn't initialize its region_info_user struct, instead passing uninitialized data to mtd_regioninfo() 2. mtd_regioninfo() fails to utilize the 'regidx' parameter to fill out the regionindex parameter properly, so the garbage from mtdinfo.c is propagated to the ioctl() This means that mtdinfo will continuously probe the same (possibly out-of-range) erase region, instead of looping over the valid regions. Let's fix this in the mtd_regioninfo() helper, and at the same time, let's zero out the mtdinfo.c buffer, as an additional precaution to keep from using uninitialized data. Initial error report from Yang, when running "mtdinfo /dev/mtd0" on a Cavium 6100 board: root@CN61XX:~# mtdinfo /dev/mtd0 mtd0 Name: phys_mapped_flash Type: nor Eraseblock size: 65536 bytes, 64.0 KiB Amount of eraseblocks: 128 (8388608 bytes, 8.0 MiB) Minimum input/output unit size: 1 byte Sub-page size: 1 byte Additional erase regions: 0 Character device major/minor: 90:0 Bad blocks are allowed: false Device is writable: true libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 0 error 22 (Invalid argument) Eraseblock region 0: info is unavailable libmtd: error!: MEMGETREGIONINFO ioctl failed for erase region 1 error 22 (Invalid argument) Eraseblock region 1: info is unavailable Reported-by: Yang Wei <Wei.Yang@windriver.com> Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- Untested so far. Can you give this a run, Yang? lib/libmtd.c | 2 ++ ubi-utils/mtdinfo.c | 2 ++ 2 files changed, 4 insertions(+)