* Re: [PATCH 01/12] ima: Have the LSM free its audit rule
From: Tyler Hicks @ 2020-06-23 3:04 UTC (permalink / raw)
To: Casey Schaufler
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Prakhar Srivastava, linux-kernel,
linux-integrity, linux-security-module, Janne Karhunen
In-Reply-To: <277dd210-c443-c067-e731-44ac53418fa5@schaufler-ca.com>
On 2020-06-22 17:55:59, Casey Schaufler wrote:
> On 6/22/2020 5:32 PM, Tyler Hicks wrote:
> > Ask the LSM to free its audit rule rather than directly calling kfree().
> > Both AppArmor and SELinux do additional work in their audit_rule_free()
> > hooks. Fix memory leaks by allowing the LSMs to perform necessary work.
> >
> > Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > Cc: Janne Karhunen <janne.karhunen@gmail.com>
> > ---
> > security/integrity/ima/ima.h | 6 ++++++
> > security/integrity/ima/ima_policy.c | 2 +-
> > 2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index df93ac258e01..de05d7f1d3ec 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
> > #ifdef CONFIG_IMA_LSM_RULES
> >
> > #define security_filter_rule_init security_audit_rule_init
> > +#define security_filter_rule_free security_audit_rule_free
> > #define security_filter_rule_match security_audit_rule_match
>
> In context this seems perfectly reasonable. If, however, you're
> working with the LSM infrastructure this set of #defines is maddening.
> The existing ones have been driving my nuts for the past few years,
> so I'd like to discourage adding another. Since the security_filter_rule
> functions are IMA specific they shouldn't be prefixed security_. I know
> that it seems to be code churn/bikesheading, but we please change these:
>
> static inline int ima_filter_rule_init(.....)
> {
> return security_audit_rule_init(.....);
> }
>
> and so forth. I understand if you don't want to make the change.
> I have plenty of other things driving me crazy just now, so this
> doesn't seem likely to push me over the edge.
I'd be happy to take a stab at that as a follow-up or a 13/12 patch. I'd
like to leave this one as-is for stable kernel reasons since it is
straightforward and simple.
Tyler
>
> >
> > #else
> > @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> > return -EINVAL;
> > }
> >
> > +static inline void security_filter_rule_free(void *lsmrule)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> > void *lsmrule)
> > {
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index e493063a3c34..236a731492d1 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> > int i;
> >
> > for (i = 0; i < MAX_LSM_RULES; i++) {
> > - kfree(entry->lsm[i].rule);
> > + security_filter_rule_free(entry->lsm[i].rule);
> > kfree(entry->lsm[i].args_p);
> > }
> > kfree(entry);
^ permalink raw reply
* Re: [PATCH v3] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-23 1:01 UTC (permalink / raw)
To: Stefan Berger
Cc: Rafael J. Wysocki, Stefan Berger, Rafael J. Wysocki,
linux-integrity, ACPI Devel Maling List,
Linux Kernel Mailing List, linux-security-module
In-Reply-To: <20200623005647.GD28795@linux.intel.com>
On Tue, Jun 23, 2020 at 03:56:53AM +0300, Jarkko Sakkinen wrote:
> On Fri, Jun 19, 2020 at 11:14:20AM -0400, Stefan Berger wrote:
> > On 4/2/20 3:21 PM, Jarkko Sakkinen wrote:
> > > On Wed, Apr 01, 2020 at 11:05:36AM +0200, Rafael J. Wysocki wrote:
> > > > On Wed, Apr 1, 2020 at 10:37 AM Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > > On Tue, Mar 31, 2020 at 05:49:49PM -0400, Stefan Berger wrote:
> > > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > > >
> > > > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > > > including 12 bytes of start method specific parameters and Log Area
> > > > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > > > the existing structure with these fields to allow non-UEFI systems
> > > > > > to access the TPM2's log.
> > > > > >
> > > > > > The specification that has the new fields is the following:
> > > > > > TCG ACPI Specification
> > > > > > Family "1.2" and "2.0"
> > > > > > Version 1.2, Revision 8
> > > > > >
> > > > > > Adapt all existing table size calculations to use
> > > > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > > > [where start_method_specific is a newly added field]
> > > > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > > > of the new fields does not affect current systems that may not
> > > > > > have them.
> > > > > >
> > > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > > Cc: linux-acpi@vger.kernel.org
> > > > > I think I'm cool with this but needs an ack from ACPI maintainer.
> > > > >
> > > > > Rafael, given that this not an intrusive change in any possible means,
> > > > > can I pick this patch and put it to my next pull request?
> > > > Yes, please.
> > > >
> > > > Thanks!
> > > Great, thanks Rafael.
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > Do you mind if I add your ack to the commit?
> >
> >
> > Any chance to get v4 applied?
>
> You should split the actbl3.h change to a separate patch and add 'Cc:'
> tag to Rafael to the commit message.
Send v5 with Rafael's ack (no need to split anymore).
/Jarkko
^ permalink raw reply
* Re: [PATCH v3] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-23 0:57 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Stefan Berger, Stefan Berger, Rafael J. Wysocki, linux-integrity,
ACPI Devel Maling List, Linux Kernel Mailing List,
linux-security-module
In-Reply-To: <CAJZ5v0isZCK8o6hopUV3SP25P5+BwZGSSRFtGQrunQ0n45t68g@mail.gmail.com>
On Fri, Jun 19, 2020 at 05:55:19PM +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 19, 2020 at 5:14 PM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> > On 4/2/20 3:21 PM, Jarkko Sakkinen wrote:
> > > On Wed, Apr 01, 2020 at 11:05:36AM +0200, Rafael J. Wysocki wrote:
> > >> On Wed, Apr 1, 2020 at 10:37 AM Jarkko Sakkinen
> > >> <jarkko.sakkinen@linux.intel.com> wrote:
> > >>> On Tue, Mar 31, 2020 at 05:49:49PM -0400, Stefan Berger wrote:
> > >>>> From: Stefan Berger <stefanb@linux.ibm.com>
> > >>>>
> > >>>> Recent extensions of the TPM2 ACPI table added 3 more fields
> > >>>> including 12 bytes of start method specific parameters and Log Area
> > >>>> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > >>>> the existing structure with these fields to allow non-UEFI systems
> > >>>> to access the TPM2's log.
> > >>>>
> > >>>> The specification that has the new fields is the following:
> > >>>> TCG ACPI Specification
> > >>>> Family "1.2" and "2.0"
> > >>>> Version 1.2, Revision 8
> > >>>>
> > >>>> Adapt all existing table size calculations to use
> > >>>> offsetof(struct acpi_table_tpm2, start_method_specific)
> > >>>> [where start_method_specific is a newly added field]
> > >>>> rather than sizeof(struct acpi_table_tpm2) so that the addition
> > >>>> of the new fields does not affect current systems that may not
> > >>>> have them.
> > >>>>
> > >>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > >>>> Cc: linux-acpi@vger.kernel.org
> > >>> I think I'm cool with this but needs an ack from ACPI maintainer.
> > >>>
> > >>> Rafael, given that this not an intrusive change in any possible means,
> > >>> can I pick this patch and put it to my next pull request?
> > >> Yes, please.
> > >>
> > >> Thanks!
> > > Great, thanks Rafael.
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > Do you mind if I add your ack to the commit?
> >
>
> It looks like I missed the previous message from Jarkko.
>
> Yes, please, feel free to add my ACK to the patch, thanks!
OK, this is great, thanks. I'll pick it to my tree then.
/Jarkko
^ permalink raw reply
* Re: [PATCH v3] acpi: Extend TPM2 ACPI table with missing log fields
From: Jarkko Sakkinen @ 2020-06-23 0:56 UTC (permalink / raw)
To: Stefan Berger
Cc: Rafael J. Wysocki, Stefan Berger, Rafael J. Wysocki,
linux-integrity, ACPI Devel Maling List,
Linux Kernel Mailing List, linux-security-module
In-Reply-To: <dfd2d622-90cb-9621-7b7d-5282f5ee7359@linux.ibm.com>
On Fri, Jun 19, 2020 at 11:14:20AM -0400, Stefan Berger wrote:
> On 4/2/20 3:21 PM, Jarkko Sakkinen wrote:
> > On Wed, Apr 01, 2020 at 11:05:36AM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Apr 1, 2020 at 10:37 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > > On Tue, Mar 31, 2020 at 05:49:49PM -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > >
> > > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > > including 12 bytes of start method specific parameters and Log Area
> > > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > > the existing structure with these fields to allow non-UEFI systems
> > > > > to access the TPM2's log.
> > > > >
> > > > > The specification that has the new fields is the following:
> > > > > TCG ACPI Specification
> > > > > Family "1.2" and "2.0"
> > > > > Version 1.2, Revision 8
> > > > >
> > > > > Adapt all existing table size calculations to use
> > > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > > [where start_method_specific is a newly added field]
> > > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > > of the new fields does not affect current systems that may not
> > > > > have them.
> > > > >
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > Cc: linux-acpi@vger.kernel.org
> > > > I think I'm cool with this but needs an ack from ACPI maintainer.
> > > >
> > > > Rafael, given that this not an intrusive change in any possible means,
> > > > can I pick this patch and put it to my next pull request?
> > > Yes, please.
> > >
> > > Thanks!
> > Great, thanks Rafael.
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > Do you mind if I add your ack to the commit?
>
>
> Any chance to get v4 applied?
You should split the actbl3.h change to a separate patch and add 'Cc:'
tag to Rafael to the commit message.
/Jarkko
^ permalink raw reply
* Re: [PATCH 01/12] ima: Have the LSM free its audit rule
From: Casey Schaufler @ 2020-06-23 0:55 UTC (permalink / raw)
To: Tyler Hicks, Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Casey Schaufler
In-Reply-To: <20200623003236.830149-2-tyhicks@linux.microsoft.com>
On 6/22/2020 5:32 PM, Tyler Hicks wrote:
> Ask the LSM to free its audit rule rather than directly calling kfree().
> Both AppArmor and SELinux do additional work in their audit_rule_free()
> hooks. Fix memory leaks by allowing the LSMs to perform necessary work.
>
> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> Cc: Janne Karhunen <janne.karhunen@gmail.com>
> ---
> security/integrity/ima/ima.h | 6 ++++++
> security/integrity/ima/ima_policy.c | 2 +-
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index df93ac258e01..de05d7f1d3ec 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
> #ifdef CONFIG_IMA_LSM_RULES
>
> #define security_filter_rule_init security_audit_rule_init
> +#define security_filter_rule_free security_audit_rule_free
> #define security_filter_rule_match security_audit_rule_match
In context this seems perfectly reasonable. If, however, you're
working with the LSM infrastructure this set of #defines is maddening.
The existing ones have been driving my nuts for the past few years,
so I'd like to discourage adding another. Since the security_filter_rule
functions are IMA specific they shouldn't be prefixed security_. I know
that it seems to be code churn/bikesheading, but we please change these:
static inline int ima_filter_rule_init(.....)
{
return security_audit_rule_init(.....);
}
and so forth. I understand if you don't want to make the change.
I have plenty of other things driving me crazy just now, so this
doesn't seem likely to push me over the edge.
>
> #else
> @@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
> return -EINVAL;
> }
>
> +static inline void security_filter_rule_free(void *lsmrule)
> +{
> + return -EINVAL;
> +}
> +
> static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
> void *lsmrule)
> {
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index e493063a3c34..236a731492d1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> int i;
>
> for (i = 0; i < MAX_LSM_RULES; i++) {
> - kfree(entry->lsm[i].rule);
> + security_filter_rule_free(entry->lsm[i].rule);
> kfree(entry->lsm[i].args_p);
> }
> kfree(entry);
^ permalink raw reply
* [PATCH 04/12] ima: Free the entire rule if it fails to parse
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
members, such as .fsname and .keyrings, when an error is encountered
during rule parsing.
Set the args_p pointer to NULL after freeing it in the error path of
ima_lsm_rule_init() so that it isn't freed twice.
This fixes a memory leak seen when loading an rule that contains an
additional piece of allocated memory, such as an fsname, followed by an
invalid conditional:
# echo "measure fsname=tmpfs bad=cond" > /sys/kernel/security/ima/policy
-bash: echo: write error: Invalid argument
# echo scan > /sys/kernel/debug/kmemleak
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff98e7e4ece6c0 (size 8):
comm "bash", pid 672, jiffies 4294791843 (age 21.855s)
hex dump (first 8 bytes):
74 6d 70 66 73 00 6b a5 tmpfs.k.
backtrace:
[<00000000abab7413>] kstrdup+0x2e/0x60
[<00000000f11ede32>] ima_parse_add_rule+0x7d4/0x1020
[<00000000f883dd7a>] ima_write_policy+0xab/0x1d0
[<00000000b17cf753>] vfs_write+0xde/0x1d0
[<00000000b8ddfdea>] ksys_write+0x68/0xe0
[<00000000b8e21e87>] do_syscall_64+0x56/0xa0
[<0000000089ea7b98>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 94ca3b8abb69..ee5152ecd3d9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -919,6 +919,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
if (ima_rules == &ima_default_rules) {
kfree(entry->lsm[lsm_rule].args_p);
+ entry->lsm[lsm_rule].args_p = NULL;
result = -EINVAL;
} else
result = 0;
@@ -1410,7 +1411,7 @@ ssize_t ima_parse_add_rule(char *rule)
result = ima_parse_rule(p, entry);
if (result) {
- kfree(entry);
+ ima_free_rule(entry);
integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL,
NULL, op, "invalid-policy", result,
audit_info);
--
2.25.1
^ permalink raw reply related
* [PATCH 05/12] ima: Fail rule parsing when buffer hook functions have an invalid action
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Buffer based hook functions, such as KEXEC_CMDLINE and KEY_CHECK, can
only measure. The process_buffer_measurement() function quietly ignores
all actions except measure so make this behavior clear at the time of
policy load.
The parsing of the keyrings conditional had a check to ensure that it
was only specified with measure actions but the check should be on the
hook function and not the keyrings conditional since
"appraise func=KEY_CHECK" is not a valid rule.
Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 36 +++++++++++++++++++++++++++--
1 file changed, 34 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ee5152ecd3d9..ecc234b956a2 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -979,6 +979,39 @@ static void check_template_modsig(const struct ima_template_desc *template)
#undef MSG
}
+static bool ima_validate_rule(struct ima_rule_entry *entry)
+{
+ if (entry->action == UNKNOWN)
+ return false;
+
+ if (entry->flags & IMA_FUNC) {
+ switch (entry->func) {
+ case NONE:
+ case FILE_CHECK:
+ case MMAP_CHECK:
+ case BPRM_CHECK:
+ case CREDS_CHECK:
+ case POST_SETATTR:
+ case MODULE_CHECK:
+ case FIRMWARE_CHECK:
+ case KEXEC_KERNEL_CHECK:
+ case KEXEC_INITRAMFS_CHECK:
+ case POLICY_CHECK:
+ break;
+ case KEXEC_CMDLINE:
+ case KEY_CHECK:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ break;
+ default:
+ return false;
+ }
+ }
+
+ return true;
+}
+
static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
{
struct audit_buffer *ab;
@@ -1156,7 +1189,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
keyrings_len = strlen(args[0].from) + 1;
if ((entry->keyrings) ||
- (entry->action != MEASURE) ||
(entry->func != KEY_CHECK) ||
(keyrings_len < 2)) {
result = -EINVAL;
@@ -1362,7 +1394,7 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
break;
}
}
- if (!result && (entry->action == UNKNOWN))
+ if (!result && !ima_validate_rule(entry))
result = -EINVAL;
else if (entry->action == APPRAISE)
temp_ima_appraise |= ima_appraise_flag(entry->func);
--
2.25.1
^ permalink raw reply related
* [PATCH 07/12] ima: Fail rule parsing when the KEY_CHECK hook is combined with an invalid cond
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
The KEY_CHECK function only supports the uid, pcr, and keyrings
conditionals. Make this clear at policy load so that IMA policy authors
don't assume that other conditionals are supported.
Fixes: 5808611cccb2 ("IMA: Add KEY_CHECK func to measure keys")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 83975ad22907..e33347148aa9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1024,6 +1024,13 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
+ if (entry->flags & ~(IMA_FUNC | IMA_UID | IMA_PCR |
+ IMA_KEYRINGS))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
break;
default:
return false;
--
2.25.1
^ permalink raw reply related
* [PATCH 08/12] ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
The args_p member is a simple string that is allocated by
ima_rule_init(). Shallow copy it like other non-LSM references in
ima_rule_entry structs.
There are no longer any necessary error path cleanups to do in
ima_lsm_copy_rule() so reference ownership from entry to nentry becomes
easier.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e33347148aa9..e9c7d318fdd4 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -306,10 +306,8 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
continue;
nentry->lsm[i].type = entry->lsm[i].type;
- nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p,
- GFP_KERNEL);
- if (!nentry->lsm[i].args_p)
- goto out_err;
+ nentry->lsm[i].args_p = entry->lsm[i].args_p;
+ entry->lsm[i].args_p = NULL;
security_filter_rule_init(nentry->lsm[i].type,
Audit_equal,
@@ -325,13 +323,6 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
entry->keyrings = NULL;
entry->template = NULL;
return nentry;
-
-out_err:
- nentry->fsname = NULL;
- nentry->keyrings = NULL;
- nentry->template = NULL;
- ima_free_rule(nentry);
- return NULL;
}
static int ima_lsm_update_rule(struct ima_rule_entry *entry)
--
2.25.1
^ permalink raw reply related
* [PATCH 09/12] ima: Use correct type for the args_p member of ima_rule_entry.lsm elements
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Make args_p be of the char pointer type rather than have it be a void
pointer that gets casted to char pointer when it is used. It is a simple
NUL-terminated string as returned by match_strdup().
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e9c7d318fdd4..514baf24d6a5 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -74,7 +74,7 @@ struct ima_rule_entry {
int pcr;
struct {
void *rule; /* LSM file metadata specific */
- void *args_p; /* audit value */
+ char *args_p; /* audit value */
int type; /* audit type */
} lsm[MAX_LSM_RULES];
char *fsname;
@@ -315,7 +315,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
&nentry->lsm[i].rule);
if (!nentry->lsm[i].rule)
pr_warn("rule for LSM \'%s\' is undefined\n",
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
}
/* Disown all references that were shallow copied */
@@ -917,7 +917,7 @@ static int ima_lsm_rule_init(struct ima_rule_entry *entry,
&entry->lsm[lsm_rule].rule);
if (!entry->lsm[lsm_rule].rule) {
pr_warn("rule for LSM \'%s\' is undefined\n",
- (char *)entry->lsm[lsm_rule].args_p);
+ entry->lsm[lsm_rule].args_p);
if (ima_rules == &ima_default_rules) {
kfree(entry->lsm[lsm_rule].args_p);
@@ -1666,27 +1666,27 @@ int ima_policy_show(struct seq_file *m, void *v)
switch (i) {
case LSM_OBJ_USER:
seq_printf(m, pt(Opt_obj_user),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_OBJ_ROLE:
seq_printf(m, pt(Opt_obj_role),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_OBJ_TYPE:
seq_printf(m, pt(Opt_obj_type),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_USER:
seq_printf(m, pt(Opt_subj_user),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_ROLE:
seq_printf(m, pt(Opt_subj_role),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
case LSM_SUBJ_TYPE:
seq_printf(m, pt(Opt_subj_type),
- (char *)entry->lsm[i].args_p);
+ entry->lsm[i].args_p);
break;
}
seq_puts(m, " ");
--
2.25.1
^ permalink raw reply related
* [PATCH 12/12] ima: Support additional conditionals in the KEXEC_CMDLINE hook function
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Eric Biederman, kexec
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Take the properties of the kexec kernel's inode and the current task
ownership into consideration when matching a KEXEC_CMDLINE operation to
the rules in the IMA policy. This allows for some uniformity when
writing IMA policy rules for KEXEC_KERNEL_CHECK, KEXEC_INITRAMFS_CHECK,
and KEXEC_CMDLINE operations.
Prior to this patch, it was not possible to write a set of rules like
this:
dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
measure func=KEXEC_KERNEL_CHECK
measure func=KEXEC_INITRAMFS_CHECK
measure func=KEXEC_CMDLINE
The inode information associated with the kernel being loaded by a
kexec_kernel_load(2) syscall can now be included in the decision to
measure or not
Additonally, the uid, euid, and subj_* conditionals can also now be
used in KEXEC_CMDLINE rules. There was no technical reason as to why
those conditionals weren't being considered previously other than
ima_match_rules() didn't have a valid inode to use so it immediately
bailed out for KEXEC_CMDLINE operations rather than going through the
full list of conditional comparisons.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: kexec@lists.infradead.org
---
include/linux/ima.h | 4 ++--
kernel/kexec_file.c | 2 +-
security/integrity/ima/ima.h | 3 ++-
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 24 +++++++++++++++-----
security/integrity/ima/ima_policy.c | 17 +++++---------
security/integrity/ima/ima_queue_keys.c | 2 +-
9 files changed, 33 insertions(+), 25 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 9164e1534ec9..d15100de6cdd 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -25,7 +25,7 @@ extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
enum kernel_read_file_id id);
extern void ima_post_path_mknod(struct dentry *dentry);
extern int ima_file_hash(struct file *file, char *buf, size_t buf_size);
-extern void ima_kexec_cmdline(const void *buf, int size);
+extern void ima_kexec_cmdline(int kernel_fd, const void *buf, int size);
#ifdef CONFIG_IMA_KEXEC
extern void ima_add_kexec_buffer(struct kimage *image);
@@ -103,7 +103,7 @@ static inline int ima_file_hash(struct file *file, char *buf, size_t buf_size)
return -EOPNOTSUPP;
}
-static inline void ima_kexec_cmdline(const void *buf, int size) {}
+static inline void ima_kexec_cmdline(int kernel_fd, const void *buf, int size) {}
#endif /* CONFIG_IMA */
#ifndef CONFIG_IMA_KEXEC
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index bb05fd52de85..07df431c1f21 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -287,7 +287,7 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
goto out;
}
- ima_kexec_cmdline(image->cmdline_buf,
+ ima_kexec_cmdline(kernel_fd, image->cmdline_buf,
image->cmdline_buf_len - 1);
}
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index de05d7f1d3ec..ed9307dd0e60 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -251,7 +251,8 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
struct ima_template_desc *template_desc);
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr, const char *keyring);
+ int pcr, struct inode *inode,
+ const char *keyring);
void ima_audit_measurement(struct integrity_iint_cache *iint,
const unsigned char *filename);
int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index bf22de8b7ce0..4f39fb93f278 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -162,7 +162,7 @@ void ima_add_violation(struct file *file, const unsigned char *filename,
/**
* ima_get_action - appraise & measure decision based on policy.
- * @inode: pointer to inode to measure
+ * @inode: pointer to the inode associated with the object being validated
* @cred: pointer to credentials structure to validate
* @secid: secid of the task being validated
* @mask: contains the permission mask (MAY_READ, MAY_WRITE, MAY_EXEC,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9649b04b9f1..0c11aeefea24 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -330,7 +330,7 @@ int ima_check_blacklist(struct integrity_iint_cache *iint,
if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
process_buffer_measurement(digest, digestsize,
"blacklisted-hash", NONE,
- pcr, NULL);
+ pcr, NULL, NULL);
}
return rc;
diff --git a/security/integrity/ima/ima_asymmetric_keys.c b/security/integrity/ima/ima_asymmetric_keys.c
index aaae80c4e376..585b64557094 100644
--- a/security/integrity/ima/ima_asymmetric_keys.c
+++ b/security/integrity/ima/ima_asymmetric_keys.c
@@ -60,5 +60,5 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
*/
process_buffer_measurement(payload, payload_len,
keyring->description, KEY_CHECK, 0,
- keyring->description);
+ NULL, keyring->description);
}
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index c1583d98c5e5..82acd66bf653 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -731,13 +731,15 @@ int ima_load_data(enum kernel_load_data_id id)
* @eventname: event name to be used for the buffer entry.
* @func: IMA hook
* @pcr: pcr to extend the measurement
+ * @inode: inode associated with the object being measured (NULL for KEY_CHECK)
* @keyring: keyring name to determine the action to be performed
*
* Based on policy, the buffer is measured into the ima log.
*/
void process_buffer_measurement(const void *buf, int size,
const char *eventname, enum ima_hooks func,
- int pcr, const char *keyring)
+ int pcr, struct inode *inode,
+ const char *keyring)
{
int ret = 0;
struct ima_template_entry *entry = NULL;
@@ -767,7 +769,7 @@ void process_buffer_measurement(const void *buf, int size,
*/
if (func) {
security_task_getsecid(current, &secid);
- action = ima_get_action(NULL, current_cred(), secid, 0, func,
+ action = ima_get_action(inode, current_cred(), secid, 0, func,
&pcr, &template, keyring);
if (!(action & IMA_MEASURE))
return;
@@ -815,16 +817,26 @@ void process_buffer_measurement(const void *buf, int size,
/**
* ima_kexec_cmdline - measure kexec cmdline boot args
+ * @kernel_fd: file descriptor of the kexec kernel being loaded
* @buf: pointer to buffer
* @size: size of buffer
*
* Buffers can only be measured, not appraised.
*/
-void ima_kexec_cmdline(const void *buf, int size)
+void ima_kexec_cmdline(int kernel_fd, const void *buf, int size)
{
- if (buf && size != 0)
- process_buffer_measurement(buf, size, "kexec-cmdline",
- KEXEC_CMDLINE, 0, NULL);
+ struct fd f;
+
+ if (!buf || !size)
+ return;
+
+ f = fdget(kernel_fd);
+ if (!f.file)
+ return;
+
+ process_buffer_measurement(buf, size, "kexec-cmdline", KEXEC_CMDLINE, 0,
+ file_inode(f.file), NULL);
+ fdput(f);
}
static int __init init_ima(void)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 0ca9902287bf..5a6aee530011 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -442,13 +442,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
{
int i;
- if ((func == KEXEC_CMDLINE) || (func == KEY_CHECK)) {
- if ((rule->flags & IMA_FUNC) && (rule->func == func)) {
- if (func == KEY_CHECK)
- return ima_match_keyring(rule, keyring, cred);
- return true;
- }
- return false;
+ if (func == KEY_CHECK) {
+ return (rule->flags & IMA_FUNC) && (rule->func == func) &&
+ ima_match_keyring(rule, keyring, cred);
}
if ((rule->flags & IMA_FUNC) &&
(rule->func != func && func != POST_SETATTR))
@@ -1003,10 +999,9 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
- if (entry->flags & ~(IMA_FUNC | IMA_PCR))
- return false;
-
- if (ima_rule_contains_lsm_cond(entry))
+ if (entry->flags & ~(IMA_FUNC | IMA_FSMAGIC | IMA_UID |
+ IMA_FOWNER | IMA_FSUUID |
+ IMA_EUID | IMA_PCR | IMA_FSNAME))
return false;
break;
diff --git a/security/integrity/ima/ima_queue_keys.c b/security/integrity/ima/ima_queue_keys.c
index cb3e3f501593..7c69d7397832 100644
--- a/security/integrity/ima/ima_queue_keys.c
+++ b/security/integrity/ima/ima_queue_keys.c
@@ -156,7 +156,7 @@ void ima_process_queued_keys(void)
process_buffer_measurement(entry->payload,
entry->payload_len,
entry->keyring_name,
- KEY_CHECK, 0,
+ KEY_CHECK, 0, NULL,
entry->keyring_name);
list_del(&entry->list);
ima_free_key_entry(entry);
--
2.25.1
^ permalink raw reply related
* [PATCH 11/12] ima: Use the common function to detect LSM conditionals in a rule
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Make broader use of ima_rule_contains_lsm_cond() to check if a given
rule contains an LSM conditional. This is a code cleanup and has no
user-facing change.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ae2ec2a9cdb9..0ca9902287bf 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -359,17 +359,10 @@ static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
static void ima_lsm_update_rules(void)
{
struct ima_rule_entry *entry, *e;
- int i, result, needs_update;
+ int result;
list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
- needs_update = 0;
- for (i = 0; i < MAX_LSM_RULES; i++) {
- if (entry->lsm[i].args_p) {
- needs_update = 1;
- break;
- }
- }
- if (!needs_update)
+ if (!ima_rule_contains_lsm_cond(entry))
continue;
result = ima_lsm_update_rule(entry);
--
2.25.1
^ permalink raw reply related
* [PATCH 10/12] ima: Move validation of the keyrings conditional into ima_validate_rule()
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Use ima_validate_rule() to ensure that the combination of a hook
function and the keyrings conditional is valid and that the keyrings
conditional is not specified without an explicit KEY_CHECK func
conditional. This is a code cleanup and has no user-facing change.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 514baf24d6a5..ae2ec2a9cdb9 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -999,6 +999,12 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
case KEXEC_KERNEL_CHECK:
case KEXEC_INITRAMFS_CHECK:
case POLICY_CHECK:
+ if (entry->flags & ~(IMA_FUNC | IMA_MASK | IMA_FSMAGIC |
+ IMA_UID | IMA_FOWNER | IMA_FSUUID |
+ IMA_INMASK | IMA_EUID | IMA_PCR |
+ IMA_FSNAME))
+ return false;
+
break;
case KEXEC_CMDLINE:
if (entry->action & ~(MEASURE | DONT_MEASURE))
@@ -1026,7 +1032,8 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
default:
return false;
}
- }
+ } else if (entry->flags & IMA_KEYRINGS)
+ return false;
return true;
}
@@ -1208,7 +1215,6 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
keyrings_len = strlen(args[0].from) + 1;
if ((entry->keyrings) ||
- (entry->func != KEY_CHECK) ||
(keyrings_len < 2)) {
result = -EINVAL;
break;
--
2.25.1
^ permalink raw reply related
* [PATCH 06/12] ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an invalid cond
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
The KEXEC_CMDLINE hook function only supports the pcr conditional. Make
this clear at policy load so that IMA policy authors don't assume that
other conditionals are supported.
Since KEXEC_CMDLINE's inception, ima_match_rules() has always returned
true on any loaded KEXEC_CMDLINE rule without any consideration for
other conditionals present in the rule. Make it clear that pcr is the
only supported KEXEC_CMDLINE conditional by returning an error during
policy load.
An example of why this is a problem can be explained with the following
rule:
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
An IMA policy author would have assumed that rule is valid because the
parser accepted it but the result was that measurements for all
KEXEC_CMDLINE operations would be disabled.
Fixes: b0935123a183 ("IMA: Define a new hook to measure the kexec boot command line arguments")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ecc234b956a2..83975ad22907 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -349,6 +349,17 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
return 0;
}
+static bool ima_rule_contains_lsm_cond(struct ima_rule_entry *entry)
+{
+ int i;
+
+ for (i = 0; i < MAX_LSM_RULES; i++)
+ if (entry->lsm[i].args_p)
+ return true;
+
+ return false;
+}
+
/*
* The LSM policy can be reloaded, leaving the IMA LSM based rules referring
* to the old, stale LSM policy. Update the IMA LSM based rules to reflect
@@ -999,6 +1010,16 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
case POLICY_CHECK:
break;
case KEXEC_CMDLINE:
+ if (entry->action & ~(MEASURE | DONT_MEASURE))
+ return false;
+
+ if (entry->flags & ~(IMA_FUNC | IMA_PCR))
+ return false;
+
+ if (ima_rule_contains_lsm_cond(entry))
+ return false;
+
+ break;
case KEY_CHECK:
if (entry->action & ~(MEASURE | DONT_MEASURE))
return false;
--
2.25.1
^ permalink raw reply related
* [PATCH 03/12] ima: Free the entire rule when deleting a list of rules
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Use ima_free_rule() to fix memory leaks of allocated ima_rule_entry
members, such as .fsname and .keyrings, when deleting a list of rules.
This fixes a memory leak seen when loading by a valid rule that contains
an additional piece of allocated memory, such as an fsname, followed by
an invalid rule that triggers a policy load failure:
# echo -e "dont_measure fsname=securityfs\nbad syntax" > \
/sys/kernel/security/ima/policy
-bash: echo: write error: Invalid argument
# echo scan > /sys/kernel/debug/kmemleak
# cat /sys/kernel/debug/kmemleak
unreferenced object 0xffff9bab67ca12c0 (size 16):
comm "tee", pid 684, jiffies 4295212803 (age 252.344s)
hex dump (first 16 bytes):
73 65 63 75 72 69 74 79 66 73 00 6b 6b 6b 6b a5 securityfs.kkkk.
backtrace:
[<00000000adc80b1b>] kstrdup+0x2e/0x60
[<00000000d504cb0d>] ima_parse_add_rule+0x7d4/0x1020
[<00000000444825ac>] ima_write_policy+0xab/0x1d0
[<000000002b7f0d6c>] vfs_write+0xde/0x1d0
[<0000000096feedcf>] ksys_write+0x68/0xe0
[<0000000052b544a2>] do_syscall_64+0x56/0xa0
[<000000007ead1ba7>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
Fixes: f1b08bbcbdaf ("ima: define a new policy condition based on the filesystem name")
Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 1320333201c6..94ca3b8abb69 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1431,15 +1431,11 @@ ssize_t ima_parse_add_rule(char *rule)
void ima_delete_rules(void)
{
struct ima_rule_entry *entry, *tmp;
- int i;
temp_ima_appraise = 0;
list_for_each_entry_safe(entry, tmp, &ima_temp_rules, list) {
- for (i = 0; i < MAX_LSM_RULES; i++)
- kfree(entry->lsm[i].args_p);
-
list_del(&entry->list);
- kfree(entry);
+ ima_free_rule(entry);
}
}
--
2.25.1
^ permalink raw reply related
* [PATCH 02/12] ima: Create a function to free a rule entry
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
There are several possible pieces of allocated memory in a rule entry.
Create a function that can free all allocated memory for a given rule
entry.
This patch introduces no functional changes but sets the groundwork for
some memory leak fixes.
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
security/integrity/ima/ima_policy.c | 33 +++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 236a731492d1..1320333201c6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -261,6 +261,27 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
security_filter_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
+}
+
+static void ima_free_rule(struct ima_rule_entry *entry)
+{
+ if (!entry)
+ return;
+
+ /*
+ * entry->template->fields may be allocated in ima_parse_rule() but that
+ * reference is owned by the corresponding ima_template_desc element in
+ * the defined_templates list and cannot be freed here
+ */
+
+ /*
+ * When freeing newly added ima_rule_entry members, consider if you
+ * need to disown any references after the shallow copy in
+ * ima_lsm_copy_rule()
+ */
+ kfree(entry->fsname);
+ kfree(entry->keyrings);
+ ima_lsm_free_rule(entry);
kfree(entry);
}
@@ -298,10 +319,18 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
pr_warn("rule for LSM \'%s\' is undefined\n",
(char *)entry->lsm[i].args_p);
}
+
+ /* Disown all references that were shallow copied */
+ entry->fsname = NULL;
+ entry->keyrings = NULL;
+ entry->template = NULL;
return nentry;
out_err:
- ima_lsm_free_rule(nentry);
+ nentry->fsname = NULL;
+ nentry->keyrings = NULL;
+ nentry->template = NULL;
+ ima_free_rule(nentry);
return NULL;
}
@@ -315,7 +344,7 @@ static int ima_lsm_update_rule(struct ima_rule_entry *entry)
list_replace_rcu(&entry->list, &nentry->list);
synchronize_rcu();
- ima_lsm_free_rule(entry);
+ ima_free_rule(entry);
return 0;
}
--
2.25.1
^ permalink raw reply related
* [PATCH 01/12] ima: Have the LSM free its audit rule
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen
In-Reply-To: <20200623003236.830149-1-tyhicks@linux.microsoft.com>
Ask the LSM to free its audit rule rather than directly calling kfree().
Both AppArmor and SELinux do additional work in their audit_rule_free()
hooks. Fix memory leaks by allowing the LSMs to perform necessary work.
Fixes: b16942455193 ("ima: use the lsm policy update notifier")
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: Janne Karhunen <janne.karhunen@gmail.com>
---
security/integrity/ima/ima.h | 6 ++++++
security/integrity/ima/ima_policy.c | 2 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df93ac258e01..de05d7f1d3ec 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -404,6 +404,7 @@ static inline void ima_free_modsig(struct modsig *modsig)
#ifdef CONFIG_IMA_LSM_RULES
#define security_filter_rule_init security_audit_rule_init
+#define security_filter_rule_free security_audit_rule_free
#define security_filter_rule_match security_audit_rule_match
#else
@@ -414,6 +415,11 @@ static inline int security_filter_rule_init(u32 field, u32 op, char *rulestr,
return -EINVAL;
}
+static inline void security_filter_rule_free(void *lsmrule)
+{
+ return -EINVAL;
+}
+
static inline int security_filter_rule_match(u32 secid, u32 field, u32 op,
void *lsmrule)
{
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index e493063a3c34..236a731492d1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -258,7 +258,7 @@ static void ima_lsm_free_rule(struct ima_rule_entry *entry)
int i;
for (i = 0; i < MAX_LSM_RULES; i++) {
- kfree(entry->lsm[i].rule);
+ security_filter_rule_free(entry->lsm[i].rule);
kfree(entry->lsm[i].args_p);
}
kfree(entry);
--
2.25.1
^ permalink raw reply related
* [PATCH 00/12] ima: Fix rule parsing bugs and extend KEXEC_CMDLINE rule support
From: Tyler Hicks @ 2020-06-23 0:32 UTC (permalink / raw)
To: Mimi Zohar, Dmitry Kasatkin
Cc: James Morris, Serge E . Hallyn, Lakshmi Ramasubramanian,
Prakhar Srivastava, linux-kernel, linux-integrity,
linux-security-module, Janne Karhunen, Eric Biederman, kexec
This series ultimately extends the supported IMA rule conditionals for
the KEXEC_CMDLINE hook function. As of today, there's an imbalance in
IMA language conditional support for KEXEC_CMDLINE rules in comparison
to KEXEC_KERNEL_CHECK and KEXEC_INITRAMFS_CHECK rules. The KEXEC_CMDLINE
rules do not support *any* conditionals so you cannot have a sequence of
rules like this:
dont_measure func=KEXEC_KERNEL_CHECK obj_type=foo_t
dont_measure func=KEXEC_INITRAMFS_CHECK obj_type=foo_t
dont_measure func=KEXEC_CMDLINE obj_type=foo_t
measure func=KEXEC_KERNEL_CHECK
measure func=KEXEC_INITRAMFS_CHECK
measure func=KEXEC_CMDLINE
Instead, KEXEC_CMDLINE rules can only be measured or not measured and
there's no additional flexibility in today's implementation of the
KEXEC_CMDLINE hook function.
With this series, the above sequence of rules becomes valid and any
calls to kexec_file_load() with a kernel and initramfs inode type of
foo_t will not be measured (that includes the kernel cmdline buffer)
while all other objects given to a kexec_file_load() syscall will be
measured. There's obviously not an inode directly associated with the
kernel cmdline buffer but this patch series ties the inode based
decision making for KEXEC_CMDLINE to the kernel's inode. I think this
will be intuitive to policy authors.
While reading IMA code and preparing to make this change, I realized
that the buffer based hook functions (KEXEC_CMDLINE and KEY_CHECK) are
quite special in comparison to longer standing hook functions. These
buffer based hook functions can only support measure actions and there
are some restrictions on the conditionals that they support. However,
the rule parser isn't enforcing any of those restrictions and IMA policy
authors wouldn't have any immediate way of knowing that the policy that
they wrote is invalid. For example, the sequence of rules above parses
successfully in today's kernel but the
"dont_measure func=KEXEC_CMDLINE ..." rule is incorrectly handled in
ima_match_rules(). The dont_measure rule is *always* considered to be a
match so, surprisingly, no KEXEC_CMDLINE measurements are made.
While making the rule parser more strict, I realized that the parser
does not correctly free all of the allocated memory associated with an
ima_rule_entry when going down some error paths. Invalid policy loaded
by the policy administrator could result in small memory leaks.
I envision patches 1-7 going to stable. The series is ordered in a way
that has all the fixes up front, followed by cleanups, followed by the
feature patch. The breakdown of patches looks like so:
Memory leak fixes: 1-4
Parser strictness fixes: 5-7
Code cleanups made possible by the fixes: 8-11
Extend KEXEC_CMDLINE rule support: 12
Perhaps the most logical ordering for code review is:
1, 2, 3, 4, 8, 9, 5, 6, 7, 10, 11, 12
If you'd like me to re-order or split up the series, just let me know.
Thanks for considering these patches!
Tyler
Tyler Hicks (12):
ima: Have the LSM free its audit rule
ima: Create a function to free a rule entry
ima: Free the entire rule when deleting a list of rules
ima: Free the entire rule if it fails to parse
ima: Fail rule parsing when buffer hook functions have an invalid
action
ima: Fail rule parsing when the KEXEC_CMDLINE hook is combined with an
invalid cond
ima: Fail rule parsing when the KEY_CHECK hook is combined with an
invalid cond
ima: Shallow copy the args_p member of ima_rule_entry.lsm elements
ima: Use correct type for the args_p member of ima_rule_entry.lsm
elements
ima: Move validation of the keyrings conditional into
ima_validate_rule()
ima: Use the common function to detect LSM conditionals in a rule
ima: Support additional conditionals in the KEXEC_CMDLINE hook
function
include/linux/ima.h | 4 +-
kernel/kexec_file.c | 2 +-
security/integrity/ima/ima.h | 9 +-
security/integrity/ima/ima_api.c | 2 +-
security/integrity/ima/ima_appraise.c | 2 +-
security/integrity/ima/ima_asymmetric_keys.c | 2 +-
security/integrity/ima/ima_main.c | 24 ++-
security/integrity/ima/ima_policy.c | 159 ++++++++++++++-----
security/integrity/ima/ima_queue_keys.c | 2 +-
9 files changed, 148 insertions(+), 58 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH] ima: extend boot_aggregate with kernel measurements
From: Maurizio Drocco @ 2020-06-22 4:50 UTC (permalink / raw)
To: zohar
Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity,
linux-kernel, linux-security-module, maurizio.drocco, mdrocco,
roberto.sassu, serge
In-Reply-To: <1592856871.4987.21.camel@linux.ibm.com>
IMA is not considering TPM registers 8-9 when calculating the boot
aggregate. When registers 8-9 are used to store measurements of the
kernel and its command line (e.g., grub2 bootloader with tpm module
enabled), IMA should include them in the boot aggregate. Registers
8-9 are only included in non-SHA1 boot_aggregate digests to avoid
ambiguity.
Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_crypto.c | 15 ++++++++++++++-
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index df93ac258e01..9d94080bdad8 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -30,7 +30,7 @@
enum ima_show_type { IMA_SHOW_BINARY, IMA_SHOW_BINARY_NO_FIELD_LEN,
IMA_SHOW_BINARY_OLD_STRING_FMT, IMA_SHOW_ASCII };
-enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
+enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
/* digest size for IMA, fits SHA1 or MD5 */
#define IMA_DIGEST_SIZE SHA1_DIGEST_SIZE
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 220b14920c37..d02917d85033 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -823,13 +823,26 @@ static int ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
if (rc != 0)
return rc;
- /* cumulative sha1 over tpm registers 0-7 */
+ /* cumulative digest over tpm registers 0-7 */
for (i = TPM_PCR0; i < TPM_PCR8; i++) {
ima_pcrread(i, &d);
/* now accumulate with current aggregate */
rc = crypto_shash_update(shash, d.digest,
crypto_shash_digestsize(tfm));
}
+ /*
+ * extend cumulative digest over tpm registers 8-9, which contain
+ * measurement for the kernel command line (reg. 8) and image (reg. 9)
+ * in a typical PCR allocation. Registers 8-9 are only included in
+ * non-SHA1 boot_aggregate digests to avoid ambiguity.
+ */
+ if (alg_id != TPM_ALG_SHA1) {
+ for (i = TPM_PCR8; i < TPM_PCR10; i++) {
+ ima_pcrread(i, &d);
+ rc = crypto_shash_update(shash, d.digest,
+ crypto_shash_digestsize(tfm));
+ }
+ }
if (!rc)
crypto_shash_final(shash, digest);
return rc;
--
2.17.1
^ permalink raw reply related
* Re: LTP: crypto: af_alg02 regression on linux-next 20200621 tag
From: Luis Chamberlain @ 2020-06-22 22:49 UTC (permalink / raw)
To: Naresh Kamboju
Cc: LTP List, open list, linux-security-module, keyrings, lkft-triage,
linux-crypto, Herbert Xu, Jan Stancek, chrubis, Serge E. Hallyn,
James Morris, Jarkko Sakkinen, David Howells, David S. Miller,
Eric Biggers
In-Reply-To: <CA+G9fYvHFs5Yx8TnT6VavtfjMN8QLPuXg6us-dXVJqUUt68adA@mail.gmail.com>
On Tue, Jun 23, 2020 at 12:04:06AM +0530, Naresh Kamboju wrote:
> LTP crypto regressions noticed on linux next 20200621.
>
> The common case for all tests is timeout after 15 minutes which
> means tests got hung and LTP timers killed those test runs after
> timeout.
> The root cause of the failure is under investigation.
>
> ltp-crypto-tests:
> * af_alg02 - failed
> * af_alg05 - failed
> ltp-syscalls-tests:
> * keyctl07 - failed
> * request_key03 - failed
>
> Output log:
> --------------
> af_alg02:
> af_alg02.c:52: BROK: Timed out while reading from request socket.
>
> Test code at line number 52 is
>
> pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
> SAFE_PTHREAD_CREATE(&thr, NULL, verify_encrypt, NULL);
>
> TST_CHECKPOINT_WAIT(0);
>
> while (pthread_kill(thr, 0) != ESRCH) {
> if (tst_timeout_remaining() <= 10) {
> pthread_cancel(thr);
> tst_brk(TBROK,
> "Timed out while reading from request socket.");
>
>
> af_alg05:
> tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
> [ 362.599868] kworker/dying (137) used greatest stack depth: 11600 bytes left
> Test timeouted, sending SIGKILL!
> tst_test.c:1286: INFO: If you are running on slow machine, try
> exporting LTP_TIMEOUT_MUL > 1
> tst_test.c:1287: BROK: Test killed! (timeout?)
>
> request_key03:
> tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
> request_key03.c:65: CONF: kernel doesn't support key type 'encrypted'
> request_key03.c:65: CONF: kernel doesn't support key type 'trusted'
> Test timeouted, sending SIGKILL!
> tst_test.c:1286: INFO: If you are running on slow machine, try
> exporting LTP_TIMEOUT_MUL > 1
> tst_test.c:1287: BROK: Test killed! (timeout?)
>
> keyctl07
> tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
> Test timeouted, sending SIGKILL!
> tst_test.c:1286: INFO: If you are running on slow machine, try
> exporting LTP_TIMEOUT_MUL > 1
> tst_test.c:1287: BROK: Test killed! (timeout?)
>
> metadata:
> git branch: master
> git repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> git commit: 5a94f5bc041ea9e4d17c93b11ea6f6a2e5ad361b
> git describe: next-20200621
> kernel-config:
> https://builds.tuxbuild.com/PB-45Luvlx0yYJ8MZgpijA/kernel.config
>
> ref:
> https://lkft.validation.linaro.org/scheduler/job/1511938#L2211
> https://lkft.validation.linaro.org/scheduler/job/1511935#L9225
Can you try reverting:
d13ef8e10756873b0a8b7cc8f230a2d1026710ea
The patch is titled "umh: fix processed error when UMH_WAIT_PROC is used"
If this fixes the issue we have to ask ourselves then why, given that
anything other than 0 is a return code and prior to this patch we were
not accepting negative return codes, but we were never getting them
as the invalid return code was being returned when UMH_WAIT_PROC was
used and an error code was returned. So now, *any* non-zero value is
a return code. The onlything I can think of is that we may want to
special-case the -ENOMEM error code, or maybe some other one to not
skip it as in the below patch.
If reverting the commit does not fix the issue, this is not the droid
we are looking for.
diff --git a/security/keys/request_key.c b/security/keys/request_key.c
index ff462f3d46ca..94ed23a8991f 100644
--- a/security/keys/request_key.c
+++ b/security/keys/request_key.c
@@ -193,7 +193,7 @@ static int call_sbin_request_key(struct key *authkey, void *aux)
ret = call_usermodehelper_keys(request_key, argv, envp, keyring,
UMH_WAIT_PROC);
kdebug("usermode -> 0x%x", ret);
- if (ret != 0) {
+ if (ret != -ENOMEM && ret != 0) {
/* ret is the exit/wait code */
if (test_bit(KEY_FLAG_USER_CONSTRUCT, &key->flags) ||
key_validate(key) < 0)
^ permalink raw reply related
* Re: [PATCH] ima_evm_utils: extended calc_bootaggr to PCRs 8 - 9
From: Mimi Zohar @ 2020-06-22 20:14 UTC (permalink / raw)
To: Maurizio Drocco, roberto.sassu
Cc: Silviu.Vlasceanu, dmitry.kasatkin, jejb, jmorris, linux-integrity,
linux-kernel, linux-security-module, serge, mdrocco
In-Reply-To: <20200618201126.2081-2-maurizio.drocco@ibm.com>
On Thu, 2020-06-18 at 16:11 -0400, Maurizio Drocco wrote:
> From: Maurizio <maurizio.drocco@ibm.com>
>
> If PCRs 8 - 9 are set (i.e. not all-zeros), cal_bootaggr should include
> them into the digest.
>
> Signed-off-by: Maurizio Drocco <maurizio.drocco@ibm.com>
> ---
> src/evmctl.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/src/evmctl.c b/src/evmctl.c
> index 1d065ce..554571e 100644
> --- a/src/evmctl.c
> +++ b/src/evmctl.c
> @@ -1930,6 +1930,18 @@ static void calc_bootaggr(struct tpm_bank_info *bank)
> }
> }
>
> + if (strcmp(bank->algo_name, "sha1") != 0) {
> + for (i = 8; i < 10; i++) {
> + if (memcmp(bank->pcr[i], zero, bank->digest_size) != 0) {
> + err = EVP_DigestUpdate(pctx, bank->pcr[i], bank->digest_size);
> + if (!err) {
> + log_err("EVP_DigestUpdate() failed\n");
> + return;
> + }
> + }
> + }
> + }
Roberto, now that we're only including the PCRs 8 & 9 in the non-sha1
"boot_aggregate", they can always be included.
Please reflect this change in the patch description and, here, in the
code.
thanks,
Mimi
^ permalink raw reply
* LTP: crypto: af_alg02 regression on linux-next 20200621 tag
From: Naresh Kamboju @ 2020-06-22 18:34 UTC (permalink / raw)
To: LTP List, open list, linux-security-module, keyrings, lkft-triage,
linux-crypto
Cc: Herbert Xu, Jan Stancek, chrubis, Serge E. Hallyn, James Morris,
Jarkko Sakkinen, David Howells, Luis Chamberlain, David S. Miller,
Eric Biggers
LTP crypto regressions noticed on linux next 20200621.
The common case for all tests is timeout after 15 minutes which
means tests got hung and LTP timers killed those test runs after
timeout.
The root cause of the failure is under investigation.
ltp-crypto-tests:
* af_alg02 - failed
* af_alg05 - failed
ltp-syscalls-tests:
* keyctl07 - failed
* request_key03 - failed
Output log:
--------------
af_alg02:
af_alg02.c:52: BROK: Timed out while reading from request socket.
Test code at line number 52 is
pthread_setcancelstate(PTHREAD_CANCEL_ENABLE, NULL);
SAFE_PTHREAD_CREATE(&thr, NULL, verify_encrypt, NULL);
TST_CHECKPOINT_WAIT(0);
while (pthread_kill(thr, 0) != ESRCH) {
if (tst_timeout_remaining() <= 10) {
pthread_cancel(thr);
tst_brk(TBROK,
"Timed out while reading from request socket.");
af_alg05:
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
[ 362.599868] kworker/dying (137) used greatest stack depth: 11600 bytes left
Test timeouted, sending SIGKILL!
tst_test.c:1286: INFO: If you are running on slow machine, try
exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1287: BROK: Test killed! (timeout?)
request_key03:
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
request_key03.c:65: CONF: kernel doesn't support key type 'encrypted'
request_key03.c:65: CONF: kernel doesn't support key type 'trusted'
Test timeouted, sending SIGKILL!
tst_test.c:1286: INFO: If you are running on slow machine, try
exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1287: BROK: Test killed! (timeout?)
keyctl07
tst_test.c:1246: INFO: Timeout per run is 0h 15m 00s
Test timeouted, sending SIGKILL!
tst_test.c:1286: INFO: If you are running on slow machine, try
exporting LTP_TIMEOUT_MUL > 1
tst_test.c:1287: BROK: Test killed! (timeout?)
metadata:
git branch: master
git repo: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
git commit: 5a94f5bc041ea9e4d17c93b11ea6f6a2e5ad361b
git describe: next-20200621
kernel-config:
https://builds.tuxbuild.com/PB-45Luvlx0yYJ8MZgpijA/kernel.config
ref:
https://lkft.validation.linaro.org/scheduler/job/1511938#L2211
https://lkft.validation.linaro.org/scheduler/job/1511935#L9225
- Naresh
^ permalink raw reply
* Re: [PATCH] security: fix the key_permission LSM hook function type
From: James Morris @ 2020-06-22 17:38 UTC (permalink / raw)
To: Sami Tolvanen
Cc: David Howells, James Morris, Kees Cook, Eric W. Biederman,
KP Singh, Casey Schaufler, Thomas Cedeno, Anders Roxell,
linux-kernel, linux-security-module
In-Reply-To: <20200615181232.119491-1-samitolvanen@google.com>
On Mon, 15 Jun 2020, Sami Tolvanen wrote:
> Commit 8c0637e950d6 ("keys: Make the KEY_NEED_* perms an enum rather than
> a mask") changed the type of the key_permission callback functions, but
> didn't change the type of the hook, which trips indirect call checking with
> Control-Flow Integrity (CFI). This change fixes the issue by changing the
> hook type to match the functions.
>
> Fixes: 8c0637e950d6 ("keys: Make the KEY_NEED_* perms an enum rather than a mask")
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git fixes-v5.8
NOTE: please cc: the LSM list with patches such as these.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [RFC PATCH] security: Add a config option to disable security mitigations
From: Qais Yousef @ 2020-06-22 12:32 UTC (permalink / raw)
To: Pranith Kumar
Cc: James Morris, Serge E. Hallyn, Thomas Gleixner, Peter Zijlstra,
Ingo Molnar, Paolo Bonzini, Arnd Bergmann, Tyler Hicks, open list,
open list:SECURITY SUBSYSTEM
In-Reply-To: <20200618010755.4179-1-bobby.prani@gmail.com>
On 06/17/20 18:07, Pranith Kumar wrote:
> Instead of having to pass 'mitigations=off' on the kernel command line,
> add a config option that has a similar effect.
>
> Adding this makes it easier to disable mitigations in scenarios where
> you cannot modify the command line or are unable to pass a command line
> while booting.
Better wait to hear from others too, but I think if you want a config, then it
better support all possible variations of the option too, not just the 'off'
one.
>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
> kernel/cpu.c | 2 +-
> security/Kconfig | 8 ++++++++
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/cpu.c b/kernel/cpu.c
> index 6ff2578ecf17..584eb39585d6 100644
> --- a/kernel/cpu.c
> +++ b/kernel/cpu.c
> @@ -2542,7 +2542,7 @@ early_param("mitigations", mitigations_parse_cmdline);
> /* mitigations=off */
> bool cpu_mitigations_off(void)
> {
> - return cpu_mitigations == CPU_MITIGATIONS_OFF;
> + return cpu_mitigations == CPU_MITIGATIONS_OFF || IS_ENABLED(CONFIG_DISABLE_MITIGATIONS);
> }
So if cmdline asked for this to be auto, but the config is compiled with off
then the config always wins according to this.
This conflict makes me think this is probably going to be a bad idea because
2 points of control would create a confusion of which one should be honored.
You can set the cmdline in the config too, have you tried this?
On arm64, I can add additional cmdline paramter in the config
Boot options --> Default kernel command string
On x86 I found this too
Processor type and features --> Built-in kernel command line
Beside that, you can always use kexec to boot a new kernel with extra cmdline
option
The command I used in the past (from memory, so worth double checking)
kexec --command-line "$(cat /proc/cmdline) migrations=off" -f Image
> EXPORT_SYMBOL_GPL(cpu_mitigations_off);
>
> diff --git a/security/Kconfig b/security/Kconfig
> index cd3cc7da3a55..90b8e9c89a6d 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -65,6 +65,14 @@ config PAGE_TABLE_ISOLATION
>
> See Documentation/x86/pti.rst for more details.
>
> +config DISABLE_MITIGATIONS
> + bool "Disable kernel security mitigations"
> + default n
No need for default n in general as it is the default anyway :)
HTH.
Thanks
--
Qais Yousef
^ permalink raw reply
* Re: [PATCH bpf-next v2 2/4] bpf: Implement bpf_local_storage for inodes
From: Quentin Monnet @ 2020-06-22 9:40 UTC (permalink / raw)
To: KP Singh, bpf, linux-security-module
Cc: Alexei Starovoitov, Daniel Borkmann, Paul Turner, Jann Horn
In-Reply-To: <20200617202941.3034-3-kpsingh@chromium.org>
2020-06-17 22:29 UTC+0200 ~ KP Singh <kpsingh@chromium.org>
> From: KP Singh <kpsingh@google.com>
>
> Similar to bpf_local_storage for sockets, add local storage for inodes.
> The life-cycle of storage is managed with the life-cycle of the inode.
> i.e. the storage is destroyed along with the owning inode.
>
> The BPF LSM allocates an __rcu pointer to the bpf_local_storage in the
> security blob which are now stackable and can co-exist with other LSMs.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> index c5fac8068ba1..e8fbafb3e87b 100644
> --- a/tools/bpf/bpftool/map.c
> +++ b/tools/bpf/bpftool/map.c
> @@ -49,6 +49,7 @@ const char * const map_type_name[] = {
> [BPF_MAP_TYPE_STACK] = "stack",
> [BPF_MAP_TYPE_SK_STORAGE] = "sk_storage",
> [BPF_MAP_TYPE_STRUCT_OPS] = "struct_ops",
> + [BPF_MAP_TYPE_INODE_STORAGE] = "inode_storage",
> };
>
> const size_t map_type_name_size = ARRAY_SIZE(map_type_name);
Thanks for the update on bpftool map types, could you also change the
relevant help message, man page and bash completion please? (See below.)
Best regards,
Quentin
------
diff --git a/tools/bpf/bpftool/Documentation/bpftool-map.rst b/tools/bpf/bpftool/Documentation/bpftool-map.rst
index 31101643e57c..a9cd15ed7187 100644
--- a/tools/bpf/bpftool/Documentation/bpftool-map.rst
+++ b/tools/bpf/bpftool/Documentation/bpftool-map.rst
@@ -49,7 +49,7 @@ MAP COMMANDS
| | **lru_percpu_hash** | **lpm_trie** | **array_of_maps** | **hash_of_maps**
| | **devmap** | **devmap_hash** | **sockmap** | **cpumap** | **xskmap** | **sockhash**
| | **cgroup_storage** | **reuseport_sockarray** | **percpu_cgroup_storage**
-| | **queue** | **stack** | **sk_storage** | **struct_ops** }
+| | **queue** | **stack** | **sk_storage** | **struct_ops** | **inode_storage** }
DESCRIPTION
===========
diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
index 25b25aca1112..34cadc081a78 100644
--- a/tools/bpf/bpftool/bash-completion/bpftool
+++ b/tools/bpf/bpftool/bash-completion/bpftool
@@ -688,7 +688,8 @@ _bpftool()
lru_percpu_hash lpm_trie array_of_maps \
hash_of_maps devmap devmap_hash sockmap cpumap \
xskmap sockhash cgroup_storage reuseport_sockarray \
- percpu_cgroup_storage queue stack' -- \
+ percpu_cgroup_storage queue stack sk_storage \
+ struct_ops inode_storage' -- \
"$cur" ) )
return 0
;;
diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
index c5fac8068ba1..f1b48a97b378 100644
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -1590,7 +1590,7 @@ static int do_help(int argc, char **argv)
" lru_percpu_hash | lpm_trie | array_of_maps | hash_of_maps |\n"
" devmap | devmap_hash | sockmap | cpumap | xskmap | sockhash |\n"
" cgroup_storage | reuseport_sockarray | percpu_cgroup_storage |\n"
- " queue | stack | sk_storage | struct_ops }\n"
+ " queue | stack | sk_storage | struct_ops | inode_storage }\n"
" " HELP_SPEC_OPTIONS "\n"
"",
bin_name, argv[-2]);
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox