Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

Brian Norris
2014-05-16T05:48:04Z

On Thu, May 15, 2014 at 05:15:18PM -0300, Ezequiel Garcia wrote:
> On 14 May 04:57 PM, Brian Norris wrote:
> > All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
> > -EOPNOTSUPP, which is an informative error code. But that's different
> > than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
> > if the block doesn't exist.
>
> Ah... I misunderstood your comment. The point is that you don't want to
> return 0, when the parameters may be are wrong.

Right. I think it is wrong to return '0' for the out-of-bounds case.

> So, I'll fix this patch by changing both, mtd_block_isbad and
> mtd_block_isreserved, just as you asked in the first place.

Sounds good.

> > (Also, is it just me, or is mtd_writev() missing bounds checking?)
>
> Hm... so it seems. I guess the check should be something like:
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index d201fee..21ce0f4 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1066,11 +1066,22 @@ static int default_mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
> int mtd_writev(struct mtd_info *mtd, const struct kvec *vecs,
> unsigned long count, loff_t to, size_t *retlen)
> {
> + int i;
> +
> *retlen = 0;
> if (!(mtd->flags & MTD_WRITEABLE))
> return -EROFS;
> if (!mtd->_writev)
> return default_mtd_writev(mtd, vecs, count, to, retlen);
> +
> + if (to < 0 || to > mtd->size)
> + return -EINVAL;
> + for (i = 0; i < count; i++) {
> + if (!vecs[i].iov_len)
> + continue;
> + if (vecs[i].iov_len > mtd->size - to)

This isn't quite the right check. It's OK for the first iteration, but
it's not a good check for vecs[i].iov_len where i > 0. Maybe just check
the sum of all the lengths against `mtd->size - to'.

> + return -EINVAL;
> + }
> return mtd->_writev(mtd, vecs, count, to, retlen);
> }
> EXPORT_SYMBOL_GPL(mtd_writev);
>
> Right?

Brian