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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAEAJfDHRutp1R1nKxih-4CENBsjhmFaaTnqD7S6Vub+pqE05Q@mail.gmail.com>
Date:	Thu, 28 Jan 2016 16:24:50 -0300
From:	Ezequiel Garcia <ezequiel@...guardiasur.com.ar>
To:	Brian Norris <computersforpeace@...il.com>
Cc:	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	Rafał Miłecki <zajec5@...il.com>,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Bayi Cheng <bayi.cheng@...iatek.com>,
	Marek Vasut <marex@...x.de>,
	Daniel Kurtz <djkurtz@...omium.org>
Subject: Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low

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