* Re: [PATCH 06/18] fsinfo: Add a uniquifier ID to struct mount [ver #21]
From: Matthew Wilcox @ 2020-08-05 19:33 UTC (permalink / raw)
To: David Howells
Cc: Miklos Szeredi, Al Viro, Linus Torvalds, Ian Kent, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <2315925.1596641410@warthog.procyon.org.uk>
On Wed, Aug 05, 2020 at 04:30:10PM +0100, David Howells wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> > idr_alloc_cyclic() seems to be a good template for doing the lower
> > 32bit allocation, and we can add code to increment the high 32bit on
> > wraparound.
> >
> > Lots of code uses idr_alloc_cyclic() so I guess it shouldn't be too
> > bad in terms of memory use or performance.
>
> It's optimised for shortness of path and trades memory for performance. It's
> currently implemented using an xarray, so memory usage is dependent on the
> sparseness of the tree. Each node in the tree is 576 bytes and in the worst
> case, each one node will contain one mount - and then you have to backfill the
> ancestry, though for lower memory costs.
>
> Systemd makes life more interesting since it sets up a whole load of
> propagations. Each mount you make may cause several others to be created, but
> that would likely make the tree more efficient.
I would recommend using xa_alloc and ignoring the ID assigned from
xa_alloc. Looking up by unique ID is then a matter of iterating every
mount (xa_for_each()) looking for a matching unique ID in the mount
struct. That's O(n) search, but it's faster than a linked list, and we
don't have that many mounts in a system.
The maple tree will handle this case more effectively, but I can't
recommend waiting for that to be ready.
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: James Bottomley @ 2020-08-05 15:01 UTC (permalink / raw)
To: Deven Bowers, Pavel Machek, Sasha Levin
Cc: snitzer, zohar, dm-devel, tyhicks, agk, paul, corbet, jmorris,
nramas, serge, pasha.tatashin, jannh, linux-block, viro, axboe,
mdsakib, linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <fb35a1f7-7633-a678-3f0f-17cf83032d2b@linux.microsoft.com>
On Tue, 2020-08-04 at 09:07 -0700, Deven Bowers wrote:
> On 8/2/2020 9:43 AM, James Bottomley wrote:
> > On Sun, 2020-08-02 at 16:31 +0200, Pavel Machek wrote:
> > > On Sun 2020-08-02 10:03:00, Sasha Levin wrote:
> > > > On Sun, Aug 02, 2020 at 01:55:45PM +0200, Pavel Machek wrote:
> > > > > Hi!
> > > > >
> > > > > > IPE is a Linux Security Module which allows for a
> > > > > > configurable policy to enforce integrity requirements on
> > > > > > the whole system. It attempts to solve the issue of Code
> > > > > > Integrity: that any code being executed (or files being
> > > > > > read), are identical to the version that was built by a
> > > > > > trusted source.
> > > > >
> > > > > How is that different from security/integrity/ima?
> > > >
> > > > Maybe if you would have read the cover letter all the way down
> > > > to the 5th paragraph which explains how IPE is different from
> > > > IMA we could avoided this mail exchange...
> > >
> > > "
> > > IPE differs from other LSMs which provide integrity checking (for
> > > instance, IMA), as it has no dependency on the filesystem
> > > metadata itself.
> > > The attributes that IPE checks are deterministic properties that
> > > exist solely in the kernel. Additionally, IPE provides no
> > > additional mechanisms of verifying these files (e.g. IMA
> > > Signatures) - all of the attributes of verifying files are
> > > existing features within the kernel, such as dm-verity
> > > or fsverity.
> > > "
> > >
> > > That is not really helpful.
>
> Perhaps I can explain (and re-word this paragraph) a bit better.
>
> As James indicates, IPE does try to close the gap of the IMA
> limitation with xattr. I honestly wasn’t familiar with the appended
> signatures, which seems fine.
>
> Regardless, this isn’t the larger benefit that IPE provides. The
> larger benefit of this is how IPE separates _mechanisms_ (properties)
> to enforce integrity requirements, from _policy_. The LSM provides
> policy, while things like dm-verity provide mechanism.
Colour me confused here, but I thought that's exactly what IMA does.
The mechanism is the gates and the policy is simply a list of rules
which are applied when a gate is triggered. The policy necessarily has
to be tailored to the information available at the gate (so the bprm
exec gate knows filesystem things like the inode for instance) but the
whole thing looks very extensible.
> So to speak, IPE acts as the glue for other mechanisms to leverage a
> customizable, system-wide policy to enforce. While this initial
> patchset only onboards dm-verity, there’s also potential for MAC
> labels, fs-verity, authenticated BTRFS, dm-integrity, etc. IPE
> leverages existing systems in the kernel, while IMA uses its own.
Is this about who does the measurement? I think there's no reason at
all why IMA can't leverage existing measurements, it's just nothing to
leverage existed when it was created.
> Another difference is the general coverage. IMA has some difficulties
> in covering mprotect[1], IPE doesn’t (the MAP_ANONYMOUS indicated by
> Jann in that thread would be denied as the file struct would be null,
> with IPE’s current set of supported mechanisms. mprotect would
> continue to function as expected if you change to PROT_EXEC).
I don't really think a debate over who does what and why is productive
at this stage. I just note that IMA policy could be updated to deny
MAP_ANONYMOUS, but no-one's asked for that (probably because of the
huge application breakage that would ensue). The policy is a product
of the use case and the current use case for IMA is working with
existing filesystem semantics.
> > Perhaps the big question is: If we used the existing IMA appended
> > signature for detached signatures (effectively becoming the
> > "properties" referred to in the cover letter) and hooked IMA into
> > device mapper using additional policy terms, would that satisfy all
> > the requirements this new LSM has?
>
> Well, Mimi, what do you think? Should we integrate all the features
> of IPE into IMA, or do you think they are sufficiently different in
> architecture that it would be worth it to keep the code base in
> separate LSMs?
I'll leave Mimi to answer, but really this is exactly the question that
should have been asked before writing IPE. However, since we have the
cart before the horse, let me break the above down into two specific
questions.
1. Could we implement IPE in IMA (as in would extensions to IMA cover
everything). I think the answers above indicate this is a "yes".
2. Should we extend IMA to implement it? This is really whether from a
usability standpoint two seperate LSMs would make sense to cover the
different use cases. I've got to say the least attractive thing
about separation is the fact that you now both have a policy parser.
You've tried to differentiate yours by making it more Kconfig
based, but policy has a way of becoming user space supplied because
the distros hate config options, so I think you're going to end up
with a policy parser very like IMAs.
James
^ permalink raw reply
* Re: [PATCH 10/18] fsinfo: Provide notification overrun handling support [ver #21]
From: Miklos Szeredi @ 2020-08-05 11:27 UTC (permalink / raw)
To: Ian Kent
Cc: David Howells, Al Viro, Linus Torvalds, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <e1caad2bff5faf9b24b59fe4ee51df255412cc56.camel@themaw.net>
On Wed, Aug 5, 2020 at 1:23 PM Ian Kent <raven@themaw.net> wrote:
>
> On Wed, 2020-08-05 at 09:45 +0200, Miklos Szeredi wrote:
> > Hmm, what's the other possibility for lost notifications?
>
> In user space that is:
>
> Multi-threaded application races, single threaded applications and
> signal processing races, other bugs ...
Okay, let's fix the bugs then.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
From: Stephen Smalley @ 2020-08-05 13:03 UTC (permalink / raw)
To: Mimi Zohar
Cc: Lakshmi Ramasubramanian, Casey Schaufler, Tyler Hicks, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <31d00876438d2652890ab8bf6ba2e80f554ca7a4.camel@linux.ibm.com>
On Wed, Aug 5, 2020 at 8:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-08-05 at 08:46 -0400, Stephen Smalley wrote:
> > On 8/4/20 11:25 PM, Mimi Zohar wrote:
> >
> > > Hi Lakshmi,
> > >
> > > There's still a number of other patch sets needing to be reviewed
> > > before my getting to this one. The comment below is from a high level.
> > >
> > > On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
> > > > Critical data structures of security modules need to be measured to
> > > > enable an attestation service to verify if the configuration and
> > > > policies for the security modules have been setup correctly and
> > > > that they haven't been tampered with at runtime. A new IMA policy is
> > > > required for handling this measurement.
> > > >
> > > > Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
> > > > measure the state and the policy provided by the security modules.
> > > > Update ima_match_rules() and ima_validate_rule() to check for
> > > > the new func and ima_parse_rule() to handle the new func.
> > > I can understand wanting to measure the in kernel LSM memory state to
> > > make sure it hasn't changed, but policies are stored as files. Buffer
> > > measurements should be limited to those things that are not files.
> > >
> > > Changing how data is passed to the kernel has been happening for a
> > > while. For example, instead of passing the kernel module or kernel
> > > image in a buffer, the new syscalls - finit_module, kexec_file_load -
> > > pass an open file descriptor. Similarly, instead of loading the IMA
> > > policy data, a pathname may be provided.
> > >
> > > Pre and post security hooks already exist for reading files. Instead
> > > of adding IMA support for measuring the policy file data, update the
> > > mechanism for loading the LSM policy. Then not only will you be able
> > > to measure the policy, you'll also be able to require the policy be
> > > signed.
> >
> > To clarify, the policy being measured by this patch series is a
> > serialized representation of the in-memory policy data structures being
> > enforced by SELinux. Not the file that was loaded. Hence, this
> > measurement would detect tampering with the in-memory policy data
> > structures after the policy has been loaded. In the case of SELinux,
> > one can read this serialized representation via /sys/fs/selinux/policy.
> > The result is not byte-for-byte identical to the policy file that was
> > loaded but can be semantically compared via sediff and other tools to
> > determine whether it is equivalent.
>
> Thank you for the clarification. Could the policy hash be included
> with the other critical data? Does it really need to be measured
> independently?
They were split into two separate functions because we wanted to be
able to support using different templates for them (ima-buf for the
state variables so that the measurement includes the original buffer,
which is small and relatively fixed-size, and ima-ng for the policy
because it is large and we just want to capture the hash for later
comparison against known-good). Also, the state variables are
available for measurement always from early initialization, whereas
the policy is only available for measurement once we have loaded an
initial policy.
^ permalink raw reply
* Re: [PATCH 15/18] fsinfo: Add an attribute that lists all the visible mounts in a namespace [ver #21]
From: David Howells @ 2020-08-05 16:44 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, viro, torvalds, raven, mszeredi, christian, jannh,
darrick.wong, kzak, jlayton, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
In-Reply-To: <20200804140558.GF32719@miu.piliscsaba.redhat.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> > where each element contains a once-in-a-system-lifetime unique ID, the
> > mount ID (which may get reused), the parent mount ID and sums of the
> > notification/change counters for the mount and its superblock.
>
> The change counters are currently conditional on CONFIG_MOUNT_NOTIFICATIONS.
> Is this is intentional?
Yeah - the counters aren't driven unless CONFIG_MOUNT_NOTIFICATIONS=y.
I could perhaps make it so they're driven in both cases, but driving the
in-subtree counter is somewhat tied up in the notification posting.
This is something that can be fixed after this patchset is taken - if it is
taken since that doesn't change the UAPI.
David
^ permalink raw reply
* Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
From: Stephen Smalley @ 2020-08-05 12:46 UTC (permalink / raw)
To: Mimi Zohar, Lakshmi Ramasubramanian, casey
Cc: tyhicks, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <4b9d2715d3ef3c8f915ef03867cfb1a39c0abc54.camel@linux.ibm.com>
On 8/4/20 11:25 PM, Mimi Zohar wrote:
> Hi Lakshmi,
>
> There's still a number of other patch sets needing to be reviewed
> before my getting to this one. The comment below is from a high level.
>
> On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
>> Critical data structures of security modules need to be measured to
>> enable an attestation service to verify if the configuration and
>> policies for the security modules have been setup correctly and
>> that they haven't been tampered with at runtime. A new IMA policy is
>> required for handling this measurement.
>>
>> Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
>> measure the state and the policy provided by the security modules.
>> Update ima_match_rules() and ima_validate_rule() to check for
>> the new func and ima_parse_rule() to handle the new func.
> I can understand wanting to measure the in kernel LSM memory state to
> make sure it hasn't changed, but policies are stored as files. Buffer
> measurements should be limited to those things that are not files.
>
> Changing how data is passed to the kernel has been happening for a
> while. For example, instead of passing the kernel module or kernel
> image in a buffer, the new syscalls - finit_module, kexec_file_load -
> pass an open file descriptor. Similarly, instead of loading the IMA
> policy data, a pathname may be provided.
>
> Pre and post security hooks already exist for reading files. Instead
> of adding IMA support for measuring the policy file data, update the
> mechanism for loading the LSM policy. Then not only will you be able
> to measure the policy, you'll also be able to require the policy be
> signed.
To clarify, the policy being measured by this patch series is a
serialized representation of the in-memory policy data structures being
enforced by SELinux. Not the file that was loaded. Hence, this
measurement would detect tampering with the in-memory policy data
structures after the policy has been loaded. In the case of SELinux,
one can read this serialized representation via /sys/fs/selinux/policy.
The result is not byte-for-byte identical to the policy file that was
loaded but can be semantically compared via sediff and other tools to
determine whether it is equivalent.
^ permalink raw reply
* Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
From: Tyler Hicks @ 2020-08-05 15:07 UTC (permalink / raw)
To: Stephen Smalley
Cc: Mimi Zohar, Lakshmi Ramasubramanian, Casey Schaufler, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel, John Johansen
In-Reply-To: <CAEjxPJ7d1yg659OCU6diXXGqegc_jSzO4ZPhkRqQtJnRn-kC0g@mail.gmail.com>
On 2020-08-05 10:27:43, Stephen Smalley wrote:
> On Wed, Aug 5, 2020 at 9:20 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2020-08-05 at 09:03 -0400, Stephen Smalley wrote:
> > > On Wed, Aug 5, 2020 at 8:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > > On Wed, 2020-08-05 at 08:46 -0400, Stephen Smalley wrote:
> > > > > On 8/4/20 11:25 PM, Mimi Zohar wrote:
> > > > >
> > > > > > Hi Lakshmi,
> > > > > >
> > > > > > There's still a number of other patch sets needing to be reviewed
> > > > > > before my getting to this one. The comment below is from a high level.
> > > > > >
> > > > > > On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
> > > > > > > Critical data structures of security modules need to be measured to
> > > > > > > enable an attestation service to verify if the configuration and
> > > > > > > policies for the security modules have been setup correctly and
> > > > > > > that they haven't been tampered with at runtime. A new IMA policy is
> > > > > > > required for handling this measurement.
> > > > > > >
> > > > > > > Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
> > > > > > > measure the state and the policy provided by the security modules.
> > > > > > > Update ima_match_rules() and ima_validate_rule() to check for
> > > > > > > the new func and ima_parse_rule() to handle the new func.
> > > > > > I can understand wanting to measure the in kernel LSM memory state to
> > > > > > make sure it hasn't changed, but policies are stored as files. Buffer
> > > > > > measurements should be limited to those things that are not files.
> > > > > >
> > > > > > Changing how data is passed to the kernel has been happening for a
> > > > > > while. For example, instead of passing the kernel module or kernel
> > > > > > image in a buffer, the new syscalls - finit_module, kexec_file_load -
> > > > > > pass an open file descriptor. Similarly, instead of loading the IMA
> > > > > > policy data, a pathname may be provided.
> > > > > >
> > > > > > Pre and post security hooks already exist for reading files. Instead
> > > > > > of adding IMA support for measuring the policy file data, update the
> > > > > > mechanism for loading the LSM policy. Then not only will you be able
> > > > > > to measure the policy, you'll also be able to require the policy be
> > > > > > signed.
> > > > >
> > > > > To clarify, the policy being measured by this patch series is a
> > > > > serialized representation of the in-memory policy data structures being
> > > > > enforced by SELinux. Not the file that was loaded. Hence, this
> > > > > measurement would detect tampering with the in-memory policy data
> > > > > structures after the policy has been loaded. In the case of SELinux,
> > > > > one can read this serialized representation via /sys/fs/selinux/policy.
> > > > > The result is not byte-for-byte identical to the policy file that was
> > > > > loaded but can be semantically compared via sediff and other tools to
> > > > > determine whether it is equivalent.
> > > >
> > > > Thank you for the clarification. Could the policy hash be included
> > > > with the other critical data? Does it really need to be measured
> > > > independently?
> > >
> > > They were split into two separate functions because we wanted to be
> > > able to support using different templates for them (ima-buf for the
> > > state variables so that the measurement includes the original buffer,
> > > which is small and relatively fixed-size, and ima-ng for the policy
> > > because it is large and we just want to capture the hash for later
> > > comparison against known-good). Also, the state variables are
> > > available for measurement always from early initialization, whereas
> > > the policy is only available for measurement once we have loaded an
> > > initial policy.
> >
> > Ok, measuring the policy separately from other critical data makes
> > sense. Instead of measuring the policy, which is large, measure the
> > policy hash.
>
> I think that was the original approach. However, I had concerns with
> adding code to SELinux to compute a hash over the policy versus
> leaving that to IMA's existing policy and mechanism. If that's
> preferred I guess we can do it that way but seems less flexible and
> duplicative.
In AppArmor, we store the sha1 of the raw policy as the policy is
loaded. The hash is exposed to userspace in apparmorfs. See commit
5ac8c355ae00 ("apparmor: allow introspecting the loaded policy pre
internal transform").
It has proved useful as a mechanism for debugging as sometimes the
on-disk policy doesn't match the loaded policy and this can be a good
way to check that while providing support to users. John also mentions
checkpoint/restore in the commit message and I could certainly see how
the policy hashes would be useful in that scenario.
When thinking through how Lakshmi's series could be extended for
AppArmor support, I was thinking that the AppArmor policy measurement
would be a measurement of these hashes that we already have in place.
Perhaps there's some general usefulness in storing/exposing an SELinux
policy hash rather than only seeing it as duplicative property required
this measurement series?
Tyler
^ permalink raw reply
* Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
From: Stephen Smalley @ 2020-08-05 14:27 UTC (permalink / raw)
To: Mimi Zohar
Cc: Lakshmi Ramasubramanian, Casey Schaufler, Tyler Hicks, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <b7df114e8e0d276e66575b6970a1e459d1dd4196.camel@linux.ibm.com>
On Wed, Aug 5, 2020 at 9:20 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-08-05 at 09:03 -0400, Stephen Smalley wrote:
> > On Wed, Aug 5, 2020 at 8:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > > On Wed, 2020-08-05 at 08:46 -0400, Stephen Smalley wrote:
> > > > On 8/4/20 11:25 PM, Mimi Zohar wrote:
> > > >
> > > > > Hi Lakshmi,
> > > > >
> > > > > There's still a number of other patch sets needing to be reviewed
> > > > > before my getting to this one. The comment below is from a high level.
> > > > >
> > > > > On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
> > > > > > Critical data structures of security modules need to be measured to
> > > > > > enable an attestation service to verify if the configuration and
> > > > > > policies for the security modules have been setup correctly and
> > > > > > that they haven't been tampered with at runtime. A new IMA policy is
> > > > > > required for handling this measurement.
> > > > > >
> > > > > > Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
> > > > > > measure the state and the policy provided by the security modules.
> > > > > > Update ima_match_rules() and ima_validate_rule() to check for
> > > > > > the new func and ima_parse_rule() to handle the new func.
> > > > > I can understand wanting to measure the in kernel LSM memory state to
> > > > > make sure it hasn't changed, but policies are stored as files. Buffer
> > > > > measurements should be limited to those things that are not files.
> > > > >
> > > > > Changing how data is passed to the kernel has been happening for a
> > > > > while. For example, instead of passing the kernel module or kernel
> > > > > image in a buffer, the new syscalls - finit_module, kexec_file_load -
> > > > > pass an open file descriptor. Similarly, instead of loading the IMA
> > > > > policy data, a pathname may be provided.
> > > > >
> > > > > Pre and post security hooks already exist for reading files. Instead
> > > > > of adding IMA support for measuring the policy file data, update the
> > > > > mechanism for loading the LSM policy. Then not only will you be able
> > > > > to measure the policy, you'll also be able to require the policy be
> > > > > signed.
> > > >
> > > > To clarify, the policy being measured by this patch series is a
> > > > serialized representation of the in-memory policy data structures being
> > > > enforced by SELinux. Not the file that was loaded. Hence, this
> > > > measurement would detect tampering with the in-memory policy data
> > > > structures after the policy has been loaded. In the case of SELinux,
> > > > one can read this serialized representation via /sys/fs/selinux/policy.
> > > > The result is not byte-for-byte identical to the policy file that was
> > > > loaded but can be semantically compared via sediff and other tools to
> > > > determine whether it is equivalent.
> > >
> > > Thank you for the clarification. Could the policy hash be included
> > > with the other critical data? Does it really need to be measured
> > > independently?
> >
> > They were split into two separate functions because we wanted to be
> > able to support using different templates for them (ima-buf for the
> > state variables so that the measurement includes the original buffer,
> > which is small and relatively fixed-size, and ima-ng for the policy
> > because it is large and we just want to capture the hash for later
> > comparison against known-good). Also, the state variables are
> > available for measurement always from early initialization, whereas
> > the policy is only available for measurement once we have loaded an
> > initial policy.
>
> Ok, measuring the policy separately from other critical data makes
> sense. Instead of measuring the policy, which is large, measure the
> policy hash.
I think that was the original approach. However, I had concerns with
adding code to SELinux to compute a hash over the policy versus
leaving that to IMA's existing policy and mechanism. If that's
preferred I guess we can do it that way but seems less flexible and
duplicative.
^ permalink raw reply
* Re: [PATCH v6 1/4] IMA: Add func to measure LSM state and policy
From: Mimi Zohar @ 2020-08-05 13:19 UTC (permalink / raw)
To: Stephen Smalley
Cc: Lakshmi Ramasubramanian, Casey Schaufler, Tyler Hicks, sashal,
James Morris, linux-integrity, SElinux list, LSM List,
linux-kernel
In-Reply-To: <CAEjxPJ6X+Cqd5QtZBmNm2cujwbg-STfRF7_8i=Ny8yuc6z9BwQ@mail.gmail.com>
On Wed, 2020-08-05 at 09:03 -0400, Stephen Smalley wrote:
> On Wed, Aug 5, 2020 at 8:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > On Wed, 2020-08-05 at 08:46 -0400, Stephen Smalley wrote:
> > > On 8/4/20 11:25 PM, Mimi Zohar wrote:
> > >
> > > > Hi Lakshmi,
> > > >
> > > > There's still a number of other patch sets needing to be reviewed
> > > > before my getting to this one. The comment below is from a high level.
> > > >
> > > > On Tue, 2020-08-04 at 17:43 -0700, Lakshmi Ramasubramanian wrote:
> > > > > Critical data structures of security modules need to be measured to
> > > > > enable an attestation service to verify if the configuration and
> > > > > policies for the security modules have been setup correctly and
> > > > > that they haven't been tampered with at runtime. A new IMA policy is
> > > > > required for handling this measurement.
> > > > >
> > > > > Define two new IMA policy func namely LSM_STATE and LSM_POLICY to
> > > > > measure the state and the policy provided by the security modules.
> > > > > Update ima_match_rules() and ima_validate_rule() to check for
> > > > > the new func and ima_parse_rule() to handle the new func.
> > > > I can understand wanting to measure the in kernel LSM memory state to
> > > > make sure it hasn't changed, but policies are stored as files. Buffer
> > > > measurements should be limited to those things that are not files.
> > > >
> > > > Changing how data is passed to the kernel has been happening for a
> > > > while. For example, instead of passing the kernel module or kernel
> > > > image in a buffer, the new syscalls - finit_module, kexec_file_load -
> > > > pass an open file descriptor. Similarly, instead of loading the IMA
> > > > policy data, a pathname may be provided.
> > > >
> > > > Pre and post security hooks already exist for reading files. Instead
> > > > of adding IMA support for measuring the policy file data, update the
> > > > mechanism for loading the LSM policy. Then not only will you be able
> > > > to measure the policy, you'll also be able to require the policy be
> > > > signed.
> > >
> > > To clarify, the policy being measured by this patch series is a
> > > serialized representation of the in-memory policy data structures being
> > > enforced by SELinux. Not the file that was loaded. Hence, this
> > > measurement would detect tampering with the in-memory policy data
> > > structures after the policy has been loaded. In the case of SELinux,
> > > one can read this serialized representation via /sys/fs/selinux/policy.
> > > The result is not byte-for-byte identical to the policy file that was
> > > loaded but can be semantically compared via sediff and other tools to
> > > determine whether it is equivalent.
> >
> > Thank you for the clarification. Could the policy hash be included
> > with the other critical data? Does it really need to be measured
> > independently?
>
> They were split into two separate functions because we wanted to be
> able to support using different templates for them (ima-buf for the
> state variables so that the measurement includes the original buffer,
> which is small and relatively fixed-size, and ima-ng for the policy
> because it is large and we just want to capture the hash for later
> comparison against known-good). Also, the state variables are
> available for measurement always from early initialization, whereas
> the policy is only available for measurement once we have loaded an
> initial policy.
Ok, measuring the policy separately from other critical data makes
sense. Instead of measuring the policy, which is large, measure the
policy hash.
Mimi
^ permalink raw reply
* Re: [PATCH v6 0/4] LSM: Measure security module data
From: Tyler Hicks @ 2020-08-05 16:32 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Casey Schaufler, zohar, stephen.smalley.work, sashal, jmorris,
linux-integrity, selinux, linux-security-module, linux-kernel
In-Reply-To: <ae80581d-a34c-51f4-d4f9-94c1e341fd15@linux.microsoft.com>
On 2020-08-05 09:21:24, Lakshmi Ramasubramanian wrote:
> On 8/5/20 9:14 AM, Tyler Hicks wrote:
> > On 2020-08-05 09:07:48, Lakshmi Ramasubramanian wrote:
> > > On 8/5/20 8:45 AM, Tyler Hicks wrote:
> > > > On 2020-08-05 08:36:40, Casey Schaufler wrote:
> > > > > On 8/4/2020 6:14 PM, Lakshmi Ramasubramanian wrote:
> > > > > > On 8/4/20 6:04 PM, Casey Schaufler wrote:
> > > > > > > On 8/4/2020 5:43 PM, Lakshmi Ramasubramanian wrote:
> > > > > > > > Critical data structures of security modules are currently not measured.
> > > > > > > > Therefore an attestation service, for instance, would not be able to
> > > > > > > > attest whether the security modules are always operating with the policies
> > > > > > > > and configuration that the system administrator had setup. The policies
> > > > > > > > and configuration for the security modules could be tampered with by
> > > > > > > > malware by exploiting kernel vulnerabilities or modified through some
> > > > > > > > inadvertent actions on the system. Measuring such critical data would
> > > > > > > > enable an attestation service to better assess the state of the system.
> > > > > > >
> > > > > > > I still wonder why you're calling this an LSM change/feature when
> > > > > > > all the change is in IMA and SELinux. You're not putting anything
> > > > > > > into the LSM infrastructure, not are you using the LSM infrastructure
> > > > > > > to achieve your ends. Sure, you *could* support other security modules
> > > > > > > using this scheme, but you have a configuration dependency on
> > > > > > > SELinux, so that's at best going to be messy. If you want this to
> > > > > > > be an LSM "feature" you need to use the LSM hooking mechanism.
> > > > > >
> > > > > > >
> > > > > > > I'm not objecting to the feature. It adds value. But as you've
> > > > > > > implemented it it is either an IMA extension to SELinux, or an
> > > > > > > SELiux extension to IMA. Could AppArmor add hooks for this without
> > > > > > > changing the IMA code? It doesn't look like it to me.
> > > > > >
> > > > > > The check in IMA to allow the new IMA hook func LSM_STATE and LSM_POLICY when SELinux is enabled is just because SELinux is the only security module using these hooks now.
> > > > > >
> > > > > > To enable AppArmor, for instance, to use the new IMA hooks to measure state and policy would just require adding the check for CONFIG_SECURITY_APPARMOR. Other than that, there are no IMA changes needed to support AppArmor or other such security modules.
> > > > >
> > > > > This is exactly what I'm objecting to. What if a system has both SELinux
> > > > > and AppArmor compiled in? What if it has both enabled?
> > > >
> > > > The SELinux state and policy would be measured but the AppArmor
> > > > state/policy would be silently ignored. This isn't ideal as the IMA
> > > > policy author would need to read the kernel code to figure out which
> > > > LSMs are going to be measured.
> > >
> > > Tyler - I am not sure why AppArmor state\policy would be ignored when both
> > > SELinux and AppArmor are enabled. Could you please clarify?
> >
> > I think Casey is talking about now (when AppArmor is not supported by
> > this feature) and you're talking about the future (when AppArmor may be
> > supported by this feature).
>
> Got it - thanks for clarifying.
>
> But with the current code if SELinux is enabled on the system, but AppArmor
> is not and the IMA policy contains "measure func=LSM_STATE" then the policy
> will be rejected as "-EINVAL".
The AppArmor policy load? Yes, the load will fail.
What Casey is talking about is when SELinux and AppArmor are enabled in
the kernel config but AppArmor is active. This scenario is true in
distro kernels such as Ubuntu's kernel:
$ grep -e CONFIG_SECURITY_SELINUX= -e CONFIG_SECURITY_APPARMOR= /boot/config-5.4.0-42-generic
CONFIG_SECURITY_SELINUX=y
CONFIG_SECURITY_APPARMOR=y
$ cat /sys/kernel/security/lsm
lockdown,capability,yama,apparmor
Casey also likely has LSM stacking in mind here where SELinux and
AppArmor could both be active at the same time but the LSM stacking
series is not yet applied.
Tyler
> So the policy author would get a feedback even now.
> Correct me if I am wrong.
>
> -lakshmi
^ permalink raw reply
* Re: [PATCH v6 0/4] LSM: Measure security module data
From: Tyler Hicks @ 2020-08-05 15:45 UTC (permalink / raw)
To: Casey Schaufler
Cc: Lakshmi Ramasubramanian, zohar, stephen.smalley.work, sashal,
jmorris, linux-integrity, selinux, linux-security-module,
linux-kernel
In-Reply-To: <c7c168f2-e30b-d2c5-abcb-1b6919197474@schaufler-ca.com>
On 2020-08-05 08:36:40, Casey Schaufler wrote:
> On 8/4/2020 6:14 PM, Lakshmi Ramasubramanian wrote:
> > On 8/4/20 6:04 PM, Casey Schaufler wrote:
> >> On 8/4/2020 5:43 PM, Lakshmi Ramasubramanian wrote:
> >>> Critical data structures of security modules are currently not measured.
> >>> Therefore an attestation service, for instance, would not be able to
> >>> attest whether the security modules are always operating with the policies
> >>> and configuration that the system administrator had setup. The policies
> >>> and configuration for the security modules could be tampered with by
> >>> malware by exploiting kernel vulnerabilities or modified through some
> >>> inadvertent actions on the system. Measuring such critical data would
> >>> enable an attestation service to better assess the state of the system.
> >>
> >> I still wonder why you're calling this an LSM change/feature when
> >> all the change is in IMA and SELinux. You're not putting anything
> >> into the LSM infrastructure, not are you using the LSM infrastructure
> >> to achieve your ends. Sure, you *could* support other security modules
> >> using this scheme, but you have a configuration dependency on
> >> SELinux, so that's at best going to be messy. If you want this to
> >> be an LSM "feature" you need to use the LSM hooking mechanism.
> >
> >>
> >> I'm not objecting to the feature. It adds value. But as you've
> >> implemented it it is either an IMA extension to SELinux, or an
> >> SELiux extension to IMA. Could AppArmor add hooks for this without
> >> changing the IMA code? It doesn't look like it to me.
> >
> > The check in IMA to allow the new IMA hook func LSM_STATE and LSM_POLICY when SELinux is enabled is just because SELinux is the only security module using these hooks now.
> >
> > To enable AppArmor, for instance, to use the new IMA hooks to measure state and policy would just require adding the check for CONFIG_SECURITY_APPARMOR. Other than that, there are no IMA changes needed to support AppArmor or other such security modules.
>
> This is exactly what I'm objecting to. What if a system has both SELinux
> and AppArmor compiled in? What if it has both enabled?
The SELinux state and policy would be measured but the AppArmor
state/policy would be silently ignored. This isn't ideal as the IMA
policy author would need to read the kernel code to figure out which
LSMs are going to be measured.
>
> >
> > Please see Patch 1/4
> >
> > + else if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&
> > + strcmp(args[0].from, "LSM_STATE") == 0)
> > + entry->func = LSM_STATE;
> > + else if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&
> > + strcmp(args[0].from, "LSM_POLICY") == 0)
> > + entry->func = LSM_POLICY;
> >
> > And, if early boot measurement is needed for AppArmor the following change in IMA's Kconfig
> >
> > Patch 4/4
> >
> > +config IMA_QUEUE_EARLY_BOOT_DATA
> > bool
> > + depends on SECURITY_SELINUX || (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING)
> > default y
> >
> > If you think calling this an "LSM feature" is not appropriate, please suggest a better phrase.
>
> In the code above you are under CONFIG_SECURITY_SELINUX.
> I would suggest that it's an SELinux feature, so you should
> be using SELINUX_STATE and SELINUX_POLICY, as I suggested
> before. Just because SELinux has state and policy to measure
> doesn't mean that a different module might not have other data,
> such as history, that should be covered as well.
In addition to SELINUX_STATE and SELINUX_POLICY, we should also consider
the proposed LSM_STATE and LSM_POLICY func values but require an "lsm"
rule conditional.
So the current proposed rules:
measure func=LSM_STATE
measure func=LSM_POLICY
Would become:
measure func=LSM_STATE lsm=selinux
measure func=LSM_POLICY lsm=selinux
The following rules would be rejected:
measure func=LSM_STATE
measure func=LSM_POLICY
measure func=LSM_STATE lsm=apparmor
measure func=LSM_POLICY lsm=smack
Of course, the apparmor and smack rules could/would be allowed when
proper support is in place.
Tyler
>
> I realize that IMA already has compile time dependencies to
> determine which xattrs to measure. There's no reason that
> the xattr list couldn't be determined at boot time, with
> each security module providing the XATTR_NAME values it
> uses.
>
> >
> > But like I said above, with minimal change in IMA other security modules can be supported to measure STATE and POLICY data.
> >
> > -lakshmi
> >
> >
^ permalink raw reply
* Re: [PATCH v6 0/4] LSM: Measure security module data
From: Mimi Zohar @ 2020-08-05 12:37 UTC (permalink / raw)
To: Casey Schaufler, Lakshmi Ramasubramanian, stephen.smalley.work
Cc: tyhicks, sashal, jmorris, linux-integrity, selinux,
linux-security-module, linux-kernel
In-Reply-To: <f3971f35-309d-c3e5-9126-69add7ad4c11@schaufler-ca.com>
On Tue, 2020-08-04 at 18:04 -0700, Casey Schaufler wrote:
> On 8/4/2020 5:43 PM, Lakshmi Ramasubramanian wrote:
> > Critical data structures of security modules are currently not measured.
> > Therefore an attestation service, for instance, would not be able to
> > attest whether the security modules are always operating with the policies
> > and configuration that the system administrator had setup. The policies
> > and configuration for the security modules could be tampered with by
> > malware by exploiting kernel vulnerabilities or modified through some
> > inadvertent actions on the system. Measuring such critical data would
> > enable an attestation service to better assess the state of the system.
>
> I still wonder why you're calling this an LSM change/feature when
> all the change is in IMA and SELinux. You're not putting anything
> into the LSM infrastructure, not are you using the LSM infrastructure
> to achieve your ends. Sure, you *could* support other security modules
> using this scheme, but you have a configuration dependency on
> SELinux, so that's at best going to be messy. If you want this to
> be an LSM "feature" you need to use the LSM hooking mechanism.
>
> I'm not objecting to the feature. It adds value. But as you've
> implemented it it is either an IMA extension to SELinux, or an
> SELiux extension to IMA. Could AppArmor add hooks for this without
> changing the IMA code? It doesn't look like it to me.
Agreed. Without reviewing the patch set in depth, I'm not quite sure
why this patch set needs to be limited to measuring just LSM critical
data and can't be generalized. The patch set could be titled "ima:
measuring critical data" or "ima: measuring critical kernel data".
Measuring SELinux critical data would be an example of its usage.
For an example, refer to the ima_file_check hook, which is an example
of IMA being called directly, not via an LSM hook.
Mimi
^ permalink raw reply
* Re: [PATCH v4 11/17] module: Call security_kernel_post_load_data()
From: Jessica Yu @ 2020-08-05 14:53 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, Scott Branden, Mimi Zohar, Luis Chamberlain,
Takashi Iwai, SeongJae Park, KP Singh, linux-efi,
linux-security-module, linux-integrity, selinux, linux-kselftest,
linux-kernel
In-Reply-To: <20200729175845.1745471-12-keescook@chromium.org>
+++ Kees Cook [29/07/20 10:58 -0700]:
>Now that there is an API for checking loaded contents for modules
>loaded without a file, call into the LSM hooks.
>
>Cc: Jessica Yu <jeyu@kernel.org>
>Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Jessica Yu <jeyu@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 0/4] LSM: Measure security module data
From: Tyler Hicks @ 2020-08-05 16:14 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: Casey Schaufler, zohar, stephen.smalley.work, sashal, jmorris,
linux-integrity, selinux, linux-security-module, linux-kernel
In-Reply-To: <7c7a076b-6ba7-2e8d-409a-b3b4e4738c41@linux.microsoft.com>
On 2020-08-05 09:07:48, Lakshmi Ramasubramanian wrote:
> On 8/5/20 8:45 AM, Tyler Hicks wrote:
> > On 2020-08-05 08:36:40, Casey Schaufler wrote:
> > > On 8/4/2020 6:14 PM, Lakshmi Ramasubramanian wrote:
> > > > On 8/4/20 6:04 PM, Casey Schaufler wrote:
> > > > > On 8/4/2020 5:43 PM, Lakshmi Ramasubramanian wrote:
> > > > > > Critical data structures of security modules are currently not measured.
> > > > > > Therefore an attestation service, for instance, would not be able to
> > > > > > attest whether the security modules are always operating with the policies
> > > > > > and configuration that the system administrator had setup. The policies
> > > > > > and configuration for the security modules could be tampered with by
> > > > > > malware by exploiting kernel vulnerabilities or modified through some
> > > > > > inadvertent actions on the system. Measuring such critical data would
> > > > > > enable an attestation service to better assess the state of the system.
> > > > >
> > > > > I still wonder why you're calling this an LSM change/feature when
> > > > > all the change is in IMA and SELinux. You're not putting anything
> > > > > into the LSM infrastructure, not are you using the LSM infrastructure
> > > > > to achieve your ends. Sure, you *could* support other security modules
> > > > > using this scheme, but you have a configuration dependency on
> > > > > SELinux, so that's at best going to be messy. If you want this to
> > > > > be an LSM "feature" you need to use the LSM hooking mechanism.
> > > >
> > > > >
> > > > > I'm not objecting to the feature. It adds value. But as you've
> > > > > implemented it it is either an IMA extension to SELinux, or an
> > > > > SELiux extension to IMA. Could AppArmor add hooks for this without
> > > > > changing the IMA code? It doesn't look like it to me.
> > > >
> > > > The check in IMA to allow the new IMA hook func LSM_STATE and LSM_POLICY when SELinux is enabled is just because SELinux is the only security module using these hooks now.
> > > >
> > > > To enable AppArmor, for instance, to use the new IMA hooks to measure state and policy would just require adding the check for CONFIG_SECURITY_APPARMOR. Other than that, there are no IMA changes needed to support AppArmor or other such security modules.
> > >
> > > This is exactly what I'm objecting to. What if a system has both SELinux
> > > and AppArmor compiled in? What if it has both enabled?
> >
> > The SELinux state and policy would be measured but the AppArmor
> > state/policy would be silently ignored. This isn't ideal as the IMA
> > policy author would need to read the kernel code to figure out which
> > LSMs are going to be measured.
>
> Tyler - I am not sure why AppArmor state\policy would be ignored when both
> SELinux and AppArmor are enabled. Could you please clarify?
I think Casey is talking about now (when AppArmor is not supported by
this feature) and you're talking about the future (when AppArmor may be
supported by this feature).
Now, a "measure func=LSM_STATE" rule would be accepted by the IMA policy
parser but do nothing for the AppArmor LSM. The rule may do something in
the future but there's no difference in feedback to the IMA policy
author between now and the future.
Tyler
>
> When both the security modules are enabled, IMA policy validator would look
> like below:
>
> if (IS_ENABLED(CONFIG_SECURITY_SELINUX) ||
> IS_ENABLED(CONFIG_SECURITY_APPARMOR)) &&
> strcmp(args[0].from, "LSM_STATE") == 0)
> entry->func = LSM_STATE;
>
> Similar one for LSM_POLICY validation.
>
> Both SELinux and AppArmor can call ima_measure_lsm_state() and
> ima_measure_lsm_policy() to measure state and policy respectively.
>
> I don't think we should go the route of defining IMA hooks per security
> module (i.e., SELINUX_STATE, APPARMOR_STATE, SELINUX_POLICY, etc.) Instead
> keep the hook generic that any SM could use - which is what I have tried to
> address in this patch series.
>
> > >
> > > >
> > > > Please see Patch 1/4
> > > >
> > > > + else if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&
> > > > + strcmp(args[0].from, "LSM_STATE") == 0)
> > > > + entry->func = LSM_STATE;
> > > > + else if (IS_ENABLED(CONFIG_SECURITY_SELINUX) &&
> > > > + strcmp(args[0].from, "LSM_POLICY") == 0)
> > > > + entry->func = LSM_POLICY;
> > > >
> > > > And, if early boot measurement is needed for AppArmor the following change in IMA's Kconfig
> > > >
> > > > Patch 4/4
> > > >
> > > > +config IMA_QUEUE_EARLY_BOOT_DATA
> > > > bool
> > > > + depends on SECURITY_SELINUX || (IMA_MEASURE_ASYMMETRIC_KEYS && SYSTEM_TRUSTED_KEYRING)
> > > > default y
> > > >
> > > > If you think calling this an "LSM feature" is not appropriate, please suggest a better phrase.
> > >
> > > In the code above you are under CONFIG_SECURITY_SELINUX.
> > > I would suggest that it's an SELinux feature, so you should
> > > be using SELINUX_STATE and SELINUX_POLICY, as I suggested
> > > before. Just because SELinux has state and policy to measure
> > > doesn't mean that a different module might not have other data,
> > > such as history, that should be covered as well.
>
> Good point - if other SMs have data besides state and policy, we can define
> IMA hooks to measure that as well.
>
> But I still think it is not the right approach to call this SELINUX_STATE
> and SELINUX_POLICY - it will lead to unnecessary code duplication in IMA as
> more SMs are onboarded, in my opinion. Correct me if I am wrong.
>
> -lakshmi
>
> >
> > In addition to SELINUX_STATE and SELINUX_POLICY, we should also consider
> > the proposed LSM_STATE and LSM_POLICY func values but require an "lsm"
> > rule conditional.
> >
> > So the current proposed rules:
> >
> > measure func=LSM_STATE
> > measure func=LSM_POLICY
> >
> > Would become:
> >
> > measure func=LSM_STATE lsm=selinux
> > measure func=LSM_POLICY lsm=selinux
> >
> > The following rules would be rejected:
> >
> > measure func=LSM_STATE
> > measure func=LSM_POLICY
> > measure func=LSM_STATE lsm=apparmor
> > measure func=LSM_POLICY lsm=smack
> >
> > Of course, the apparmor and smack rules could/would be allowed when
> > proper support is in place.
> >
>
> >
> > >
> > > I realize that IMA already has compile time dependencies to
> > > determine which xattrs to measure. There's no reason that
> > > the xattr list couldn't be determined at boot time, with
> > > each security module providing the XATTR_NAME values it
> > > uses.
> > >
> > > >
> > > > But like I said above, with minimal change in IMA other security modules can be supported to measure STATE and POLICY data.
> > > >
> > > > -lakshmi
> > > >
> > > >
^ permalink raw reply
* Re: [PATCH 13/17] watch_queue: Implement mount topology and attribute change notifications [ver #5]
From: Ian Kent @ 2020-08-05 11:36 UTC (permalink / raw)
To: Miklos Szeredi
Cc: David Howells, Linus Torvalds, Al Viro, Casey Schaufler,
Stephen Smalley, Nicolas Dichtel, Christian Brauner, andres,
Jeff Layton, dray, Karel Zak, keyrings, Linux API, linux-fsdevel,
LSM, linux-kernel
In-Reply-To: <CAJfpegvxKTy+4Zk6banvxQ83PeFV7Xnt2Qv=kkOg57rxFKqVEg@mail.gmail.com>
On Wed, 2020-08-05 at 09:43 +0200, Miklos Szeredi wrote:
> On Wed, Aug 5, 2020 at 3:54 AM Ian Kent <raven@themaw.net> wrote:
> > > > It's way more useful to have these in the notification than
> > > > obtainable
> > > > via fsinfo() IMHO.
> > >
> > > What is it useful for?
> >
> > Only to verify that you have seen all the notifications.
> >
> > If you have to grab that info with a separate call then the count
> > isn't necessarily consistent because other notifications can occur
> > while you grab it.
>
> No, no no. The watch queue will signal an overflow, without any
> additional overhead for the normal case. If you think of this as a
> protocol stack, then the overflow detection happens on the transport
> layer, instead of the application layer. The application layer is
> responsible for restoring state in case of a transport layer error,
> but detection of that error is not the responsibility of the
> application layer.
I can see in the kernel code that an error is returned if the message
buffer is full when trying to add a message, I just can't see where
to get it in the libmount code.
That's not really a communication protocol problem.
Still I need to work out how to detect it, maybe it is seen by
the code in libmount already and I simply can't see what I need
to do to recognise it ...
So I'm stuck wanting to verify I have got everything that was
sent and am having trouble moving on from that.
Ian
^ permalink raw reply
* Re: [PATCH 10/18] fsinfo: Provide notification overrun handling support [ver #21]
From: Ian Kent @ 2020-08-05 11:23 UTC (permalink / raw)
To: Miklos Szeredi
Cc: David Howells, Al Viro, Linus Torvalds, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <CAJfpegs1NLaamFA12f=EJRN4B3_iC+Uzi2NQKTV-fBSypcufLQ@mail.gmail.com>
On Wed, 2020-08-05 at 09:45 +0200, Miklos Szeredi wrote:
> On Wed, Aug 5, 2020 at 4:46 AM Ian Kent <raven@themaw.net> wrote:
> > Coming back to an actual use case.
> >
> > What I said above is one aspect but, since I'm looking at this
> > right
> > now with systemd, and I do have the legacy code to fall back to,
> > the
> > "just reset everything" suggestion does make sense.
> >
> > But I'm struggling to see how I can identify notification buffer
> > overrun in libmount, and overrun is just one possibility for lost
> > notifications, so I like the idea that, as a library user, I can
> > work out that I need to take action based on what I have in the
> > notifications themselves.
>
> Hmm, what's the other possibility for lost notifications?
In user space that is:
Multi-threaded application races, single threaded applications and
signal processing races, other bugs ...
For example systemd has it's own event handling sub-system and handles
half a dozen or so event types of which the mount changes are one. It's
fairly complex so I find myself wondering if I can trust it and
wondering if there are undiscovered bugs in it. The answer to the
former is probably yes but the answer to the later is also probably
yes.
Maybe I just paranoid!
Ian
^ permalink raw reply
* Re: [GIT PULL] Filesystem Information
From: Ian Kent @ 2020-08-05 11:13 UTC (permalink / raw)
To: Miklos Szeredi
Cc: David Howells, Linus Torvalds, Al Viro, Karel Zak, Jeff Layton,
Miklos Szeredi, Nicolas Dichtel, Christian Brauner,
Lennart Poettering, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <CAJfpegvQdCr+u51_xkpbS7eMZyNqtnk_tdK1KVhsiCuiFWWJJw@mail.gmail.com>
On Wed, 2020-08-05 at 10:00 +0200, Miklos Szeredi wrote:
> On Wed, Aug 5, 2020 at 3:33 AM Ian Kent <raven@themaw.net> wrote:
> > On Tue, 2020-08-04 at 16:36 +0200, Miklos Szeredi wrote:
> > > And notice how similar the above interface is to getxattr(), or
> > > the
> > > proposed readfile(). Where has the "everything is a file"
> > > philosophy
> > > gone?
> >
> > Maybe, but that philosophy (in a roundabout way) is what's resulted
> > in some of the problems we now have. Granted it's blind application
> > of that philosophy rather than the philosophy itself but that is
> > what happens.
>
> Agree. What people don't seem to realize, even though there are
> blindingly obvious examples, that binary interfaces like the proposed
> fsinfo(2) syscall can also result in a multitude of problems at the
> same time as solving some others.
>
> There's no magic solution in API design, it's not balck and white.
> We just need to strive for a good enough solution. The problem seems
> to be that trying to discuss the merits of other approaches seems to
> hit a brick wall. We just see repeated pull requests from David,
> without any real discussion of the proposed alternatives.
>
> > I get that your comments are driven by the way that philosophy
> > should
> > be applied which is more of a "if it works best doing it that way
> > then
> > do it that way, and that's usually a file".
> >
> > In this case there is a logical division of various types of file
> > system information and the underlying suggestion is maybe it's time
> > to move away from the "everything is a file" hard and fast rule,
> > and get rid of some of the problems that have resulted from it.
> >
> > The notifications is an example, yes, the delivery mechanism is
> > a "file" but the design of the queueing mechanism makes a lot of
> > sense for the throughput that's going to be needed as time marches
> > on. Then there's different sub-systems each with unique information
> > that needs to be deliverable some other way because delivering
> > "all"
> > the information via the notification would be just plain wrong so
> > a multi-faceted information delivery mechanism makes the most
> > sense to allow specific targeted retrieval of individual items of
> > information.
> >
> > But that also supposes your at least open to the idea that "maybe
> > not everything should be a file".
>
> Sure. I've learned pragmatism, although idealist at heart. And I'm
> not saying all API's from David are shit. statx(2) is doing fine.
> It's a simple binary interface that does its job well. Compare the
> header files for statx and fsinfo, though, and maybe you'll see what
> I'm getting at...
Yeah, but I'm biased so not much joy there ... ;)
Ian
^ permalink raw reply
* Re: [PATCH v6 0/4] LSM: Measure security module data
From: Lakshmi Ramasubramanian @ 2020-08-05 16:21 UTC (permalink / raw)
To: Tyler Hicks
Cc: Casey Schaufler, zohar, stephen.smalley.work, sashal, jmorris,
linux-integrity, selinux, linux-security-module, linux-kernel
In-Reply-To: <20200805161449.GC4365@sequoia>
On 8/5/20 9:14 AM, Tyler Hicks wrote:
> On 2020-08-05 09:07:48, Lakshmi Ramasubramanian wrote:
>> On 8/5/20 8:45 AM, Tyler Hicks wrote:
>>> On 2020-08-05 08:36:40, Casey Schaufler wrote:
>>>> On 8/4/2020 6:14 PM, Lakshmi Ramasubramanian wrote:
>>>>> On 8/4/20 6:04 PM, Casey Schaufler wrote:
>>>>>> On 8/4/2020 5:43 PM, Lakshmi Ramasubramanian wrote:
>>>>>>> Critical data structures of security modules are currently not measured.
>>>>>>> Therefore an attestation service, for instance, would not be able to
>>>>>>> attest whether the security modules are always operating with the policies
>>>>>>> and configuration that the system administrator had setup. The policies
>>>>>>> and configuration for the security modules could be tampered with by
>>>>>>> malware by exploiting kernel vulnerabilities or modified through some
>>>>>>> inadvertent actions on the system. Measuring such critical data would
>>>>>>> enable an attestation service to better assess the state of the system.
>>>>>>
>>>>>> I still wonder why you're calling this an LSM change/feature when
>>>>>> all the change is in IMA and SELinux. You're not putting anything
>>>>>> into the LSM infrastructure, not are you using the LSM infrastructure
>>>>>> to achieve your ends. Sure, you *could* support other security modules
>>>>>> using this scheme, but you have a configuration dependency on
>>>>>> SELinux, so that's at best going to be messy. If you want this to
>>>>>> be an LSM "feature" you need to use the LSM hooking mechanism.
>>>>>
>>>>>>
>>>>>> I'm not objecting to the feature. It adds value. But as you've
>>>>>> implemented it it is either an IMA extension to SELinux, or an
>>>>>> SELiux extension to IMA. Could AppArmor add hooks for this without
>>>>>> changing the IMA code? It doesn't look like it to me.
>>>>>
>>>>> The check in IMA to allow the new IMA hook func LSM_STATE and LSM_POLICY when SELinux is enabled is just because SELinux is the only security module using these hooks now.
>>>>>
>>>>> To enable AppArmor, for instance, to use the new IMA hooks to measure state and policy would just require adding the check for CONFIG_SECURITY_APPARMOR. Other than that, there are no IMA changes needed to support AppArmor or other such security modules.
>>>>
>>>> This is exactly what I'm objecting to. What if a system has both SELinux
>>>> and AppArmor compiled in? What if it has both enabled?
>>>
>>> The SELinux state and policy would be measured but the AppArmor
>>> state/policy would be silently ignored. This isn't ideal as the IMA
>>> policy author would need to read the kernel code to figure out which
>>> LSMs are going to be measured.
>>
>> Tyler - I am not sure why AppArmor state\policy would be ignored when both
>> SELinux and AppArmor are enabled. Could you please clarify?
>
> I think Casey is talking about now (when AppArmor is not supported by
> this feature) and you're talking about the future (when AppArmor may be
> supported by this feature).
Got it - thanks for clarifying.
But with the current code if SELinux is enabled on the system, but
AppArmor is not and the IMA policy contains "measure func=LSM_STATE"
then the policy will be rejected as "-EINVAL".
So the policy author would get a feedback even now.
Correct me if I am wrong.
-lakshmi
^ permalink raw reply
* Re: [PATCH 10/18] fsinfo: Provide notification overrun handling support [ver #21]
From: David Howells @ 2020-08-05 16:06 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, viro, torvalds, raven, mszeredi, christian, jannh,
darrick.wong, kzak, jlayton, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
In-Reply-To: <20200804135641.GE32719@miu.piliscsaba.redhat.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> Shoun't we just make sure that the likelyhood of overruns is low
That's not necessarily easy. To avoid overruns you need a bigger buffer. The
buffer is preallocated from unswappable kernel space. Yes, you can increase
the size of the buffer, but it eats out of your pipe bufferage limit.
Further, it's a *general* notifications queue, not just for a specific
purpose, but that means it might get connected to multiple sources, and doing
something like tearing down a container might generate enough notifications to
overrun the queue.
> and if it happens, just reinitialize everthing from scratch (shouldn't be
> *that* expensive).
If you then spend time reinitialising everything, you're increasing the
likelihood of racing with further events. Further, there multiple expenses:
firstly, you have to tear down and discard all the data that you've spent time
setting up; secondly, it takes time doing all this; thirdly, it takes cpu
cycles away from applications.
The reason I put the event counters in there and made it so that fsinfo()
could read all the mounts in a subtree and their event counters in one go is
to make it faster for the user to find out what changed in the event that a
notification is lost.
I have a patch (not included here as it occasionally induces oopses) that
attempts to make this doable under the RCU read lock so that it doesn't
prevent mounts from taking place during the scan.
David
^ permalink raw reply
* Re: [PATCH 08/18] fsinfo: Allow mount topology and propagation info to be retrieved [ver #21]
From: David Howells @ 2020-08-05 15:37 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, viro, torvalds, raven, mszeredi, christian, jannh,
darrick.wong, kzak, jlayton, linux-api, linux-fsdevel,
linux-security-module, linux-kernel
In-Reply-To: <20200804133817.GD32719@miu.piliscsaba.redhat.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> > + __u32 shared_group_id; /* Shared: mount group ID */
> > + __u32 dependent_source_id; /* Dependent: source mount group ID */
> > + __u32 dependent_clone_of_id; /* Dependent: ID of mount this was cloned from */
>
> Another set of ID's that are currently 32bit *internally* but that doesn't
> mean they will always be 32 bit.
>
> And that last one (apart from "slave" being obfuscated)
I had "slave" in there. It got objected to. See
Documentation/process/coding-style.rst section 4.
> is simply incorrect. It has nothing to do with cloning. It's the "ID of
> the closest peer group in the propagation chain that has a representative
> mount in the current root".
You appear to be in disagreement with others that I've asked.
David
^ permalink raw reply
* Re: [dm-devel] [RFC PATCH v5 00/11] Integrity Policy Enforcement LSM (IPE)
From: James Morris @ 2020-08-05 23:51 UTC (permalink / raw)
To: Mimi Zohar
Cc: James Bottomley, Deven Bowers, Pavel Machek, Sasha Levin, snitzer,
dm-devel, tyhicks, agk, paul, corbet, nramas, serge,
pasha.tatashin, jannh, linux-block, viro, axboe, mdsakib,
linux-kernel, eparis, linux-security-module, linux-audit,
linux-fsdevel, linux-integrity, jaskarankhurana
In-Reply-To: <b08ae82102f35936427bf138085484f75532cff1.camel@linux.ibm.com>
On Wed, 5 Aug 2020, Mimi Zohar wrote:
> If block layer integrity was enough, there wouldn't have been a need
> for fs-verity. Even fs-verity is limited to read only filesystems,
> which makes validating file integrity so much easier. From the
> beginning, we've said that fs-verity signatures should be included in
> the measurement list. (I thought someone signed on to add that support
> to IMA, but have not yet seen anything.)
>
> Going forward I see a lot of what we've accomplished being incorporated
> into the filesystems. When IMA will be limited to defining a system
> wide policy, I'll have completed my job.
What are your thoughts on IPE being a standalone LSM? Would you prefer to
see its functionality integrated into IMA?
^ permalink raw reply
* Re: [PATCH 10/18] fsinfo: Provide notification overrun handling support [ver #21]
From: Ian Kent @ 2020-08-06 1:47 UTC (permalink / raw)
To: Miklos Szeredi
Cc: David Howells, Al Viro, Linus Torvalds, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <CAJfpeguvTspY7pi52n1aznebCF2jYki40hy5idkgu1D2y6C6mg@mail.gmail.com>
On Wed, 2020-08-05 at 13:27 +0200, Miklos Szeredi wrote:
> On Wed, Aug 5, 2020 at 1:23 PM Ian Kent <raven@themaw.net> wrote:
> > On Wed, 2020-08-05 at 09:45 +0200, Miklos Szeredi wrote:
> > > Hmm, what's the other possibility for lost notifications?
> >
> > In user space that is:
> >
> > Multi-threaded application races, single threaded applications and
> > signal processing races, other bugs ...
>
> Okay, let's fix the bugs then.
It's the the bugs you don't know about that get you, in this case
the world "is" actually out to get you, ;)
Ian
^ permalink raw reply
* Re: [PATCH 06/18] fsinfo: Add a uniquifier ID to struct mount [ver #21]
From: Ian Kent @ 2020-08-06 5:43 UTC (permalink / raw)
To: Matthew Wilcox, David Howells
Cc: Miklos Szeredi, Al Viro, Linus Torvalds, Miklos Szeredi,
Christian Brauner, Jann Horn, Darrick J. Wong, Karel Zak,
Jeff Layton, Linux API, linux-fsdevel, LSM, linux-kernel
In-Reply-To: <20200805193303.GM23808@casper.infradead.org>
On Wed, 2020-08-05 at 20:33 +0100, Matthew Wilcox wrote:
> On Wed, Aug 05, 2020 at 04:30:10PM +0100, David Howells wrote:
> > Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > > idr_alloc_cyclic() seems to be a good template for doing the
> > > lower
> > > 32bit allocation, and we can add code to increment the high 32bit
> > > on
> > > wraparound.
> > >
> > > Lots of code uses idr_alloc_cyclic() so I guess it shouldn't be
> > > too
> > > bad in terms of memory use or performance.
> >
> > It's optimised for shortness of path and trades memory for
> > performance. It's
> > currently implemented using an xarray, so memory usage is dependent
> > on the
> > sparseness of the tree. Each node in the tree is 576 bytes and in
> > the worst
> > case, each one node will contain one mount - and then you have to
> > backfill the
> > ancestry, though for lower memory costs.
> >
> > Systemd makes life more interesting since it sets up a whole load
> > of
> > propagations. Each mount you make may cause several others to be
> > created, but
> > that would likely make the tree more efficient.
>
> I would recommend using xa_alloc and ignoring the ID assigned from
> xa_alloc. Looking up by unique ID is then a matter of iterating
> every
> mount (xa_for_each()) looking for a matching unique ID in the mount
> struct. That's O(n) search, but it's faster than a linked list, and
> we
> don't have that many mounts in a system.
How many is not many, 5000, 10000, I agree that 30000 plus is fairly
rare, even for the autofs direct mount case I hope the implementation
here will help to fix.
Ian
^ permalink raw reply
* Re: [PATCH 1/2] ima: Pre-parse the list of keyrings in a KEY_CHECK rule
From: Nayna @ 2020-08-06 15:34 UTC (permalink / raw)
To: Tyler Hicks
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Tushar Sugandhi, Nayna Jain,
linux-kernel, linux-integrity, linux-security-module
In-Reply-To: <20200727140831.64251-2-tyhicks@linux.microsoft.com>
On 7/27/20 10:08 AM, Tyler Hicks wrote:
> The ima_keyrings buffer was used as a work buffer for strsep()-based
> parsing of the "keyrings=" option of an IMA policy rule. This parsing
> was re-performed each time an asymmetric key was added to a kernel
> keyring for each loaded policy rule that contained a "keyrings=" option.
>
> An example rule specifying this option is:
>
> measure func=KEY_CHECK keyrings=a|b|c
>
> The rule says to measure asymmetric keys added to any of the kernel
> keyrings named "a", "b", or "c". The size of the buffer size was
> equal to the size of the largest "keyrings=" value seen in a previously
> loaded rule (5 + 1 for the NUL-terminator in the previous example) and
> the buffer was pre-allocated at the time of policy load.
>
> The pre-allocated buffer approach suffered from a couple bugs:
>
> 1) There was no locking around the use of the buffer so concurrent key
> add operations, to two different keyrings, would result in the
> strsep() loop of ima_match_keyring() to modify the buffer at the same
> time. This resulted in unexpected results from ima_match_keyring()
> and, therefore, could cause unintended keys to be measured or keys to
> not be measured when IMA policy intended for them to be measured.
>
> 2) If the kstrdup() that initialized entry->keyrings in ima_parse_rule()
> failed, the ima_keyrings buffer was freed and set to NULL even when a
> valid KEY_CHECK rule was previously loaded. The next KEY_CHECK event
> would trigger a call to strcpy() with a NULL destination pointer and
> crash the kernel.
>
> Remove the need for a pre-allocated global buffer by parsing the list of
> keyrings in a KEY_CHECK rule at the time of policy load. The
> ima_rule_entry will contain an array of string pointers which point to
> the name of each keyring specified in the rule. No string processing
> needs to happen at the time of asymmetric key add so iterating through
> the list and doing a string comparison is all that's required at the
> time of policy check.
>
> In the process of changing how the "keyrings=" policy option is handled,
> a couple additional bugs were fixed:
>
> 1) The rule parser accepted rules containing invalid "keyrings=" values
> such as "a|b||c", "a|b|", or simply "|".
>
> 2) The /sys/kernel/security/ima/policy file did not display the entire
> "keyrings=" value if the list of keyrings was longer than what could
> fit in the fixed size tbuf buffer in ima_policy_show().
>
> Fixes: 5c7bac9fb2c5 ("IMA: pre-allocate buffer to hold keyrings string")
> Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---
> security/integrity/ima/ima_policy.c | 138 +++++++++++++++++++---------
> 1 file changed, 93 insertions(+), 45 deletions(-)
>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 07f033634b27..c328cfa0fc49 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -59,6 +59,11 @@ enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
>
> enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
>
> +struct ima_rule_opt_list {
> + size_t count;
> + char *items[];
> +};
> +
> struct ima_rule_entry {
> struct list_head list;
> int action;
> @@ -78,7 +83,7 @@ struct ima_rule_entry {
> int type; /* audit type */
> } lsm[MAX_LSM_RULES];
> char *fsname;
> - char *keyrings; /* Measure keys added to these keyrings */
> + struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> struct ima_template_desc *template;
> };
>
> @@ -206,10 +211,6 @@ static LIST_HEAD(ima_policy_rules);
> static LIST_HEAD(ima_temp_rules);
> static struct list_head *ima_rules = &ima_default_rules;
>
> -/* Pre-allocated buffer used for matching keyrings. */
> -static char *ima_keyrings;
> -static size_t ima_keyrings_len;
> -
> static int ima_policy __initdata;
>
> static int __init default_measure_policy_setup(char *str)
> @@ -253,6 +254,72 @@ static int __init default_appraise_policy_setup(char *str)
> }
> __setup("ima_appraise_tcb", default_appraise_policy_setup);
>
> +static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
> +{
> + struct ima_rule_opt_list *opt_list;
> + size_t count = 0;
> + char *src_copy;
> + char *cur, *next;
> + size_t i;
> +
> + src_copy = match_strdup(src);
> + if (!src_copy)
> + return NULL;
The caller of this function checks for IS_ERR(..) and not
IS_ERR_OR_NULL(..). Shouldn't it return ERR_PTR(-EINVAL) instead of NULL ?
Thanks & Regards,
- Nayna
^ permalink raw reply
* Re: [PATCH 1/2] ima: Pre-parse the list of keyrings in a KEY_CHECK rule
From: Tyler Hicks @ 2020-08-06 15:46 UTC (permalink / raw)
To: Nayna
Cc: Mimi Zohar, Dmitry Kasatkin, James Morris, Serge E . Hallyn,
Lakshmi Ramasubramanian, Tushar Sugandhi, Nayna Jain,
linux-kernel, linux-integrity, linux-security-module
In-Reply-To: <8f749594-1214-9f2d-4614-d360772a2ab6@linux.vnet.ibm.com>
On 2020-08-06 11:34:43, Nayna wrote:
>
> On 7/27/20 10:08 AM, Tyler Hicks wrote:
> > The ima_keyrings buffer was used as a work buffer for strsep()-based
> > parsing of the "keyrings=" option of an IMA policy rule. This parsing
> > was re-performed each time an asymmetric key was added to a kernel
> > keyring for each loaded policy rule that contained a "keyrings=" option.
> >
> > An example rule specifying this option is:
> >
> > measure func=KEY_CHECK keyrings=a|b|c
> >
> > The rule says to measure asymmetric keys added to any of the kernel
> > keyrings named "a", "b", or "c". The size of the buffer size was
> > equal to the size of the largest "keyrings=" value seen in a previously
> > loaded rule (5 + 1 for the NUL-terminator in the previous example) and
> > the buffer was pre-allocated at the time of policy load.
> >
> > The pre-allocated buffer approach suffered from a couple bugs:
> >
> > 1) There was no locking around the use of the buffer so concurrent key
> > add operations, to two different keyrings, would result in the
> > strsep() loop of ima_match_keyring() to modify the buffer at the same
> > time. This resulted in unexpected results from ima_match_keyring()
> > and, therefore, could cause unintended keys to be measured or keys to
> > not be measured when IMA policy intended for them to be measured.
> >
> > 2) If the kstrdup() that initialized entry->keyrings in ima_parse_rule()
> > failed, the ima_keyrings buffer was freed and set to NULL even when a
> > valid KEY_CHECK rule was previously loaded. The next KEY_CHECK event
> > would trigger a call to strcpy() with a NULL destination pointer and
> > crash the kernel.
> >
> > Remove the need for a pre-allocated global buffer by parsing the list of
> > keyrings in a KEY_CHECK rule at the time of policy load. The
> > ima_rule_entry will contain an array of string pointers which point to
> > the name of each keyring specified in the rule. No string processing
> > needs to happen at the time of asymmetric key add so iterating through
> > the list and doing a string comparison is all that's required at the
> > time of policy check.
> >
> > In the process of changing how the "keyrings=" policy option is handled,
> > a couple additional bugs were fixed:
> >
> > 1) The rule parser accepted rules containing invalid "keyrings=" values
> > such as "a|b||c", "a|b|", or simply "|".
> >
> > 2) The /sys/kernel/security/ima/policy file did not display the entire
> > "keyrings=" value if the list of keyrings was longer than what could
> > fit in the fixed size tbuf buffer in ima_policy_show().
> >
> > Fixes: 5c7bac9fb2c5 ("IMA: pre-allocate buffer to hold keyrings string")
> > Fixes: 2b60c0ecedf8 ("IMA: Read keyrings= option from the IMA policy")
> > Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> > ---
> > security/integrity/ima/ima_policy.c | 138 +++++++++++++++++++---------
> > 1 file changed, 93 insertions(+), 45 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 07f033634b27..c328cfa0fc49 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -59,6 +59,11 @@ enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB };
> > enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY };
> > +struct ima_rule_opt_list {
> > + size_t count;
> > + char *items[];
> > +};
> > +
> > struct ima_rule_entry {
> > struct list_head list;
> > int action;
> > @@ -78,7 +83,7 @@ struct ima_rule_entry {
> > int type; /* audit type */
> > } lsm[MAX_LSM_RULES];
> > char *fsname;
> > - char *keyrings; /* Measure keys added to these keyrings */
> > + struct ima_rule_opt_list *keyrings; /* Measure keys added to these keyrings */
> > struct ima_template_desc *template;
> > };
> > @@ -206,10 +211,6 @@ static LIST_HEAD(ima_policy_rules);
> > static LIST_HEAD(ima_temp_rules);
> > static struct list_head *ima_rules = &ima_default_rules;
> > -/* Pre-allocated buffer used for matching keyrings. */
> > -static char *ima_keyrings;
> > -static size_t ima_keyrings_len;
> > -
> > static int ima_policy __initdata;
> > static int __init default_measure_policy_setup(char *str)
> > @@ -253,6 +254,72 @@ static int __init default_appraise_policy_setup(char *str)
> > }
> > __setup("ima_appraise_tcb", default_appraise_policy_setup);
> > +static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src)
> > +{
> > + struct ima_rule_opt_list *opt_list;
> > + size_t count = 0;
> > + char *src_copy;
> > + char *cur, *next;
> > + size_t i;
> > +
> > + src_copy = match_strdup(src);
> > + if (!src_copy)
> > + return NULL;
>
> The caller of this function checks for IS_ERR(..) and not
> IS_ERR_OR_NULL(..). Shouldn't it return ERR_PTR(-EINVAL) instead of NULL ?
Yes! Thank you for catching this.
I switched this function to returning an ERR_PTR() towards the end of my
development for this series and missed this particular return.
I'll send out a v2 ASAP.
Tyler
>
> Thanks & Regards,
>
> - Nayna
^ 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