Re: [flashrom] [PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Reinauer]

On 5/23/11 4:23 PM, Stefan Tauner wrote:
> Signed-off-by: Stefan Tauner
>
> chipset_enable.c |   12 +++++++++++-
>   1 files changed, 11 insertions(+), 1 deletions(-)
>
> diff --git a/chipset_enable.c b/chipset_enable.c
> index 83b49ad..339c6bb 100644
> --- a/chipset_enable.c
> +++ b/chipset_enable.c
> @@ -264,8 +264,18 @@ static int enable_flash_ich(struct pci_dev *dev, const char *name,
>    	     (old&  (1<<  0)) ? "en" : "dis");
>    msg_pdbg("BIOS_CNTL is 0x%x\n", old);
> new = old | 1;
> /*
> * Quote from the 6 Series datasheet:
> * "5: SMM BIOS Write Protect Disable (SMM_BWP)
> * 1 = BIOS region SMM protection is enabled.
> * The BIOS Region is not writable unless all processors are in SMM."
> * In earlier chipsets this bit is reserved. */
> if (old&  (5<<  1)) {
> msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> return -1;

You might still be successful doing the write, in case the SMM handler 
does not enforce the protection, so maybe you should just print a 
warning but not return here?
> }
>
> new = old | 1;
> if (new == old)
> return 0;

[PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Tauner]

On Tue, 24 May 2011 18:15:52 -0700
Stefan Reinauer  wrote:
> > if (old&  (5<<  1)) {
> > msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> > return -1;  
>
> You might still be successful doing the write, in case the SMM handler 
> does not enforce the protection, so maybe you should just print a 
> warning but not return here?

ok

also:
1<<5 of course!
thanks joshua for reporting this

[PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Tauner]

On Tue, 24 May 2011 18:15:52 -0700
Stefan Reinauer  wrote:
> On 5/23/11 4:23 PM, Stefan Tauner wrote:
> > Signed-off-by: Stefan Tauner
> >
> > chipset_enable.c |   12 +++++++++++-
> >   1 files changed, 11 insertions(+), 1 deletions(-)
> >
> > diff --git a/chipset_enable.c b/chipset_enable.c
> > index 83b49ad..339c6bb 100644
> > --- a/chipset_enable.c
> > +++ b/chipset_enable.c
> > @@ -264,8 +264,18 @@ static int enable_flash_ich(struct pci_dev *dev, const char *name,
> >    	     (old&  (1<<  0)) ? "en" : "dis");
> >    msg_pdbg("BIOS_CNTL is 0x%x\n", old);
> > new = old | 1;
> > /*
> > * Quote from the 6 Series datasheet:
> > * "5: SMM BIOS Write Protect Disable (SMM_BWP)
> > * 1 = BIOS region SMM protection is enabled.
> > * The BIOS Region is not writable unless all processors are in SMM."
> > * In earlier chipsets this bit is reserved. */
> > if (old&  (5<<  1)) {
> > msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> > return -1;
>
> You might still be successful doing the write, in case the SMM handler 
> does not enforce the protection, so maybe you should just print a 
> warning but not return here?

in chromium-os you are trying to unset that bit[1], but according to the
data sheet this is impossible - it is R/W LO (read/write lock once).
and you degraded the warning to dbg level... certainly not suited for
upstream, but maybe desirable for chromium(?).
have you tested this on a board where SMM_BWP is really set to 1? we
may wanna try to write it anyway, but it would be far more interesting
if it really works on some chipsets :)

1: http://git.chromium.org/gitweb/?p=chromiumos/third_party/flashrom.git;a=comm...

[PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Reinauer]

* Stefan Tauner  [111103 12:04]:
> > > new = old | 1;
> > > /*
> > > * Quote from the 6 Series datasheet:
> > > * "5: SMM BIOS Write Protect Disable (SMM_BWP)
> > > * 1 = BIOS region SMM protection is enabled.
> > > * The BIOS Region is not writable unless all processors are in SMM."
> > > * In earlier chipsets this bit is reserved. */
> > > if (old&  (5<<  1)) {
> > > msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> > > return -1;
> >
> > You might still be successful doing the write, in case the SMM handler 
> > does not enforce the protection, so maybe you should just print a 
> > warning but not return here?
>
> in chromium-os you are trying to unset that bit[1], but according to the
> data sheet this is impossible - it is R/W LO (read/write lock once).

Maybe there is some confusion about R/W LO. The bit can be locked, but
that does not mean it is locked automatically by writing / clearing it.
Once it is locked, the lock can not be undone except by a chipset reset.
> and you degraded the warning to dbg level... certainly not suited for
> upstream, but maybe desirable for chromium(?).

Yes, that was done on purpose, because the lock bit that prevents the
bit from being cleared also produces a warning.
> have you tested this on a board where SMM_BWP is really set to 1? we
> may wanna try to write it anyway, but it would be far more interesting
> if it really works on some chipsets :)

Yes, this was implemented to fix a problem I was seeing, and it solved
the problem. :-)

Stefan

[PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Tauner]

On Thu, 3 Nov 2011 19:08:37 +0100
Stefan Reinauer  wrote:
> Stefan Tauner  [111103 12:04]:
> > > > new = old | 1;
> > > > /*
> > > > * Quote from the 6 Series datasheet:
> > > > * "5: SMM BIOS Write Protect Disable (SMM_BWP)
> > > > * 1 = BIOS region SMM protection is enabled.
> > > > * The BIOS Region is not writable unless all processors are in SMM."
> > > > * In earlier chipsets this bit is reserved. */
> > > > if (old&  (5<<  1)) {
> > > > msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> > > > return -1;
> > >
> > > You might still be successful doing the write, in case the SMM handler 
> > > does not enforce the protection, so maybe you should just print a 
> > > warning but not return here?
> >
> > in chromium-os you are trying to unset that bit[1], but according to the
> > data sheet this is impossible - it is R/W LO (read/write lock once).
>
> Maybe there is some confusion about R/W LO. The bit can be locked, but
> that does not mean it is locked automatically by writing / clearing it.
> Once it is locked, the lock can not be undone except by a chipset reset.

hm.
quote 6 series datasheet:
"R/WLO
Read/Write, Lock-Once. A register bit with this attribute can be
written to the non-locked value multiple times, but to the locked value
only once. After the locked value has been written, the bit becomes
read only."
> > and you degraded the warning to dbg level... certainly not suited for
> > upstream, but maybe desirable for chromium(?).
>
> Yes, that was done on purpose, because the lock bit that prevents the
> bit from being cleared also produces a warning.

and that is? BLE? i do not interpret the public datasheet like that,
but...
> > have you tested this on a board where SMM_BWP is really set to 1? we
> > may wanna try to write it anyway, but it would be far more interesting
> > if it really works on some chipsets :)
>
> Yes, this was implemented to fix a problem I was seeing, and it solved
> the problem. :-)

hm ok, we should get this upstream then.

[PATCH 5/7] warn if SMM BIOS Write Protection is detected in BIOS_CNTL

[Stefan Tauner]

On Fri, 4 Nov 2011 00:38:34 +0100
Stefan Tauner  wrote:
> On Thu, 3 Nov 2011 19:08:37 +0100
> Stefan Reinauer  wrote:
> > Stefan Tauner  [111103 12:04]:
> > > > > new = old | 1;
> > > > > /*
> > > > > * Quote from the 6 Series datasheet:
> > > > > * "5: SMM BIOS Write Protect Disable (SMM_BWP)
> > > > > * 1 = BIOS region SMM protection is enabled.
> > > > > * The BIOS Region is not writable unless all processors are in SMM."
> > > > > * In earlier chipsets this bit is reserved. */
> > > > > if (old&  (5<<  1)) {
> > > > > msg_pinfo("WARNING: BIOS region SMM protection is enabled!\n");
> > > > > return -1;
> > > >
> > > > You might still be successful doing the write, in case the SMM handler 
> > > > does not enforce the protection, so maybe you should just print a 
> > > > warning but not return here?
> > >
> > > in chromium-os you are trying to unset that bit[1], but according to the
> > > data sheet this is impossible - it is R/W LO (read/write lock once).
> >
> > Maybe there is some confusion about R/W LO. The bit can be locked, but
> > that does not mean it is locked automatically by writing / clearing it.
> > Once it is locked, the lock can not be undone except by a chipset reset.
>
> hm.
> quote 6 series datasheet:
> "R/WLO
> Read/Write, Lock-Once. A register bit with this attribute can be
> written to the non-locked value multiple times, but to the locked value
> only once. After the locked value has been written, the bit becomes
> read only."
> > > and you degraded the warning to dbg level... certainly not suited for
> > > upstream, but maybe desirable for chromium(?).
> >
> > Yes, that was done on purpose, because the lock bit that prevents the
> > bit from being cleared also produces a warning.
>
> and that is? BLE? i do not interpret the public datasheet like that,
> but...
> > > have you tested this on a board where SMM_BWP is really set to 1? we
> > > may wanna try to write it anyway, but it would be far more interesting
> > > if it really works on some chipsets :)
> >
> > Yes, this was implemented to fix a problem I was seeing, and it solved
> > the problem. :-)
>
> hm ok, we should get this upstream then.

but i would have hoped for an answer to my question before that... :)
anyway, i have now committed something similar to what is already in
chromiumos in r1582.