mtd comments

mtd comments

[Alexander Larsson]

Hi,

I'm just browsing trough the mtd code.
Here are some comments:

The following code in mtd.c::mtd_read() is pretty bad from a security
standpoint:

if (!mtd->point || (ret = MTD_POINT(mtd, *ppos, count, &retlen, &mtdbuf)) != 0)
{
   /* mtd->point() failed; use mtd->read instead */
   ret = MTD_READ(mtd, *ppos, count, &retlen, buf);

The passing of buf, which is a userspace pointer to a kernel API is
dubious at the very least. And in the case of i.e. slram.c the 
physmem_read() function in fact just does an memcpy leading to a direct
security problem. 

FTL uses the read functions directly to though, so read must be able to
handle kernel pointers also.

Is the buffer pointer in the read call supposed to be a kernel or
userspace pointer? Who is responsible for verifying it?
Maybe there needs to be two API:s, one for userspace buffers and one for
kernelspace buffers. In this context the MTD_POINT api is actually very
nice.

Also, what is oob (out of band) blocks? And how are flash-memories that
have non-constant erase-sector size (i.e. boot sectors etc.) handled?

/ Alex




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[David Woodhouse]

(this found in my drafts folder - evidently it never got sent - sorry for the 
delay)

> Hi,
> 
> I'm just browsing trough the mtd code.

Thanks - it's high time this was done. 

> The following code in mtd.c::mtd_read() is pretty bad from a security
> standpoint:
> 
> if (!mtd->point || (ret = MTD_POINT(mtd, *ppos, count, &retlen, &mtdbuf)) != 0)
> {
>    /* mtd->point() failed; use mtd->read instead */
>    ret = MTD_READ(mtd, *ppos, count, &retlen, buf);
> 
> The passing of buf, which is a userspace pointer to a kernel API is
> dubious at the very least. And in the case of i.e. slram.c the 
> physmem_read() function in fact just does an memcpy leading to a direct
> security problem. 

I wasn't sure how to do this at the time I wrote this section of code (about
three years ago), but now I think we ought to declare that the MTD read() et
al. functions ought to take user-space pointers and use verify_area/
copy_{to,from}_user.

If they're actually called with a kernel-space buffer, we use 
set_fs(KERNEL_DS) beforehand, which handles the rest of your question.

> Also, what is oob (out of band) blocks? 

NAND flash chips have 16 bytes of extra storage per 512-byte block. I'm not 
particularly impressed with the 'out-of-band' routines, but I couldn't see a 
better way to do it. Suggestions are welcome.

Likewise, the DiskOnChip hardware also does ECC on the blocks it writes to the 
flash, and automatically checks the ECC on the blocks it reads back. I'm not 
wonderfully happy with the interface I've defined for that either, but the 
same applies - suggestions are welcome.

> And how are flash-memories that
> have non-constant erase-sector size (i.e. boot sectors etc.) handled?

I've never met a flash memory with non-constant erase-sector size, and hence 
didn't allow for them. Can you provide more information about such beasts?




--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[Alexander Larsson]

On Tue, 7 Mar 2000, David Woodhouse wrote:

> > Hi,
> > 
> > I'm just browsing trough the mtd code.
> 
> Thanks - it's high time this was done. 

The core (interfaces etc.) is very good. Some parts could probably
use some polish though.
 
> > The following code in mtd.c::mtd_read() is pretty bad from a security
> > standpoint:
> > 
> > if (!mtd->point || (ret = MTD_POINT(mtd, *ppos, count, &retlen, &mtdbuf)) != 0)
> > {
> >    /* mtd->point() failed; use mtd->read instead */
> >    ret = MTD_READ(mtd, *ppos, count, &retlen, buf);
> > 
> > The passing of buf, which is a userspace pointer to a kernel API is
> > dubious at the very least. And in the case of i.e. slram.c the 
> > physmem_read() function in fact just does an memcpy leading to a direct
> > security problem. 
> 
> I wasn't sure how to do this at the time I wrote this section of code (about
> three years ago), but now I think we ought to declare that the MTD read() et
> al. functions ought to take user-space pointers and use verify_area/
> copy_{to,from}_user.
> 
> If they're actually called with a kernel-space buffer, we use 
> set_fs(KERNEL_DS) beforehand, which handles the rest of your question.

Oh, i didn't know about that trick. Well that seems like a good idea
then. I suppose we have to fix all driver too. 
Kernel space buffers are used from the block driver request code, the ftl
(and nftl i suppose) request code and bam cache read/writes. Soo, are oob
pointers to kernel space or user space? (I would think kernel space, as
they are not exported via the char device).
 
> > Also, what is oob (out of band) blocks? 
> 
> NAND flash chips have 16 bytes of extra storage per 512-byte block. I'm not 
> particularly impressed with the 'out-of-band' routines, but I couldn't see a 
> better way to do it. Suggestions are welcome.
> 
> Likewise, the DiskOnChip hardware also does ECC on the blocks it writes to the 
> flash, and automatically checks the ECC on the blocks it reads back. I'm not 
> wonderfully happy with the interface I've defined for that either, but the 
> same applies - suggestions are welcome.

I have no other suggestions.
 
> > And how are flash-memories that
> > have non-constant erase-sector size (i.e. boot sectors etc.) handled?
> 
> I've never met a flash memory with non-constant erase-sector size, and hence 
> didn't allow for them. Can you provide more information about such beasts?

Some flash chips (I have no reference right now, i can find it if you
want) have different sector size of the first few sectors. The rest are a
fixed size. 

Here is an example of how a 64kb sector size flash  might look:

 4 k \
 4 k  |
 8 k   > This can be seen as the "first sector" of 64k
16 k  |
32 k /
64k
64k
.
.
.

This is mainly used as a bootsector type of thing. The idea seems to be
that if your boot-code is a lot less than 64k you don't waste so much
space. And you can also store configuration data in one of the smaller
sectors, and rewrite it without destroying the boot sector.

/ Alex




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[David Woodhouse]

alex@cendio.se said:
>  .
> This is mainly used as a bootsector type of thing. The idea seems to
> be that if your boot-code is a lot less than 64k you don't waste so
> much space. And you can also store configuration data in one of the
> smaller sectors, and rewrite it without destroying the boot sector. 

Options include...

1. Ignore the smaller erasesize - pretend it's 64Kb and force you to erase the 
whole lot at once.

2. Treat the parts with different erasesize as different logical devices as 
far as the MTD layer is concerned.

3. Change the MTD interface so it can handle variable sector sizes somehow.

As you seem to be implying that the parts with different sector sizes are used 
for a different purpose than the majority of the flash device, I'm inclined 
to favour #2 for now.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[Alexander Larsson]

On Wed, 8 Mar 2000, David Woodhouse wrote:

> 
> alex@cendio.se said:
> >  .
> > This is mainly used as a bootsector type of thing. The idea seems to
> > be that if your boot-code is a lot less than 64k you don't waste so
> > much space. And you can also store configuration data in one of the
> > smaller sectors, and rewrite it without destroying the boot sector. 
> 
> Options include...
> 
> 1. Ignore the smaller erasesize - pretend it's 64Kb and force you to erase the 
> whole lot at once.
> 
> 2. Treat the parts with different erasesize as different logical devices as 
> far as the MTD layer is concerned.
> 
> 3. Change the MTD interface so it can handle variable sector sizes somehow.
> 
> As you seem to be implying that the parts with different sector sizes are used 
> for a different purpose than the majority of the flash device, I'm inclined 
> to favour #2 for now.

Yes, i agree. Sometimes #1 might be a good idea too, it really depends on
how the flash is used. But we basically ignore this issue and let the user
make sure that their MTD devices look like constant size sectors.

/ Alex




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[David Woodhouse]

> > If they're actually called with a kernel-space buffer, we use 
> > set_fs(KERNEL_DS) beforehand, which handles the rest of your question.

alex@cendio.se said:
> Oh, i didn't know about that trick. Well that seems like a good idea
> then. I suppose we have to fix all driver too. 

I'm not convinced that this is such a good idea. While copy_to_user() is free
on decent CPUs, we expect people to be using 386en for embedded systems, and
copy_to_user() is unnecessary overhead. 

The only front end which copies directly to userspace is the chardevice, isn't 
it? Everything else copies to kernel-space. As the chardevice is mainly a 
hacking and config tool - it's generally only used to put the filesystem on 
the device in the first place - I think it's best to slow it down rather than 
slowing down everything else - i.e. to make it allocate buffers and copy to 
userspace itself.

--
dwmw2




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org

Re: mtd comments

[Alexander Larsson]

On Tue, 14 Mar 2000, David Woodhouse wrote:

> 
> > > If they're actually called with a kernel-space buffer, we use 
> > > set_fs(KERNEL_DS) beforehand, which handles the rest of your question.
> 
> alex@cendio.se said:
> > Oh, i didn't know about that trick. Well that seems like a good idea
> > then. I suppose we have to fix all driver too. 
> 
> I'm not convinced that this is such a good idea. While copy_to_user() is free
> on decent CPUs, we expect people to be using 386en for embedded systems, and
> copy_to_user() is unnecessary overhead. 
> 
> The only front end which copies directly to userspace is the chardevice, isn't 
> it? Everything else copies to kernel-space. As the chardevice is mainly a 
> hacking and config tool - it's generally only used to put the filesystem on 
> the device in the first place - I think it's best to slow it down rather than 
> slowing down everything else - i.e. to make it allocate buffers and copy to 
> userspace itself.

 Agreed.

/ Alex




To unsubscribe, send "unsubscribe mtd" to majordomo@infradead.org