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

Ezequiel Garcia
2014-05-15T20:16:17Z

On 14 May 04:57 PM, Brian Norris wrote:
> On Wed, May 14, 2014 at 08:37:21PM -0300, Ezequiel Garcia wrote:
> > On 12 May 06:31 PM, Brian Norris wrote:
> > > On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> > [..]
> > > >
> > > > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > > > +{
> > > > + if (!mtd->_block_isreserved)
> > > > + return 0;
> > > > + if (ofs < 0 || ofs > mtd->size)
> > > > + return -EINVAL;
> > >
> > > At first, I was going to recommend that the out-of-bounds check go
> > > before the !mtd->_block_isreserved check, since it's best to warn users
> > > for invalid input. But then, mtd_block_isbad() has the same ordering, so
> > > it'd be nice to consistent...
> > >
> > > Do we flip a coin to decide whether to change both or leave as-is? :)
> > >
> >
> > Actually, I just followed the same convention as all the other functions,
> > not just mtd_block_isbad().
>
> 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.

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

>
> (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)
+ return -EINVAL;
+ }
return mtd->_writev(mtd, vecs, count, to, retlen);
}
EXPORT_SYMBOL_GPL(mtd_writev);

Right?
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering