Discussion:
[PATCH 1/2] mtd: fix mtd_oobavail() incoherent returned value
Miquel Raynal
2018-11-18 20:18:30 UTC
Permalink
mtd_oobavail() returns either mtd->oovabail or mtd->oobsize. Both
values are unsigned 32-bit entities, so there is no reason to pretend
returning a signed one.

Signed-off-by: Miquel Raynal <***@bootlin.com>
---
include/linux/mtd/mtd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index cd0be91bdefa..035d641e8847 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -386,7 +386,7 @@ static inline struct device_node *mtd_get_of_node(struct mtd_info *mtd)
return dev_of_node(&mtd->dev);
}

-static inline int mtd_oobavail(struct mtd_info *mtd, struct mtd_oob_ops *ops)
+static inline u32 mtd_oobavail(struct mtd_info *mtd, struct mtd_oob_ops *ops)
{
return ops->mode == MTD_OPS_AUTO_OOB ? mtd->oobavail : mtd->oobsize;
}
--
2.17.1
Miquel Raynal
2018-11-18 20:18:31 UTC
Permalink
A Coverity robot reported an integer handling issue
(OVERFLOW_BEFORE_WIDEN) in the potentially overflowing expression:

(mtd_div_by_ws(mtd->size, mtd) - mtd_div_by_ws(offs, mtd)) *
mtd_oobavail(mtd, ops)

While such overflow will certainly never happen due to the numbers
handled, it is cleaner to fix this operation anyway.

The problem is that all the maths include 32-bit quantities, while the
result is stored in an explicit 64-bit value.

As maxooblen will just be compared with a size_t, let's change the
type of the variable to a size_t. This will not fix anything but will
clarify a bit the situation. Then, do an explicit cast to fix Coverity
warning.

Signed-off-by: Miquel Raynal <***@bootlin.com>
---
drivers/mtd/mtdcore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 97ac219c082e..afb4b17fb670 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -1136,13 +1136,13 @@ static int mtd_check_oob_ops(struct mtd_info *mtd, loff_t offs,
return -EINVAL;

if (ops->ooblen) {
- u64 maxooblen;
+ size_t maxooblen;

if (ops->ooboffs >= mtd_oobavail(mtd, ops))
return -EINVAL;

- maxooblen = ((mtd_div_by_ws(mtd->size, mtd) -
- mtd_div_by_ws(offs, mtd)) *
+ maxooblen = ((size_t)(mtd_div_by_ws(mtd->size, mtd) -
+ mtd_div_by_ws(offs, mtd)) *
mtd_oobavail(mtd, ops)) - ops->ooboffs;
if (ops->ooblen > maxooblen)
return -EINVAL;
--
2.17.1
Boris Brezillon
2018-12-03 07:52:06 UTC
Permalink
Post by Miquel Raynal
A Coverity robot reported an integer handling issue
(mtd_div_by_ws(mtd->size, mtd) - mtd_div_by_ws(offs, mtd)) *
mtd_oobavail(mtd, ops)
While such overflow will certainly never happen due to the numbers
handled, it is cleaner to fix this operation anyway.
The problem is that all the maths include 32-bit quantities, while the
result is stored in an explicit 64-bit value.
As maxooblen will just be compared with a size_t, let's change the
type of the variable to a size_t. This will not fix anything but will
clarify a bit the situation. Then, do an explicit cast to fix Coverity
warning.
Applied to http://git.infradead.org/linux-mtd.git mtd/next, thanks.

Boris
Boris Brezillon
2018-12-03 07:52:10 UTC
Permalink
Post by Miquel Raynal
mtd_oobavail() returns either mtd->oovabail or mtd->oobsize. Both
values are unsigned 32-bit entities, so there is no reason to pretend
returning a signed one.
Applied to http://git.infradead.org/linux-mtd.git mtd/next, thanks.

Boris

Loading...