| Message ID | 1453960307-10181-5-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 28 January 2016 at 02:51, Brian Norris <computersforpeace@gmail.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@gmail.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?
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@gmail.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@gmail.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? So, maybe we want to clear SR_SRWD only when we unlock the *entire* flash? What do you think? Brian
On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.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@gmail.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@gmail.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?
On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote: > On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.com> wrote: > > So, maybe we want to clear SR_SRWD only when we unlock the *entire* > > flash? What do you think? I'll paste in the relevant datasheet details from w25q32fw, to make sure we're on the same page here, noting that 'SRP0' is our 'SR_SRWD', and we're not touching SRP1 (i.e., SRP1=0): "SRP1=0, SRP0=0, /WP=X: Software Protection /WP pin has no control. The Status register can be written to after a Write Enable instruction, WEL=1. [Factory Default] SRP1=0, SRP0=1, /WP=0: Hardware Protected When /WP pin is low the Status Register locked [sic] and cannot be written to. SRP1=0, SRP0=1, /WP=1: Hardware Unprotected When /WP pin is high the Status register is unlocked and can be written to after a Write Enable instruction, WEL=1." > 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 I might be confused; are you making a suggestion of a new behavior, or are you just trying to clarify my proposal? Because this sounds like it matches my proposal. > Not sure this use case make sense, I suppose it could make sense, if you (e.g.) have some intermediate steps toward determining the locked regions during factory programming. Maybe a process would start by doing #1 and #2, then decide conditionally whether to do #3. And only after the whole process is done does something assert /WP=0 (in my case, a factory process would tie /WP low). > but would (3) be allowed given > SW_SRWD is set? Yes, if /WP=1 (high). Brian
On 28 January 2016 at 16:48, Brian Norris <computersforpeace@gmail.com> wrote: > On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote: >> On 28 January 2016 at 14:59, Brian Norris <computersforpeace@gmail.com> wrote: >> > So, maybe we want to clear SR_SRWD only when we unlock the *entire* >> > flash? What do you think? > > I'll paste in the relevant datasheet details from w25q32fw, to make sure > we're on the same page here, noting that 'SRP0' is our 'SR_SRWD', and > we're not touching SRP1 (i.e., SRP1=0): > > "SRP1=0, SRP0=0, /WP=X: Software Protection > /WP pin has no control. The Status register can be written to after > a Write Enable instruction, WEL=1. [Factory Default] > SRP1=0, SRP0=1, /WP=0: Hardware Protected > When /WP pin is low the Status Register locked [sic] and cannot be > written to. > SRP1=0, SRP0=1, /WP=1: Hardware Unprotected > When /WP pin is high the Status register is unlocked and can be > written to after a Write Enable instruction, WEL=1." > Yes, we are on the same page. >> 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 > > I might be confused; are you making a suggestion of a new behavior, or > are you just trying to clarify my proposal? Because this sounds like it > matches my proposal. > I was trying to clarify how the SRWD would work in that case, but I forgot about /WP when I asked that! >> Not sure this use case make sense, > > I suppose it could make sense, if you (e.g.) have some intermediate > steps toward determining the locked regions during factory programming. > Maybe a process would start by doing #1 and #2, then decide > conditionally whether to do #3. And only after the whole process is done > does something assert /WP=0 (in my case, a factory process would tie /WP > low). > >> but would (3) be allowed given >> SW_SRWD is set? > > Yes, if /WP=1 (high). > Right. So, after giving some more thought do this, I'd say it might make sense to clear SRWD only when unlocking the entire flash. If anything else, it would allow a path to disable hardware protection on the lock range?
On Fri, Jan 29, 2016 at 10:22:34AM -0300, Ezequiel Garcia wrote: > On 28 January 2016 at 16:48, Brian Norris <computersforpeace@gmail.com> wrote: > > On Thu, Jan 28, 2016 at 04:24:50PM -0300, Ezequiel Garcia wrote: > >> 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 ... > >> but would (3) be allowed given > >> SW_SRWD is set? > > > > Yes, if /WP=1 (high). > > > > Right. So, after giving some more thought do this, I'd say it might > make sense to clear SRWD only when unlocking the entire flash. If > anything else, it would allow a path to disable hardware protection on > the lock range? Yes, that sounds fine to me, as it does allow removal of the HW protection. So one could, for example, do: 0. bring /WP=1 (high) 1. unlock the whole flash 2. bring /WP=0 (low) -- flash is still unlocked 3. allow a one-time relocking of the flash via MEMLOCK 4. no more locking changes I'll send out v2. Brian
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; + /* Don't bother if they're the same */ if (status_new == status_old) return 0;
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@gmail.com> --- drivers/mtd/spi-nor/spi-nor.c | 3 +++ 1 file changed, 3 insertions(+)