* Re: [PATCH v2 08/79] docs: cgroup-v1: convert docs to ReST and rename to *.rst
From: Tejun Heo @ 2019-05-06 15:47 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Doc Mailing List, Mauro Carvalho Chehab, linux-kernel,
Jonathan Corbet, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
H. Peter Anvin, x86, Jens Axboe, Li Zefan, Johannes Weiner,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, James Morris, Serge E. Hallyn, linux-block,
cgroups, netdev, bpf, linux-security-module
In-Reply-To: <c6e79690c038fc6bbf9265a065c1f861d6e156fa.1555938375.git.mchehab+samsung@kernel.org>
On Mon, Apr 22, 2019 at 10:26:57AM -0300, Mauro Carvalho Chehab wrote:
> Convert the cgroup-v1 files to ReST format, in order to
> allow a later addition to the admin-guide.
>
> The conversion is actually:
> - add blank lines and identation in order to identify paragraphs;
> - fix tables markups;
> - add some lists markups;
> - mark literal blocks;
> - adjust title markups.
>
> At its new index.rst, let's add a :orphan: while this is not linked to
> the main index.rst file, in order to avoid build warnings.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
Acked-by: Tejun Heo <tj@kernel.org>
Please feel free to route with other patches in the series.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH 01/10] LSM: SafeSetID: fix pr_warn() to include newline
From: Micah Morton @ 2019-05-06 15:53 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jKid6ggjD7G=qLoAGdpprxQiXziKu5g=rcGw+d0sUPr9g@mail.gmail.com>
These 10 patches got buried, but Jann, Kees and myself are in
agreement on how they look.
Could they get merged? Patches 8/10 and 9/10 have a v2 that should get
merged instead of the originals. I can respond on all the patches that
should get merged if that is helpful?
On Wed, Apr 10, 2019 at 10:09 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:54 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > Fix the pr_warn() calls in the SafeSetID LSM to have newlines at the end.
> > Without this, denial messages will be buffered as incomplete lines in
> > log_output(), and will then only show up once something else prints into
> > dmesg.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/lsm.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index cecd38e2ac80..2daecab3a4c0 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -91,7 +91,7 @@ static int safesetid_security_capable(const struct cred *cred,
> > * to functionality other than calling set*uid() (e.g.
> > * allowing user to set up userns uid mappings).
> > */
> > - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> > + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > __kuid_val(cred->uid));
> > return -1;
> > }
> > @@ -103,7 +103,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> > {
> > if (check_setuid_policy_hashtable_key_value(parent, child))
> > return 0;
> > - pr_warn("UID transition (%d -> %d) blocked",
> > + pr_warn("UID transition (%d -> %d) blocked\n",
> > __kuid_val(parent),
> > __kuid_val(child));
> > /*
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PULL] Smack: Repair for 5.2 build issue.
From: Casey Schaufler @ 2019-05-06 15:53 UTC (permalink / raw)
To: James Morris, Tetsuo Handa, Linux Security Module list
In-Reply-To: <b1d1d0b8-51cb-657b-c8be-aa365a29c8a5@schaufler-ca.com>
On 4/30/2019 2:29 PM, Casey Schaufler wrote:
> James, please pull this repair for a build problem with the last change.
James, did you get this request? It's an important build fix.
> The following changes since commit b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e:
> smack: Check address length before reading address family (2019-04-29 17:32:27 -0700)
> are available in the git repository at:
> https://github.com/cschaufler/next-smack.git smack-for-5.2-b
> for you to fetch changes up to 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
> Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
> ----------------------------------------------------------------
> Casey Schaufler (1):
> Smack: Fix kbuild reported build error
> security/smack/smack_lsm.c | 2 ++
> 1 file changed, 2 insertions(+)
>
^ permalink raw reply
* Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
From: Bartosz Szczepanek @ 2019-05-06 19:20 UTC (permalink / raw)
To: Matthew Garrett, Jarkko Sakkinen
Cc: linux-integrity, Peter Huewe, Jason Gunthorpe, Roberto Sassu,
linux-efi, LSM List, Linux Kernel Mailing List,
Thiébaud Weksteen
In-Reply-To: <CACdnJutMC2GBiXYUnFze+E-cigwb1gOK_wRfyWp77XQhTJuw9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1672 bytes --]
Nope, it doesn't work. It compiled (after correcting one more leftover
mapping), but panicked the same way.
I've came up with a set of changes that make it working in my setup,
see attached patch. There was a problem with passing already remapped
address to tpm2_calc_event_log_size(), which tried to remap it for
second time. Few more adjustments were needed in remap-as-you-go code
so that new address is used consequently. Also, I've removed remapping
digest itself because it was never used.
I'm not certain these changes make it 100% correct, but it worked on
35 events I had in final log. Its ending seems fine now:
> [root@localhost ~]# hexdump -C /sys/kernel/security/tpm0/binary_bios_measurements | tail
> 00003720 42 6f 6f 74 20 53 65 72 76 69 63 65 73 20 49 6e |Boot Services In|
> 00003730 76 6f 63 61 74 69 6f 6e 05 00 00 00 07 00 00 80 |vocation........|
> 00003740 02 00 00 00 04 00 47 55 45 dd c9 78 d7 bf d0 36 |......GUE..x...6|
> 00003750 fa cc 7e 2e 98 7f 48 18 9f 0d 0b 00 b5 4f 75 42 |..~...H......OuB|
> 00003760 cb d8 72 a8 1a 9d 9d ea 83 9b 2b 8d 74 7c 7e bd |..r.......+.t|~.|
> 00003770 5e a6 61 5c 40 f4 2f 44 a6 db eb a0 28 00 00 00 |^.a\@./D....(...|
> 00003780 45 78 69 74 20 42 6f 6f 74 20 53 65 72 76 69 63 |Exit Boot Servic|
> 00003790 65 73 20 52 65 74 75 72 6e 65 64 20 77 69 74 68 |es Returned with|
> 000037a0 20 53 75 63 63 65 73 73 | Success|
> 000037a8
Still, some refactoring could help here as __calc_tpm2_event_size has
grown and its logic became hard to follow. IMO it's far too complex
for inline function.
Attached patch should be applied on top of jjs/master.
Bartosz
[-- Attachment #2: eventlog_fix.diff --]
[-- Type: text/x-patch, Size: 4227 bytes --]
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index fe48150f06d1..2c912ea08166 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,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 ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,22 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
- tbl_size = tpm2_calc_event_log_size(final_tbl->events,
+ tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+ + sizeof(final_tbl->version)
+ + sizeof(final_tbl->nr_events),
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 0ca27bc053af..63238c84dc0b 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -185,8 +185,12 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
size = 0;
goto out;
}
+ } else {
+ mapping = marker_start;
}
+ event = (struct tcg_pcr_event2_head *)mapping;
+
efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
/* Check if event is malformed. */
@@ -201,34 +205,24 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest's algorithm identifier */
if (do_mapping) {
TPM_MEMUNMAP(mapping, mapping_size);
- mapping_size = marker - marker_start + halg_size;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
+ mapping_size = halg_size;
+ mapping = TPM_MEMREMAP((unsigned long)marker,
+ mapping_size);
if (!mapping) {
size = 0;
goto out;
}
+ } else {
+ mapping = marker;
}
- memcpy(&halg, marker, halg_size);
+ memcpy(&halg, mapping, halg_size);
marker = marker + halg_size;
for (j = 0; j < efispecid->num_algs; j++) {
if (halg == efispecid->digest_sizes[j].alg_id) {
marker +=
efispecid->digest_sizes[j].digest_size;
-
- /* Map the digest content itself */
- if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
- mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
- size = 0;
- goto out;
- }
- }
break;
}
}
@@ -239,23 +233,25 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
}
}
- event_field = (struct tcg_event_field *)marker;
-
/*
* Map the event size - we don't read from the event itself, so
* we don't need to map it
*/
if (do_mapping) {
- TPM_MEMUNMAP(marker_start, mapping_size);
+ TPM_MEMUNMAP(mapping, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping = TPM_MEMREMAP((unsigned long)marker,
mapping_size);
if (!mapping) {
size = 0;
goto out;
}
+ } else {
+ mapping = marker;
}
+ event_field = (struct tcg_event_field *)mapping;
+
marker = marker + sizeof(event_field->event_size)
+ event_field->event_size;
size = marker - marker_start;
^ permalink raw reply related
* Re: [PULL] Smack: Repair for 5.2 build issue.
From: James Morris @ 2019-05-06 21:31 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Tetsuo Handa, Linux Security Module list
In-Reply-To: <c028ab99-c042-191c-8a18-d34b8ecfc3bd@schaufler-ca.com>
[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]
On Mon, 6 May 2019, Casey Schaufler wrote:
> On 4/30/2019 2:29 PM, Casey Schaufler wrote:
>
> > James, please pull this repair for a build problem with the last change.
>
> James, did you get this request? It's an important build fix.
I pulled this branch, please check next-smack in my repo.
>
> > The following changes since commit b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e:
> > smack: Check address length before reading address family (2019-04-29
> > 17:32:27 -0700)
> > are available in the git repository at:
> > https://github.com/cschaufler/next-smack.git smack-for-5.2-b
> > for you to fetch changes up to 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
> > Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
> > ----------------------------------------------------------------
> > Casey Schaufler (1):
> > Smack: Fix kbuild reported build error
> > security/smack/smack_lsm.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH 01/10] LSM: SafeSetID: fix pr_warn() to include newline
From: James Morris @ 2019-05-06 23:39 UTC (permalink / raw)
To: Micah Morton; +Cc: Kees Cook, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAJ-EccPz3fZXR-485szP28kTsFLN=Y6040GacxdNuSn9io-kPw@mail.gmail.com>
On Mon, 6 May 2019, Micah Morton wrote:
> These 10 patches got buried, but Jann, Kees and myself are in
> agreement on how they look.
>
> Could they get merged? Patches 8/10 and 9/10 have a v2 that should get
> merged instead of the originals. I can respond on all the patches that
> should get merged if that is helpful?
Please do so.
>
> On Wed, Apr 10, 2019 at 10:09 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Apr 10, 2019 at 9:54 AM Micah Morton <mortonm@chromium.org> wrote:
> > >
> > > From: Jann Horn <jannh@google.com>
> > >
> > > Fix the pr_warn() calls in the SafeSetID LSM to have newlines at the end.
> > > Without this, denial messages will be buffered as incomplete lines in
> > > log_output(), and will then only show up once something else prints into
> > > dmesg.
> > >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > Signed-off-by: Micah Morton <mortonm@chromium.org>
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > -Kees
> >
> > > ---
> > > security/safesetid/lsm.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > > index cecd38e2ac80..2daecab3a4c0 100644
> > > --- a/security/safesetid/lsm.c
> > > +++ b/security/safesetid/lsm.c
> > > @@ -91,7 +91,7 @@ static int safesetid_security_capable(const struct cred *cred,
> > > * to functionality other than calling set*uid() (e.g.
> > > * allowing user to set up userns uid mappings).
> > > */
> > > - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> > > + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > > __kuid_val(cred->uid));
> > > return -1;
> > > }
> > > @@ -103,7 +103,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> > > {
> > > if (check_setuid_policy_hashtable_key_value(parent, child))
> > > return 0;
> > > - pr_warn("UID transition (%d -> %d) blocked",
> > > + pr_warn("UID transition (%d -> %d) blocked\n",
> > > __kuid_val(parent),
> > > __kuid_val(child));
> > > /*
> > > --
> > > 2.21.0.392.gf8f6787159e-goog
> > >
> >
> >
> > --
> > Kees Cook
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PULL] Smack: Repair for 5.2 build issue.
From: Casey Schaufler @ 2019-05-06 23:45 UTC (permalink / raw)
To: James Morris; +Cc: Tetsuo Handa, Linux Security Module list
In-Reply-To: <alpine.LRH.2.21.1905070730500.29030@namei.org>
On 5/6/2019 2:31 PM, James Morris wrote:
> On Mon, 6 May 2019, Casey Schaufler wrote:
>
>> On 4/30/2019 2:29 PM, Casey Schaufler wrote:
>>
>>> James, please pull this repair for a build problem with the last change.
>> James, did you get this request? It's an important build fix.
> I pulled this branch, please check next-smack in my repo.
There was an additional change:
Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
that came in on that same branch in response to a kbuild report.
>
>>> The following changes since commit b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e:
>>> ?? smack: Check address length before reading address family (2019-04-29
>>> 17:32:27 -0700)
>>> are available in the git repository at:
>>> ?? https://github.com/cschaufler/next-smack.git smack-for-5.2-b
>>> for you to fetch changes up to 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
>>> ?? Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
>>> ----------------------------------------------------------------
>>> Casey Schaufler (1):
>>> ?????????? Smack: Fix kbuild reported build error
>>> ??security/smack/smack_lsm.c | 2 ++
>>> ??1 file changed, 2 insertions(+)
>>>
^ permalink raw reply
* Re: [PULL] Smack: Repair for 5.2 build issue.
From: James Morris @ 2019-05-07 3:29 UTC (permalink / raw)
To: Casey Schaufler; +Cc: Tetsuo Handa, Linux Security Module list
In-Reply-To: <ba1bd79c-2917-2e1a-1380-3ca352178034@schaufler-ca.com>
On Mon, 6 May 2019, Casey Schaufler wrote:
> On 5/6/2019 2:31 PM, James Morris wrote:
> > On Mon, 6 May 2019, Casey Schaufler wrote:
> >
> > > On 4/30/2019 2:29 PM, Casey Schaufler wrote:
> > >
> > > > James, please pull this repair for a build problem with the last change.
> > > James, did you get this request? It's an important build fix.
> > I pulled this branch, please check next-smack in my repo.
>
> There was an additional change:
> Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
>
> that came in on that same branch in response to a kbuild report.
Now applied.
>
> >
> > > > The following changes since commit
> > > > b9ef5513c99bf9c8bfd9c9e8051b67f52b2dee1e:
> > > > ?? smack: Check address length before reading address family
> > > > (2019-04-29
> > > > 17:32:27 -0700)
> > > > are available in the git repository at:
> > > > ?? https://github.com/cschaufler/next-smack.git smack-for-5.2-b
> > > > for you to fetch changes up to 619ae03e922b65a1a5d4269ceae1e9e13a058d6b:
> > > > ?? Smack: Fix kbuild reported build error (2019-04-30 14:13:32 -0700)
> > > > ----------------------------------------------------------------
> > > > Casey Schaufler (1):
> > > > ?????????? Smack: Fix kbuild reported build error
> > > > ??security/smack/smack_lsm.c | 2 ++
> > > > ??1 file changed, 2 insertions(+)
> > > >
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [GIT PULL] Security subsystem: general updates for v5.2
From: James Morris @ 2019-05-07 3:35 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-security-module
Just a few bugfixes and documentation updates.
Please pull.
The following changes since commit 8c2ffd9174779014c3fe1f96d9dc3641d9175f00:
Linux 5.1-rc2 (2019-03-24 14:02:26 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next-general
for you to fetch changes up to 6beff00b79ca0b5caf0ce6fb8e11f57311bd95f8:
seccomp: fix up grammar in comment (2019-04-23 16:21:12 -0700)
----------------------------------------------------------------
Denis Efremov (11):
LSM: fix documentation for sb_copy_data hook
LSM: fix documentation for the syslog hook
LSM: fix documentation for the socket_post_create hook
LSM: fix documentation for the task_setscheduler hook
LSM: fix documentation for the socket_getpeersec_dgram hook
LSM: fix documentation for the path_chmod hook
LSM: fix documentation for the audit_* hooks
LSM: fix documentation for the msg_queue_* hooks
LSM: fix documentation for the sem_* hooks
LSM: fix documentation for the shm_* hooks
LSM: lsm_hooks.h: fix documentation format
James Morris (2):
Merge tag 'v5.1-rc2' into next-general
Revert "security: inode: fix a missing check for securityfs_create_file"
Jann Horn (3):
Yama: mark local symbols as static
security: don't use RCU accessors for cred->session_keyring
keys: safe concurrent user->{session,uid}_keyring access
Kangjie Lu (1):
security: inode: fix a missing check for securityfs_create_file
Mukesh Ojha (1):
Yama: mark function as static
Tycho Andersen (1):
seccomp: fix up grammar in comment
include/linux/cred.h | 2 +-
include/linux/lsm_hooks.h | 170 +++++++++++++++++++++----------------------
include/linux/sched/user.h | 7 ++
kernel/seccomp.c | 2 +-
security/keys/process_keys.c | 41 +++++------
security/keys/request_key.c | 14 ++--
security/yama/yama_lsm.c | 8 +-
7 files changed, 119 insertions(+), 125 deletions(-)
^ permalink raw reply
* [PATCH AUTOSEL 4.14 41/95] ima: open a new file instance if no read permissions
From: Sasha Levin @ 2019-05-07 5:37 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Goldwyn Rodrigues, Goldwyn Rodrigues, Mimi Zohar, Sasha Levin,
linux-integrity, linux-security-module
In-Reply-To: <20190507053826.31622-1-sashal@kernel.org>
From: Goldwyn Rodrigues <rgoldwyn@suse.de>
[ Upstream commit a408e4a86b36bf98ad15b9ada531cf0e5118ac67 ]
Open a new file instance as opposed to changing file->f_mode when
the file is not readable. This is done to accomodate overlayfs
stacked file operations change. The real struct file is hidden
behind the overlays struct file. So, any file->f_mode manipulations are
not reflected on the real struct file. Open the file again in read mode
if original file cannot be read, read and calculate the hash.
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Cc: stable@vger.kernel.org (linux-4.19)
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Sasha Levin <alexander.levin@microsoft.com>
---
security/integrity/ima/ima_crypto.c | 54 ++++++++++++++++++-----------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index cb041af9eddb..af680b5b678a 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -232,7 +232,7 @@ static int ima_calc_file_hash_atfm(struct file *file,
{
loff_t i_size, offset;
char *rbuf[2] = { NULL, };
- int rc, read = 0, rbuf_len, active = 0, ahash_rc = 0;
+ int rc, rbuf_len, active = 0, ahash_rc = 0;
struct ahash_request *req;
struct scatterlist sg[1];
struct ahash_completion res;
@@ -279,11 +279,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
&rbuf_size[1], 0);
}
- if (!(file->f_mode & FMODE_READ)) {
- file->f_mode |= FMODE_READ;
- read = 1;
- }
-
for (offset = 0; offset < i_size; offset += rbuf_len) {
if (!rbuf[1] && offset) {
/* Not using two buffers, and it is not the first
@@ -322,8 +317,6 @@ static int ima_calc_file_hash_atfm(struct file *file,
/* wait for the last update request to complete */
rc = ahash_wait(ahash_rc, &res);
out3:
- if (read)
- file->f_mode &= ~FMODE_READ;
ima_free_pages(rbuf[0], rbuf_size[0]);
ima_free_pages(rbuf[1], rbuf_size[1]);
out2:
@@ -358,7 +351,7 @@ static int ima_calc_file_hash_tfm(struct file *file,
{
loff_t i_size, offset = 0;
char *rbuf;
- int rc, read = 0;
+ int rc;
SHASH_DESC_ON_STACK(shash, tfm);
shash->tfm = tfm;
@@ -379,11 +372,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
if (!rbuf)
return -ENOMEM;
- if (!(file->f_mode & FMODE_READ)) {
- file->f_mode |= FMODE_READ;
- read = 1;
- }
-
while (offset < i_size) {
int rbuf_len;
@@ -400,8 +388,6 @@ static int ima_calc_file_hash_tfm(struct file *file,
if (rc)
break;
}
- if (read)
- file->f_mode &= ~FMODE_READ;
kfree(rbuf);
out:
if (!rc)
@@ -442,6 +428,8 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
{
loff_t i_size;
int rc;
+ struct file *f = file;
+ bool new_file_instance = false, modified_flags = false;
/*
* For consistency, fail file's opened with the O_DIRECT flag on
@@ -453,15 +441,41 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
return -EINVAL;
}
- i_size = i_size_read(file_inode(file));
+ /* Open a new file instance in O_RDONLY if we cannot read */
+ if (!(file->f_mode & FMODE_READ)) {
+ int flags = file->f_flags & ~(O_WRONLY | O_APPEND |
+ O_TRUNC | O_CREAT | O_NOCTTY | O_EXCL);
+ flags |= O_RDONLY;
+ f = dentry_open(&file->f_path, flags, file->f_cred);
+ if (IS_ERR(f)) {
+ /*
+ * Cannot open the file again, lets modify f_flags
+ * of original and continue
+ */
+ pr_info_ratelimited("Unable to reopen file for reading.\n");
+ f = file;
+ f->f_flags |= FMODE_READ;
+ modified_flags = true;
+ } else {
+ new_file_instance = true;
+ }
+ }
+
+ i_size = i_size_read(file_inode(f));
if (ima_ahash_minsize && i_size >= ima_ahash_minsize) {
- rc = ima_calc_file_ahash(file, hash);
+ rc = ima_calc_file_ahash(f, hash);
if (!rc)
- return 0;
+ goto out;
}
- return ima_calc_file_shash(file, hash);
+ rc = ima_calc_file_shash(f, hash);
+out:
+ if (new_file_instance)
+ fput(f);
+ else if (modified_flags)
+ f->f_flags &= ~FMODE_READ;
+ return rc;
}
/*
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 2/2 v3] efi: print appropriate status message when loading certificates
From: joeyli @ 2019-05-07 8:43 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Peter Jones, Matthew Garrett, Lee, Chun-Yi, Mimi Zohar,
David Howells, James Morris, Serge E . Hallyn, Josh Boyer,
Nayna Jain, linux-efi, linux-security-module,
Linux Kernel Mailing List
In-Reply-To: <CAKv+Gu-AuoObBW5Ha_cXtYQOdeYYpTRBu=6RqH7+DYvwACKR1A@mail.gmail.com>
On Fri, May 03, 2019 at 04:23:51PM +0200, Ard Biesheuvel wrote:
> On Fri, 3 May 2019 at 10:59, joeyli <jlee@suse.com> wrote:
> >
> > On Fri, May 03, 2019 at 10:07:59AM +0200, Ard Biesheuvel wrote:
> > > On Fri, 3 May 2019 at 09:18, joeyli <jlee@suse.com> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Thu, May 02, 2019 at 11:04:34AM +0200, Ard Biesheuvel wrote:
> > > > > On Thu, 2 May 2019 at 06:04, Lee, Chun-Yi <joeyli.kernel@gmail.com> wrote:
> > > > > >
> > > > > > When loading certificates list from UEFI variable, the original error
> > > > > > message direct shows the efi status code from UEFI firmware. It looks
> > > > > > ugly:
> > > > > >
> > > > > > [ 2.335031] Couldn't get size: 0x800000000000000e
> > > > > > [ 2.335032] Couldn't get UEFI MokListRT
> > > > > > [ 2.339985] Couldn't get size: 0x800000000000000e
> > > > > > [ 2.339987] Couldn't get UEFI dbx list
> > > > > >
> > > > > > So, this patch shows the status string instead of status code.
> > > > > >
> > > > > > On the other hand, the "Couldn't get UEFI" message doesn't need
> > > > > > to be exposed when db/dbx/mok variable do not exist. So, this
> > > > > > patch set the message level to debug.
> > > > > >
> > > > > > v3.
> > > > > > - Print messages similar to db/mok when loading dbx hash to blacklist:
> > > > > > [ 1.500952] EFI: Blacklisting hash of an executable: UEFI:dbx
> > > > > > [ 1.501773] blacklist: Loaded blacklisting hash
> > > > > > 'bin:80b4d96931bf0d02fd91a61e19d14f1da452e66db2408ca8604d411f92659f0a'
> > > > > >
> > > > > > - Setting messages for the existence of db/mok/dbx lists to debug level.
> > > > > >
> > > > > > v2.
> > > > > > Setting the MODSIGN messages level to debug.
> > > > > >
> > > > > > Link:
> > > > > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516
> > > > > > Cc: James Morris <jmorris@namei.org>
> > > > > > Cc: Serge E. Hallyn" <serge@hallyn.com>
> > > > > > Cc: David Howells <dhowells@redhat.com>
> > > > > > Cc: Nayna Jain <nayna@linux.ibm.com>
> > > > > > Cc: Josh Boyer <jwboyer@fedoraproject.org>
> > > > > > Cc: Mimi Zohar <zohar@linux.ibm.com>
> > > > > > Signed-off-by: "Lee, Chun-Yi" <jlee@suse.com>
> > > > > > ---
> > > > > > certs/blacklist.c | 3 +-
> > > > > > security/integrity/platform_certs/load_uefi.c | 40 +++++++++++++++++++--------
> > > > > > 2 files changed, 31 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/certs/blacklist.c b/certs/blacklist.c
> > > > > > index 3a507b9e2568..f91437e39e44 100644
> > > > > > --- a/certs/blacklist.c
> > > > > > +++ b/certs/blacklist.c
> > > > > > @@ -100,7 +100,8 @@ int mark_hash_blacklisted(const char *hash)
> > > > > > if (IS_ERR(key)) {
> > > > > > pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
> > > > > > return PTR_ERR(key);
> > > > > > - }
> > > > > > + } else
> > > > > > + pr_notice("Loaded blacklisting hash '%s'\n", hash);
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> > > > > > index 81b19c52832b..6b6996e5bc27 100644
> > > > > > --- a/security/integrity/platform_certs/load_uefi.c
> > > > > > +++ b/security/integrity/platform_certs/load_uefi.c
> > > > > > @@ -1,5 +1,7 @@
> > > > > > // SPDX-License-Identifier: GPL-2.0
> > > > > >
> > > > > > +#define pr_fmt(fmt) "EFI: "fmt
> > > > > > +
> > > > > > #include <linux/kernel.h>
> > > > > > #include <linux/sched.h>
> > > > > > #include <linux/cred.h>
> > > > > > @@ -35,6 +37,18 @@ static __init bool uefi_check_ignore_db(void)
> > > > > > return status == EFI_SUCCESS;
> > > > > > }
> > > > > >
> > > > > > +static void str16_to_str(efi_char16_t *str16, char *str, int str_size)
> > > > > > +{
> > > > > > + int i = 0;
> > > > > > +
> > > > > > + while (str16[i] != '\0' && i < (str_size - 1)) {
> > > > > > + str[i] = str16[i];
> > > > > > + i++;
> > > > > > + }
> > > > > > +
> > > > > > + str[i] = '\0';
> > > > > > +}
> > > > > > +
> > > > > > /*
> > > > > > * Get a certificate list blob from the named EFI variable.
> > > > > > */
> > > > > > @@ -44,13 +58,20 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > > > > efi_status_t status;
> > > > > > unsigned long lsize = 4;
> > > > > > unsigned long tmpdb[4];
> > > > > > + char namestr[16];
> > > > > > void *db;
> > > > > >
> > > > > > + str16_to_str(name, namestr, ARRAY_SIZE(namestr));
> > > > >
> > > > > Please drop this (and the function above) - instead, just return NULL
> > > > > if the variable is not found (without reporting an error).
> > > > >
> > > >
> > > > This name string is for printing debug level message, not error message.
> > > > This function already returns NULL when EFI_NOT_FOUND be returned by
> > > > firmware.
> > > >
> > > > > > status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> > > > > > if (status != EFI_BUFFER_TOO_SMALL) {
> > > > > > - pr_err("Couldn't get size: 0x%lx\n", status);
> > > > > > + if (status == EFI_NOT_FOUND)
> > > > > > + pr_debug("UEFI %s list doesn't exist\n", namestr);
> > > > > > + else
> > > > > > + pr_err("Couldn't get size for UEFI %s list: %s\n",
> > > > > > + namestr, efi_status_to_str(status));
> > > > > > return NULL;
> > > >
> > > > here returns NULL when EFI_NOT_FOUND. The message of existence is for
> > > > debugging.
> > > >
> > >
> > > I understand that. But I don't think we need it.
> > >
> >
> > OK. I will remove the debug message.
> >
> > > > > > }
> > > > > > + pr_debug("UEFI %s list exists\n", namestr);
> > > > > >
> > > > > > db = kmalloc(lsize, GFP_KERNEL);
> > > > > > if (!db)
> > > > > > @@ -59,7 +80,8 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> > > > > > status = efi.get_variable(name, guid, NULL, &lsize, db);
> > > > > > if (status != EFI_SUCCESS) {
> > > > > > kfree(db);
> > > > > > - pr_err("Error reading db var: 0x%lx\n", status);
> > > > > > + pr_err("Error reading UEFI %s list: %s\n",
> > > > > > + namestr, efi_status_to_str(status));
> > > > > > return NULL;
> > > > > > }
> > > > > >
> > > > > > @@ -95,6 +117,7 @@ static __init void uefi_blacklist_hash(const char *source, const void *data,
> > > > > > static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > > > const void *data, size_t len)
> > > > > > {
> > > > > > + pr_info("Blacklisting X.509 TBS hash: %s\n", source);
> > > > > > uefi_blacklist_hash(source, data, len, "tbs:", 4);
> > > > > > }
> > > > > >
> > > > > > @@ -104,6 +127,7 @@ static __init void uefi_blacklist_x509_tbs(const char *source,
> > > > > > static __init void uefi_blacklist_binary(const char *source,
> > > > > > const void *data, size_t len)
> > > > > > {
> > > > > > + pr_info("Blacklisting hash of an executable: %s\n", source);
> > > > > > uefi_blacklist_hash(source, data, len, "bin:", 4);
> > > > > > }
> > > > > >
> > > > >
> > > > > These are separate changes - I don't have an opinion whether they are
> > > > > appropriate or not, but they should be in a separate patch.
> > > > >
> > > >
> > > > I will move the message of blacklising hash to other patch. Thanks!
> > > >
> > > > > > @@ -154,9 +178,7 @@ static int __init load_uefi_certs(void)
> > > > > > */
> > > > > > if (!uefi_check_ignore_db()) {
> > > > > > db = get_cert_list(L"db", &secure_var, &dbsize);
> > > > > > - if (!db) {
> > > > > > - pr_err("MODSIGN: Couldn't get UEFI db list\n");
> > > > > > - } else {
> > > > > > + if (db) {
> > > > > > rc = parse_efi_signature_list("UEFI:db",
> > > > > > db, dbsize, get_handler_for_db);
> > > > > > if (rc)
> > > > > > @@ -167,9 +189,7 @@ static int __init load_uefi_certs(void)
> > > > > > }
> > > > > >
> > > > > > mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
> > > > > > - if (!mok) {
> > > > > > - pr_info("Couldn't get UEFI MokListRT\n");
> > > > > > - } else {
> > > > > > + if (mok) {
> > > > > > rc = parse_efi_signature_list("UEFI:MokListRT",
> > > > > > mok, moksize, get_handler_for_db);
> > > > > > if (rc)
> > > > > > @@ -178,9 +198,7 @@ static int __init load_uefi_certs(void)
> > > > > > }
> > > > > >
> > > > > > dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> > > > > > - if (!dbx) {
> > > > > > - pr_info("Couldn't get UEFI dbx list\n");
> > > > > > - } else {
> > > > > > + if (dbx) {
> > > > > > rc = parse_efi_signature_list("UEFI:dbx",
> > > > > > dbx, dbxsize,
> > > > > > get_handler_for_dbx);
> > > > > > --
> > > > > > 2.16.4
> > > > > >
> > > > >
> > > > > I think we should consider carefully what it means if some of these
> > > > > variables don't exist:
> > > > > - if secure boot is enabled, db and dbx must exist, so if they don't,
> > > > > something is wrong
> > > >
> > > > The existence of db/dbx is not related to secure boot. If manufacturer/user
> > > > enrolled certificate/hash to db or dbx, then the variable will be created.
> > > > If user didn't enroll anything to db/dbx, then variables will not show up.
> > > >
> > >
> > > Yes, but if secure boot is enabled and db is empty, how could you have
> > > booted in the first place?
> > >
> >
> > I agree. When secure boot enabled, kernel can not be booted without db.
> >
> > > And what about the converse case: if secure boot is not enabled, why
> > > should we trust the contents of db?
> > >
> >
> > The db and dbx are authenticated variables that it protected by KEK.
> > So it can be trusted even secure boot is disabled. Unless manufacturer
> > or user's KEK is leaked.
> >
>
> Is that true for non-secureboot capable firmware? Of course, in that
> case, we cannot be sure that the kernel itself is trusted, but we
> shouldn't open up another hole either.
>
I agree with you that we should filer-out non-secureboot firmware. I
think that we can check the existence of SecureBoot variable before loading
db/dbx. On the other hand, the SetupMode should also be checked because
db/dbx can be changed in setup mode.
> > > > > - secure boot might be enabled but we may be booting without shim.
> > > >
> > > > Shim always creates MokListRT no matter secure boot enabled or disabled.
> > > >
> > >
> > > That is not my point. What happens if you booted with secure boot
> > > enabled but without the help of shim?
> > >
> >
> > Without shim, the signed EFI stub can still be booted by EFI boot manager.
> > But the MokListRT will not be created for runtime. So MOK signed kernel
> > module can not be verified. (or IMA can not verify MOK signed kernel image
> > for kexec...)
> >
>
> So if we opt out of using shim, we lose kexec capability as well? That
> doesn't make any sense.
>
MOK is just one of options that it can be used to sign crash kernel image.
We can also use the key in db or the kernel embedded keys.
> In general, I am very uncomfortable with the way we have painted
> ourselves into a corner with shim. If I install Suse on a system that
> has the Suse cert in db or KEK, why would I need shim in the first
> place?
>
Yes! We don't need MOK if distro's certificates are preloaded in most
machines' db. Unfortunately it's not the case on most x86 machines.
> > > > > - secure boot might be disabled.
> > > > >
> > > >
> > > > It's not about secure boot, db/dbx/MokListRT are always available at
> > > > runtime if user was enrolled something to those list.
> > > >
> > >
> > > Yes, but again, depending on whether shim was involved, and/or whether
> > > secure boot was enabled or not, the way we interpret these things may
> > > be very different.
> > >
> > > I want the reasoning to be sound before merging any patches that deal
> > > with these variables.
> >
> > Here is a simple summary:
> >
> > When secure boot is enabled:
> > - db/dbx: Can be trusted because they are authenticated variables.
> > (unless end user doesn't want to trust db/dbx)
> > - MokListRT:
> > - with shim: MokListRT will be created. It can be trusted.
> > - without shim: MokListRT will not be created.
> > MOK protected kernel module or file can not be
> > verified.
> >
>
> So who polices that MoKlistRT is not created?
>
At boot time, shim copies MokList (boot variable) to MokListRT (runtime
variable). The MokListRT is a read-only runtime volatile variable. So,
without shim, the MokListRT will not be created for runtime.
> > When secure boot is disabled:
> > - db/dbx: Can be trusted because they are authenticated variables.
> > (unless end user doesn't want to trust db/dbx)
>
> Is this true? Does non-secure boot capable firmware still guarantee
> that db/dbx can't be created?
>
I agree. So we should check the existence of SecureBoot variable as I
mentioned above.
> > - MokListRT:
> > - with shim: MokListRT will be created. But it can not be trusted.
> > MOK protected kernel module or file can not be
> > verified.
> > - without shim: MokListRT will not be created.
> > MOK protected kernel module or file can not be
> > verified.
> >
>
> Enabling this securely involves more than connecting the dots when it
> comes to db, MokListRT etc. I would like for someone to convince me as
> a maintainer, as well as the IMA maintainers, that the security
> requirements are still met in all cases we care about. Just copying db
> and MokListRT into a kernel keychain doesn't appear to do that, and if
> it does, it needs more documentation to clarify that.
Welcome for more input from other maintainers. But I hope that my cosmetic
can be accepted because current warning messages looks ugly.
Thanks a lot!
Joey Lee
^ permalink raw reply
* [PATCH] tomoyo: Don't emit WARNING: string while fuzzing testing.
From: Tetsuo Handa @ 2019-05-07 11:34 UTC (permalink / raw)
To: James Morris; +Cc: linux-security-module, Tetsuo Handa, Dmitry Vyukov
Commit cff0e6c3ec3e6230 ("tomoyo: Add a kernel config option for fuzzing
testing.") enabled the learning mode, and syzbot started crashing by
encountering this warning message. Disable this warning if built for
fuzzing testing; otherwise syzbot can't start fuzzing testing.
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Cc: Dmitry Vyukov <dvyukov@google.com>
---
security/tomoyo/util.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
index 0517cbd..52752e1 100644
--- a/security/tomoyo/util.c
+++ b/security/tomoyo/util.c
@@ -1076,8 +1076,10 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true;
/* r->granted = false; */
tomoyo_write_log(r, "%s", tomoyo_dif[TOMOYO_DIF_QUOTA_WARNED]);
+#ifndef CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING
pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
domain->domainname->name);
+#endif
}
return false;
}
--
1.8.3.1
^ permalink raw reply related
* Fwd: Re: kernel panic: MAC Initialization failed. (3)
From: Tetsuo Handa @ 2019-05-07 11:37 UTC (permalink / raw)
To: James Morris; +Cc: linux-security-module
In-Reply-To: <CACT4Y+bm4fSXLjR-JQ5nbVLsvFCLAH03yMG=-8mYpafLho-vRw@mail.gmail.com>
James, please include
[PATCH] tomoyo: Don't emit WARNING: string while fuzzing testing.
before sending to linux.git .
Regards.
-------- Forwarded Message --------
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Date: Thu, May 2, 2019 at 2:16 AM
To: Dmitry Vyukov
Cc: syzbot
> The commit for avoiding this problem was sent to linux-next.git .
> Please add CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING=y into
> kernel configs with CONFIG_SECURITY_TOMOYO=y.
>
>
>
> By the way, does syzbot stop upon encountering any "WARNING" string?
> If yes, I guess I need to change
>
> pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
>
> string in security/tomoyo/util.c because
> CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING enables learning mode.
Yes, syzkaller detects all "WARNING:" strings as kernel bug. There
does not seem to be a better way to detect kernel bugs.
I've tried to enable the config, but all instances indeed immediately
detected as bugged:
2019/05/07 13:11:37 vm-10: crash: WARNING: Domain '<kernel> /sbin/init
/etc/init.d/rc /sbin/startpar /etc/init.d/ssh /sbin/start-stop-daemon
/usr/sbin/ssh [corrupted]
So we will need to wait until removal of the "WARNING:" messages
reaches linux-next.
Perhaps we may skip printing this warning if
CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING is set.
^ permalink raw reply
* Re: [PATCH 01/10] LSM: SafeSetID: fix pr_warn() to include newline
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: James Morris; +Cc: Kees Cook, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <alpine.LRH.2.21.1905070939100.12945@namei.org>
Ok, this ones ready for merge.
On Mon, May 6, 2019 at 4:39 PM James Morris <jmorris@namei.org> wrote:
>
> On Mon, 6 May 2019, Micah Morton wrote:
>
> > These 10 patches got buried, but Jann, Kees and myself are in
> > agreement on how they look.
> >
> > Could they get merged? Patches 8/10 and 9/10 have a v2 that should get
> > merged instead of the originals. I can respond on all the patches that
> > should get merged if that is helpful?
>
> Please do so.
>
> >
> > On Wed, Apr 10, 2019 at 10:09 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Apr 10, 2019 at 9:54 AM Micah Morton <mortonm@chromium.org> wrote:
> > > >
> > > > From: Jann Horn <jannh@google.com>
> > > >
> > > > Fix the pr_warn() calls in the SafeSetID LSM to have newlines at the end.
> > > > Without this, denial messages will be buffered as incomplete lines in
> > > > log_output(), and will then only show up once something else prints into
> > > > dmesg.
> > > >
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > Signed-off-by: Micah Morton <mortonm@chromium.org>
> > >
> > > Reviewed-by: Kees Cook <keescook@chromium.org>
> > >
> > > -Kees
> > >
> > > > ---
> > > > security/safesetid/lsm.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > > > index cecd38e2ac80..2daecab3a4c0 100644
> > > > --- a/security/safesetid/lsm.c
> > > > +++ b/security/safesetid/lsm.c
> > > > @@ -91,7 +91,7 @@ static int safesetid_security_capable(const struct cred *cred,
> > > > * to functionality other than calling set*uid() (e.g.
> > > > * allowing user to set up userns uid mappings).
> > > > */
> > > > - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions",
> > > > + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > > > __kuid_val(cred->uid));
> > > > return -1;
> > > > }
> > > > @@ -103,7 +103,7 @@ static int check_uid_transition(kuid_t parent, kuid_t child)
> > > > {
> > > > if (check_setuid_policy_hashtable_key_value(parent, child))
> > > > return 0;
> > > > - pr_warn("UID transition (%d -> %d) blocked",
> > > > + pr_warn("UID transition (%d -> %d) blocked\n",
> > > > __kuid_val(parent),
> > > > __kuid_val(child));
> > > > /*
> > > > --
> > > > 2.21.0.392.gf8f6787159e-goog
> > > >
> > >
> > >
> > > --
> > > Kees Cook
> >
>
> --
> James Morris
> <jmorris@namei.org>
>
^ permalink raw reply
* Re: [PATCH 02/10] LSM: SafeSetID: fix check for setresuid(new1, new2, new3)
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jL-jAc-jsDeRXnAXbETT+EUf8R0gFiqNvm60F42=tdhAQ@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:11 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > With the old code, when a process with the (real,effective,saved) UID set
> > (1,1,1) calls setresuid(2,3,4), safesetid_task_fix_setuid() only checks
> > whether the transition 1->2 is permitted; the transitions 1->3 and 1->4 are
> > not checked. Fix this.
> >
> > This is also a good opportunity to refactor safesetid_task_fix_setuid() to
> > be less verbose - having one branch per set*uid() syscall is unnecessary.
> >
> > Note that this slightly changes semantics: The UID transition check for
> > UIDs that were not in the old cred struct is now always performed against
> > the policy of the RUID. I think that's more consistent anyway, since the
> > RUID is also the one that decides whether any policy is enforced at all.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/lsm.c | 125 +++++++++++----------------------------
> > 1 file changed, 35 insertions(+), 90 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 2daecab3a4c0..5310fcf3052a 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -99,20 +99,30 @@ static int safesetid_security_capable(const struct cred *cred,
> > return 0;
> > }
> >
> > -static int check_uid_transition(kuid_t parent, kuid_t child)
> > +/*
> > + * Check whether a caller with old credentials @old is allowed to switch to
> > + * credentials that contain @new_uid.
> > + */
> > +static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
> > {
> > - if (check_setuid_policy_hashtable_key_value(parent, child))
> > - return 0;
> > - pr_warn("UID transition (%d -> %d) blocked\n",
> > - __kuid_val(parent),
> > - __kuid_val(child));
> > + bool permitted;
> > +
> > + /* If our old creds already had this UID in it, it's fine. */
> > + if (uid_eq(new_uid, old->uid) || uid_eq(new_uid, old->euid) ||
> > + uid_eq(new_uid, old->suid))
> > + return true;
> > +
> > /*
> > - * Kill this process to avoid potential security vulnerabilities
> > - * that could arise from a missing whitelist entry preventing a
> > - * privileged process from dropping to a lesser-privileged one.
> > + * Transitions to new UIDs require a check against the policy of the old
> > + * RUID.
> > */
> > - force_sig(SIGKILL, current);
> > - return -EACCES;
> > + permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
> > + if (!permitted) {
> > + pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> > + __kuid_val(old->uid), __kuid_val(old->euid),
> > + __kuid_val(old->suid), __kuid_val(new_uid));
> > + }
> > + return permitted;
> > }
> >
> > /*
> > @@ -125,88 +135,23 @@ static int safesetid_task_fix_setuid(struct cred *new,
> > int flags)
> > {
> >
> > - /* Do nothing if there are no setuid restrictions for this UID. */
> > + /* Do nothing if there are no setuid restrictions for our old RUID. */
> > if (!check_setuid_policy_hashtable_key(old->uid))
> > return 0;
> >
> > - switch (flags) {
> > - case LSM_SETID_RE:
> > - /*
> > - * Users for which setuid restrictions exist can only set the
> > - * real UID to the real UID or the effective UID, unless an
> > - * explicit whitelist policy allows the transition.
> > - */
> > - if (!uid_eq(old->uid, new->uid) &&
> > - !uid_eq(old->euid, new->uid)) {
> > - return check_uid_transition(old->uid, new->uid);
> > - }
> > - /*
> > - * Users for which setuid restrictions exist can only set the
> > - * effective UID to the real UID, the effective UID, or the
> > - * saved set-UID, unless an explicit whitelist policy allows
> > - * the transition.
> > - */
> > - if (!uid_eq(old->uid, new->euid) &&
> > - !uid_eq(old->euid, new->euid) &&
> > - !uid_eq(old->suid, new->euid)) {
> > - return check_uid_transition(old->euid, new->euid);
> > - }
> > - break;
> > - case LSM_SETID_ID:
> > - /*
> > - * Users for which setuid restrictions exist cannot change the
> > - * real UID or saved set-UID unless an explicit whitelist
> > - * policy allows the transition.
> > - */
> > - if (!uid_eq(old->uid, new->uid))
> > - return check_uid_transition(old->uid, new->uid);
> > - if (!uid_eq(old->suid, new->suid))
> > - return check_uid_transition(old->suid, new->suid);
> > - break;
> > - case LSM_SETID_RES:
> > - /*
> > - * Users for which setuid restrictions exist cannot change the
> > - * real UID, effective UID, or saved set-UID to anything but
> > - * one of: the current real UID, the current effective UID or
> > - * the current saved set-user-ID unless an explicit whitelist
> > - * policy allows the transition.
> > - */
> > - if (!uid_eq(new->uid, old->uid) &&
> > - !uid_eq(new->uid, old->euid) &&
> > - !uid_eq(new->uid, old->suid)) {
> > - return check_uid_transition(old->uid, new->uid);
> > - }
> > - if (!uid_eq(new->euid, old->uid) &&
> > - !uid_eq(new->euid, old->euid) &&
> > - !uid_eq(new->euid, old->suid)) {
> > - return check_uid_transition(old->euid, new->euid);
> > - }
> > - if (!uid_eq(new->suid, old->uid) &&
> > - !uid_eq(new->suid, old->euid) &&
> > - !uid_eq(new->suid, old->suid)) {
> > - return check_uid_transition(old->suid, new->suid);
> > - }
> > - break;
> > - case LSM_SETID_FS:
> > - /*
> > - * Users for which setuid restrictions exist cannot change the
> > - * filesystem UID to anything but one of: the current real UID,
> > - * the current effective UID or the current saved set-UID
> > - * unless an explicit whitelist policy allows the transition.
> > - */
> > - if (!uid_eq(new->fsuid, old->uid) &&
> > - !uid_eq(new->fsuid, old->euid) &&
> > - !uid_eq(new->fsuid, old->suid) &&
> > - !uid_eq(new->fsuid, old->fsuid)) {
> > - return check_uid_transition(old->fsuid, new->fsuid);
> > - }
> > - break;
> > - default:
> > - pr_warn("Unknown setid state %d\n", flags);
> > - force_sig(SIGKILL, current);
> > - return -EINVAL;
> > - }
> > - return 0;
> > + if (uid_permitted_for_cred(old, new->uid) &&
> > + uid_permitted_for_cred(old, new->euid) &&
> > + uid_permitted_for_cred(old, new->suid) &&
> > + uid_permitted_for_cred(old, new->fsuid))
> > + return 0;
> > +
> > + /*
> > + * Kill this process to avoid potential security vulnerabilities
> > + * that could arise from a missing whitelist entry preventing a
> > + * privileged process from dropping to a lesser-privileged one.
> > + */
> > + force_sig(SIGKILL, current);
> > + return -EACCES;
> > }
> >
> > int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 03/10] LSM: SafeSetID: refactor policy hash table
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jKmS48n7gajMVeNBfRdhCNgr5VTHLP8n83ktTc+kaskSQ@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:13 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > parent_kuid and child_kuid are kuids, there is no reason to make them
> > uint64_t. (And anyway, in the kernel, the normal name for that would be
> > u64, not uint64_t.)
> >
> > check_setuid_policy_hashtable_key() and
> > check_setuid_policy_hashtable_key_value() are basically the same thing,
> > merge them.
> >
> > Also fix the comment that claimed that (1<<8)==128.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/lsm.c | 62 ++++++++++++----------------------------
> > security/safesetid/lsm.h | 19 ++++++++++++
> > 2 files changed, 37 insertions(+), 44 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 5310fcf3052a..15cd13b5a211 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -14,67 +14,40 @@
> >
> > #define pr_fmt(fmt) "SafeSetID: " fmt
> >
> > -#include <linux/hashtable.h>
> > #include <linux/lsm_hooks.h>
> > #include <linux/module.h>
> > #include <linux/ptrace.h>
> > #include <linux/sched/task_stack.h>
> > #include <linux/security.h>
> > +#include "lsm.h"
> >
> > /* Flag indicating whether initialization completed */
> > int safesetid_initialized;
> >
> > -#define NUM_BITS 8 /* 128 buckets in hash table */
> > +#define NUM_BITS 8 /* 256 buckets in hash table */
> >
> > static DEFINE_HASHTABLE(safesetid_whitelist_hashtable, NUM_BITS);
> >
> > -/*
> > - * Hash table entry to store safesetid policy signifying that 'parent' user
> > - * can setid to 'child' user.
> > - */
> > -struct entry {
> > - struct hlist_node next;
> > - struct hlist_node dlist; /* for deletion cleanup */
> > - uint64_t parent_kuid;
> > - uint64_t child_kuid;
> > -};
> > -
> > static DEFINE_SPINLOCK(safesetid_whitelist_hashtable_spinlock);
> >
> > -static bool check_setuid_policy_hashtable_key(kuid_t parent)
> > +static enum sid_policy_type setuid_policy_lookup(kuid_t src, kuid_t dst)
> > {
> > struct entry *entry;
> > + enum sid_policy_type result = SIDPOL_DEFAULT;
> >
> > rcu_read_lock();
> > hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > - entry, next, __kuid_val(parent)) {
> > - if (entry->parent_kuid == __kuid_val(parent)) {
> > + entry, next, __kuid_val(src)) {
> > + if (!uid_eq(entry->src_uid, src))
> > + continue;
> > + if (uid_eq(entry->dst_uid, dst)) {
> > rcu_read_unlock();
> > - return true;
> > + return SIDPOL_ALLOWED;
> > }
> > + result = SIDPOL_CONSTRAINED;
> > }
> > rcu_read_unlock();
> > -
> > - return false;
> > -}
> > -
> > -static bool check_setuid_policy_hashtable_key_value(kuid_t parent,
> > - kuid_t child)
> > -{
> > - struct entry *entry;
> > -
> > - rcu_read_lock();
> > - hash_for_each_possible_rcu(safesetid_whitelist_hashtable,
> > - entry, next, __kuid_val(parent)) {
> > - if (entry->parent_kuid == __kuid_val(parent) &&
> > - entry->child_kuid == __kuid_val(child)) {
> > - rcu_read_unlock();
> > - return true;
> > - }
> > - }
> > - rcu_read_unlock();
> > -
> > - return false;
> > + return result;
> > }
> >
> > static int safesetid_security_capable(const struct cred *cred,
> > @@ -83,7 +56,7 @@ static int safesetid_security_capable(const struct cred *cred,
> > unsigned int opts)
> > {
> > if (cap == CAP_SETUID &&
> > - check_setuid_policy_hashtable_key(cred->uid)) {
> > + setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
> > if (!(opts & CAP_OPT_INSETID)) {
> > /*
> > * Deny if we're not in a set*uid() syscall to avoid
> > @@ -116,7 +89,8 @@ static bool uid_permitted_for_cred(const struct cred *old, kuid_t new_uid)
> > * Transitions to new UIDs require a check against the policy of the old
> > * RUID.
> > */
> > - permitted = check_setuid_policy_hashtable_key_value(old->uid, new_uid);
> > + permitted =
> > + setuid_policy_lookup(old->uid, new_uid) != SIDPOL_CONSTRAINED;
> > if (!permitted) {
> > pr_warn("UID transition ((%d,%d,%d) -> %d) blocked\n",
> > __kuid_val(old->uid), __kuid_val(old->euid),
> > @@ -136,7 +110,7 @@ static int safesetid_task_fix_setuid(struct cred *new,
> > {
> >
> > /* Do nothing if there are no setuid restrictions for our old RUID. */
> > - if (!check_setuid_policy_hashtable_key(old->uid))
> > + if (setuid_policy_lookup(old->uid, INVALID_UID) == SIDPOL_DEFAULT)
> > return 0;
> >
> > if (uid_permitted_for_cred(old, new->uid) &&
> > @@ -159,14 +133,14 @@ int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child)
> > struct entry *new;
> >
> > /* Return if entry already exists */
> > - if (check_setuid_policy_hashtable_key_value(parent, child))
> > + if (setuid_policy_lookup(parent, child) == SIDPOL_ALLOWED)
> > return 0;
> >
> > new = kzalloc(sizeof(struct entry), GFP_KERNEL);
> > if (!new)
> > return -ENOMEM;
> > - new->parent_kuid = __kuid_val(parent);
> > - new->child_kuid = __kuid_val(child);
> > + new->src_uid = parent;
> > + new->dst_uid = child;
> > spin_lock(&safesetid_whitelist_hashtable_spinlock);
> > hash_add_rcu(safesetid_whitelist_hashtable,
> > &new->next,
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index c1ea3c265fcf..6806f902794c 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -15,6 +15,8 @@
> > #define _SAFESETID_H
> >
> > #include <linux/types.h>
> > +#include <linux/uidgid.h>
> > +#include <linux/hashtable.h>
> >
> > /* Flag indicating whether initialization completed */
> > extern int safesetid_initialized;
> > @@ -25,6 +27,23 @@ enum safesetid_whitelist_file_write_type {
> > SAFESETID_WHITELIST_FLUSH, /* Flush whitelist policies. */
> > };
> >
> > +enum sid_policy_type {
> > + SIDPOL_DEFAULT, /* source ID is unaffected by policy */
> > + SIDPOL_CONSTRAINED, /* source ID is affected by policy */
> > + SIDPOL_ALLOWED /* target ID explicitly allowed */
> > +};
> > +
> > +/*
> > + * Hash table entry to store safesetid policy signifying that 'src_uid'
> > + * can setid to 'dst_uid'.
> > + */
> > +struct entry {
> > + struct hlist_node next;
> > + struct hlist_node dlist; /* for deletion cleanup */
> > + kuid_t src_uid;
> > + kuid_t dst_uid;
> > +};
> > +
> > /* Add entry to safesetid whitelist to allow 'parent' to setid to 'child'. */
> > int add_safesetid_whitelist_entry(kuid_t parent, kuid_t child);
> >
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 04/10] LSM: SafeSetID: refactor safesetid_security_capable()
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5j+tssZ24eGXJ9cQ+EFyYCiAkS_i3+gk1O84OrJS=JXu3g@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:14 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > At the moment, safesetid_security_capable() has two nested conditional
> > blocks, and one big comment for all the logic. Chop it up and reduce the
> > amount of indentation.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/lsm.c | 41 +++++++++++++++++++++++++---------------
> > 1 file changed, 26 insertions(+), 15 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 15cd13b5a211..ab429e1816c5 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -55,21 +55,32 @@ static int safesetid_security_capable(const struct cred *cred,
> > int cap,
> > unsigned int opts)
> > {
> > - if (cap == CAP_SETUID &&
> > - setuid_policy_lookup(cred->uid, INVALID_UID) != SIDPOL_DEFAULT) {
> > - if (!(opts & CAP_OPT_INSETID)) {
> > - /*
> > - * Deny if we're not in a set*uid() syscall to avoid
> > - * giving powers gated by CAP_SETUID that are related
> > - * to functionality other than calling set*uid() (e.g.
> > - * allowing user to set up userns uid mappings).
> > - */
> > - pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > - __kuid_val(cred->uid));
> > - return -1;
> > - }
> > - }
> > - return 0;
> > + /* We're only interested in CAP_SETUID. */
> > + if (cap != CAP_SETUID)
> > + return 0;
> > +
> > + /*
> > + * If CAP_SETUID is currently used for a set*uid() syscall, we want to
> > + * let it go through here; the real security check happens later, in the
> > + * task_fix_setuid hook.
> > + */
> > + if ((opts & CAP_OPT_INSETID) != 0)
> > + return 0;
> > +
> > + /*
> > + * If no policy applies to this task, allow the use of CAP_SETUID for
> > + * other purposes.
> > + */
> > + if (setuid_policy_lookup(cred->uid, INVALID_UID) == SIDPOL_DEFAULT)
> > + return 0;
> > +
> > + /*
> > + * Reject use of CAP_SETUID for functionality other than calling
> > + * set*uid() (e.g. setting up userns uid mappings).
> > + */
> > + pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > + __kuid_val(cred->uid));
> > + return -1;
> > }
> >
> > /*
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 05/10] LSM: SafeSetID: refactor policy parsing
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jK0GpT3MJfbNB=1bGGEGCah65v37_QXxOfhLOU0GM=Bwg@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:15 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > In preparation for changing the policy parsing logic, refactor the line
> > parsing logic to be less verbose and move it into a separate function.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > I made a minor change to Jann's original patch to use u32 instead of
> > s32 for the 'parsed_parent' and 'parsed_child' variables.
> >
> > security/safesetid/securityfs.c | 84 +++++++++++++--------------------
> > 1 file changed, 33 insertions(+), 51 deletions(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 2c6c829be044..90784a8d950a 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = {
> >
> > /*
> > * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> > - * variables pointed to by 'parent' and 'child' will get updated but this
> > + * variables pointed to by @parent and @child will get updated but this
> > * function will return an error.
> > + * Contents of @buf may be modified.
> > */
> > -static int parse_safesetid_whitelist_policy(const char __user *buf,
> > - size_t len,
> > - kuid_t *parent,
> > - kuid_t *child)
> > +static int parse_policy_line(
> > + struct file *file, char *buf, kuid_t *parent, kuid_t *child)
> > {
> > - char *kern_buf;
> > - char *parent_buf;
> > - char *child_buf;
> > - const char separator[] = ":";
> > + char *child_str;
> > int ret;
> > - size_t first_substring_length;
> > - long parsed_parent;
> > - long parsed_child;
> > + u32 parsed_parent, parsed_child;
> >
> > - /* Duplicate string from user memory and NULL-terminate */
> > - kern_buf = memdup_user_nul(buf, len);
> > - if (IS_ERR(kern_buf))
> > - return PTR_ERR(kern_buf);
> > -
> > - /*
> > - * Format of |buf| string should be <UID>:<UID>.
> > - * Find location of ":" in kern_buf (copied from |buf|).
> > - */
> > - first_substring_length = strcspn(kern_buf, separator);
> > - if (first_substring_length == 0 || first_substring_length == len) {
> > - ret = -EINVAL;
> > - goto free_kern;
> > - }
> > -
> > - parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
> > - if (!parent_buf) {
> > - ret = -ENOMEM;
> > - goto free_kern;
> > - }
> > + /* Format of |buf| string should be <UID>:<UID>. */
> > + child_str = strchr(buf, ':');
> > + if (child_str == NULL)
> > + return -EINVAL;
> > + *child_str = '\0';
> > + child_str++;
> >
> > - ret = kstrtol(parent_buf, 0, &parsed_parent);
> > + ret = kstrtou32(buf, 0, &parsed_parent);
> > if (ret)
> > - goto free_both;
> > + return ret;
> >
> > - child_buf = kern_buf + first_substring_length + 1;
> > - ret = kstrtol(child_buf, 0, &parsed_child);
> > + ret = kstrtou32(child_str, 0, &parsed_child);
> > if (ret)
> > - goto free_both;
> > + return ret;
> >
> > *parent = make_kuid(current_user_ns(), parsed_parent);
> > - if (!uid_valid(*parent)) {
> > - ret = -EINVAL;
> > - goto free_both;
> > - }
> > -
> > *child = make_kuid(current_user_ns(), parsed_child);
> > - if (!uid_valid(*child)) {
> > - ret = -EINVAL;
> > - goto free_both;
> > - }
> > + if (!uid_valid(*parent) || !uid_valid(*child))
> > + return -EINVAL;
> >
> > -free_both:
> > - kfree(parent_buf);
> > -free_kern:
> > + return 0;
> > +}
> > +
> > +static int parse_safesetid_whitelist_policy(
> > + struct file *file, const char __user *buf, size_t len,
> > + kuid_t *parent, kuid_t *child)
> > +{
> > + char *kern_buf = memdup_user_nul(buf, len);
> > + int ret;
> > +
> > + if (IS_ERR(kern_buf))
> > + return PTR_ERR(kern_buf);
> > + ret = parse_policy_line(file, kern_buf, parent, child);
> > kfree(kern_buf);
> > return ret;
> > }
> > @@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file,
> > flush_safesetid_whitelist_entries();
> > break;
> > case SAFESETID_WHITELIST_ADD:
> > - ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> > - &child);
> > + ret = parse_safesetid_whitelist_policy(file, buf, len,
> > + &parent, &child);
> > if (ret)
> > return ret;
> >
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 06/10] LSM: SafeSetID: fix userns handling in securityfs
From: Micah Morton @ 2019-05-07 15:01 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jJJTRgahj_hhiqkeWxLETWD5+=0800E0WNatq+3MrdPag@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:16 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > Looking at current_cred() in write handlers is bad form, stop doing that.
> >
> > Also, let's just require that the write is coming from the initial user
> > namespace. Especially SAFESETID_WHITELIST_FLUSH requires privilege over all
> > namespaces, and SAFESETID_WHITELIST_ADD should probably require it as well.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/securityfs.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 87e42b7f3e33..76c1e8a6ab93 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -59,8 +59,8 @@ static int parse_policy_line(
> > if (ret)
> > return ret;
> >
> > - *parent = make_kuid(current_user_ns(), parsed_parent);
> > - *child = make_kuid(current_user_ns(), parsed_child);
> > + *parent = make_kuid(file->f_cred->user_ns, parsed_parent);
> > + *child = make_kuid(file->f_cred->user_ns, parsed_child);
> > if (!uid_valid(*parent) || !uid_valid(*child))
> > return -EINVAL;
> >
> > @@ -92,7 +92,7 @@ static ssize_t safesetid_file_write(struct file *file,
> > kuid_t child;
> > int ret;
> >
> > - if (!ns_capable(current_user_ns(), CAP_MAC_ADMIN))
> > + if (!file_ns_capable(file, &init_user_ns, CAP_MAC_ADMIN))
> > return -EPERM;
> >
> > if (*ppos != 0)
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 07/10] LSM: SafeSetID: rewrite userspace API to atomic updates
From: Micah Morton @ 2019-05-07 15:02 UTC (permalink / raw)
To: Kees Cook; +Cc: Jann Horn, James Morris, Casey Schaufler, linux-security-module
In-Reply-To: <CAGXu5jL8YYG-jTuQkfH1kBPSdY-FamO_YobcL_rCyNKqq3NHmw@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 11:20 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 10:47 AM Jann Horn <jannh@google.com> wrote:
> >
> > On Wed, Apr 10, 2019 at 7:24 PM Kees Cook <keescook@chromium.org> wrote:
> > > On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> > > > From: Jann Horn <jannh@google.com>
> > > >
> > > > The current API of the SafeSetID LSM uses one write() per rule, and applies
> > > > each written rule instantly. This has several downsides:
> > > >
> > > > - While a policy is being loaded, once a single parent-child pair has been
> > > > loaded, the parent is restricted to that specific child, even if
> > > > subsequent rules would allow transitions to other child UIDs. This means
> > > > that during policy loading, set*uid() can randomly fail.
> > > > - To replace the policy without rebooting, it is necessary to first flush
> > > > all old rules. This creates a time window in which no constraints are
> > > > placed on the use of CAP_SETUID.
> > > > - If we want to perform sanity checks on the final policy, this requires
> > > > that the policy isn't constructed in a piecemeal fashion without telling
> > > > the kernel when it's done.
> > > >
> > > > Other kernel APIs - including things like the userns code and netfilter -
> > > > avoid this problem by performing updates atomically. Luckily, SafeSetID
> > > > hasn't landed in a stable (upstream) release yet, so maybe it's not too
> > > > late to completely change the API.
> > > >
> > > > The new API for SafeSetID is: If you want to change the policy, open
> > > > "safesetid/whitelist_policy" and write the entire policy,
> > > > newline-delimited, in there.
> > >
> > > So the entire policy is expected to be sent in a single write() call?
> > >
> > > open()
> > > write(policy1)
> > > write(policy2)
> > > close()
> > >
> > > means only policy2 is active?
> >
> > No; if you do that, the first write() sets policy1, and the second
> > write() fails with -EINVAL because of the "if (*ppos != 0) return
> > -EINVAL;" in safesetid_file_write() (which already exists in the
> > current version of the LSM).
>
> Ah yes, thanks! I missed that check. Good!
>
> >
> > > I thought policy was meant to be built
> > > over time? i.e. new policy could get appended to existing?
> >
> > That's what the current API does; as I've explained in the commit
> > message, I think that that's a bad idea.
>
> Okay, sounds fine. It wasn't clear to me from the commit message if
> you meant "write the whole policy during a single open/close" or
> "write whole policy with a single initial write".
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v2 08/10] LSM: SafeSetID: add read handler
From: Micah Morton @ 2019-05-07 15:02 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5j+B0RcNVJXOyn4FZ8NsGR-RgqDbnL+4Ws-8RBOtaAHg=w@mail.gmail.com>
Ready for merge.
On Thu, Apr 11, 2019 at 1:38 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > For debugging a running system, it is very helpful to be able to see what
> > policy the system is using. Add a read handler that can dump out a copy of
> > the loaded policy.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > Changes since the last patch set: Instead of doing refcounting, change
> > policy_update_lock to a mutex and hold the mutex across the policy read.
> > security/safesetid/lsm.h | 1 +
> > security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
> > 2 files changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index 4a34f558d964..db6d16e6bbc3 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -41,6 +41,7 @@ struct setuid_rule {
> >
> > struct setuid_ruleset {
> > DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> > + char *policy_str;
> > struct rcu_head rcu;
> > };
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 250d59e046c1..997b403c6255 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -19,7 +19,7 @@
> >
> > #include "lsm.h"
> >
> > -static DEFINE_SPINLOCK(policy_update_lock);
> > +static DEFINE_MUTEX(policy_update_lock);
> >
> > /*
> > * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> > @@ -67,6 +67,7 @@ static void __release_ruleset(struct rcu_head *rcu)
> >
> > hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
> > kfree(rule);
> > + kfree(pol->policy_str);
> > kfree(pol);
> > }
> >
> > @@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file,
> > pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> > if (!pol)
> > return -ENOMEM;
> > + pol->policy_str = NULL;
> > hash_init(pol->rules);
> >
> > p = buf = memdup_user_nul(ubuf, len);
> > @@ -92,6 +94,11 @@ static ssize_t handle_policy_update(struct file *file,
> > err = PTR_ERR(buf);
> > goto out_free_pol;
> > }
> > + pol->policy_str = kstrdup(buf, GFP_KERNEL);
> > + if (pol->policy_str == NULL) {
> > + err = -ENOMEM;
> > + goto out_free_buf;
> > + }
> >
> > /* policy lines, including the last one, end with \n */
> > while (*p != '\0') {
> > @@ -135,10 +142,10 @@ static ssize_t handle_policy_update(struct file *file,
> > * What we really want here is an xchg() wrapper for RCU, but since that
> > * doesn't currently exist, just use a spinlock for now.
> > */
> > - spin_lock(&policy_update_lock);
> > + mutex_lock(&policy_update_lock);
> > rcu_swap_protected(safesetid_setuid_rules, pol,
> > lockdep_is_held(&policy_update_lock));
> > - spin_unlock(&policy_update_lock);
> > + mutex_unlock(&policy_update_lock);
> > err = len;
> >
> > out_free_buf:
> > @@ -162,7 +169,27 @@ static ssize_t safesetid_file_write(struct file *file,
> > return handle_policy_update(file, buf, len);
> > }
> >
> > +static ssize_t safesetid_file_read(struct file *file, char __user *buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + ssize_t res = 0;
> > + struct setuid_ruleset *pol;
> > + const char *kbuf;
> > +
> > + mutex_lock(&policy_update_lock);
> > + pol = rcu_dereference_protected(safesetid_setuid_rules,
> > + lockdep_is_held(&policy_update_lock));
> > + if (pol) {
> > + kbuf = pol->policy_str;
> > + res = simple_read_from_buffer(buf, len, ppos,
> > + kbuf, strlen(kbuf));
> > + }
> > + mutex_unlock(&policy_update_lock);
> > + return res;
> > +}
> > +
> > static const struct file_operations safesetid_file_fops = {
> > + .read = safesetid_file_read,
> > .write = safesetid_file_write,
> > };
> >
> > @@ -181,7 +208,7 @@ static int __init safesetid_init_securityfs(void)
> > goto error;
> > }
> >
> > - policy_file = securityfs_create_file("whitelist_policy", 0200,
> > + policy_file = securityfs_create_file("whitelist_policy", 0600,
> > policy_dir, NULL, &safesetid_file_fops);
> > if (IS_ERR(policy_file)) {
> > ret = PTR_ERR(policy_file);
> > --
> > 2.21.0.392.gf8f6787159e-goog
>
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v2 09/10] LSM: SafeSetID: verify transitive constrainedness
From: Micah Morton @ 2019-05-07 15:03 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jJgQME5-LRzFG1rxr5eb84-+o4HiTuPCf6PWRHWMPkagw@mail.gmail.com>
Ready for merge.
On Thu, Apr 11, 2019 at 1:38 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > Someone might write a ruleset like the following, expecting that it
> > securely constrains UID 1 to UIDs 1, 2 and 3:
> >
> > 1:2
> > 1:3
> >
> > However, because no constraints are applied to UIDs 2 and 3, an attacker
> > with UID 1 can simply first switch to UID 2, then switch to any UID from
> > there. The secure way to write this ruleset would be:
> >
> > 1:2
> > 1:3
> > 2:2
> > 3:3
> >
> > , which uses "transition to self" as a way to inhibit the default-allow
> > policy without allowing anything specific.
> >
> > This is somewhat unintuitive. To make sure that policy authors don't
> > accidentally write insecure policies because of this, let the kernel verify
> > that a new ruleset does not contain any entries that are constrained, but
> > transitively unconstrained.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > Changes since the last patch: Instead of failing open when userspace
> > configures an unconstrained (and vulnerable) policy, fix up the policy
> > to make sure it is safe by restricting the un-constrained UIDs. Return
> > EINVAL from the policy write in the case that userspace writes an
> > unconstrained policy. Also move hash_add() into a small helper function.
> > security/safesetid/securityfs.c | 38 ++++++++++++++++++-
> > .../selftests/safesetid/safesetid-test.c | 4 +-
> > 2 files changed, 40 insertions(+), 2 deletions(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 997b403c6255..d568e17dd773 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -76,6 +76,37 @@ static void release_ruleset(struct setuid_ruleset *pol)
> > call_rcu(&pol->rcu, __release_ruleset);
> > }
> >
> > +static void insert_rule(struct setuid_ruleset *pol, struct setuid_rule *rule)
> > +{
> > + hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> > +}
> > +
> > +static int verify_ruleset(struct setuid_ruleset *pol)
> > +{
> > + int bucket;
> > + struct setuid_rule *rule, *nrule;
> > + int res = 0;
> > +
> > + hash_for_each(pol->rules, bucket, rule, next) {
> > + if (_setuid_policy_lookup(pol, rule->dst_uid, INVALID_UID) ==
> > + SIDPOL_DEFAULT) {
> > + pr_warn("insecure policy detected: uid %d is constrained but transitively unconstrained through uid %d\n",
> > + __kuid_val(rule->src_uid),
> > + __kuid_val(rule->dst_uid));
> > + res = -EINVAL;
> > +
> > + /* fix it up */
> > + nrule = kmalloc(sizeof(struct setuid_rule), GFP_KERNEL);
> > + if (!nrule)
> > + return -ENOMEM;
> > + nrule->src_uid = rule->dst_uid;
> > + nrule->dst_uid = rule->dst_uid;
> > + insert_rule(pol, nrule);
> > + }
> > + }
> > + return res;
> > +}
> > +
> > static ssize_t handle_policy_update(struct file *file,
> > const char __user *ubuf, size_t len)
> > {
> > @@ -128,7 +159,7 @@ static ssize_t handle_policy_update(struct file *file,
> > goto out_free_rule;
> > }
> >
> > - hash_add(pol->rules, &rule->next, __kuid_val(rule->src_uid));
> > + insert_rule(pol, rule);
> > p = end + 1;
> > continue;
> >
> > @@ -137,6 +168,11 @@ static ssize_t handle_policy_update(struct file *file,
> > goto out_free_buf;
> > }
> >
> > + err = verify_ruleset(pol);
> > + /* bogus policy falls through after fixing it up */
> > + if (err && err != -EINVAL)
> > + goto out_free_buf;
> > +
> > /*
> > * Everything looks good, apply the policy and release the old one.
> > * What we really want here is an xchg() wrapper for RCU, but since that
> > diff --git a/tools/testing/selftests/safesetid/safesetid-test.c b/tools/testing/selftests/safesetid/safesetid-test.c
> > index 4f03813d1911..8f40c6ecdad1 100644
> > --- a/tools/testing/selftests/safesetid/safesetid-test.c
> > +++ b/tools/testing/selftests/safesetid/safesetid-test.c
> > @@ -144,7 +144,9 @@ static void write_policies(void)
> > {
> > static char *policy_str =
> > "1:2\n"
> > - "1:3\n";
> > + "1:3\n"
> > + "2:2\n"
> > + "3:3\n";
> > ssize_t written;
> > int fd;
> >
> > --
> > 2.21.0.392.gf8f6787159e-goog
>
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH 10/10] LSM: SafeSetID: fix use of literal -1 in capable hook
From: Micah Morton @ 2019-05-07 15:03 UTC (permalink / raw)
To: Kees Cook; +Cc: James Morris, Casey Schaufler, linux-security-module, Jann Horn
In-Reply-To: <CAGXu5jLETb47pDRbmt4gRJN9+n1WPQAL9n9nhjDg8mhz9Vw8tA@mail.gmail.com>
Ready for merge.
On Wed, Apr 10, 2019 at 10:29 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > The capable() hook returns an error number. -EPERM is actually the same as
> > -1, so this doesn't make a difference in behavior.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > security/safesetid/lsm.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
> > index 4ab4d7cdba31..61b84e20f2dd 100644
> > --- a/security/safesetid/lsm.c
> > +++ b/security/safesetid/lsm.c
> > @@ -90,7 +90,7 @@ static int safesetid_security_capable(const struct cred *cred,
> > */
> > pr_warn("Operation requires CAP_SETUID, which is not available to UID %u for operations besides approved set*uid transitions\n",
> > __kuid_val(cred->uid));
> > - return -1;
> > + return -EPERM;
> > }
> >
> > /*
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH] tomoyo: Don't emit WARNING: string while fuzzing testing.
From: James Morris @ 2019-05-07 16:45 UTC (permalink / raw)
To: Tetsuo Handa; +Cc: linux-security-module, Dmitry Vyukov
In-Reply-To: <1557228862-9277-1-git-send-email-penguin-kernel@I-love.SAKURA.ne.jp>
On Tue, 7 May 2019, Tetsuo Handa wrote:
> Commit cff0e6c3ec3e6230 ("tomoyo: Add a kernel config option for fuzzing
> testing.") enabled the learning mode, and syzbot started crashing by
> encountering this warning message. Disable this warning if built for
> fuzzing testing; otherwise syzbot can't start fuzzing testing.
syzbot crashed? Sounds like a bug in syzbot which should be fixed rather
than this approach.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> ---
> security/tomoyo/util.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> index 0517cbd..52752e1 100644
> --- a/security/tomoyo/util.c
> +++ b/security/tomoyo/util.c
> @@ -1076,8 +1076,10 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
> domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true;
> /* r->granted = false; */
> tomoyo_write_log(r, "%s", tomoyo_dif[TOMOYO_DIF_QUOTA_WARNED]);
> +#ifndef CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING
> pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
> domain->domainname->name);
> +#endif
> }
> return false;
> }
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [PATCH] tomoyo: Don't emit WARNING: string while fuzzing testing.
From: Dmitry Vyukov @ 2019-05-07 16:49 UTC (permalink / raw)
To: James Morris; +Cc: Tetsuo Handa, linux-security-module
In-Reply-To: <alpine.LRH.2.21.1905080244530.13589@namei.org>
From: James Morris <jmorris@namei.org>
Date: Tue, May 7, 2019 at 6:45 PM
To: Tetsuo Handa
Cc: <linux-security-module@vger.kernel.org>, Dmitry Vyukov
> On Tue, 7 May 2019, Tetsuo Handa wrote:
>
> > Commit cff0e6c3ec3e6230 ("tomoyo: Add a kernel config option for fuzzing
> > testing.") enabled the learning mode, and syzbot started crashing by
> > encountering this warning message. Disable this warning if built for
> > fuzzing testing; otherwise syzbot can't start fuzzing testing.
>
> syzbot crashed? Sounds like a bug in syzbot which should be fixed rather
> than this approach.
syzbot did not crash, it detected this as kernel crash.
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > ---
> > security/tomoyo/util.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/security/tomoyo/util.c b/security/tomoyo/util.c
> > index 0517cbd..52752e1 100644
> > --- a/security/tomoyo/util.c
> > +++ b/security/tomoyo/util.c
> > @@ -1076,8 +1076,10 @@ bool tomoyo_domain_quota_is_ok(struct tomoyo_request_info *r)
> > domain->flags[TOMOYO_DIF_QUOTA_WARNED] = true;
> > /* r->granted = false; */
> > tomoyo_write_log(r, "%s", tomoyo_dif[TOMOYO_DIF_QUOTA_WARNED]);
> > +#ifndef CONFIG_SECURITY_TOMOYO_INSECURE_BUILTIN_SETTING
> > pr_warn("WARNING: Domain '%s' has too many ACLs to hold. Stopped learning mode.\n",
> > domain->domainname->name);
> > +#endif
> > }
> > return false;
> > }
> >
>
> --
> James Morris
> <jmorris@namei.org>
>
^ permalink raw reply
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