From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935363AbcA1R7L (ORCPT ); Thu, 28 Jan 2016 12:59:11 -0500 Received: from mail-pa0-f68.google.com ([209.85.220.68]:33312 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934118AbcA1R7J (ORCPT ); Thu, 28 Jan 2016 12:59:09 -0500 Date: Thu, 28 Jan 2016 09:59:05 -0800 From: Brian Norris To: Ezequiel Garcia Cc: "linux-mtd@lists.infradead.org" , =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , Boris Brezillon , "linux-kernel@vger.kernel.org" , Bayi Cheng , Marek Vasut , Daniel Kurtz Subject: Re: [PATCH 4/8] mtd: spi-nor: disallow further writes to SR if WP# is low Message-ID: <20160128175905.GA33340@google.com> References: <1453960307-10181-1-git-send-email-computersforpeace@gmail.com> <1453960307-10181-5-git-send-email-computersforpeace@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 > > --- > > 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