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

[Ezequiel Garcia]

On 28 January 2016 at 14:59, Brian Norris <computersforpeace@xxxxxxxxx> 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 <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?
>

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