On 28 January 2016 at 14:59, Brian Norris <[email protected]> wrote: > Hi Ezequiel, > > Thanks for the review. > > On Thu, Jan 28, 2016 at 11:36:13AM -0300, Ezequiel Garcia wrote: >> On 28 January 2016 at 02:51, Brian Norris <[email protected]> >> wrote: >> > Locking the flash is most useful if it provides real hardware security. >> > Otherwise, it's little more than a software permission bit. >> > >> > A reasonable use case that provides real HW security might be like >> > follows: >> > >> > (1) hardware WP# is deasserted >> > (2) program flash >> > (3) flash range is protected via status register >> > (4) hardware WP# is asserted >> > (5) flash protection range can no longer be changed, until WP# is >> > deasserted >> > >> > In this way, flash protection is co-owned by hardware and software. >> > >> > Now, one would expect to be able to perform step (3) with >> > ioctl(MEMLOCK), except that the spi-nor driver does not set the Status >> > Register Protect bit (a.k.a. Status Register Write Disable (SRWD)), so >> > even though the range is now locked, it does not satisfy step (5) -- it >> > can still be changed by a call to ioctl(MEMUNLOCK). >> > >> > So, let's enable status register protection after the first lock >> > command. >> > >> > Signed-off-by: Brian Norris <[email protected]> >> > --- >> > drivers/mtd/spi-nor/spi-nor.c | 3 +++ >> > 1 file changed, 3 insertions(+) >> > >> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c >> > index 3a08aa53c171..46da6bb706fa 100644 >> > --- a/drivers/mtd/spi-nor/spi-nor.c >> > +++ b/drivers/mtd/spi-nor/spi-nor.c >> > @@ -518,6 +518,9 @@ static int stm_lock(struct spi_nor *nor, loff_t ofs, >> > uint64_t len) >> > >> > status_new = (status_old & ~mask) | val; >> > >> > + /* Disallow further writes if WP pin is asserted */ >> > + status_new |= SR_SRWD; >> > + >> >> No need to clear SR_SRWD in stm_unlock? > > Good point. > > I actually had thought about that earlier, but I didn't come up with a > great plan, and then I forgot about it when I was preparing this RFC. I > don't think we want *all* "unlock" operations to unprotect the status > register. What if we had the whole flash locked, and we're just calling > unlock on the top half, with the intention of leaving the bottom half > protected still? >
Right. > So, maybe we want to clear SR_SRWD only when we unlock the *entire* > flash? What do you think? > How about this: 1) ioctl(MEMLOCK) the entire flash (SR_SRWD is set) 2) ioctl(MEMUNLOCK) partially (SW_SRWD keeps set) 3) ioctl(MEMLOCK) the entire flash again Not sure this use case make sense, but would (3) be allowed given SW_SRWD is set? -- Ezequiel García, VanguardiaSur www.vanguardiasur.com.ar
