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

[Ezequiel Garcia <ezequiel.garcia@free-electrons.com>]

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
http://free-electrons.com