diff mbox

[23/31] ich9: implement SMI_LOCK

Message ID 1431352157-40283-24-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 11, 2015, 1:49 p.m. UTC 
From: Gerd Hoffmann <kraxel@redhat.com>

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/acpi/ich9.c         |  4 +++-
 hw/isa/lpc_ich9.c      | 19 +++++++++++++++++++
 include/hw/acpi/ich9.h |  1 +
 include/hw/i386/ich9.h |  6 ++++++
 4 files changed, 29 insertions(+), 1 deletion(-)

Comments

Laszlo Ersek May 11, 2015, 3:17 p.m. UTC | #1 
On 05/11/15 15:49, Paolo Bonzini wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/acpi/ich9.c         |  4 +++-
>  hw/isa/lpc_ich9.c      | 19 +++++++++++++++++++
>  include/hw/acpi/ich9.h |  1 +
>  include/hw/i386/ich9.h |  6 ++++++
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5352e19..310aa64 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -94,7 +94,8 @@ static void ich9_smi_writel(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCPMRegs *pm = opaque;
>      switch (addr) {
>      case 0:
> -        pm->smi_en = val;
> +        pm->smi_en &= ~pm->smi_en_wmask;
> +        pm->smi_en |= (val & pm->smi_en_wmask);
>          break;
>      }
>  }
> @@ -198,6 +199,7 @@ static void pm_reset(void *opaque)
>           * support SMM mode. */
>          pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
>      }
> +    pm->smi_en_wmask = ~0;
>  
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index dba7585..0269cfe 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -410,12 +410,28 @@ static void ich9_lpc_rcba_update(ICH9LPCState *lpc, uint32_t rbca_old)
>      }
>  }
>  
> +/* config:GEN_PMCON* */
> +static void
> +ich9_lpc_pmcon_update(ICH9LPCState *lpc)
> +{
> +    uint16_t gen_pmcon_1 = pci_get_word(lpc->d.config + ICH9_LPC_GEN_PMCON_1);
> +    uint16_t wmask;
> +
> +    if (gen_pmcon_1 & ICH9_LPC_GEN_PMCON_1_SMI_LOCK) {
> +        wmask = pci_get_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1);
> +        wmask &= ~ICH9_LPC_GEN_PMCON_1_SMI_LOCK;
> +        pci_set_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1, wmask);
> +        lpc->pm.smi_en_wmask &= ~1;

If I understand correctly, this makes SMI_LOCK lock down the GBL_SMI_EN
bit (and my OVMF patch that relies on that / tests it is satisfied too).

But, it doesn't seem to lock down APMC_EN. According to the ICH9 spec,
it doesn't need to -- however when we discussed this earlier (see
Message-Id: <553F4D23.3060305@redhat.com>), the idea was to lock down
APMC_EN as well. (And I don't understand why the ICH9 spec / hw
implementation doesn't lock APMC_EN; without that, APM_CNT won't
necessarily trigger an SMI.)

Thanks!
Laszlo

> +    }
> +}
> +
>  static int ich9_lpc_post_load(void *opaque, int version_id)
>  {
>      ICH9LPCState *lpc = opaque;
>  
>      ich9_lpc_pmbase_update(lpc);
>      ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RBCA_EN */);
> +    ich9_lpc_pmcon_update(lpc);
>      return 0;
>  }
>  
> @@ -438,6 +454,9 @@ static void ich9_lpc_config_write(PCIDevice *d,
>      if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
>          pci_bus_fire_intx_routing_notifier(lpc->d.bus);
>      }
> +    if (ranges_overlap(addr, len, ICH9_LPC_GEN_PMCON_1, 8)) {
> +        ich9_lpc_pmcon_update(lpc);
> +    }
>  }
>  
>  static void ich9_lpc_reset(DeviceState *qdev)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index c2d3dba..77cc65c 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -39,6 +39,7 @@ typedef struct ICH9LPCPMRegs {
>      MemoryRegion io_smi;
>  
>      uint32_t smi_en;
> +    uint32_t smi_en_wmask;
>      uint32_t smi_sts;
>  
>      qemu_irq irq;      /* SCI */
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index f4e522c..a2cc15c 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -152,6 +152,12 @@ Object *ich9_lpc_find(void);
>  #define ICH9_LPC_PIRQ_ROUT_MASK                 Q35_MASK(8, 3, 0)
>  #define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
>  
> +#define ICH9_LPC_GEN_PMCON_1                    0xa0
> +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
> +#define ICH9_LPC_GEN_PMCON_2                    0xa2
> +#define ICH9_LPC_GEN_PMCON_3                    0xa4
> +#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
> +
>  #define ICH9_LPC_RCBA                           0xf0
>  #define ICH9_LPC_RCBA_BA_MASK                   Q35_MASK(32, 31, 14)
>  #define ICH9_LPC_RCBA_EN                        0x1
> 

Thanks
Laszlo
Paolo Bonzini May 11, 2015, 3:21 p.m. UTC | #2 
On 11/05/2015 17:17, Laszlo Ersek wrote:
> If I understand correctly, this makes SMI_LOCK lock down the GBL_SMI_EN
> bit (and my OVMF patch that relies on that / tests it is satisfied too).
> 
> But, it doesn't seem to lock down APMC_EN. According to the ICH9 spec,
> it doesn't need to -- however when we discussed this earlier (see
> Message-Id: <553F4D23.3060305@redhat.com>), the idea was to lock down
> APMC_EN as well. (And I don't understand why the ICH9 spec / hw
> implementation doesn't lock APMC_EN; without that, APM_CNT won't
> necessarily trigger an SMI.)

I don't think it should.  See here
<https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02758.html>
where I wrote explicitly "Even if the OS tries to maliciously set
APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN)...".

Paolo
Laszlo Ersek May 11, 2015, 3:36 p.m. UTC | #3 
On 05/11/15 17:21, Paolo Bonzini wrote:
> 
> 
> On 11/05/2015 17:17, Laszlo Ersek wrote:
>> If I understand correctly, this makes SMI_LOCK lock down the GBL_SMI_EN
>> bit (and my OVMF patch that relies on that / tests it is satisfied too).
>>
>> But, it doesn't seem to lock down APMC_EN. According to the ICH9 spec,
>> it doesn't need to -- however when we discussed this earlier (see
>> Message-Id: <553F4D23.3060305@redhat.com>), the idea was to lock down
>> APMC_EN as well. (And I don't understand why the ICH9 spec / hw
>> implementation doesn't lock APMC_EN; without that, APM_CNT won't
>> necessarily trigger an SMI.)
> 
> I don't think it should.  See here
> <https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02758.html>
> where I wrote explicitly "Even if the OS tries to maliciously set
> APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN)...".

It's not about feature detection -- the question (from
<553F4D23.3060305@redhat.com>) is whether I should set APMC_EN myself
*every time* before writing to APM_CNT, in the
EFI_SMM_CONTROL2_PROTOCOL.Trigger() method. That protocol is provided by
a runtime DXE driver and would be exercised by eg. the non-privileged
half of the runtime variable service driver.

It's no problem to set it, I have the code ready, I was just wondering
if I should keep that hunk. (In fact it might not even matter: if the OS
interferes and clears APMC_EN before the non-privileged half mentioned
above manages to raise the SMI, then the call / transition to SMM will
simply not happen, which is bad for the OS, and probably irrelevant for
the firmware (... the security thereof).)

I guess I'll keep re-setting APMC_EN in the Trigger() method; it won't hurt.

Thanks
Laszlo
Paolo Bonzini May 11, 2015, 3:45 p.m. UTC | #4 
On 11/05/2015 17:36, Laszlo Ersek wrote:
> It's not about feature detection -- the question (from
> <553F4D23.3060305@redhat.com>) is whether I should set APMC_EN myself
> *every time* before writing to APM_CNT, in the
> EFI_SMM_CONTROL2_PROTOCOL.Trigger() method. That protocol is provided by
> a runtime DXE driver and would be exercised by eg. the non-privileged
> half of the runtime variable service driver.

Oh sorry, I couldn't find that message ID.

> It's no problem to set it, I have the code ready, I was just wondering
> if I should keep that hunk. (In fact it might not even matter: if the OS
> interferes and clears APMC_EN before the non-privileged half mentioned
> above manages to raise the SMI, then the call / transition to SMM will
> simply not happen, which is bad for the OS, and probably irrelevant for
> the firmware (... the security thereof).)

The OS can also race against you and clear APMC_EN, so it's even
unnecessary to reset it.

Paolo
Gerd Hoffmann May 12, 2015, 7:07 a.m. UTC | #5 
On Mo, 2015-05-11 at 15:49 +0200, Paolo Bonzini wrote:
> From: Gerd Hoffmann <kraxel@redhat.com>

[ more verbose commit message for squashing in ]

Add write mask for the smi enable register, so we can disable write
access to certain bits.  Open all bits on reset.  Disable write access
to GBL_SMI_EN when SMI_LOCK (in ich9 lpc pci config space) is set.
Write access to SMI_LOCK itself is disabled too.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/acpi/ich9.c         |  4 +++-
>  hw/isa/lpc_ich9.c      | 19 +++++++++++++++++++
>  include/hw/acpi/ich9.h |  1 +
>  include/hw/i386/ich9.h |  6 ++++++
>  4 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 5352e19..310aa64 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -94,7 +94,8 @@ static void ich9_smi_writel(void *opaque, hwaddr addr, uint64_t val,
>      ICH9LPCPMRegs *pm = opaque;
>      switch (addr) {
>      case 0:
> -        pm->smi_en = val;
> +        pm->smi_en &= ~pm->smi_en_wmask;
> +        pm->smi_en |= (val & pm->smi_en_wmask);
>          break;
>      }
>  }
> @@ -198,6 +199,7 @@ static void pm_reset(void *opaque)
>           * support SMM mode. */
>          pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
>      }
> +    pm->smi_en_wmask = ~0;
>  
>      acpi_update_sci(&pm->acpi_regs, pm->irq);
>  }
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index dba7585..0269cfe 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -410,12 +410,28 @@ static void ich9_lpc_rcba_update(ICH9LPCState *lpc, uint32_t rbca_old)
>      }
>  }
>  
> +/* config:GEN_PMCON* */
> +static void
> +ich9_lpc_pmcon_update(ICH9LPCState *lpc)
> +{
> +    uint16_t gen_pmcon_1 = pci_get_word(lpc->d.config + ICH9_LPC_GEN_PMCON_1);
> +    uint16_t wmask;
> +
> +    if (gen_pmcon_1 & ICH9_LPC_GEN_PMCON_1_SMI_LOCK) {
> +        wmask = pci_get_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1);
> +        wmask &= ~ICH9_LPC_GEN_PMCON_1_SMI_LOCK;
> +        pci_set_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1, wmask);
> +        lpc->pm.smi_en_wmask &= ~1;
> +    }
> +}
> +
>  static int ich9_lpc_post_load(void *opaque, int version_id)
>  {
>      ICH9LPCState *lpc = opaque;
>  
>      ich9_lpc_pmbase_update(lpc);
>      ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RBCA_EN */);
> +    ich9_lpc_pmcon_update(lpc);
>      return 0;
>  }
>  
> @@ -438,6 +454,9 @@ static void ich9_lpc_config_write(PCIDevice *d,
>      if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
>          pci_bus_fire_intx_routing_notifier(lpc->d.bus);
>      }
> +    if (ranges_overlap(addr, len, ICH9_LPC_GEN_PMCON_1, 8)) {
> +        ich9_lpc_pmcon_update(lpc);
> +    }
>  }
>  
>  static void ich9_lpc_reset(DeviceState *qdev)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index c2d3dba..77cc65c 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -39,6 +39,7 @@ typedef struct ICH9LPCPMRegs {
>      MemoryRegion io_smi;
>  
>      uint32_t smi_en;
> +    uint32_t smi_en_wmask;
>      uint32_t smi_sts;
>  
>      qemu_irq irq;      /* SCI */
> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
> index f4e522c..a2cc15c 100644
> --- a/include/hw/i386/ich9.h
> +++ b/include/hw/i386/ich9.h
> @@ -152,6 +152,12 @@ Object *ich9_lpc_find(void);
>  #define ICH9_LPC_PIRQ_ROUT_MASK                 Q35_MASK(8, 3, 0)
>  #define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
>  
> +#define ICH9_LPC_GEN_PMCON_1                    0xa0
> +#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
> +#define ICH9_LPC_GEN_PMCON_2                    0xa2
> +#define ICH9_LPC_GEN_PMCON_3                    0xa4
> +#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
> +
>  #define ICH9_LPC_RCBA                           0xf0
>  #define ICH9_LPC_RCBA_BA_MASK                   Q35_MASK(32, 31, 14)
>  #define ICH9_LPC_RCBA_EN                        0x1
diff mbox

Patch

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5352e19..310aa64 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -94,7 +94,8 @@  static void ich9_smi_writel(void *opaque, hwaddr addr, uint64_t val,
     ICH9LPCPMRegs *pm = opaque;
     switch (addr) {
     case 0:
-        pm->smi_en = val;
+        pm->smi_en &= ~pm->smi_en_wmask;
+        pm->smi_en |= (val & pm->smi_en_wmask);
         break;
     }
 }
@@ -198,6 +199,7 @@  static void pm_reset(void *opaque)
          * support SMM mode. */
         pm->smi_en |= ICH9_PMIO_SMI_EN_APMC_EN;
     }
+    pm->smi_en_wmask = ~0;
 
     acpi_update_sci(&pm->acpi_regs, pm->irq);
 }
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index dba7585..0269cfe 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -410,12 +410,28 @@  static void ich9_lpc_rcba_update(ICH9LPCState *lpc, uint32_t rbca_old)
     }
 }
 
+/* config:GEN_PMCON* */
+static void
+ich9_lpc_pmcon_update(ICH9LPCState *lpc)
+{
+    uint16_t gen_pmcon_1 = pci_get_word(lpc->d.config + ICH9_LPC_GEN_PMCON_1);
+    uint16_t wmask;
+
+    if (gen_pmcon_1 & ICH9_LPC_GEN_PMCON_1_SMI_LOCK) {
+        wmask = pci_get_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1);
+        wmask &= ~ICH9_LPC_GEN_PMCON_1_SMI_LOCK;
+        pci_set_word(lpc->d.wmask + ICH9_LPC_GEN_PMCON_1, wmask);
+        lpc->pm.smi_en_wmask &= ~1;
+    }
+}
+
 static int ich9_lpc_post_load(void *opaque, int version_id)
 {
     ICH9LPCState *lpc = opaque;
 
     ich9_lpc_pmbase_update(lpc);
     ich9_lpc_rcba_update(lpc, 0 /* disabled ICH9_LPC_RBCA_EN */);
+    ich9_lpc_pmcon_update(lpc);
     return 0;
 }
 
@@ -438,6 +454,9 @@  static void ich9_lpc_config_write(PCIDevice *d,
     if (ranges_overlap(addr, len, ICH9_LPC_PIRQE_ROUT, 4)) {
         pci_bus_fire_intx_routing_notifier(lpc->d.bus);
     }
+    if (ranges_overlap(addr, len, ICH9_LPC_GEN_PMCON_1, 8)) {
+        ich9_lpc_pmcon_update(lpc);
+    }
 }
 
 static void ich9_lpc_reset(DeviceState *qdev)
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index c2d3dba..77cc65c 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -39,6 +39,7 @@  typedef struct ICH9LPCPMRegs {
     MemoryRegion io_smi;
 
     uint32_t smi_en;
+    uint32_t smi_en_wmask;
     uint32_t smi_sts;
 
     qemu_irq irq;      /* SCI */
diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
index f4e522c..a2cc15c 100644
--- a/include/hw/i386/ich9.h
+++ b/include/hw/i386/ich9.h
@@ -152,6 +152,12 @@  Object *ich9_lpc_find(void);
 #define ICH9_LPC_PIRQ_ROUT_MASK                 Q35_MASK(8, 3, 0)
 #define ICH9_LPC_PIRQ_ROUT_DEFAULT              0x80
 
+#define ICH9_LPC_GEN_PMCON_1                    0xa0
+#define ICH9_LPC_GEN_PMCON_1_SMI_LOCK           (1 << 4)
+#define ICH9_LPC_GEN_PMCON_2                    0xa2
+#define ICH9_LPC_GEN_PMCON_3                    0xa4
+#define ICH9_LPC_GEN_PMCON_LOCK                 0xa6
+
 #define ICH9_LPC_RCBA                           0xf0
 #define ICH9_LPC_RCBA_BA_MASK                   Q35_MASK(32, 31, 14)
 #define ICH9_LPC_RCBA_EN                        0x1