* [PATCH v1] mtdinfo: Initialize reginfo variable before invoking mtd_regioninfo
@ 2014-08-06 3:00 Wei.Yang
2014-09-15 17:31 ` Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Wei.Yang @ 2014-08-06 3:00 UTC (permalink / raw)
To: computersforpeace, dedekind1; +Cc: linux-mtd, wei.yang, w90p710
From: Yang Wei <Wei.Yang@windriver.com>
If not initializing this variable, its value would be random. So
once the value of the regionindex field of region_info_user structure
is greater than mtd->numeraseregions. ioctl,which comes with MEMGETREGIONINFO,
would return -EINVAL
Signed-off-by: Yang Wei <Wei.Yang@windriver.com>
---
ubi-utils/mtdinfo.c | 2 ++
1 file changed, 2 insertions(+)
Hi Guys,
I got the following errors when running "mtdinfo /dev/mtd0" on 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
This patch is to fix the above errors. I have already verified it
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
Eraseblock region 0: offset: 0 size: 0x10000 numblocks: 0x7f
Eraseblock region 1: offset: 0 size: 0x10000 numblocks: 0x7f
root@CN61XX:~#
diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
index 5ac95aa..a70db00 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(region_info_t));
+
/* First open the device so we can query it */
fd = open(args.node, O_RDONLY | O_CLOEXEC);
if (fd == -1) {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1] mtdinfo: Initialize reginfo variable before invoking mtd_regioninfo
2014-08-06 3:00 [PATCH v1] mtdinfo: Initialize reginfo variable before invoking mtd_regioninfo Wei.Yang
@ 2014-09-15 17:31 ` Brian Norris
2014-09-15 17:48 ` [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo() Brian Norris
0 siblings, 1 reply; 5+ messages in thread
From: Brian Norris @ 2014-09-15 17:31 UTC (permalink / raw)
To: Wei.Yang; +Cc: linux-mtd, w90p710, dedekind1
Hi Yang,
On Wed, Aug 06, 2014 at 11:00:15AM +0800, Wei.Yang@windriver.com wrote:
> From: Yang Wei <Wei.Yang@windriver.com>
>
> If not initializing this variable, its value would be random. So
> once the value of the regionindex field of region_info_user structure
> is greater than mtd->numeraseregions. ioctl,which comes with MEMGETREGIONINFO,
> would return -EINVAL
>
> Signed-off-by: Yang Wei <Wei.Yang@windriver.com>
> ---
> ubi-utils/mtdinfo.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> Hi Guys,
>
> I got the following errors when running "mtdinfo /dev/mtd0" on 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
These error messages look deceptive. The problem is that while
mtd_regioninfo() takes a region index parameter, it only uses it for
printing the error message; its value is not actually propagated to the
ioctl().
>
> This patch is to fix the above errors. I have already verified it
>
> 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
> Eraseblock region 0: offset: 0 size: 0x10000 numblocks: 0x7f
> Eraseblock region 1: offset: 0 size: 0x10000 numblocks: 0x7f
These last two lines look wrong. Why would the device have two identical
erase regions? In fact, it looks like your patch just means that we'll
call ioctl(MEMGETREGIONINFO) repeatedly for region 0, instead of once
for each indexed region.
>
> root@CN61XX:~#
>
>
> diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c
> index 5ac95aa..a70db00 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(region_info_t));
> +
This might be good for security (to make sure that we never leak garbage
or use garbage stack data), but it doesn't actually fix anything.
> /* First open the device so we can query it */
> fd = open(args.node, O_RDONLY | O_CLOEXEC);
> if (fd == -1) {
I think you need a patch like this instead (untested):
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..cd61105c5c01 100644
--- a/ubi-utils/mtdinfo.c
+++ b/ubi-utils/mtdinfo.c
@@ -253,6 +253,9 @@ static void print_region_info(const struct mtd_dev_info *mtd)
if (!args.node || (!args.map && mtd->region_cnt == 0))
return;
+ /* Just in case */
+ 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) {
Brian
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo()
2014-09-15 17:31 ` Brian Norris
@ 2014-09-15 17:48 ` Brian Norris
2014-09-16 15:26 ` Artem Bityutskiy
2014-11-05 3:01 ` Brian Norris
0 siblings, 2 replies; 5+ messages in thread
From: Brian Norris @ 2014-09-15 17:48 UTC (permalink / raw)
To: Wei.Yang; +Cc: linux-mtd, w90p710, dedekind1
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(+)
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) {
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo()
2014-09-15 17:48 ` [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo() Brian Norris
@ 2014-09-16 15:26 ` Artem Bityutskiy
2014-11-05 3:01 ` Brian Norris
1 sibling, 0 replies; 5+ messages in thread
From: Artem Bityutskiy @ 2014-09-16 15:26 UTC (permalink / raw)
To: Brian Norris; +Cc: linux-mtd, Wei.Yang, w90p710
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.
--
Best Regards,
Artem Bityutskiy
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo()
2014-09-15 17:48 ` [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo() Brian Norris
2014-09-16 15:26 ` Artem Bityutskiy
@ 2014-11-05 3:01 ` Brian Norris
1 sibling, 0 replies; 5+ messages in thread
From: Brian Norris @ 2014-11-05 3:01 UTC (permalink / raw)
To: Wei.Yang; +Cc: linux-mtd, w90p710, dedekind1
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-05 3:02 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-06 3:00 [PATCH v1] mtdinfo: Initialize reginfo variable before invoking mtd_regioninfo Wei.Yang
2014-09-15 17:31 ` Brian Norris
2014-09-15 17:48 ` [PATCH mtd-utils] libmtd: don't ignore "region index" parameter in mtd_regioninfo() Brian Norris
2014-09-16 15:26 ` Artem Bityutskiy
2014-11-05 3:01 ` Brian Norris
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.