* Re: [PATCH v8 03/11] proc: move /proc/{self|thread-self} dentries to proc_fs_info
From: Andy Lutomirski @ 2020-02-10 18:23 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton,
Jonathan Corbet, Kees Cook, Linus Torvalds, Oleg Nesterov,
Solar Designer
In-Reply-To: <20200210150519.538333-4-gladkov.alexey@gmail.com>
On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> This is a preparation patch that moves /proc/{self|thread-self} dentries
> to be stored inside procfs fs_info struct instead of making them per pid
> namespace. Since we want to support multiple procfs instances we need to
> make sure that these dentries are also per-superblock instead of
> per-pidns,
The changelog makes perfect sense so far...
> unmounting a private procfs won't clash with other procfs
> mounts.
This doesn't parse as part of the previous sentence. I'm also not
convinced that this really involves unmounting per se. Maybe just
delete these words.
^ permalink raw reply
* Re: [PATCH v8 10/11] docs: proc: add documentation for "hidepid=4" and "subset=pidfs" options and new mount behavior
From: Andy Lutomirski @ 2020-02-10 18:29 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton,
Jonathan Corbet, Kees Cook, Linus Torvalds, Oleg Nesterov,
Solar Designer
In-Reply-To: <20200210150519.538333-11-gladkov.alexey@gmail.com>
On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
> Documentation/filesystems/proc.txt | 53 ++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
> index 99ca040e3f90..4741fd092f36 100644
> --- a/Documentation/filesystems/proc.txt
> +++ b/Documentation/filesystems/proc.txt
> @@ -50,6 +50,8 @@ Table of Contents
> 4 Configuring procfs
> 4.1 Mount options
>
> + 5 Filesystem behavior
> +
> ------------------------------------------------------------------------------
> Preface
> ------------------------------------------------------------------------------
> @@ -2021,6 +2023,7 @@ The following mount options are supported:
>
> hidepid= Set /proc/<pid>/ access mode.
> gid= Set the group authorized to learn processes information.
> + subset= Show only the specified subset of procfs.
>
> hidepid=0 means classic mode - everybody may access all /proc/<pid>/ directories
> (default).
> @@ -2042,6 +2045,56 @@ information about running processes, whether some daemon runs with elevated
> privileges, whether other user runs some sensitive program, whether other users
> run any program at all, etc.
>
> +hidepid=4 means that procfs should only contain /proc/<pid>/ directories
> +that the caller can ptrace.
I have a couple of minor nits here.
First, perhaps we could stop using magic numbers and use words.
hidepid=ptraceable is actually comprehensible, whereas hidepid=4
requires looking up what '4' means.
Second, there is PTRACE_MODE_ATTACH and PTRACE_MODE_READ. Which is it?
> +
> gid= defines a group authorized to learn processes information otherwise
> prohibited by hidepid=. If you use some daemon like identd which needs to learn
> information about processes information, just add identd to this group.
How is this better than just creating an entirely separate mount a
different hidepid and a different gid owning it? In any event,
usually gid= means that this gid is the group owner of inodes. Let's
call it something different. gid_override_hidepid might be credible.
But it's also really weird -- do different groups really see different
contents when they read a directory?
^ permalink raw reply
* Re: [PATCH v8 05/11] proc: add helpers to set and get proc hidepid and gid mount options
From: Andy Lutomirski @ 2020-02-10 18:30 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton,
Jonathan Corbet, Kees Cook, Linus Torvalds, Oleg Nesterov,
Solar Designer
In-Reply-To: <20200210150519.538333-6-gladkov.alexey@gmail.com>
On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> This is a cleaning patch to add helpers to set and get proc mount
> options instead of directly using them. This make it easy to track
> what's happening and easy to update in future.
On a cursory inspection, this looks like it obfuscates the code, and I
don't see where it does something useful later in the series. What is
this abstraction for?
--Andy
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-02-10 18:32 UTC (permalink / raw)
To: Stephen Smalley, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <04442c9f-430e-c922-b078-7cff8f36a45f@tycho.nsa.gov>
On 2/10/2020 6:55 AM, Stephen Smalley wrote:
> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>> previous discussions.
>>>
>>> I thought in previous discussions, we had come to the conclusion that
>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>> API, that's even better.)
>>>
>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>> that the same thing you mean?
>>
>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
>
> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname?
Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
there is no need to exactly duplicate what is in attr/current (later
SO_PEERSEC). I already plan to omit the "mode" component of the
AppArmor data in the AppArmor hook, as was discussed earlier. I would
prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
have to do.
>
>>> I thought the conclusion we had come to in previous conversations was
>>> that the LSM context is what GLib calls a "bytestring", the same as
>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>> with no further guarantees, and no specified encoding or mapping to/from
>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>> but nobody knows which one, and they coould equally well be some binary
>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>
>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>> APIs for user-space features built over it. If that isn't possible, the
>>> next best thing is a "bytestring" like filenames, environment variables,
>>> and most kernel <-> user-space strings in general.
>>>
>>> smcv
>>>
>>
>
^ permalink raw reply
* Re: [PATCH 1/2] crypto: sm3 - add a new alias name sm3-256
From: Eric Biggers @ 2020-02-10 18:36 UTC (permalink / raw)
To: Ken Goldman
Cc: Van Leeuwen, Pascal, James Bottomley, Ken Goldman, Tianjia Zhang,
herbert, davem, zohar, dmitry.kasatkin, jmorris, serge,
linux-crypto, linux-integrity, linux-security-module,
linux-kernel
In-Reply-To: <9683f764-c8c7-e123-b5f6-4f155bd1b10b@linux.ibm.com>
[Please fix your email client; you dropped all non-list recipients from Cc,
and I had to manually add them back...]
On Mon, Feb 10, 2020 at 01:02:42PM -0500, Ken Goldman wrote:
> On 2/10/2020 12:01 PM, Van Leeuwen, Pascal wrote:
> > Well, the current specification surely doesn't define anything else and is
> > already over a decade old. So what would be the odds that they add a
> > different blocksize variant_now_ AND still call that SM3-something?
>
> I just got a note from a cryptographer who said there were discussions last
> year about a future SM3 with 512 bit output.
>
> Given that, why not plan ahead and use sm3-256? Is there any downside?
> Is the cost any more than 4 bytes in some source code?
If renaming sm3 to sm3-256 in the crypto API, no. If adding sm3-256 alongside
sm3, then yes there is a cost to that because from the crypto API's perspective
they will be separate algorithms that each need to be registered, tested, etc.
- Eric
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-10 18:56 UTC (permalink / raw)
To: Stephen Smalley, Simon McVittie
Cc: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, keescook, penguin-kernel, paul
In-Reply-To: <04442c9f-430e-c922-b078-7cff8f36a45f@tycho.nsa.gov>
On 2/10/20 6:55 AM, Stephen Smalley wrote:
> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>> previous discussions.
>>>
>>> I thought in previous discussions, we had come to the conclusion that
>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>> API, that's even better.)
>>>
>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>> that the same thing you mean?
>>
>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
>
> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname?
>
yes sadly, as much as I would like to depracate/remove it the pathname based profile names are a byte string. AppArmor supports a more restricted profile name and I have been trying to get people to move to it for 12 years with only limited success.
>>> I thought the conclusion we had come to in previous conversations was
>>> that the LSM context is what GLib calls a "bytestring", the same as
>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>> with no further guarantees, and no specified encoding or mapping to/from
>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>> but nobody knows which one, and they could equally well be some binary
>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>
>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>> APIs for user-space features built over it. If that isn't possible, the
>>> next best thing is a "bytestring" like filenames, environment variables,
>>> and most kernel <-> user-space strings in general.
>>>
>>> smcv
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: John Johansen @ 2020-02-10 19:00 UTC (permalink / raw)
To: Casey Schaufler, Stephen Smalley, Simon McVittie
Cc: casey.schaufler, jmorris, linux-security-module, selinux,
keescook, penguin-kernel, paul
In-Reply-To: <37fa9076-6f15-0261-3bcf-1883236f9c3f@schaufler-ca.com>
On 2/10/20 10:32 AM, Casey Schaufler wrote:
> On 2/10/2020 6:55 AM, Stephen Smalley wrote:
>> On 2/10/20 8:25 AM, Stephen Smalley wrote:
>>> On 2/10/20 6:56 AM, Simon McVittie wrote:
>>>> On Mon, 03 Feb 2020 at 13:54:45 -0500, Stephen Smalley wrote:
>>>>> The printable ASCII bit is based on what the dbus maintainer requested in
>>>>> previous discussions.
>>>>
>>>> I thought in previous discussions, we had come to the conclusion that
>>>> I can't assume it's 7-bit ASCII. (If I *can* assume that for this new
>>>> API, that's even better.)
>>>>
>>>> To be clear, when I say ASCII I mean a sequence of bytes != '\0' with
>>>> their high bit unset (x & 0x7f == x) and the obvious mapping to/from
>>>> Unicode (bytes '\1' to '\x7f' represent codepoints U+0001 to U+007F). Is
>>>> that the same thing you mean?
>>>
>>> I mean the subset of 7-bit ASCII that satisfies isprint() using the "C" locale. That is already true for SELinux with the existing interfaces. I can't necessarily speak for the others.
>>
>> Looks like Smack labels are similarly restricted, per Documentation/admin-guide/LSM/Smack.rst. So I guess the only one that is perhaps unclear is AppArmor, since its labels are typically derived from pathnames? Can an AppArmor label returned via its getprocattr() hook be any legal pathname?
>
> Because attr/context (and later, SO_PEERCONTEXT) are new interfaces
> there is no need to exactly duplicate what is in attr/current (later
> SO_PEERSEC). I already plan to omit the "mode" component of the
> AppArmor data in the AppArmor hook, as was discussed earlier. I would
> prefer ASCII, but if AppArmor needs bytestrings, that's what we'll
> have to do.
>
sadly, to not break userspace its a byte string because that is what the path based profile names are. AppArmor does support a more limited non path based profile name but I can't guarantee that is what userspace is using in policy.
>>
>>>> I thought the conclusion we had come to in previous conversations was
>>>> that the LSM context is what GLib calls a "bytestring", the same as
>>>> filenames and environment variables - an opaque sequence of bytes != '\0',
>>>> with no further guarantees, and no specified encoding or mapping to/from
>>>> Unicode (most likely some superset of ASCII like UTF-8 or Latin-1,
>>>> but nobody knows which one, and they coould equally well be some binary
>>>> encoding with no Unicode meaning, as long as it avoids '\0').
>>>>
>>>> If I can safely assume that a new kernel <-> user-space API is constrained
>>>> to UTF-8 or a UTF-8 subset like ASCII, then I can provide more friendly
>>>> APIs for user-space features built over it. If that isn't possible, the
>>>> next best thing is a "bytestring" like filenames, environment variables,
>>>> and most kernel <-> user-space strings in general.
>>>>
>>>> smcv
>>>>
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-10 19:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexey Gladkov, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wh05FniF0xJYqcFrmGeCvOJUqR0UL4jTC-_LvpsfNCkNw@mail.gmail.com>
On Mon, Feb 10, 2020 at 09:46:26AM -0800, Linus Torvalds wrote:
> On Mon, Feb 10, 2020 at 7:06 AM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
> >
> > This allows to flush dcache entries of a task on multiple procfs mounts
> > per pid namespace.
> >
> > The RCU lock is used because the number of reads at the task exit time
> > is much larger than the number of procfs mounts.
>
> Ok, this looks better to me than the previous version.
>
> But that may be the "pee-in-the-snow" effect, and I _really_ want
> others to take a good look at the whole series.
>
> The right people seem to be cc'd, but this is pretty core, and /proc
> has a tendency to cause interesting issues because of how it's
> involved in a lot of areas indirectly.
>
> Al, Oleg, Andy, Eric?
Will check tonight (ears-deep in sorting out the old branches right now)
^ permalink raw reply
* Re: [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Eric Snowberg @ 2020-02-10 19:24 UTC (permalink / raw)
To: Mimi Zohar
Cc: Nayna, dmitry.kasatkin, jmorris, serge, dhowells, geert, gregkh,
nayna, tglx, bauerman, mpe, linux-integrity,
linux-security-module, linux-kernel, Roberto Sassu
In-Reply-To: <1581354556.5585.827.camel@linux.ibm.com>
> On Feb 10, 2020, at 10:09 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2020-02-10 at 09:34 -0700, Eric Snowberg wrote:
>>> On Feb 8, 2020, at 4:43 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>
>>> On Fri, 2020-02-07 at 14:38 -0700, Eric Snowberg wrote:
>>>>> On Feb 7, 2020, at 11:54 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>
>>>>> On Fri, 2020-02-07 at 11:45 -0700, Eric Snowberg wrote:
>>>>>>
>>>>>>> On Feb 7, 2020, at 11:28 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> On Fri, 2020-02-07 at 10:49 -0700, Eric Snowberg wrote:
>>>>>>>>
>>>>>>>>> On Feb 7, 2020, at 10:40 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
>>>>>>>>>
>>>>>>>>>> $ insmod ./foo.ko
>>>>>>>>>> insmod: ERROR: could not insert module ./foo.ko: Permission denied
>>>>>>>>>>
>>>>>>>>>> last entry from audit log:
>>>>>>>>>> type=INTEGRITY_DATA msg=audit(1581089373.076:83): pid=2874 uid=0
>>>>>>>>>> auid=0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-
>>>>>>>>>> s0:c0.c1023 op=appraise_data cause=invalid-signature comm="insmod"
>>>>>>>>>> name="/root/keys/modules/foo.ko" dev="dm-0" ino=10918365
>>>>>>>>>> res=0^]UID="root" AUID=“root"
>>>>>>>>>>
>>>>>>>>>> This is because modsig_verify() will be called from within
>>>>>>>>>> ima_appraise_measurement(),
>>>>>>>>>> since try_modsig is true. Then modsig_verify() will return
>>>>>>>>>> INTEGRITY_FAIL.
>>>>>>>>>
>>>>>>>>> Why is it an "invalid signature"? For that you need to look at the
>>>>>>>>> kernel messages. Most likely it can't find the public key on the .ima
>>>>>>>>> keyring to verify the signature.
>>>>>>>>
>>>>>>>> It is invalid because the module has not been ima signed.
>>>>>>>
>>>>>>> With the IMA policy rule "appraise func=MODULE_CHECK
>>>>>>> appraise_type=imasig|modsig", IMA first tries to verify the IMA
>>>>>>> signature stored as an xattr and on failure then attempts to verify
>>>>>>> the appended signatures.
>>>>>>>
>>>>>>> The audit message above indicates that there was a signature, but the
>>>>>>> signature validation failed.
>>>>>>>
>>>>>>
>>>>>> I do have CONFIG_IMA_APPRAISE_MODSIG enabled. I believe the audit message above
>>>>>> is coming from modsig_verify in security/integrity/ima/ima_appraise.c.
>>>>>
>>>>> Right, and it's calling:
>>>>>
>>>>> rc = integrity_modsig_verify(INTEGRITY_KEYRING_IMA, modsig);
>>>>>
>>>>> It's failing because it is trying to find the public key on the .ima
>>>>> keyring. Make sure that the public needed to validate the kernel
>>>>> module is on the IMA keyring (eg. keyctl show %keyring:.ima).
>>>>>
>>>>
>>>> I know that will validate the module properly, but that is not what I’m
>>>> trying to solve here. I thought the point of adding “|modsig” to the
>>>> ima policy was to tell ima it can either validate against an ima keyring OR
>>>> default back to the kernel keyring. This is what happens with the compressed
>>>> module. There isn’t anything in the ima keyring to validate the compressed
>>>> modules and it loads when I add “|modsig”.
>>>
>>> "modsig" has nothing to do with keyrings. The term "modsig" is
>>> juxtaposed to "imasig". "modsig" refers to kernel module appended
>>> signature.
>>
>> Ok, understood, “modsig” refers to strictly kernel module appended signatures
>> without regard to the keyring that verifies it. Since there are inconsistencies
>> here, would you consider something like my first patch? It will verify an
>> uncompressed kernel module containing an appended signature when the public key
>> is contained within the kernel keyring instead of the ima keyring. Why force a
>> person to add the same keys into the ima keyring for validation? Especially when
>> the kernel keyring is now used to verify appended signatures in the compressed
>> modules.
>
> Different use case scenarios have different requirements. Suppose for
> example that the group creating the kernel image is not the same as
> using it. The group using the kernel image could sign all files,
> including kernel modules (imasig), with their own private key. Only
> files that they signed would be permitted. Your proposal would break
> the current expectations, allowing kernel modules signed by someone
> else to be loaded.
>
All the end user needs to do is compress any module created by the group that built
the original kernel image to work around the scenario above. Then the appended
signature in the compressed module will be verified by the kernel keyring. Does
this mean there is a security problem that should be fixed, if this is a concern?
For the use case above, wouldn’t it be better to use a module policy like:
appraise func=MODULE_CHECK appraise_type=imasig
Obviously the downside is it disables appended signatures. It would prevent
compressed modules from loading, and only allow ima signed modules to load.
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Mimi Zohar @ 2020-02-10 20:24 UTC (permalink / raw)
To: david.safford, Janne Karhunen, linux-integrity,
linux-security-module
Cc: Ken Goldman, monty.wiseman, Amir Goldstein, linux-fsdevel
In-Reply-To: <40f780ffe2ddc879e5fa4443c098c0f1d331390f.camel@gmail.com>
On Mon, 2020-02-10 at 13:18 -0500, david.safford@gmail.com wrote:
> On Thu, 2020-02-06 at 09:13 -0500, Mimi Zohar wrote:
> > Hi Janne,
> >
> > On Fri, 2020-01-10 at 10:48 +0200, Janne Karhunen wrote:
> > > On Wed, Jan 8, 2020 at 1:18 PM Janne Karhunen <janne.karhunen@gmail.com> wrote:
> > > > Some systems can end up carrying lots of entries in the ima
> > > > measurement list. Since every entry is using a bit of kernel
> > > > memory, allow the sysadmin to export the measurement list to
> > > > the filesystem to free up some memory.
> > >
> > > Hopefully this addressed comments from everyone. The flush event can
> > > now be triggered by the admin anytime and unique file names can be
> > > used for each flush (log.1, log.2, ...) etc, so getting to the correct
> > > item should be easy.
> > >
> > > While it can now be argued that since this is an admin-driven event,
> > > kernel does not need to write the file. However, the intention is to
> > > bring out a second patch a bit later that adds a variable to define
> > > the max number of entries to be kept in the kernel memory and
> > > workqueue based automatic flushing. In those cases the kernel has to
> > > be able to write the file without any help from the admin..
> >
> > The implications of exporting and removing records from the IMA-
> > measurement list needs to be considered carefully. Verifying a TPM
> > quote will become dependent on knowing where the measurements are
> > stored. The existing measurement list is stored in kernel memory and,
> > barring a kernel memory attack, is protected from modification.
> > Before upstreaming this or a similar patch, there needs to be a
> > discussion as to how the measurement list will be protected once is it
> > exported to userspace.
>
> "Protected" here can mean two different aspects: cryptographically
> protected from tampering, which is covered with the TPM_QUOTE, and
> availability protected from even accidental deletion, which is what
> I suspect you are concerned about. Certainly my original TLV patches
> were too flippant about this, as userspace had to be trusted not to
> drop any records. In this patch, the kernel writes the data in an
> atomic fashion. Either all records are successfully written, or none
> are, and an error is returned.
A third aspect, which I'm concerned about, is removing records from
the measurement list. This changes the existing userspace
expectations of returning the entire measurement list. Now userspace
will need some out of band method of knowing where to look for the
measurements.
>
> > This patch now attempts to address two very different scenarios. The
> > first scenario is where userspace is requesting exporting and removing
> > of the measurement list records. The other scenario is the kernel
> > exporting and removing of the measurement list records. Conflating
> > these two different use cases might not be the right solution, as we
> > originally thought.
>
> Actually there are at least four significant use cases: userspace
> requested, and kernel initiated, both for running out of memory or
> for saving the list prior to a kexec. Exporting everything to a file
> prior to kexec can really simplify all the vaious use cases of
> template vs TLV formatted lists across kexec. (Consider a modern
> TLV firmware kernel wanting to boot an older kernel that only
> understands template formats. How simple it would be for the first
> kernel to export its list to a file, and the second kernel keeps
> its list in template.)
When Thiago and I added support for carrying the measurement list
across kexec, there were a number of additional measurements after the
kexec load. These additional measurements will need to be safely
written out to file in order to validate the TPM quote.
> I have been testing this patch on all of these scenarios, and it
> provides a simple, powerful approach for all of them.
Were you able to walk the measurement list and validate the TPM quote
after a kexec?
>
> > The kernel already exports the IMA measurement list to userspace via a
> > securityfs file. From a userspace perspective, missing is the ability
> > of removing N number of records. In this scenario, userspace would be
> > responsible for safely storing the measurements (e.g. blockchain).
> > The kernel would only be responsible for limiting permission, perhaps
> > based on a capability, before removing records from the measurement
> > list.
>
> I don't think we want to export 'N' records, as this would
> be really hard to understand and coordinate with userspace.
> Exporting all or none seems simpler.
Userspace already has the ability of exporting the measurement list.
However, beetween saving the measurement list to a file and telling
the kernel to delete the records from the kernel, additional
measurement could have been added.
>
> > In the kernel usecase, somehow the kernel would need to safely export
> > the measurement list, or some portion of the measurement list, to a
> > file and then delete that portion. What protects the exported records
> > stored in a file from modification?
>
> Tampering is prevented with the TPM_QUOTE. Accidental deletion is
> protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you
> have bigger problems, and even then it will be detected.
Agreed, attestation will detect any tampering, but up to now we didn't
have to rely on DAC/MAC to prevent tampering of the measurement list.
> > Instead of exporting the measurement records, one option as suggested
> > by Amir Goldstein, would be to use a vfs_tmpfile() to get an anonymous
> > file for backing store. The existing securityfs measurement lists
> > would then read from this private copy of the anonymous file.
>
> This doesn't help in use cases where we really do want to
> export to a persistent file, without userspace help.
Is to prevent needing to carry the measurement list across kexec the
only reason for the kernel needing to write to a persistent file?
Mimi
^ permalink raw reply
* Re: [RFC PATCH 0/2] ima: uncompressed module appraisal support
From: Mimi Zohar @ 2020-02-10 20:33 UTC (permalink / raw)
To: Eric Snowberg
Cc: Nayna, dmitry.kasatkin, jmorris, serge, dhowells, geert, gregkh,
nayna, tglx, bauerman, mpe, linux-integrity,
linux-security-module, linux-kernel, Roberto Sassu
In-Reply-To: <90E53A33-530B-40FB-9982-2818FFD78D73@oracle.com>
On Mon, 2020-02-10 at 12:24 -0700, Eric Snowberg wrote:
> > On Feb 10, 2020, at 10:09 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >>
> >> Ok, understood, “modsig” refers to strictly kernel module appended signatures
> >> without regard to the keyring that verifies it. Since there are inconsistencies
> >> here, would you consider something like my first patch? It will verify an
> >> uncompressed kernel module containing an appended signature when the public key
> >> is contained within the kernel keyring instead of the ima keyring. Why force a
> >> person to add the same keys into the ima keyring for validation? Especially when
> >> the kernel keyring is now used to verify appended signatures in the compressed
> >> modules.
> >
> > Different use case scenarios have different requirements. Suppose for
> > example that the group creating the kernel image is not the same as
> > using it. The group using the kernel image could sign all files,
> > including kernel modules (imasig), with their own private key. Only
> > files that they signed would be permitted. Your proposal would break
> > the current expectations, allowing kernel modules signed by someone
> > else to be loaded.
> >
>
> All the end user needs to do is compress any module created by the group that built
> the original kernel image to work around the scenario above. Then the appended
> signature in the compressed module will be verified by the kernel keyring. Does
> this mean there is a security problem that should be fixed, if this is a concern?
Again, the issue isn't compressed/uncompressed kernel modules, but the
syscall used to load the kernel module. IMA can prevent using the the
init_module syscall. Refer to the ima_load_data() LOADING_MODULE
case.
Mimi
^ permalink raw reply
* [GIT PULL] SELinux fixes for v5.6 (#1)
From: Paul Moore @ 2020-02-10 21:27 UTC (permalink / raw)
To: Linus Torvalds; +Cc: selinux, linux-security-module, linux-kernel
Hi Linus,
Two small fixes for v5.6-rcX: one fixes a locking problem in the
recently merged label translation code, the other fixes an
embarrassing "binderfs"/"binder" filesystem name check.
Please merge for the next -rcX release, thank you.
-Paul
--
The following changes since commit 98aa00345de54b8340dc2ddcd87f446d33387b5e:
selinux: fix regression introduced by move_mount(2) syscall
(2020-01-20 07:42:37 -0500)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
tags/selinux-pr-20200210
for you to fetch changes up to 39a706fbcf2694bfb651bed9041d44c3f4fa8078:
selinux: fix sidtab string cache locking (2020-02-05 18:31:10 -0500)
----------------------------------------------------------------
selinux/stable-5.6 PR 20200210
----------------------------------------------------------------
Hridya Valsaraju (1):
selinux: fix typo in filesystem name
Ondrej Mosnacek (1):
selinux: fix sidtab string cache locking
security/selinux/hooks.c | 2 +-
security/selinux/ss/sidtab.c | 12 +++---------
2 files changed, 4 insertions(+), 10 deletions(-)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Mimi Zohar @ 2020-02-10 22:23 UTC (permalink / raw)
To: Roberto Sassu, James.Bottomley, jarkko.sakkinen
Cc: linux-integrity, linux-security-module, linux-kernel,
silviu.vlasceanu, stable
In-Reply-To: <20200210100048.21448-3-roberto.sassu@huawei.com>
On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> boot_aggregate is the first entry of IMA measurement list. Its purpose is
> to link pre-boot measurements to IMA measurements. As IMA was designed to
> work with a TPM 1.2, the SHA1 PCR bank was always selected.
>
> Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> However, the assumption that the SHA1 PCR bank is always available is not
> correct, as PCR banks can be selected with the PCR_Allocate() TPM command.
>
> This patch tries to use ima_hash_algo as hash algorithm for boot_aggregate.
> If no PCR bank uses that algorithm, the patch tries to find the SHA256 PCR
> bank (which is mandatory in the TCG PC Client specification).
Up to here, the patch description matches the code.
> If also this
> bank is not found, the patch selects the first one. If the TPM algorithm
> of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
This comment and the one inline are left over from previous version.
>
> Changelog
>
> v1:
> - add Mimi's comments
> - if there is no PCR bank for the IMA default algorithm use SHA256
> (suggested by James Bottomley)
>
> Cc: stable@vger.kernel.org # 5.1.x
> Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with PCR read")
> Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> Suggested-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Thanks, Roberto. This patch is dependent on 1/8. As soon as there's
a topic branch, I'll queue this, removing the extraneous comments.
Mimi
> ---
> security/integrity/ima/ima_crypto.c | 45 +++++++++++++++++++++++++----
> security/integrity/ima/ima_init.c | 22 ++++++++++----
> 2 files changed, 56 insertions(+), 11 deletions(-)
>
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 73044fc6a952..f2f41a2bc3d4 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
> }
>
> /*
> - * Calculate the boot aggregate hash
> + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7. With
> + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but with
> + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> + * allowing firmware to configure and enable different banks.
> + *
> + * Knowing which TPM bank is read to calculate the boot_aggregate digest
> + * needs to be conveyed to a verifier. For this reason, use the same
> + * hash algorithm for reading the TPM PCRs as for calculating the boot
> + * aggregate digest as stored in the measurement list.
> */
> -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> struct crypto_shash *tfm)
> {
> - struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> + struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> int rc;
> u32 i;
> SHASH_DESC_ON_STACK(shash, tfm);
>
> shash->tfm = tfm;
>
> + pr_devel("calculating the boot-aggregate based on TPM bank: %04x\n",
> + d.alg_id);
> +
> rc = crypto_shash_init(shash);
> if (rc != 0)
> return rc;
> @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
> for (i = TPM_PCR0; i < TPM_PCR8; i++) {
> ima_pcrread(i, &d);
> /* now accumulate with current aggregate */
> - rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
> + rc = crypto_shash_update(shash, d.digest,
> + crypto_shash_digestsize(tfm));
> }
> if (!rc)
> crypto_shash_final(shash, digest);
> @@ -685,14 +697,35 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
> int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
> {
> struct crypto_shash *tfm;
> - int rc;
> + u16 crypto_id, alg_id;
> + int rc, i, bank_idx = 0;
> +
> + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> + if (crypto_id == hash->algo) {
> + bank_idx = i;
> + break;
> + }
> +
> + if (crypto_id == HASH_ALGO_SHA256)
> + bank_idx = i;
> + }
> +
> + if (bank_idx == 0 &&
> + ima_tpm_chip->allocated_banks[0].crypto_id == HASH_ALGO__LAST) {
> + pr_err("No suitable TPM algorithm for boot aggregate\n");
> + return 0;
> + }
> +
> + hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
>
> tfm = ima_alloc_tfm(hash->algo);
> if (IS_ERR(tfm))
> return PTR_ERR(tfm);
>
> hash->length = crypto_shash_digestsize(tfm);
> - rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> + alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> + rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
>
> ima_free_tfm(tfm);
>
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index 5d55ade5f3b9..fbd7a8e28a6b 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
> /* Add the boot aggregate to the IMA measurement list and extend
> * the PCR register.
> *
> - * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
> + * Calculate the boot aggregate, a hash over tpm registers 0-7,
> * assuming a TPM chip exists, and zeroes if the TPM chip does not
> * exist. Add the boot aggregate measurement to the measurement
> * list and extend the PCR register.
> @@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
> int violation = 0;
> struct {
> struct ima_digest_data hdr;
> - char digest[TPM_DIGEST_SIZE];
> + char digest[TPM_MAX_DIGEST_SIZE];
> } hash;
>
> memset(iint, 0, sizeof(*iint));
> memset(&hash, 0, sizeof(hash));
> iint->ima_hash = &hash.hdr;
> - iint->ima_hash->algo = HASH_ALGO_SHA1;
> - iint->ima_hash->length = SHA1_DIGEST_SIZE;
> -
> + iint->ima_hash->algo = ima_hash_algo;
> + iint->ima_hash->length = hash_digest_size[ima_hash_algo];
> +
> + /*
> + * With TPM 2.0 hash agility, TPM chips could support multiple TPM
> + * PCR banks, allowing firmware to configure and enable different
> + * banks. The SHA1 bank is not necessarily enabled.
> + *
> + * Use the same hash algorithm for reading the TPM PCRs as for
> + * calculating the boot aggregate digest. Preference is given to
> + * the configured IMA default hash algorithm. Otherwise, use the
> + * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2. If
> + * SHA256 is not available, use the first PCR bank and if that is
> + * not mapped to a crypto ID, set boot_aggregate to zero.
The last line of the above comment is left over from the previous
version.
> + */
> if (ima_tpm_chip) {
> result = ima_calc_boot_aggregate(&hash.hdr);
> if (result < 0) {
^ permalink raw reply
* Re: [PATCH bpf-next v3 02/10] bpf: lsm: Add a skeleton and config options
From: Alexei Starovoitov @ 2020-02-10 23:52 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200123152440.28956-3-kpsingh@chromium.org>
On Thu, Jan 23, 2020 at 07:24:32AM -0800, KP Singh wrote:
>
> +BPF SECURITY MODULE
> +M: KP Singh <kpsingh@chromium.org>
> +L: linux-security-module@vger.kernel.org
> +L: bpf@vger.kernel.org
> +S: Maintained
> +F: security/bpf/
Instead of creating new entry I think it's more appropriate
to add this to main BPF entry like:
diff --git a/MAINTAINERS b/MAINTAINERS
index c74e4ea714a5..f656ddec0722 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3147,6 +3147,7 @@ R: Martin KaFai Lau <kafai@fb.com>
R: Song Liu <songliubraving@fb.com>
R: Yonghong Song <yhs@fb.com>
R: Andrii Nakryiko <andriin@fb.com>
+R: KP Singh <kpsingh@chromium.org>
L: netdev@vger.kernel.org
L: bpf@vger.kernel.org
T: git git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git
@@ -3172,6 +3173,7 @@ F: samples/bpf/
F: tools/bpf/
F: tools/lib/bpf/
F: tools/testing/selftests/bpf/
+F: security/bpf/
K: bpf
N: bpf
^ permalink raw reply related
* Re: [PATCH bpf-next v3 03/10] bpf: lsm: Introduce types for eBPF based LSM
From: Alexei Starovoitov @ 2020-02-10 23:58 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200123152440.28956-4-kpsingh@chromium.org>
On Thu, Jan 23, 2020 at 07:24:33AM -0800, KP Singh wrote:
> +
> +static const struct bpf_func_proto *get_bpf_func_proto(
> + enum bpf_func_id func_id, const struct bpf_prog *prog)
> +{
> + switch (func_id) {
> + case BPF_FUNC_map_lookup_elem:
> + return &bpf_map_lookup_elem_proto;
> + case BPF_FUNC_get_current_pid_tgid:
> + return &bpf_get_current_pid_tgid_proto;
> + default:
> + return NULL;
> + }
> +}
> +
> +const struct bpf_verifier_ops lsm_verifier_ops = {
> + .get_func_proto = get_bpf_func_proto,
> +};
Why artificially limit it like this?
It will cause a lot of churn in the future. Like allowing map update and
delete, in addition to lookup, will be an obvious next step.
I think allowing tracing_func_proto() from the start is cleaner.
^ permalink raw reply
* Re: [RFC PATCH 00/14] pipe: Keyrings, Block and USB notifications [ver #3]
From: Andres Freund @ 2020-02-11 0:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Howells, Greg Kroah-Hartman, Casey Schaufler,
Stephen Smalley, Nicolas Dichtel, Ian Kent, Christian Brauner,
keyrings, linux-usb, linux-block, LSM List, linux-fsdevel,
Linux API, Linux Kernel Mailing List
In-Reply-To: <CAHk-=wjrrOgznCy3yUmcmQY1z_7vXVr6GbvKiy8cLvWbxpmzcw@mail.gmail.com>
Hi,
I only just now noticed this work after Dave Chinner pointed towards the
feature in the email leading to
https://lore.kernel.org/linux-fsdevel/20200211000405.5fohxgpt554gmnhu@alap3.anarazel.de/
On 2020-01-15 12:10:32 -0800, Linus Torvalds wrote:
> So I no longer hate the implementation, but I do want to see the
> actual user space users come out of the woodwork and try this out for
> their use cases.
Postgres has been looking for something roughly like this, fwiw (or
well, been forced to).
While it's better than it used to be (due to b4678df184b3), we still
have problems to reliably detect buffered IO errors, especially when
done across multiple processes. We can't easily keep an fd open that
predates all writes to a file until, and ensure that fsyncs will happen
only on that fd. The primary reasons for that are
1) every connection (& some internal jobs) is a process, and neither do
want to to fsyncing each touched file in short-lived connections, nor is
it desirable to have to add the complication of having to transfer fds
between processes just to reliably get an error in fsync().
2) we have to cope with having more files open than allowed, so we have
a layer that limits the number of OS level FDs open at the same time. We
don't want to fsync whenever we have to juggle open fds though, as
that'd be too costly.
So it'd good to have a way to *reliably* know when writeback io failed,
so we can abort a checkpoint if necessary, and instead perform journal
replay.
For our purposes we'd probably want errors on the fs/superblock level,
rather than block devices. It's not always easy to map between blockdevs
and relevant filesystems, there are errors above the block layer, and we
definitely don'tt want to crash & restart a database just because
somebody pulled an USB storage device that didn't have any of the
database's data on it.
An earlier version of this patchset had some support for that, albeit
perhaps not fully implemented (no errors raised, afaict?):
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=notifications&id=338eec77a0cb29a7d2ae9350066c1990408ae58e
Is the plan to pick this up again once the basic feature is in?
A few notes from the email referenced above (that actually seem to belong
into this thread more than the other:
1) From the angle of reliably needing to detect writeback errors, I find it
somewhat concerning that an LSM may end up entirely filtering away error
notifications, without a consumer being able to detect that:
+void __post_watch_notification(struct watch_list *wlist,
+ struct watch_notification *n,
+ const struct cred *cred,
+ u64 id)
+{
...
+ if (security_post_notification(watch->cred, cred, n) < 0)
+ continue;
It's an unpleasant thought that an overly restrictive [-ly configured]
LSM could lead to silently swallowing data integrity errors.
2) It'd be good if there were documentation, aimed at userland consumers
of this, explaining what the delivery guarantees are. To be useful for
us, it needs to be guaranteed that consuming all notifications ensures
that there are no pending notifications queued up somewhere (so we can
do fsync(data); fsync(journal); check_for_errors();
durable_rename(checkpoint_state.tmp, checkpoint_state);).
3) What will the permission model for accessing the notifications be?
It seems currently anyone, even within a container/namespace or
something, will see blockdev errors from everywhere? The earlier
superblock support (I'm not sure I like that name btw, hard to
understand for us userspace folks), seems to have required exec
permission, but nothing else.
Greetings,
Andres Freund
^ permalink raw reply
* Re: [GIT PULL] SELinux fixes for v5.6 (#1)
From: pr-tracker-bot @ 2020-02-11 1:10 UTC (permalink / raw)
To: Paul Moore; +Cc: Linus Torvalds, selinux, linux-security-module, linux-kernel
In-Reply-To: <CAHC9VhRegJC9MP2pAGV_htOVNiBYSzxJ3xBFk7trwRpLniBY+w@mail.gmail.com>
The pull request you sent on Mon, 10 Feb 2020 16:27:42 -0500:
> git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git tags/selinux-pr-20200210
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a5650acb5f33d8c7c26941136b2d3c46b8a243ea
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-11 1:36 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <20200210150519.538333-8-gladkov.alexey@gmail.com>
Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> This allows to flush dcache entries of a task on multiple procfs mounts
> per pid namespace.
>
> The RCU lock is used because the number of reads at the task exit time
> is much larger than the number of procfs mounts.
A couple of quick comments.
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
> fs/proc/base.c | 20 +++++++++++++++-----
> fs/proc/root.c | 27 ++++++++++++++++++++++++++-
> include/linux/pid_namespace.h | 2 ++
> include/linux/proc_fs.h | 2 ++
> 4 files changed, 45 insertions(+), 6 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 4ccb280a3e79..24b7c620ded3 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> .permission = proc_pid_permission,
> };
>
> -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
Perhaps just rename things like:
> +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid)
> {
I don't think the mnt_ prefix conveys any information, and it certainly
makes everything longer and more cumbersome.
> struct dentry *dentry, *leader, *dir;
> char buf[10 + 1];
> @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> name.name = buf;
> name.len = snprintf(buf, sizeof(buf), "%u", pid);
> /* no ->d_hash() rejects on procfs */
> - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
> + dentry = d_hash_and_lookup(mnt_root, &name);
> if (dentry) {
> d_invalidate(dentry);
> dput(dentry);
> @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>
> name.name = buf;
> name.len = snprintf(buf, sizeof(buf), "%u", tgid);
> - leader = d_hash_and_lookup(mnt->mnt_root, &name);
> + leader = d_hash_and_lookup(mnt_root, &name);
> if (!leader)
> goto out;
>
> @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task)
> int i;
> struct pid *pid, *tgid;
> struct upid *upid;
> + struct dentry *mnt_root;
> + struct proc_fs_info *fs_info;
>
> pid = task_pid(task);
> tgid = task_tgid(task);
>
> for (i = 0; i <= pid->level; i++) {
> upid = &pid->numbers[i];
> - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
> - tgid->numbers[i].nr);
> +
> + rcu_read_lock();
> + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
> + mnt_root = fs_info->m_super->s_root;
> + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
> + }
> + rcu_read_unlock();
> +
> + mnt_root = upid->ns->proc_mnt->mnt_root;
> + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
I don't think this following of proc_mnt is needed. It certainly
shouldn't be. The loop through all of the super blocks should be
enough.
Once this change goes through. UML can be given it's own dedicated
proc_mnt for the initial pid namespace, and proc_mnt can be removed
entirely.
Unless something has changed recently UML is the only other user of
pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in
proc_flush_task easy to write.
It also probably makes sense to take the rcu_read_lock() over
that entire for loop.
> }
> }
>
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 5d5cba4c899b..e2bb015da1a8 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -149,7 +149,22 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
> if (ret) {
> return ret;
> }
> - return proc_setup_thread_self(s);
> +
> + ret = proc_setup_thread_self(s);
> + if (ret) {
> + return ret;
> + }
> +
> + /*
> + * back reference to flush dcache entries at process exit time.
> + */
> + ctx->fs_info->m_super = s;
> +
> + spin_lock(&pid_ns->proc_mounts_lock);
> + list_add_tail_rcu(&ctx->fs_info->pidns_entry, &pid_ns->proc_mounts);
> + spin_unlock(&pid_ns->proc_mounts_lock);
> +
> + return 0;
> }
>
> static int proc_reconfigure(struct fs_context *fc)
> @@ -211,10 +226,17 @@ static void proc_kill_sb(struct super_block *sb)
> {
> struct proc_fs_info *fs_info = proc_sb_info(sb);
>
> + spin_lock(&fs_info->pid_ns->proc_mounts_lock);
> + list_del_rcu(&fs_info->pidns_entry);
> + spin_unlock(&fs_info->pid_ns->proc_mounts_lock);
> +
> + synchronize_rcu();
> +
Rather than a heavyweight synchronize_rcu here,
it probably makes sense to instead to change
kfree(fs_info)
to
kfree_rcu(fs_info, some_rcu_head_in_fs_info)
Or possibly doing a full call_rcu.
The problem is that synchronize_rcu takes about 10ms when HZ=100.
Which makes synchronize_rcu incredibly expensive on any path where
anything can wait for it.
> if (fs_info->proc_self)
> dput(fs_info->proc_self);
> if (fs_info->proc_thread_self)
> dput(fs_info->proc_thread_self);
> +
> kill_anon_super(sb);
> put_pid_ns(fs_info->pid_ns);
> kfree(fs_info);
> @@ -336,6 +358,9 @@ int pid_ns_prepare_proc(struct pid_namespace *ns)
> ctx->fs_info->pid_ns = ns;
> }
>
> + spin_lock_init(&ns->proc_mounts_lock);
> + INIT_LIST_HEAD_RCU(&ns->proc_mounts);
> +
> mnt = fc_mount(fc);
> put_fs_context(fc);
> if (IS_ERR(mnt))
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 66f47f1afe0d..c36af1dfd862 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -26,6 +26,8 @@ struct pid_namespace {
> struct pid_namespace *parent;
> #ifdef CONFIG_PROC_FS
> struct vfsmount *proc_mnt; /* Internal proc mounted during each new pidns */
> + spinlock_t proc_mounts_lock;
> + struct list_head proc_mounts; /* list of separated procfs mounts */
> #endif
> #ifdef CONFIG_BSD_PROCESS_ACCT
> struct fs_pin *bacct;
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 5f0b1b7e4271..f307940f8311 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -20,6 +20,8 @@ enum {
> };
>
> struct proc_fs_info {
> + struct list_head pidns_entry; /* Node in procfs_mounts of a pidns */
> + struct super_block *m_super;
> struct pid_namespace *pid_ns;
> struct dentry *proc_self; /* For /proc/self */
> struct dentry *proc_thread_self; /* For /proc/thread-self */
Eric
^ permalink raw reply
* Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: Alexei Starovoitov @ 2020-02-11 3:12 UTC (permalink / raw)
To: KP Singh
Cc: linux-kernel, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
kernel-team
In-Reply-To: <20200123152440.28956-5-kpsingh@chromium.org>
On Thu, Jan 23, 2020 at 07:24:34AM -0800, KP Singh wrote:
> +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) ({ \
> + int _ret = 0; \
> + do { \
> + struct security_hook_list *P; \
> + int _idx; \
> + \
> + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> + break; \
> + \
> + _idx = bpf_lsm_srcu_read_lock(); \
> + \
> + hlist_for_each_entry(P, \
> + &bpf_lsm_hook_heads.FUNC, list) { \
> + _ret = P->hook.FUNC(__VA_ARGS__); \
> + if (_ret && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> + break; \
> + } \
> + bpf_lsm_srcu_read_unlock(_idx); \
> + } while (0); \
> + IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? _ret : 0; \
> +})
This extra CONFIG_SECURITY_BPF_ENFORCE doesn't make sense to me.
Why do all the work for bpf-lsm and ignore return code? Such framework already
exists. For audit only case the user could have kprobed security_*() in
security/security.c and had access to exactly the same data. There is no need
in any of these patches if audit the only use case.
Obviously bpf-lsm has to be capable of making go/no-go decision, so
my preference is to drop this extra kconfig knob.
I think the agreement seen in earlier comments in this thread that the prefered
choice is to always have bpf-based lsm to be equivalent to LSM_ORDER_LAST. In
such case how about using bpf-trampoline fexit logic instead?
Pros:
- no changes to security/ directory
- no changes to call_int_hook() macro
- patches 4, 5, 6 no longer necessary
- when security is off all security_*() hooks do single
if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) check.
With patch 4 there will two such checks. Tiny perf penalty.
With fexit approach there will be no extra check.
- fexit approach is fast even on kernels compiled with retpoline, since
its using direct calls
Cons:
- bpf trampoline so far is x86 only and arm64 support is wip
By plugging into fexit I'm proposing to let bpf-lsm prog type modify return
value. Currently bpf-fexit prog type has read-only access to it. Adding write
access is a straightforward verifier change. The bpf progs from patch 9 will
still look exactly the same way:
SEC("lsm/file_mprotect")
int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
unsigned long reqprot, unsigned long prot) { ... }
The difference that libbpf will be finding btf_id of security_file_mprotect()
function and adding fexit trampoline to it instead of going via
security_list_options and its own lsm_hook_idx uapi. I think reusing existing
tracing facilities to attach and multiplex multiple programs is cleaner. More
code reuse. Unified testing of lsm and tracing, etc.
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-11 4:01 UTC (permalink / raw)
To: Alexey Gladkov
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <87v9odlxbr.fsf@x220.int.ebiederm.org>
ebiederm@xmission.com (Eric W. Biederman) writes:
> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
>
>> This allows to flush dcache entries of a task on multiple procfs mounts
>> per pid namespace.
>>
>> The RCU lock is used because the number of reads at the task exit time
>> is much larger than the number of procfs mounts.
>
> A couple of quick comments.
>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
>> Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
>> ---
>> fs/proc/base.c | 20 +++++++++++++++-----
>> fs/proc/root.c | 27 ++++++++++++++++++++++++++-
>> include/linux/pid_namespace.h | 2 ++
>> include/linux/proc_fs.h | 2 ++
>> 4 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 4ccb280a3e79..24b7c620ded3 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
>> .permission = proc_pid_permission,
>> };
>>
>> -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>> +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
> Perhaps just rename things like:
>> +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid)
>> {
>
> I don't think the mnt_ prefix conveys any information, and it certainly
> makes everything longer and more cumbersome.
>
>> struct dentry *dentry, *leader, *dir;
>> char buf[10 + 1];
>> @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>> name.name = buf;
>> name.len = snprintf(buf, sizeof(buf), "%u", pid);
>> /* no ->d_hash() rejects on procfs */
>> - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
>> + dentry = d_hash_and_lookup(mnt_root, &name);
>> if (dentry) {
>> d_invalidate(dentry);
>> dput(dentry);
>> @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
>>
>> name.name = buf;
>> name.len = snprintf(buf, sizeof(buf), "%u", tgid);
>> - leader = d_hash_and_lookup(mnt->mnt_root, &name);
>> + leader = d_hash_and_lookup(mnt_root, &name);
>> if (!leader)
>> goto out;
>>
>> @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task)
>> int i;
>> struct pid *pid, *tgid;
>> struct upid *upid;
>> + struct dentry *mnt_root;
>> + struct proc_fs_info *fs_info;
>>
>> pid = task_pid(task);
>> tgid = task_tgid(task);
>>
>> for (i = 0; i <= pid->level; i++) {
>> upid = &pid->numbers[i];
>> - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
>> - tgid->numbers[i].nr);
>> +
>> + rcu_read_lock();
>> + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
>> + mnt_root = fs_info->m_super->s_root;
>> + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>> + }
>> + rcu_read_unlock();
>> +
>> + mnt_root = upid->ns->proc_mnt->mnt_root;
>> + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
>
> I don't think this following of proc_mnt is needed. It certainly
> shouldn't be. The loop through all of the super blocks should be
> enough.
>
> Once this change goes through. UML can be given it's own dedicated
> proc_mnt for the initial pid namespace, and proc_mnt can be removed
> entirely.
>
> Unless something has changed recently UML is the only other user of
> pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in
> proc_flush_task easy to write.
>
> It also probably makes sense to take the rcu_read_lock() over
> that entire for loop.
>
>
>> }
>> }
>>
>> diff --git a/fs/proc/root.c b/fs/proc/root.c
>> index 5d5cba4c899b..e2bb015da1a8 100644
>> --- a/fs/proc/root.c
>> +++ b/fs/proc/root.c
>> @@ -149,7 +149,22 @@ static int proc_fill_super(struct super_block *s, struct fs_context *fc)
>> if (ret) {
>> return ret;
>> }
>> - return proc_setup_thread_self(s);
>> +
>> + ret = proc_setup_thread_self(s);
>> + if (ret) {
>> + return ret;
>> + }
>> +
>> + /*
>> + * back reference to flush dcache entries at process exit time.
>> + */
>> + ctx->fs_info->m_super = s;
>> +
>> + spin_lock(&pid_ns->proc_mounts_lock);
>> + list_add_tail_rcu(&ctx->fs_info->pidns_entry, &pid_ns->proc_mounts);
>> + spin_unlock(&pid_ns->proc_mounts_lock);
>> +
>> + return 0;
>> }
>>
>> static int proc_reconfigure(struct fs_context *fc)
>> @@ -211,10 +226,17 @@ static void proc_kill_sb(struct super_block *sb)
>> {
>> struct proc_fs_info *fs_info = proc_sb_info(sb);
>>
>> + spin_lock(&fs_info->pid_ns->proc_mounts_lock);
>> + list_del_rcu(&fs_info->pidns_entry);
>> + spin_unlock(&fs_info->pid_ns->proc_mounts_lock);
>> +
>> + synchronize_rcu();
>> +
>
> Rather than a heavyweight synchronize_rcu here,
> it probably makes sense to instead to change
>
> kfree(fs_info)
> to
> kfree_rcu(fs_info, some_rcu_head_in_fs_info)
>
> Or possibly doing a full call_rcu.
>
> The problem is that synchronize_rcu takes about 10ms when HZ=100.
> Which makes synchronize_rcu incredibly expensive on any path where
> anything can wait for it.
I take it back.
The crucial thing to insert a rcu delay before is
shrink_dcache_for_umount.
With the call chain:
kill_anon_super
generic_shutdown_super
shrink_dcache_for_umount.
So we do need synchronize_rcu or to put everything below this point into
a call_rcu function.
The practical concern is mntput calls does task_work_add.
The task_work function is cleanup_mnt.
When called cleanup_mnt calls deactivate_locked_super
which in turn calls proc_kill_sb.
Which is a long way of saying that the code will delay the
exit of the process that calls mntput by 10ms (or
however syncrhonize_rcu runs).
We probably don't care. But that is long enough in to be user
visible, and potentially cause problems.
But all it requires putting the code below in it's own
function and triggering it with call_rcu if that causes a regression
by bying so slow.
>> if (fs_info->proc_self)
>> dput(fs_info->proc_self);
>> if (fs_info->proc_thread_self)
>> dput(fs_info->proc_thread_self);
>> +
>> kill_anon_super(sb);
>> put_pid_ns(fs_info->pid_ns);
>> kfree(fs_info);
>> @@ -336,6 +358,9 @@ int pid_ns_prepare_proc(struct pid_namespace *ns)
>> ctx->fs_info->pid_ns = ns;
>> }
>>
>> + spin_lock_init(&ns->proc_mounts_lock);
>> + INIT_LIST_HEAD_RCU(&ns->proc_mounts);
>> +
>> mnt = fc_mount(fc);
>> put_fs_context(fc);
>> if (IS_ERR(mnt))
>> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
>> index 66f47f1afe0d..c36af1dfd862 100644
>> --- a/include/linux/pid_namespace.h
>> +++ b/include/linux/pid_namespace.h
>> @@ -26,6 +26,8 @@ struct pid_namespace {
>> struct pid_namespace *parent;
>> #ifdef CONFIG_PROC_FS
>> struct vfsmount *proc_mnt; /* Internal proc mounted during each new pidns */
>> + spinlock_t proc_mounts_lock;
>> + struct list_head proc_mounts; /* list of separated procfs mounts */
>> #endif
>> #ifdef CONFIG_BSD_PROCESS_ACCT
>> struct fs_pin *bacct;
>> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
>> index 5f0b1b7e4271..f307940f8311 100644
>> --- a/include/linux/proc_fs.h
>> +++ b/include/linux/proc_fs.h
>> @@ -20,6 +20,8 @@ enum {
>> };
>>
>> struct proc_fs_info {
>> + struct list_head pidns_entry; /* Node in procfs_mounts of a pidns */
>> + struct super_block *m_super;
>> struct pid_namespace *pid_ns;
>> struct dentry *proc_self; /* For /proc/self */
>> struct dentry *proc_thread_self; /* For /proc/thread-self */
>
>
> Eric
^ permalink raw reply
* RE: [PATCH 1/2] crypto: rename sm3-256 to sm3 in hash_algo_name
From: Van Leeuwen, Pascal @ 2020-02-11 7:47 UTC (permalink / raw)
To: Tianjia Zhang, herbert@gondor.apana.org.au, davem@davemloft.net,
zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, jmorris@namei.org,
serge@hallyn.com, ebiggers@kernel.org
Cc: linux-crypto@vger.kernel.org, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <42b6db00b1864da9ad76a8d5964fdb74@MN2PR20MB2973.namprd20.prod.outlook.com>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Tianjia Zhang
> Sent: Monday, February 10, 2020 1:45 PM
> To: herbert@gondor.apana.org.au; davem@davemloft.net; zohar@linux.ibm.com; dmitry.kasatkin@gmail.com; jmorris@namei.org;
> serge@hallyn.com; ebiggers@kernel.org
> Cc: linux-crypto@vger.kernel.org; linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH 1/2] crypto: rename sm3-256 to sm3 in hash_algo_name
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> The name sm3-256 is defined in hash_algo_name in hash_info, but the
> algorithm name implemented in sm3_generic.c is sm3, which will cause
> the sm3-256 algorithm to be not found in some application scenarios of
> the hash algorithm, and an ENOENT error will occur. For example,
> IMA, keys, and other subsystems that reference hash_algo_name all use
> the hash algorithm of sm3.
>
> Signed-off-by: Tianjia Zhang <tianjia.zhang@linux.alibaba.com>
Reviewed-by: Pascal van Leeuwen <pvanleeuwen@rambus.com>
> ---
> crypto/hash_info.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/hash_info.c b/crypto/hash_info.c
> index c754cb75dd1a..a49ff96bde77 100644
> --- a/crypto/hash_info.c
> +++ b/crypto/hash_info.c
> @@ -26,7 +26,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
> [HASH_ALGO_TGR_128] = "tgr128",
> [HASH_ALGO_TGR_160] = "tgr160",
> [HASH_ALGO_TGR_192] = "tgr192",
> - [HASH_ALGO_SM3_256] = "sm3-256",
> + [HASH_ALGO_SM3_256] = "sm3",
> [HASH_ALGO_STREEBOG_256] = "streebog256",
> [HASH_ALGO_STREEBOG_512] = "streebog512",
> };
> --
> 2.17.1
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
^ permalink raw reply
* RE: [PATCH 1/2] crypto: sm3 - add a new alias name sm3-256
From: Van Leeuwen, Pascal @ 2020-02-11 7:56 UTC (permalink / raw)
To: Ken Goldman
Cc: linux-crypto@vger.kernel.org, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <3b21122352a44cb9a20030a32f07e38a@MN2PR20MB2973.namprd20.prod.outlook.com>
> -----Original Message-----
> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Ken Goldman
> Sent: Monday, February 10, 2020 7:03 PM
> Cc: linux-crypto@vger.kernel.org; linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] crypto: sm3 - add a new alias name sm3-256
>
> <<< External Email >>>
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the
> sender/sender address and know the content is safe.
>
>
> On 2/10/2020 12:01 PM, Van Leeuwen, Pascal wrote:
> > Well, the current specification surely doesn't define anything else and is
> > already over a decade old. So what would be the odds that they add a
> > different blocksize variant_now_ AND still call that SM3-something?
>
> I just got a note from a cryptographer who said there were discussions
> last year about a future SM3 with 512 bit output.
>
> Given that, why not plan ahead and use sm3-256? Is there any downside?
> Is the cost any more than 4 bytes in some source code?
>
It is actually exported as "sm3" by all implementations, it's just this one reference that was off.
So fixing that one reference is less effort than fixing all implementations.
I don't think anyone cares about the 4 bytes of source code ...
As for SM3-512: that would by silly, considering recent attacks found against similar
Merkle-Darmgard structures. Then again, I'm not talking to Chinese cryptographers.
In any case, what would be the problem with having "sm3" and "sm3-512"?
Note that nobody in the world refers to the current SM3 as "SM3-256".
Regards,
Pascal van Leeuwen
Silicon IP Architect Multi-Protocol Engines, Rambus Security
Rambus ROTW Holding BV
+31-73 6581953
Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus.
Please be so kind to update your e-mail address book with my new e-mail address.
** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. **
Rambus Inc.<http://www.rambus.com>
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: Janne Karhunen @ 2020-02-11 8:06 UTC (permalink / raw)
To: Mimi Zohar
Cc: david.safford, linux-integrity, linux-security-module,
Ken Goldman, Wiseman, Monty (GE Global Research, US),
Amir Goldstein, linux-fsdevel
In-Reply-To: <1581366258.5585.891.camel@linux.ibm.com>
On Mon, Feb 10, 2020 at 10:25 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> A third aspect, which I'm concerned about, is removing records from
> the measurement list. This changes the existing userspace
> expectations of returning the entire measurement list. Now userspace
> will need some out of band method of knowing where to look for the
> measurements.
Well, the original patch has a mechanism with one bit flip. You can
read the list name given that you are in the right namespace and/or
mount.
> > > The kernel already exports the IMA measurement list to userspace via a
> > > securityfs file. From a userspace perspective, missing is the ability
> > > of removing N number of records. In this scenario, userspace would be
> > > responsible for safely storing the measurements (e.g. blockchain).
> > > The kernel would only be responsible for limiting permission, perhaps
> > > based on a capability, before removing records from the measurement
> > > list.
> >
> > I don't think we want to export 'N' records, as this would
> > be really hard to understand and coordinate with userspace.
> > Exporting all or none seems simpler.
>
> Userspace already has the ability of exporting the measurement list.
> However, beetween saving the measurement list to a file and telling
> the kernel to delete the records from the kernel, additional
> measurement could have been added.
This is not an issue as long as there is no 'ALL' alias. We can't
agree on what 'ALL' is, but we can agree on 'N'.
> > Tampering is prevented with the TPM_QUOTE. Accidental deletion is
> > protected with CAP_SYS_ADMIN. If CAP_SYS_ADMIN is untrusted, you
> > have bigger problems, and even then it will be detected.
>
> Agreed, attestation will detect any tampering, but up to now we didn't
> have to rely on DAC/MAC to prevent tampering of the measurement list.
True. How about storing a hmac of the file in a securityfs entry?
--
Janne
^ permalink raw reply
* RE: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot aggregate
From: Roberto Sassu @ 2020-02-11 10:09 UTC (permalink / raw)
To: Mimi Zohar, James.Bottomley@HansenPartnership.com,
jarkko.sakkinen@linux.intel.com
Cc: linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org, Silviu Vlasceanu,
stable@vger.kernel.org
In-Reply-To: <1581373420.5585.920.camel@linux.ibm.com>
> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, February 10, 2020 11:24 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>;
> James.Bottomley@HansenPartnership.com;
> jarkko.sakkinen@linux.intel.com
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v3 2/8] ima: Switch to ima_hash_algo for boot
> aggregate
>
> On Mon, 2020-02-10 at 11:00 +0100, Roberto Sassu wrote:
> > boot_aggregate is the first entry of IMA measurement list. Its purpose is
> > to link pre-boot measurements to IMA measurements. As IMA was
> designed to
> > work with a TPM 1.2, the SHA1 PCR bank was always selected.
> >
> > Currently, even if a TPM 2.0 is used, the SHA1 PCR bank is selected.
> > However, the assumption that the SHA1 PCR bank is always available is not
> > correct, as PCR banks can be selected with the PCR_Allocate() TPM
> command.
> >
> > This patch tries to use ima_hash_algo as hash algorithm for
> boot_aggregate.
> > If no PCR bank uses that algorithm, the patch tries to find the SHA256 PCR
> > bank (which is mandatory in the TCG PC Client specification).
>
> Up to here, the patch description matches the code.
> > If also this
> > bank is not found, the patch selects the first one. If the TPM algorithm
> > of that bank is not mapped to a crypto ID, boot_aggregate is set to zero.
>
> This comment and the one inline are left over from previous version.
Hi Mimi
actually the code does what is described above. bank_idx is initially
set to zero and remains as it is if there is no PCR bank for the default
IMA algorithm or SHA256.
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> > Changelog
> >
> > v1:
> > - add Mimi's comments
> > - if there is no PCR bank for the IMA default algorithm use SHA256
> > (suggested by James Bottomley)
> >
> > Cc: stable@vger.kernel.org # 5.1.x
> > Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms
> with PCR read")
> > Reported-by: Jerry Snitselaar <jsnitsel@redhat.com>
> > Suggested-by: James Bottomley
> <James.Bottomley@HansenPartnership.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>
> Thanks, Roberto. This patch is dependent on 1/8. As soon as there's
> a topic branch, I'll queue this, removing the extraneous comments.
>
> Mimi
>
> > ---
> > security/integrity/ima/ima_crypto.c | 45
> +++++++++++++++++++++++++----
> > security/integrity/ima/ima_init.c | 22 ++++++++++----
> > 2 files changed, 56 insertions(+), 11 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_crypto.c
> b/security/integrity/ima/ima_crypto.c
> > index 73044fc6a952..f2f41a2bc3d4 100644
> > --- a/security/integrity/ima/ima_crypto.c
> > +++ b/security/integrity/ima/ima_crypto.c
> > @@ -655,18 +655,29 @@ static void __init ima_pcrread(u32 idx, struct
> tpm_digest *d)
> > }
> >
> > /*
> > - * Calculate the boot aggregate hash
> > + * The boot_aggregate is a cumulative hash over TPM registers 0 - 7.
> With
> > + * TPM 1.2 the boot_aggregate was based on reading the SHA1 PCRs, but
> with
> > + * TPM 2.0 hash agility, TPM chips could support multiple TPM PCR banks,
> > + * allowing firmware to configure and enable different banks.
> > + *
> > + * Knowing which TPM bank is read to calculate the boot_aggregate
> digest
> > + * needs to be conveyed to a verifier. For this reason, use the same
> > + * hash algorithm for reading the TPM PCRs as for calculating the boot
> > + * aggregate digest as stored in the measurement list.
> > */
> > -static int __init ima_calc_boot_aggregate_tfm(char *digest,
> > +static int __init ima_calc_boot_aggregate_tfm(char *digest, u16 alg_id,
> > struct crypto_shash *tfm)
> > {
> > - struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
> > + struct tpm_digest d = { .alg_id = alg_id, .digest = {0} };
> > int rc;
> > u32 i;
> > SHASH_DESC_ON_STACK(shash, tfm);
> >
> > shash->tfm = tfm;
> >
> > + pr_devel("calculating the boot-aggregate based on TPM
> bank: %04x\n",
> > + d.alg_id);
> > +
> > rc = crypto_shash_init(shash);
> > if (rc != 0)
> > return rc;
> > @@ -675,7 +686,8 @@ static int __init ima_calc_boot_aggregate_tfm(char
> *digest,
> > for (i = TPM_PCR0; i < TPM_PCR8; i++) {
> > ima_pcrread(i, &d);
> > /* now accumulate with current aggregate */
> > - rc = crypto_shash_update(shash, d.digest,
> TPM_DIGEST_SIZE);
> > + rc = crypto_shash_update(shash, d.digest,
> > + crypto_shash_digestsize(tfm));
> > }
> > if (!rc)
> > crypto_shash_final(shash, digest);
> > @@ -685,14 +697,35 @@ static int __init
> ima_calc_boot_aggregate_tfm(char *digest,
> > int __init ima_calc_boot_aggregate(struct ima_digest_data *hash)
> > {
> > struct crypto_shash *tfm;
> > - int rc;
> > + u16 crypto_id, alg_id;
> > + int rc, i, bank_idx = 0;
> > +
> > + for (i = 0; i < ima_tpm_chip->nr_allocated_banks; i++) {
> > + crypto_id = ima_tpm_chip->allocated_banks[i].crypto_id;
> > + if (crypto_id == hash->algo) {
> > + bank_idx = i;
> > + break;
> > + }
> > +
> > + if (crypto_id == HASH_ALGO_SHA256)
> > + bank_idx = i;
> > + }
> > +
> > + if (bank_idx == 0 &&
> > + ima_tpm_chip->allocated_banks[0].crypto_id ==
> HASH_ALGO__LAST) {
> > + pr_err("No suitable TPM algorithm for boot aggregate\n");
> > + return 0;
> > + }
> > +
> > + hash->algo = ima_tpm_chip->allocated_banks[bank_idx].crypto_id;
> >
> > tfm = ima_alloc_tfm(hash->algo);
> > if (IS_ERR(tfm))
> > return PTR_ERR(tfm);
> >
> > hash->length = crypto_shash_digestsize(tfm);
> > - rc = ima_calc_boot_aggregate_tfm(hash->digest, tfm);
> > + alg_id = ima_tpm_chip->allocated_banks[bank_idx].alg_id;
> > + rc = ima_calc_boot_aggregate_tfm(hash->digest, alg_id, tfm);
> >
> > ima_free_tfm(tfm);
> >
> > diff --git a/security/integrity/ima/ima_init.c
> b/security/integrity/ima/ima_init.c
> > index 5d55ade5f3b9..fbd7a8e28a6b 100644
> > --- a/security/integrity/ima/ima_init.c
> > +++ b/security/integrity/ima/ima_init.c
> > @@ -27,7 +27,7 @@ struct tpm_chip *ima_tpm_chip;
> > /* Add the boot aggregate to the IMA measurement list and extend
> > * the PCR register.
> > *
> > - * Calculate the boot aggregate, a SHA1 over tpm registers 0-7,
> > + * Calculate the boot aggregate, a hash over tpm registers 0-7,
> > * assuming a TPM chip exists, and zeroes if the TPM chip does not
> > * exist. Add the boot aggregate measurement to the measurement
> > * list and extend the PCR register.
> > @@ -51,15 +51,27 @@ static int __init ima_add_boot_aggregate(void)
> > int violation = 0;
> > struct {
> > struct ima_digest_data hdr;
> > - char digest[TPM_DIGEST_SIZE];
> > + char digest[TPM_MAX_DIGEST_SIZE];
> > } hash;
> >
> > memset(iint, 0, sizeof(*iint));
> > memset(&hash, 0, sizeof(hash));
> > iint->ima_hash = &hash.hdr;
> > - iint->ima_hash->algo = HASH_ALGO_SHA1;
> > - iint->ima_hash->length = SHA1_DIGEST_SIZE;
> > -
> > + iint->ima_hash->algo = ima_hash_algo;
> > + iint->ima_hash->length = hash_digest_size[ima_hash_algo];
> > +
> > + /*
> > + * With TPM 2.0 hash agility, TPM chips could support multiple TPM
> > + * PCR banks, allowing firmware to configure and enable different
> > + * banks. The SHA1 bank is not necessarily enabled.
> > + *
> > + * Use the same hash algorithm for reading the TPM PCRs as for
> > + * calculating the boot aggregate digest. Preference is given to
> > + * the configured IMA default hash algorithm. Otherwise, use the
> > + * TPM required banks - SHA256 for TPM 2.0, SHA1 for TPM 1.2. If
> > + * SHA256 is not available, use the first PCR bank and if that is
> > + * not mapped to a crypto ID, set boot_aggregate to zero.
>
> The last line of the above comment is left over from the previous
> version.
>
> > + */
> > if (ima_tpm_chip) {
> > result = ima_calc_boot_aggregate(&hash.hdr);
> > if (result < 0) {
^ permalink raw reply
* Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: KP Singh @ 2020-02-11 12:43 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: KP Singh, linux-kernel, bpf, linux-security-module,
Brendan Jackman, Florent Revest, Thomas Garnier,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Jann Horn, Matthew Garrett, Christian Brauner,
Mickaël Salaün, Florent Revest, Brendan Jackman,
Serge E. Hallyn, Mauro Carvalho Chehab, David S. Miller,
Greg Kroah-Hartman, kernel-team
In-Reply-To: <20200211031208.e6osrcathampoog7@ast-mbp>
On 10-Feb 19:12, Alexei Starovoitov wrote:
> On Thu, Jan 23, 2020 at 07:24:34AM -0800, KP Singh wrote:
> > +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) ({ \
> > + int _ret = 0; \
> > + do { \
> > + struct security_hook_list *P; \
> > + int _idx; \
> > + \
> > + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> > + break; \
> > + \
> > + _idx = bpf_lsm_srcu_read_lock(); \
> > + \
> > + hlist_for_each_entry(P, \
> > + &bpf_lsm_hook_heads.FUNC, list) { \
> > + _ret = P->hook.FUNC(__VA_ARGS__); \
> > + if (_ret && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> > + break; \
> > + } \
> > + bpf_lsm_srcu_read_unlock(_idx); \
> > + } while (0); \
> > + IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? _ret : 0; \
> > +})
>
> This extra CONFIG_SECURITY_BPF_ENFORCE doesn't make sense to me.
We found it easier to debug the programs if enforcement is disabled.
But we can remove this option if you feel strongly about it.
> Why do all the work for bpf-lsm and ignore return code? Such framework already
> exists. For audit only case the user could have kprobed security_*() in
> security/security.c and had access to exactly the same data. There is no need
> in any of these patches if audit the only use case.
> Obviously bpf-lsm has to be capable of making go/no-go decision, so
> my preference is to drop this extra kconfig knob.
> I think the agreement seen in earlier comments in this thread that the prefered
> choice is to always have bpf-based lsm to be equivalent to LSM_ORDER_LAST. In
> such case how about using bpf-trampoline fexit logic instead?
Even if the BPF LSM is LSM_ORDER_LAST this is still different from
adding a trampoline to the exit of the security hooks for a few other
reasons.
> Pros:
> - no changes to security/ directory
* We do need to initialize the BPF LSM as a proper LSM (i.e. in
security/bpf) because it needs access to security blobs. This is
only possible statically for now as they should be set after boot
time to provide guarantees to any helper that uses information in
security blobs. I have proposed how we could make this dynamic as a
discussion topic for the BPF conference:
https://lore.kernel.org/bpf/20200127171516.GA2710@chromium.org
As you can see from some of the prototype use cases e.g:
https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c
that they do rely on security blobs and that they are key in doing
meaningful security analysis.
* When using the semantic provided by fexit, the BPF LSM program will
always be executed and will be able to override / clobber the
decision of LSMs which appear before it in the ordered list. This
semantic is very different from what we currently have (i.e. the BPF
LSM hook is only called if all the other LSMs allow the action) and
seems to be bypassing the LSM framework.
* Not all security_* wrappers simply call the attached hooks and return
their exit code and not all of them pass the same arguments to the
hook e.g. security_bprm_check, security_file_open,
security_task_alloc to just name a few. Illustrating this further
using security_task_alloc as an example:
rc = call_int_hook(task_alloc, 0, task, clone_flags);
if (unlikely(rc))
security_task_free(task);
return rc;
Which means we would leak task_structs in this case. While
call_int_hook is sort of equivalent to the fexit trampoline for most
hooks, it's not really the case for some (quite important) LSM hooks.
- KP
> - no changes to call_int_hook() macro
> - patches 4, 5, 6 no longer necessary
> - when security is off all security_*() hooks do single
> if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) check.
> With patch 4 there will two such checks. Tiny perf penalty.
> With fexit approach there will be no extra check.
> - fexit approach is fast even on kernels compiled with retpoline, since
> its using direct calls
> Cons:
> - bpf trampoline so far is x86 only and arm64 support is wip
>
> By plugging into fexit I'm proposing to let bpf-lsm prog type modify return
> value. Currently bpf-fexit prog type has read-only access to it. Adding write
> access is a straightforward verifier change. The bpf progs from patch 9 will
> still look exactly the same way:
> SEC("lsm/file_mprotect")
> int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
> unsigned long reqprot, unsigned long prot) { ... }
> The difference that libbpf will be finding btf_id of security_file_mprotect()
> function and adding fexit trampoline to it instead of going via
> security_list_options and its own lsm_hook_idx uapi. I think reusing existing
> tracing facilities to attach and multiplex multiple programs is cleaner. More
> code reuse. Unified testing of lsm and tracing, etc.
^ 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