Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

[Brian Norris]

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 <computersforpeace@xxxxxxxxx> 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 <computersforpeace@xxxxxxxxx>
> > ---
> >  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?

So, maybe we want to clear SR_SRWD only when we unlock the *entire*
flash? What do you think?

Brian