[PATCH] Add error checking to slram

[PATCH] Add error checking to slram

[Josh Boyer]

Howdy,

The following patch fixes a couple issues I saw with the slram driver.

1) No error checking was being done at all.

2) There was no eraseblock size for slram, which made JFFS2 unhappy.

I think the phram driver might suffer from the eraseblock issue.  Also,
the current version of phram has another problem that this patch fixes
for slram.

Basically, the driver doesn't return shorted reads.  I had a problem
where -EINVAL was being returned on a read past the end of the device. 
Seemed fine at first glance, but it's not.

If JFFS2 is used on slram/phram, a dirent node (for example) can be
written to the very end of the device.  When get_inode_nodes reads off
the node it uses the union of the node types which caused the read to go
past the end of the device.  JFFS2 deals fine with shorted reads in that
case, but slram was returning -EINVAL which caused problems.  Once the
slram driver was fixed to do shorted reads, the problem went away.

Just thought I'd point that out if anyone dealing with phram is
listening.

Please take a look at the patch and commit if it looks good.  Comments
are always welcome too :).

thx,
josh

Signed-off-by: Josh Boyer <jdub@us.ibm.com>

--- mtd/drivers/mtd/devices/slram.c	2005-01-05 14:37:06.000000000 -0600
+++ mtd.jwb/drivers/mtd/devices/slram.c	2005-01-05 14:36:41.000000000 -0600
@@ -50,6 +50,7 @@
 #include <linux/mtd/mtd.h>
 
 #define SLRAM_MAX_DEVICES_PARAMS 6		/* 3 parameters / device */
+#define SLRAM_BLK_SZ 0x4000
 
 #define T(fmt, args...) printk(KERN_DEBUG fmt, ## args)
 #define E(fmt, args...) printk(KERN_NOTICE fmt, ## args)
@@ -108,6 +109,9 @@
 {
 	slram_priv_t *priv = mtd->priv;
 
+	if (from + len > mtd->size)
+		return -EINVAL;
+
 	*mtdbuf = priv->start + from;
 	*retlen = len;
 	return(0);
@@ -121,7 +125,13 @@
 		size_t *retlen, u_char *buf)
 {
 	slram_priv_t *priv = mtd->priv;
-	
+
+	if (from > mtd->size)
+		return -EINVAL;
+
+	if (from + len > mtd->size)
+		len = mtd->size - from;
+
 	memcpy(buf, priv->start + from, len);
 
 	*retlen = len;
@@ -133,6 +143,9 @@
 {
 	slram_priv_t *priv = mtd->priv;
 
+	if (to + len > mtd->size)
+		return -EINVAL;
+
 	memcpy(priv->start + to, buf, len);
 
 	*retlen = len;
@@ -188,7 +201,7 @@
 	(*curmtd)->mtdinfo->name = name;
 	(*curmtd)->mtdinfo->size = length;
 	(*curmtd)->mtdinfo->flags = MTD_CLEAR_BITS | MTD_SET_BITS |
-					MTD_WRITEB_WRITEABLE | MTD_VOLATILE;
+					MTD_WRITEB_WRITEABLE | MTD_VOLATILE | MTD_CAP_RAM;
         (*curmtd)->mtdinfo->erase = slram_erase;
 	(*curmtd)->mtdinfo->point = slram_point;
 	(*curmtd)->mtdinfo->unpoint = slram_unpoint;
@@ -196,7 +209,7 @@
 	(*curmtd)->mtdinfo->write = slram_write;
 	(*curmtd)->mtdinfo->owner = THIS_MODULE;
 	(*curmtd)->mtdinfo->type = MTD_RAM;
-	(*curmtd)->mtdinfo->erasesize = 0x0;
+	(*curmtd)->mtdinfo->erasesize = SLRAM_BLK_SZ;
 
 	if (add_mtd_device((*curmtd)->mtdinfo))	{
 		E("slram: Failed to register new device\n");
@@ -261,7 +274,7 @@
 	}
 	T("slram: devname=%s, devstart=0x%lx, devlength=0x%lx\n",
 			devname, devstart, devlength);
-	if ((devstart < 0) || (devlength < 0)) {
+	if ((devstart < 0) || (devlength < 0) || (devlength % SLRAM_BLK_SZ != 0)) {
 		E("slram: Illegal start / length parameter.\n");
 		return(-EINVAL);
 	}

Re: [PATCH] Add error checking to slram

[Jörn Engel]

On Wed, 5 January 2005 15:50:32 -0600, Josh Boyer wrote:
> 
> The following patch fixes a couple issues I saw with the slram driver.
> 
> 1) No error checking was being done at all.
> 
> 2) There was no eraseblock size for slram, which made JFFS2 unhappy.

Had a look at that one and imo it's jffs2 that should get fixed.  But
for the moment, your patch is fine.

> I think the phram driver might suffer from the eraseblock issue.  Also,
> the current version of phram has another problem that this patch fixes
> for slram.

Patch is in my tree already.  Will commit it soonish.  Thanks for the
report.

> Basically, the driver doesn't return shorted reads.  I had a problem
> where -EINVAL was being returned on a read past the end of the device. 
> Seemed fine at first glance, but it's not.
> 
> If JFFS2 is used on slram/phram, a dirent node (for example) can be
> written to the very end of the device.  When get_inode_nodes reads off
> the node it uses the union of the node types which caused the read to go
> past the end of the device.  JFFS2 deals fine with shorted reads in that
> case, but slram was returning -EINVAL which caused problems.  Once the
> slram driver was fixed to do shorted reads, the problem went away.

Imo, this might be a jffs2 problem as well.  In it's current state,
jffs2 is a bitch to work with, though, so your patch is the simple
solution.  More on jffs2 in a seperate mail.

> Just thought I'd point that out if anyone dealing with phram is
> listening.

;)

> Please take a look at the patch and commit if it looks good.  Comments
> are always welcome too :).

Just one, rest looks fine to me.

> thx,
> josh
> 
> Signed-off-by: Josh Boyer <jdub@us.ibm.com>
> 
> --- mtd/drivers/mtd/devices/slram.c	2005-01-05 14:37:06.000000000 -0600
> +++ mtd.jwb/drivers/mtd/devices/slram.c	2005-01-05 14:36:41.000000000 -0600
> @@ -50,6 +50,7 @@
>  #include <linux/mtd/mtd.h>
>  
>  #define SLRAM_MAX_DEVICES_PARAMS 6		/* 3 parameters / device */
> +#define SLRAM_BLK_SZ 0x4000
>  
>  #define T(fmt, args...) printk(KERN_DEBUG fmt, ## args)
>  #define E(fmt, args...) printk(KERN_NOTICE fmt, ## args)
> @@ -108,6 +109,9 @@
>  {
>  	slram_priv_t *priv = mtd->priv;
>  
> +	if (from + len > mtd->size)
> +		return -EINVAL;
> +
>  	*mtdbuf = priv->start + from;
>  	*retlen = len;
>  	return(0);
> @@ -121,7 +125,13 @@
>  		size_t *retlen, u_char *buf)
>  {
>  	slram_priv_t *priv = mtd->priv;
> -	
> +
> +	if (from > mtd->size)
> +		return -EINVAL;
> +
> +	if (from + len > mtd->size)
> +		len = mtd->size - from;
> +

My code is slightly different:
	if (len > mtd->size - from)
		...

And yes, this _can_ make a difference. ;)

>  	memcpy(buf, priv->start + from, len);
>  
>  	*retlen = len;
> @@ -133,6 +143,9 @@
>  {
>  	slram_priv_t *priv = mtd->priv;
>  
> +	if (to + len > mtd->size)
> +		return -EINVAL;
> +
>  	memcpy(priv->start + to, buf, len);
>  
>  	*retlen = len;
> @@ -188,7 +201,7 @@
>  	(*curmtd)->mtdinfo->name = name;
>  	(*curmtd)->mtdinfo->size = length;
>  	(*curmtd)->mtdinfo->flags = MTD_CLEAR_BITS | MTD_SET_BITS |
> -					MTD_WRITEB_WRITEABLE | MTD_VOLATILE;
> +					MTD_WRITEB_WRITEABLE | MTD_VOLATILE | MTD_CAP_RAM;
>          (*curmtd)->mtdinfo->erase = slram_erase;
>  	(*curmtd)->mtdinfo->point = slram_point;
>  	(*curmtd)->mtdinfo->unpoint = slram_unpoint;
> @@ -196,7 +209,7 @@
>  	(*curmtd)->mtdinfo->write = slram_write;
>  	(*curmtd)->mtdinfo->owner = THIS_MODULE;
>  	(*curmtd)->mtdinfo->type = MTD_RAM;
> -	(*curmtd)->mtdinfo->erasesize = 0x0;
> +	(*curmtd)->mtdinfo->erasesize = SLRAM_BLK_SZ;
>  
>  	if (add_mtd_device((*curmtd)->mtdinfo))	{
>  		E("slram: Failed to register new device\n");
> @@ -261,7 +274,7 @@
>  	}
>  	T("slram: devname=%s, devstart=0x%lx, devlength=0x%lx\n",
>  			devname, devstart, devlength);
> -	if ((devstart < 0) || (devlength < 0)) {
> +	if ((devstart < 0) || (devlength < 0) || (devlength % SLRAM_BLK_SZ != 0)) {
>  		E("slram: Illegal start / length parameter.\n");
>  		return(-EINVAL);
>  	}
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Jörn

-- 
To announce that there must be no criticism of the President, or that we
are to stand by the President, right or wrong, is not only unpatriotic
and servile, but is morally treasonable to the American public.
-- Theodore Roosevelt, Kansas City Star, 1918

Re: [PATCH] Add error checking to slram

[Josh Boyer]

On Sat, 2005-01-08 at 21:51 +0100, Jörn Engel wrote:
> On Wed, 5 January 2005 15:50:32 -0600, Josh Boyer wrote:
> > 
> > The following patch fixes a couple issues I saw with the slram driver.
> > 
> > 1) No error checking was being done at all.
> > 
> > 2) There was no eraseblock size for slram, which made JFFS2 unhappy.
> 
> Had a look at that one and imo it's jffs2 that should get fixed.  But
> for the moment, your patch is fine.

No argument from me there.  I've been sitting on this patch for a long
time and it was simply the easiest fix.  Until JFFS2 gets fixed, this
should suffice.

> > Please take a look at the patch and commit if it looks good.  Comments
> > are always welcome too :).
> 
> Just one, rest looks fine to me.

<snip>

> > +	if (from > mtd->size)
> > +		return -EINVAL;
> > +
> > +	if (from + len > mtd->size)
> > +		len = mtd->size - from;
> > +
> 
> My code is slightly different:
> 	if (len > mtd->size - from)
> 		...
> 
> And yes, this _can_ make a difference. ;)

It's early and I haven't had my coffee yet :).  Care to educate me on
why that makes a difference?

josh

Re: [PATCH] Add error checking to slram

[Jörn Engel]

On Mon, 10 January 2005 08:41:15 -0600, Josh Boyer wrote:
> On Sat, 2005-01-08 at 21:51 +0100, Jörn Engel wrote:
> > On Wed, 5 January 2005 15:50:32 -0600, Josh Boyer wrote:
> 
> > > +	if (from > mtd->size)
> > > +		return -EINVAL;
> > > +
> > > +	if (from + len > mtd->size)
> > > +		len = mtd->size - from;
> > > +
> > 
> > My code is slightly different:
> > 	if (len > mtd->size - from)
> > 		...
> > 
> > And yes, this _can_ make a difference. ;)
> 
> It's early and I haven't had my coffee yet :).  Care to educate me on
> why that makes a difference?

from = (uint32_t) -50;
len = 100;

Currently, I wouldn't dare creating a 4GiB mtd. ;)

Jörn

-- 
And spam is a useful source of entropy for /dev/random too!
-- Jasmine Strong