Re: [PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN

2019-10-08 Thread Matthew Garrett 
On Tue, Oct 8, 2019 at 9:55 PM Javier Martinez Canillas
 wrote:
> Signed-off-by: Javier Martinez Canillas 
> Acked-by: Laszlo Ersek 

Acked-by: Matthew Garrett

Re: [PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN

2019-10-08 Thread Laszlo Ersek 
On 10/08/19 12:55, Javier Martinez Canillas wrote:
> The driver exposes EFI runtime services to user-space through an IOCTL
> interface, calling the EFI services function pointers directly without
> using the efivar API.
> 
> Disallow access to the /dev/efi_test character device when the kernel is
> locked down to prevent arbitrary user-space to call EFI runtime services.
> 
> Also require CAP_SYS_ADMIN to open the chardev to prevent unprivileged
> users to call the EFI runtime services, instead of just relying on the
> chardev file mode bits for this.
> 
> The main user of this driver is the fwts [0] tool that already checks if
> the effective user ID is 0 and fails otherwise. So this change shouldn't
> cause any regression to this tool.
> 
> [0]: https://wiki.ubuntu.com/FirmwareTestSuite/Reference/uefivarinfo
> 
> Signed-off-by: Javier Martinez Canillas 
> Acked-by: Laszlo Ersek 
> 
> ---
> 
> Changes in v2:
> - Also disable /dev/efi_test access when the kernel is locked down as
>   suggested by Matthew Garrett.

Right; if you remember the pre-patch discussion off-list, we kind of
expected that lockdown might affect this. :)

... And, I can see Matt's comment now, at
. Thanks for that!

While this change decreases the usability of the module, I fully agree
it is justified for production use. While it's more convenient for me to
keep SB enabled in the test VM(s) in general, and just run the test
whenever I need it, security trumps convenience. I can disable SB when
necessary, or even dedicate separate VMs (with SB generally disabled) to
this kind of testing.

> - Add Acked-by tag from Laszlo Ersek.

My ACK stands -- I don't know enough to validate the
security_locked_down() call and its friends, but I'm OK with the intent.

Thanks all!
Laszlo

> 
>  drivers/firmware/efi/test/efi_test.c | 8 
>  include/linux/security.h | 1 +
>  security/lockdown/lockdown.c | 1 +
>  3 files changed, 10 insertions(+)
> 
> diff --git a/drivers/firmware/efi/test/efi_test.c 
> b/drivers/firmware/efi/test/efi_test.c
> index 877745c3aaf..7baf48c01e7 100644
> --- a/drivers/firmware/efi/test/efi_test.c
> +++ b/drivers/firmware/efi/test/efi_test.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -717,6 +718,13 @@ static long efi_test_ioctl(struct file *file, unsigned 
> int cmd,
>  
>  static int efi_test_open(struct inode *inode, struct file *file)
>  {
> + int ret = security_locked_down(LOCKDOWN_EFI_TEST);
> +
> + if (ret)
> + return ret;
> +
> + if (!capable(CAP_SYS_ADMIN))
> + return -EACCES;
>   /*
>* nothing special to do here
>* We do accept multiple open files at the same time as we
> diff --git a/include/linux/security.h b/include/linux/security.h
> index a8d59d612d2..9df7547afc0 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -105,6 +105,7 @@ enum lockdown_reason {
>   LOCKDOWN_NONE,
>   LOCKDOWN_MODULE_SIGNATURE,
>   LOCKDOWN_DEV_MEM,
> + LOCKDOWN_EFI_TEST,
>   LOCKDOWN_KEXEC,
>   LOCKDOWN_HIBERNATION,
>   LOCKDOWN_PCI_ACCESS,
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index 8a10b43daf7..40b790536de 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -20,6 +20,7 @@ static const char *const 
> lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
>   [LOCKDOWN_NONE] = "none",
>   [LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
>   [LOCKDOWN_DEV_MEM] = "/dev/mem,kmem,port",
> + [LOCKDOWN_EFI_TEST] = "/dev/efi_test access",
>   [LOCKDOWN_KEXEC] = "kexec of unsigned images",
>   [LOCKDOWN_HIBERNATION] = "hibernation",
>   [LOCKDOWN_PCI_ACCESS] = "direct PCI access",
>

Re: [PATCH][next] efi/tpm: fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread Colin Ian King 
On 08/10/2019 17:15, Jerry Snitselaar wrote:
> On Tue Oct 08 19, Dan Carpenter wrote:
>> On Tue, Oct 08, 2019 at 11:01:53AM +0100, Colin King wrote:
>>> From: Colin Ian King 
>>>
>>> Currently the check for tbl_size being less than zero is always false
>>> because tbl_size is unsigned. Fix this by making it a signed int.
>>>
>>> Addresses-Coverity: ("Unsigned compared against 0")
>>> Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size'
>>> after successful event log parsing")
>>> Signed-off-by: Colin Ian King 
>>> ---
>>>  drivers/firmware/efi/tpm.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
>>> index 703469c1ab8e..ebd7977653a8 100644
>>> --- a/drivers/firmware/efi/tpm.c
>>> +++ b/drivers/firmware/efi/tpm.c
>>> @@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
>>>  {
>>>  struct linux_efi_tpm_eventlog *log_tbl;
>>>  struct efi_tcg2_final_events_table *final_tbl;
>>> -    unsigned int tbl_size;
>>> +    int tbl_size;
>>>  int ret = 0;
>>
>>
>> Do we need to do a "ret = tbl_size;"?  Currently we return success.
>> It's a pitty that tpm2_calc_event_log_size() returns a -1 instead of
>> -EINVAL.
>>
>> regards,
>> dan carpenter
>>
> 
> perhaps "ret = -EINVAL;"? Currently nothing checks the return value of
> efi_tpm_eventlog_init though.

I doubt I'll fix that for my current fix as a V2.

Re: [PATCH][next] efi/tpm: fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread Jerry Snitselaar 

On Tue Oct 08 19, Dan Carpenter wrote:

On Tue, Oct 08, 2019 at 11:01:53AM +0100, Colin King wrote:

From: Colin Ian King 

Currently the check for tbl_size being less than zero is always false
because tbl_size is unsigned. Fix this by making it a signed int.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after successful 
event log parsing")
Signed-off-by: Colin Ian King 
---
 drivers/firmware/efi/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 703469c1ab8e..ebd7977653a8 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
 {
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
-   unsigned int tbl_size;
+   int tbl_size;
int ret = 0;



Do we need to do a "ret = tbl_size;"?  Currently we return success.
It's a pitty that tpm2_calc_event_log_size() returns a -1 instead of
-EINVAL.

regards,
dan carpenter



perhaps "ret = -EINVAL;"? Currently nothing checks the return value of 
efi_tpm_eventlog_init though.

Re: [PATCH][next] efi/tpm: fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread Jerry Snitselaar 

On Tue Oct 08 19, Colin King wrote:

From: Colin Ian King 

Currently the check for tbl_size being less than zero is always false
because tbl_size is unsigned. Fix this by making it a signed int.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after successful 
event log parsing")
Signed-off-by: Colin Ian King 
---
drivers/firmware/efi/tpm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 703469c1ab8e..ebd7977653a8 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
{
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
-   unsigned int tbl_size;
+   int tbl_size;
int ret = 0;

if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
--
2.20.1



Thanks for catching that. Somehow I dropped it from v2 to v3.

Re: [PATCH v6 3/9] powerpc: add support to initialize ima policy rules

2019-10-08 Thread Nayna 




On 10/02/2019 05:49 PM, Mimi Zohar wrote:

On Tue, 2019-10-01 at 12:07 -0400, Nayna wrote:

On 09/30/2019 09:04 PM, Thiago Jung Bauermann wrote:

Hello,

Hi,


diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index ..39401b67f19e
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+
+#include 
+#include 
+
+bool arch_ima_get_secureboot(void)
+{
+   return is_powerpc_os_secureboot_enabled();
+}
+
+/* Defines IMA appraise rules for secureboot */
+static const char *const arch_rules[] = {
+   "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if !IS_ENABLED(CONFIG_MODULE_SIG)
+   "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+   NULL
+};
+
+/*
+ * Returns the relevant IMA arch policies based on the system secureboot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+   if (is_powerpc_os_secureboot_enabled())
+   return arch_rules;
+
+   return NULL;
+}

If CONFIG_MODULE_SIG is enabled but module signatures aren't enforced,
then IMA won't enforce module signature either. x86's
arch_get_ima_policy() calls set_module_sig_enforced(). Doesn't the
powerpc version need to do that as well?

On the flip side, if module signatures are enforced by the module
subsystem then IMA will verify the signature a second time since there's
no sharing of signature verification results between the module
subsystem and IMA (this was observed by Mimi).

IMHO this is a minor issue, since module loading isn't a hot path and
the duplicate work shouldn't impact anything. But it could be avoided by
having a NULL entry in arch_rules, which arch_get_ima_policy() would
dynamically update with the "appraise func=MODULE_CHECK" rule if
is_module_sig_enforced() is true.

Thanks Thiago for reviewing.  I am wondering that this will give two
meanings for NULL. Can we do something like below, there are possibly
two options ?

1. Set IMA_APPRAISED in the iint->flags if is_module_sig_enforced().

OR

2. Let ima_get_action() check for is_module_sig_enforced() when policy
is appraise and func is MODULE_CHECK.

I'm a bit hesitant about mixing the module subsystem signature
verification method with the IMA measure "template=ima-modsig" rules.
  Does it actually work?

We can at least limit verifying the same appended signature twice to
when "module.sig_enforce" is specified on the boot command line, by
changing "!IS_ENABLED(CONFIG_MODULE_SIG)" to test
"CONFIG_MODULE_SIG_FORCE".


Yes this seems to be a better idea. I have implemented this in the v7 
version of the ima_arch version.


Thanks & Regards,
 - Nayna

Re: [PATCH][next] efi/tpm: fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread Dan Carpenter 
On Tue, Oct 08, 2019 at 11:01:53AM +0100, Colin King wrote:
> From: Colin Ian King 
> 
> Currently the check for tbl_size being less than zero is always false
> because tbl_size is unsigned. Fix this by making it a signed int.
> 
> Addresses-Coverity: ("Unsigned compared against 0")
> Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after 
> successful event log parsing")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/firmware/efi/tpm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 703469c1ab8e..ebd7977653a8 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
>  {
>   struct linux_efi_tpm_eventlog *log_tbl;
>   struct efi_tcg2_final_events_table *final_tbl;
> - unsigned int tbl_size;
> + int tbl_size;
>   int ret = 0;


Do we need to do a "ret = tbl_size;"?  Currently we return success.
It's a pitty that tpm2_calc_event_log_size() returns a -1 instead of
-EINVAL.

regards,
dan carpenter

[tip: efi/urgent] efi/tpm: Fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread tip-bot2 for Colin Ian King 
The following commit has been merged into the efi/urgent branch of tip:

Commit-ID: be59d57f98065af0b8472f66a0a969207b168680
Gitweb:
https://git.kernel.org/tip/be59d57f98065af0b8472f66a0a969207b168680
Author:Colin Ian King 
AuthorDate:Tue, 08 Oct 2019 11:01:53 +01:00
Committer: Ingo Molnar 
CommitterDate: Tue, 08 Oct 2019 13:01:09 +02:00

efi/tpm: Fix sanity check of unsigned tbl_size being less than zero

Currently the check for tbl_size being less than zero is always false
because tbl_size is unsigned. Fix this by making it a signed int.

Addresses-Coverity: ("Unsigned compared against 0")
Signed-off-by: Colin Ian King 
Cc: Ard Biesheuvel 
Cc: Jerry Snitselaar 
Cc: Linus Torvalds 
Cc: Peter Zijlstra 
Cc: Thomas Gleixner 
Cc: kernel-janit...@vger.kernel.org
Cc: linux-efi@vger.kernel.org
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after 
successful event log parsing")
Link: https://lkml.kernel.org/r/20191008100153.8499-1-colin.k...@canonical.com
Signed-off-by: Ingo Molnar 
---
 drivers/firmware/efi/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 703469c..ebd7977 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
 {
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
-   unsigned int tbl_size;
+   int tbl_size;
int ret = 0;
 
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {

[PATCH v2] efi/efi_test: lock down /dev/efi_test and require CAP_SYS_ADMIN

2019-10-08 Thread Javier Martinez Canillas 
The driver exposes EFI runtime services to user-space through an IOCTL
interface, calling the EFI services function pointers directly without
using the efivar API.

Disallow access to the /dev/efi_test character device when the kernel is
locked down to prevent arbitrary user-space to call EFI runtime services.

Also require CAP_SYS_ADMIN to open the chardev to prevent unprivileged
users to call the EFI runtime services, instead of just relying on the
chardev file mode bits for this.

The main user of this driver is the fwts [0] tool that already checks if
the effective user ID is 0 and fails otherwise. So this change shouldn't
cause any regression to this tool.

[0]: https://wiki.ubuntu.com/FirmwareTestSuite/Reference/uefivarinfo

Signed-off-by: Javier Martinez Canillas 
Acked-by: Laszlo Ersek 

---

Changes in v2:
- Also disable /dev/efi_test access when the kernel is locked down as
  suggested by Matthew Garrett.
- Add Acked-by tag from Laszlo Ersek.

 drivers/firmware/efi/test/efi_test.c | 8 
 include/linux/security.h | 1 +
 security/lockdown/lockdown.c | 1 +
 3 files changed, 10 insertions(+)

diff --git a/drivers/firmware/efi/test/efi_test.c 
b/drivers/firmware/efi/test/efi_test.c
index 877745c3aaf..7baf48c01e7 100644
--- a/drivers/firmware/efi/test/efi_test.c
+++ b/drivers/firmware/efi/test/efi_test.c
@@ -14,6 +14,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -717,6 +718,13 @@ static long efi_test_ioctl(struct file *file, unsigned int 
cmd,
 
 static int efi_test_open(struct inode *inode, struct file *file)
 {
+   int ret = security_locked_down(LOCKDOWN_EFI_TEST);
+
+   if (ret)
+   return ret;
+
+   if (!capable(CAP_SYS_ADMIN))
+   return -EACCES;
/*
 * nothing special to do here
 * We do accept multiple open files at the same time as we
diff --git a/include/linux/security.h b/include/linux/security.h
index a8d59d612d2..9df7547afc0 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -105,6 +105,7 @@ enum lockdown_reason {
LOCKDOWN_NONE,
LOCKDOWN_MODULE_SIGNATURE,
LOCKDOWN_DEV_MEM,
+   LOCKDOWN_EFI_TEST,
LOCKDOWN_KEXEC,
LOCKDOWN_HIBERNATION,
LOCKDOWN_PCI_ACCESS,
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index 8a10b43daf7..40b790536de 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -20,6 +20,7 @@ static const char *const 
lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1] = {
[LOCKDOWN_NONE] = "none",
[LOCKDOWN_MODULE_SIGNATURE] = "unsigned module loading",
[LOCKDOWN_DEV_MEM] = "/dev/mem,kmem,port",
+   [LOCKDOWN_EFI_TEST] = "/dev/efi_test access",
[LOCKDOWN_KEXEC] = "kexec of unsigned images",
[LOCKDOWN_HIBERNATION] = "hibernation",
[LOCKDOWN_PCI_ACCESS] = "direct PCI access",
-- 
2.21.0

[PATCH][next] efi/tpm: fix sanity check of unsigned tbl_size being less than zero

2019-10-08 Thread Colin King 
From: Colin Ian King 

Currently the check for tbl_size being less than zero is always false
because tbl_size is unsigned. Fix this by making it a signed int.

Addresses-Coverity: ("Unsigned compared against 0")
Fixes: e658c82be556 ("efi/tpm: Only set 'efi_tpm_final_log_size' after 
successful event log parsing")
Signed-off-by: Colin Ian King 
---
 drivers/firmware/efi/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 703469c1ab8e..ebd7977653a8 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -40,7 +40,7 @@ int __init efi_tpm_eventlog_init(void)
 {
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
-   unsigned int tbl_size;
+   int tbl_size;
int ret = 0;
 
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
-- 
2.20.1