| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20140305083153.GR13420@norris-Latitude-E6410>
Date: Wed, 5 Mar 2014 00:31:53 -0800
From: Brian Norris <computersforpeace@...il.com>
To: Gerlando Falauto <gerlando.falauto@...mile.com>
Cc: Austin Boyle <boyle.austin@...il.com>,
linux-mtd@...ts.infradead.org,
David Woodhouse <dwmw2@...radead.org>,
linux-kernel@...r.kernel.org,
Artem Bityutskiy <artem.bityutskiy@...ux.intel.com>,
Marek Vasut <marex@...x.de>, Angus Clark <angus.clark@...com>
Subject: Re: [PATCH] mtd: m25p80: Flash protection support for STmicro chips
Different email for Austin?
On Wed, Mar 05, 2014 at 12:10:08AM -0800, Brian Norris wrote:
> + Marek, Angus
>
> On Thu, Feb 27, 2014 at 03:34:06PM +0100, Gerlando Falauto wrote:
> > Hi,
> >
> > it's me again.
> > In my opinion (and experience) this introduces a pretty serious bug
> > (not to mention the compatibility issues), yet I haven't heard a
> > single word or found a patch applied about it in three months.
> > Am I the only one having this issue? Or maybe I'm just "seeing things"?
>
> I agree that this doesn't look quite like the best implementation. As
> you note, this feature is not available on *ALL* ST SPI flash. I think
> it should require yet another device flag in m25p_ids[]...
>
> But I don't see why this is a concern for "certain bootloaders". If your
> bootloader doesn't support locked blocks, then don't run ioctl(MEMLOCK)
> on the device.
>
> Leaving the following context intact for now, since it's old. But please
> trim your replies and bottom-post in the future. Thanks!
>
> Regards,
> Brian
>
> > Thank you,
> > Gerlando
> >
> > P.S. FWIW, the original author of the patch seem to have disappeared.
> >
> > On 11/20/2013 09:04 PM, Gerlando Falauto wrote:
> > >Hi,
> > >
> > >On 01/04/2013 01:02 AM, Austin Boyle wrote:
> > >>This patch adds generic support for flash protection on STmicro chips.
> > >>On chips with less than 3 protection bits, the unused bits are don't
> > >>cares
> > >>and so can be written anyway.
> > >
> > >I have two remarks:
> > >
> > >1) I believe this introduces incompatibilities with existing bootloaders
> > >which do not support this feature.
> > >Namely, u-boot is not able (to the best of my knowledge) to treat these
> > >bits properly. So as soon as you write something to your SPI nor flash
> > >from within linux, u-boot is not able to erase/rewrite those blocks
> > >anymore.
> > >
> > >Wouldn't it make more sense to selectively enable this feature, only if
> > >explicity configured to do so (e.g. through its device tree node)?
> > >Like what was used for the Spansion's PPB, see:
> > >
> > >http://lists.infradead.org/pipermail/linux-mtd/2013-January/045536.html
> > >
> > > > The lock function will only change the
> > >>protection bits if it would not unlock other areas. Similarly, the unlock
> > >>function will not lock currently unlocked areas. Tested on the m25p64.
> > > >
> > >>From: Austin Boyle <Austin.Boyle@...atnet.com>
> > >>Signed-off-by: Austin Boyle <Austin.Boyle@...atnet.com>
> > >>---
> > >>diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > >>index 4eeeb2d..069e34f 100644
> > >>--- a/drivers/mtd/devices/m25p80.c
> > >>+++ b/drivers/mtd/devices/m25p80.c
> > >>@@ -565,6 +565,96 @@ time_out:
> > >> return ret;
> > >> }
> > >>
> > >>+static int m25p80_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > >>+{
> > >>+ struct m25p *flash = mtd_to_m25p(mtd);
> > >>+ uint32_t offset = ofs;
> > >>+ uint8_t status_old, status_new;
> > >>+ int res = 0;
> > >>+
> > >>+ mutex_lock(&flash->lock);
> > >>+ /* Wait until finished previous command */
> > >>+ if (wait_till_ready(flash)) {
> > >>+ res = 1;
> > >>+ goto err;
> > >>+ }
> > >>+
> > >>+ status_old = read_sr(flash);
> > >>+
> > >>+ if (offset < flash->mtd.size-(flash->mtd.size/2))
> > >>+ status_new = status_old | SR_BP2 | SR_BP1 | SR_BP0;
> > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/4))
> > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/8))
> > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/16))
> > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/32))
> > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> > >>+ else if (offset < flash->mtd.size-(flash->mtd.size/64))
> > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> > >>+ else
> > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> > >
> > >2) While I believe this might work on m25p32, m25p64 and m25p128 (i.e.
> > >flashes with 64 blocks or more), it looks incorrect for smaller chips
> > >(namely our m25p80, with just 16 blocks). There, the 1/64 logic scales
> > >down to 1/16, e.g.
> > >- 000 means protect nothing
> > >- 001 means protect 1/16th (=1 blocks) [m25p64 => 1/64th]
> > >- 010 means protect 1/8th (=2 blocks) [m25p64 => 1/32th]
> > >- ...
> > >- 100 means protect 1/2nd (=8 blocks)
> > >- 101,110, 111 mean protect everything
> > >
> > >and I assume the same goes for chips with fewer sectors.
> > >
> > >Any comments?
> > >
> > >Thanks,
> > >Gerlando
> > >
> > >>+
> > >>+ /* Only modify protection if it will not unlock other areas */
> > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) >
> > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> > >>+ write_enable(flash);
> > >>+ if (write_sr(flash, status_new) < 0) {
> > >>+ res = 1;
> > >>+ goto err;
> > >>+ }
> > >>+ }
> > >>+
> > >>+err: mutex_unlock(&flash->lock);
> > >>+ return res;
> > >>+}
> > >>+
> > >>+static int m25p80_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> > >>+{
> > >>+ struct m25p *flash = mtd_to_m25p(mtd);
> > >>+ uint32_t offset = ofs;
> > >>+ uint8_t status_old, status_new;
> > >>+ int res = 0;
> > >>+
> > >>+ mutex_lock(&flash->lock);
> > >>+ /* Wait until finished previous command */
> > >>+ if (wait_till_ready(flash)) {
> > >>+ res = 1;
> > >>+ goto err;
> > >>+ }
> > >>+
> > >>+ status_old = read_sr(flash);
> > >>+
> > >>+ if (offset+len > flash->mtd.size-(flash->mtd.size/64))
> > >>+ status_new = status_old & ~(SR_BP2|SR_BP1|SR_BP0);
> > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/32))
> > >>+ status_new = (status_old & ~(SR_BP2|SR_BP1)) | SR_BP0;
> > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/16))
> > >>+ status_new = (status_old & ~(SR_BP2|SR_BP0)) | SR_BP1;
> > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/8))
> > >>+ status_new = (status_old & ~SR_BP2) | SR_BP1 | SR_BP0;
> > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/4))
> > >>+ status_new = (status_old & ~(SR_BP0|SR_BP1)) | SR_BP2;
> > >>+ else if (offset+len > flash->mtd.size-(flash->mtd.size/2))
> > >>+ status_new = (status_old & ~SR_BP1) | SR_BP2 | SR_BP0;
> > >>+ else
> > >>+ status_new = (status_old & ~SR_BP0) | SR_BP2 | SR_BP1;
> > >>+
> > >>+ /* Only modify protection if it will not lock other areas */
> > >>+ if ((status_new&(SR_BP2|SR_BP1|SR_BP0)) <
> > >>+ (status_old&(SR_BP2|SR_BP1|SR_BP0))) {
> > >>+ write_enable(flash);
> > >>+ if (write_sr(flash, status_new) < 0) {
> > >>+ res = 1;
> > >>+ goto err;
> > >>+ }
> > >>+ }
> > >>+
> > >>+err: mutex_unlock(&flash->lock);
> > >>+ return res;
> > >>+}
> > >>+
> > >>
> > >>/****************************************************************************/
> > >>
> > >>
> > >> /*
> > >>@@ -899,6 +989,12 @@ static int m25p_probe(struct spi_device *spi)
> > >> flash->mtd._erase = m25p80_erase;
> > >> flash->mtd._read = m25p80_read;
> > >>
> > >>+ /* flash protection support for STmicro chips */
> > >>+ if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
> > >>+ flash->mtd._lock = m25p80_lock;
> > >>+ flash->mtd._unlock = m25p80_unlock;
> > >>+ }
> > >>+
> > >> /* sst flash chips use AAI word program */
> > >> if (JEDEC_MFR(info->jedec_id) == CFI_MFR_SST)
> > >> flash->mtd._write = sst_write;
> > >>
> > >>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists